-
Notifications
You must be signed in to change notification settings - Fork 29
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
Enforce member validity via WebIDL #255
Conversation
In `setPositionState(state)` method, position can be zero if the `position` is is not present. And zero is not a positive number.
`null` will be converted to `0`[1] when `null` is set to the member within MediaPositionState. [1] http://www.ecma-international.org/ecma-262/5.1/#sec-9.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with those changes, should we use more WebIDL?
For instance:
- Make duration a required member of MediaPositionState.
- Add default values for position and playbackRate.
It would also be good to have corresponding WPT tests that cover those changes.
I think the PR mostly looks good and should be completed with the following bits:
@ChunMinChang, can you revise the PR according yesterday's discussion or do you want me to take over? |
throw a <a exception>TypeError</a>. | ||
</li> | ||
<li> | ||
If the <a dict-member for="MediaPositionState">position</a> is not present | ||
or its value is null, set it to zero. | ||
If the <a dict-member for="MediaPositionState">position</a> is not present, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible to just default these in the IDL itself?
Can't it just be this?:
That should set all the defaults and the duration being required. |
Conclusion from TPAC meeting: So we accept 0, positive, positive infinity but throw on negative and NaN. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes based on the proposed WebIDL.
Let's continue the work at #304. |
Closing in favor of #304 |
Fixes #252.
Fixes #303.
Let me know if I overlook something ;)
Preview | Diff
Preview | Diff