The media controller should correctly show and remove the start button based on the right media criteria.
<rdar://problem/28845656>
Created attachment 292060 [details] Patch
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
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
Raised https://bugs.webkit.org/show_bug.cgi?id=163669 to track the iOS failure.
Created attachment 292079 [details] Patch
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?
(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().
Created attachment 292086 [details] Patch for landing
(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.
Comment on attachment 292086 [details] Patch for landing Clearing flags on attachment: 292086 Committed r207554: <http://trac.webkit.org/changeset/207554>
All reviewed patches have been landed. Closing bug.