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.

Sign up for GitHub

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

Jump to bottom

Specify behavior when playbackRate is set to unsupported value. #2829

Merged
merged 9 commits into from
Jul 19, 2017
Merged

Specify behavior when playbackRate is set to unsupported value. #2829

merged 9 commits into from
Jul 19, 2017

Conversation

japacible
Copy link
Contributor

For #2754. web-platform-tests PR: web-platform-tests/wpt#6522

PTAL, thanks!

@annevk
Copy link
Member

annevk commented Jul 12, 2017

Thanks! This looks pretty good as a start but there's no requirements in your text. You want something like:

on setting, it must run these steps:

  1. If the given value is not supported, then throw a X.
  2. Set the attribute to the given value and change the speed of playback (if the element is potentially playing).

(The "must" requirement automatically applies to the individual steps in the algorithm and doesn't have to be repeated. Infra has more on this.)

@japacible
Copy link
Contributor Author

Thanks for the feedback! I've updated the patch.

annevk
annevk reviewed Jul 17, 2017
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks much better! Couple minor nits left.

source Outdated

<ol>

<li><p>If the given value is not supported by the user agent, throw a <span>"<code>
Copy link
Member

Choose a reason for hiding this comment

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

You'll have to wrap before the <span>. Otherwise it affects the content of the <code> element.

Copy link
Member

Choose a reason for hiding this comment

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

Also, could you make it "then throw"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and done.

source Outdated
on setting, the user agent must follow these steps:</p>

<ol>

Copy link
Member

Choose a reason for hiding this comment

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

This newline can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

source Outdated

<li><p>Set <code data-x="dom-media-playbackRate">playbackRate</code> to the new value, and change
the playback speed (if the element is <span>potentially playing</span>).</p></li>

Copy link
Member

Choose a reason for hiding this comment

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

This newline can be removed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@japacible
Copy link
Contributor Author

Thanks! Updated with your comments.

@annevk
Copy link
Member

annevk commented Jul 18, 2017

Thank you, this looks good to me.

@jernoble @cpearce any final feedback?

@domenic could you review the tests once more?

@cpearce
Copy link

cpearce commented Jul 18, 2017

Looks good to me.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Jul 19, 2017
annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 19, 2017
@japacible @annevk
Update playbackRate tests to check for behavior when set to an unsupp…
a5c429f 
…orted value

See whatwg/html#2829 for HTML Standard change.
@annevk annevk merged commit 0aac96c into whatwg:master Jul 19, 2017
@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Jul 19, 2017
@jernoble
Copy link

I realize I'm late to respond (was OOTO for the last week), but this does look good to me.

@annevk
Copy link
Member

annevk commented Jul 24, 2017

Still good to know though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

Assignees
No one assigned
Labels
topic: media
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@japacible @annevk @cpearce @jernoble @domenic