Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MediaPositionState WebIDL #304

Merged
merged 5 commits into from
Jan 22, 2024
Merged

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Oct 18, 2023

Fixes #303
Fixes #252

We make duration a required member of MediaPositionState and add default values for position and playbackRate. duration is now unrestricted to allow Infinity, we add a special check for NaN.

We update MediaSession.setPositionState to accept null since MediaPositionState now has a required member.

This overtakes #303 and should fix #303 and #252.


Preview | Diff

@youennf youennf force-pushed the update-MediaPositionState branch 2 times, most recently from f2a317d to 31f3cf7 Compare October 24, 2023 11:48
We add default values for position and playbackRate.
duration is now unrestricted to allow Infinity and we add a special check for NaN.
We do not make duration required so that MediaSession.setPositionState can still take an optional state argument.

Fixes w3c#303 and w3c#252.
@youennf
Copy link
Contributor Author

youennf commented Oct 24, 2023

@marcoscaceres, @steimelchrome, PTAL

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay here...

@marcoscaceres marcoscaceres self-requested a review January 9, 2024 07:42
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits... but if it's not too late, it might be worth considering adding an explicit method for "clear" or reset.

@youennf
Copy link
Contributor Author

youennf commented Jan 14, 2024

it might be worth considering adding an explicit method for "clear" or reset.

It is probably worth an issue. Implementations do clear in case of setPositionState() so I am not sure we can break this.
We could do like WebKit and change setPositionState to accept setPositionState(null)

@marcoscaceres
Copy link
Member

Ok, filed #315 ... can follow up on that.

youennf and others added 3 commits January 18, 2024 10:22
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Copy link
Member

@chrisn chrisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@youennf youennf merged commit 42221de into w3c:main Jan 22, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jan 22, 2024
SHA: 42221de
Reason: push, by youennf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 7, 2024
Update for the spec change w3c/mediasession#304

Bug: 324153759
Change-Id: I006b079954cdb5818e97200b65e6ea42333ad6fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5274534
Reviewed-by: Tommy Steimel <[email protected]>
Commit-Queue: Yiren Wang <[email protected]>
Reviewed-by: Matthew Denton <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257586}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants