RESOLVED FIXED 163659
[Modern Media Controls] Media Controller: click-to-start support
https://bugs.webkit.org/show_bug.cgi?id=163659
Summary [Modern Media Controls] Media Controller: click-to-start support
Antoine Quint
Reported 2016-10-19 05:30:05 PDT
The media controller should correctly show and remove the start button based on the right media criteria.
Attachments
Patch (64.21 KB, patch)
2016-10-19 05:37 PDT, Antoine Quint
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2016-10-19 06:52 PDT, Build Bot
no flags
Patch (63.90 KB, patch)
2016-10-19 10:21 PDT, Antoine Quint
no flags
Patch for landing (63.72 KB, patch)
2016-10-19 11:32 PDT, Antoine Quint
no flags
Radar WebKit Bug Importer
Comment 1 2016-10-19 05:31:08 PDT
Antoine Quint
Comment 2 2016-10-19 05:37:08 PDT
Build Bot
Comment 3 2016-10-19 06:52:01 PDT
Comment on attachment 292060 [details] Patch Attachment 292060 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2324130 New failing tests: media/modern-media-controls/start-support/start-support-fullscreen.html
Build Bot
Comment 4 2016-10-19 06:52:04 PDT
Created attachment 292065 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Antoine Quint
Comment 5 2016-10-19 10:18:18 PDT
Raised https://bugs.webkit.org/show_bug.cgi?id=163669 to track the iOS failure.
Antoine Quint
Comment 6 2016-10-19 10:21:43 PDT
Dean Jackson
Comment 7 2016-10-19 10:58:41 PDT
Comment on attachment 292079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292079&action=review > Source/WebCore/ChangeLog:30 > + * Modules/modern-media-controls/media/media-controller-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. I wonder what you should do with this line? > Source/WebCore/ChangeLog:40 > + * Modules/modern-media-controls/media/start-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. And this one. > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:35 > + const media = mediaController.media; > + for (let eventType of this.mediaEvents) > + media.addEventListener(eventType, this); Don't bother with the local variable? Just use mediaController.media.addEventListener. > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:49 > + // Implemented by subclasses. Must these be implemented by subclasses? Is this parent version never called? If so, maybe put a console error here to make it clear (e.g. the closest we can get to an ASSERT) > Source/WebCore/Modules/modern-media-controls/media/start-support.js:57 > + syncControl() > + { > + this.mediaController.controls.showsStartButton = this._shouldShowStartButton(); > + } When is this called? > LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > + media.addEventListener("pause", function() { > + debug(""); > + debug("Media is paused"); > + shouldBeFalse("mediaController.controls.showsStartButton"); > + debug(""); > + shadowRoot.host.remove(); > + media.remove(); > + finishJSTest(); > + }); What guarantees that this event listener will run after the media controls have reacted to the same event?
Antoine Quint
Comment 8 2016-10-19 11:05:29 PDT
(In reply to comment #7) > Comment on attachment 292079 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292079&action=review > > > Source/WebCore/ChangeLog:30 > > + * Modules/modern-media-controls/media/media-controller-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. > > I wonder what you should do with this line? I think I know what you mean. > > Source/WebCore/ChangeLog:40 > > + * Modules/modern-media-controls/media/start-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. > > And this one. Sure. > > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:35 > > + const media = mediaController.media; > > + for (let eventType of this.mediaEvents) > > + media.addEventListener(eventType, this); > > Don't bother with the local variable? Just use > mediaController.media.addEventListener. OK. > > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:49 > > + // Implemented by subclasses. > > Must these be implemented by subclasses? Is this parent version never > called? If so, maybe put a console error here to make it clear (e.g. the > closest we can get to an ASSERT) They need not be implemented by subclasses, but are designed to be. If there is no control provided, neither buttonWasClicked() not syncControl() would ever be called. > > Source/WebCore/Modules/modern-media-controls/media/start-support.js:57 > > + syncControl() > > + { > > + this.mediaController.controls.showsStartButton = this._shouldShowStartButton(); > > + } > > When is this called? At the end of construction and as a result of media events being triggered. > > LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > > + media.addEventListener("pause", function() { > > + debug(""); > > + debug("Media is paused"); > > + shouldBeFalse("mediaController.controls.showsStartButton"); > > + debug(""); > > + shadowRoot.host.remove(); > > + media.remove(); > > + finishJSTest(); > > + }); > > What guarantees that this event listener will run after the media controls > have reacted to the same event? Nothing as it stands, but it works! I supposed I could evaluate all of these in a setTimeout().
Antoine Quint
Comment 9 2016-10-19 11:32:20 PDT
Created attachment 292086 [details] Patch for landing
Antoine Quint
Comment 10 2016-10-19 11:36:11 PDT
(In reply to comment #8) > (In reply to comment #7) > > Comment on attachment 292079 [details] > > > LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > > > + media.addEventListener("pause", function() { > > > + debug(""); > > > + debug("Media is paused"); > > > + shouldBeFalse("mediaController.controls.showsStartButton"); > > > + debug(""); > > > + shadowRoot.host.remove(); > > > + media.remove(); > > > + finishJSTest(); > > > + }); > > > > What guarantees that this event listener will run after the media controls > > have reacted to the same event? > > Nothing as it stands, but it works! I supposed I could evaluate all of these > in a setTimeout(). Actually, they're registered first in the MediaControllerSupport constructor, and only after in the tests, so they're fired first in MediaControllerSupport.
WebKit Commit Bot
Comment 11 2016-10-19 12:07:18 PDT
Comment on attachment 292086 [details] Patch for landing Clearing flags on attachment: 292086 Committed r207554: <http://trac.webkit.org/changeset/207554>
WebKit Commit Bot
Comment 12 2016-10-19 12:07:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.