Bug 163659 – [Modern Media Controls] Media Controller: click-to-start support

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

The media controller should correctly show and remove the start button based on the right media criteria.

[Radar WebKit Bug Importer]

<rdar://problem/28845656>

[Antoine Quint]

Created attachment 292060 [details]
Patch

[Build Bot]

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]

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]

Raised https://bugs.webkit.org/show_bug.cgi?id=163669 to track the iOS failure.

[Antoine Quint]

Created attachment 292079 [details]
Patch

[Dean Jackson]

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]

(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]

Created attachment 292086 [details]
Patch for landing

[Antoine Quint]

(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 on attachment 292086 [details]
Patch for landing

Clearing flags on attachment: 292086

Committed r207554: <http://trac.webkit.org/changeset/207554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.