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

Enforce member validity via WebIDL #255

Closed
wants to merge 2 commits into from

Conversation

ChunMinChang
Copy link
Member

@ChunMinChang ChunMinChang commented Mar 26, 2020

Fixes #252.
Fixes #303.

Let me know if I overlook something ;)


Preview | Diff


Preview | Diff

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
Copy link
Contributor

@youennf youennf left a 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.

@chrisn chrisn mentioned this pull request Aug 10, 2023
@youennf
Copy link
Contributor

youennf commented Sep 12, 2023

I think the PR mostly looks good and should be completed with the following bits:

  • Revert the negative to not positive check, 0 being allowed.
  • Change the WebIDL to state that duration is unrestricted.
  • Add a new check so that if duration is NaN, an exception is thrown.

@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,
Copy link
Member

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?

@marcoscaceres
Copy link
Member

marcoscaceres commented Oct 4, 2023

Can't it just be this?:

dictionary MediaPositionState {
  required unrestricted double duration;
  double playbackRate = 1.0;
  double position = 0.0;
};

That should set all the defaults and the duration being required.

@marcoscaceres marcoscaceres changed the title Fix 252 Enforce member validity via WebIDL Oct 4, 2023
@chrisn
Copy link
Member

chrisn commented Oct 4, 2023

Conclusion from TPAC meeting: So we accept 0, positive, positive infinity but throw on negative and NaN.

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.

Requesting changes based on the proposed WebIDL.

@youennf
Copy link
Contributor

youennf commented Oct 18, 2023

Let's continue the work at #304.
@ChunMinChang, thanks for your efforts and input.

@marcoscaceres
Copy link
Member

Closing in favor of #304

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