Bug 163725 – [Modern Media Controls] Media Controller: skip back support

• Component: Media
• Status: RESOLVED FIXED

[Antoine Quint]

Support for the skip back button.

[Antoine Quint]

<rdar://problem/27989480>

[Antoine Quint]

Created attachment 292174 [details]
Patch

[Antoine Quint]

No test coverage for now as I couldn't locate a piece of media that was > 30s in our current media assets, I will look for one. The code can be reviewed though.

[Dean Jackson]

Comment on attachment 292174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292174&action=review

> Source/WebCore/Modules/modern-media-controls/media/skip-back-support.js:26
> +class SkipBackSupport extends MediaControllerSupport

And another! :(

> Source/WebCore/Modules/modern-media-controls/media/skip-back-support.js:39
> +        media.currentTime = Math.max(media.currentTime - 30, media.seekable.start(0));

Can we make the 30 external? Or a parameter to the constructor.

[Antoine Quint]

(In reply to comment #4)
> Comment on attachment 292174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292174&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/media/skip-back-support.js:26
> > +class SkipBackSupport extends MediaControllerSupport
> 
> And another! :(

I don't see that as a problem, this maintains the same approach used for subclasses of IconButton in the UI part of the code. The code remains simple: subclassing doesn't add complexity and it's easy to navigate the source to find code related to a specific feature / button.

> > Source/WebCore/Modules/modern-media-controls/media/skip-back-support.js:39
> > +        media.currentTime = Math.max(media.currentTime - 30, media.seekable.start(0));
> 
> Can we make the 30 external? Or a parameter to the constructor.

We could make it a `const` in this file. Note that changing this value takes more than just changing it in this file, the asset for the skip back button embeds this value as well.

[Antoine Quint]

Created attachment 292768 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292768 [details]
Patch for landing

Rejecting attachment 292768 [details] from commit-queue.

New failing tests:
http/tests/media/modern-media-controls/skip-back-support/skip-back-support-button-click.html
Full output: http://webkit-queues.webkit.org/results/2368828

[WebKit Commit Bot]

Created attachment 292784 [details]
Archive of layout-test-results from webkit-cq-01 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antoine Quint]

Created attachment 292790 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292790 [details]
Patch for landing

Clearing flags on attachment: 292790

Committed r207835: <http://trac.webkit.org/changeset/207835>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The test added with this change is a flaky failure on macOS:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fmedia%2Fmodern-media-controls%2Fskip-back-support%2Fskip-back-support-button-click.html

[Antoine Quint]

Fixing flakiness in https://bugs.webkit.org/show_bug.cgi?id=164013.