Bug 163725 - [Modern Media Controls] Media Controller: skip back support
Summary: [Modern Media Controls] Media Controller: skip back support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-20 06:48 PDT by Antoine Quint
Modified: 2016-10-26 06:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (25.82 KB, patch)
2016-10-20 06:50 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (34.02 KB, patch)
2016-10-25 10:04 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-yosemite (925.71 KB, application/zip)
2016-10-25 11:08 PDT, WebKit Commit Bot
no flags Details
Patch for landing (34.05 KB, patch)
2016-10-25 11:19 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-20 06:48:04 PDT
Support for the skip back button.
Comment 1 Antoine Quint 2016-10-20 06:48:22 PDT
<rdar://problem/27989480>
Comment 2 Antoine Quint 2016-10-20 06:50:39 PDT
Created attachment 292174 [details]
Patch
Comment 3 Antoine Quint 2016-10-20 06:51:28 PDT
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.
Comment 4 Dean Jackson 2016-10-20 18:58:35 PDT
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.
Comment 5 Antoine Quint 2016-10-21 01:21:40 PDT
(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.
Comment 6 Antoine Quint 2016-10-25 10:04:28 PDT
Created attachment 292768 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2016-10-25 11:08:25 PDT
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
Comment 8 WebKit Commit Bot 2016-10-25 11:08:28 PDT
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
Comment 9 Antoine Quint 2016-10-25 11:19:05 PDT
Created attachment 292790 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-10-25 12:25:44 PDT
Comment on attachment 292790 [details]
Patch for landing

Clearing flags on attachment: 292790

Committed r207835: <http://trac.webkit.org/changeset/207835>
Comment 11 WebKit Commit Bot 2016-10-25 12:25:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Antoine Quint 2016-10-26 06:46:37 PDT
Fixing flakiness in https://bugs.webkit.org/show_bug.cgi?id=164013.