Bug 182297 – [Modern Media Controls] Turn media/modern-media-controls/start-support tests back on

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

[Antoine Quint]

[Modern Media Controls] Turn media/modern-media-controls/start-support tests back on

[Antoine Quint]

Created attachment 332662 [details]
Patch

[Antoine Quint]

Created attachment 332751 [details]
Patch

[Antoine Quint]

*** Bug 174683 has been marked as a duplicate of this bug. ***

[Eric Carlson]

Comment on attachment 332751 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/buttons-container.js:45
> +        child.visible = true;

Is a button actually made visible when it is about to be removed, or is this to for some other state change?

> Source/WebCore/Modules/modern-media-controls/controls/buttons-container.js:63
> +            button.visible = !(!button.enabled || button.dropped);

Nit: the double negative made me scratch my head for a second. This would be slightly clearer as "(button.enabled && !button.dropped)"

> LayoutTests/media/modern-media-controls/pip-support/pip-support-click.html:44
> +setTimeout(finishMediaControlsTest, 4000);

Won't this make the test succeed even if the webkitpresentationmodechanged isn't fired?

> LayoutTests/media/modern-media-controls/start-support/start-support-click-to-start.html:36
> +setTimeout(() => {
> +    console.log(mediaController.controls.playPauseButton.parent.element.className);
> +    finishJSTest();
> +}, 3500);

Ditto for the play event.

[Antoine Quint]

(In reply to Eric Carlson from comment #4)
> Comment on attachment 332751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332751&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/buttons-container.js:45
> > +        child.visible = true;
> 
> Is a button actually made visible when it is about to be removed, or is this
> to for some other state change?

I'll be adding a comment in the commit that explains that we're resetting default values that may have been changed during a prior layout.

> > Source/WebCore/Modules/modern-media-controls/controls/buttons-container.js:63
> > +            button.visible = !(!button.enabled || button.dropped);
> 
> Nit: the double negative made me scratch my head for a second. This would be
> slightly clearer as "(button.enabled && !button.dropped)"

Yeah, I'll change that.

> > LayoutTests/media/modern-media-controls/pip-support/pip-support-click.html:44
> > +setTimeout(finishMediaControlsTest, 4000);
> 
> Won't this make the test succeed even if the webkitpresentationmodechanged
> isn't fired?
> 
> > LayoutTests/media/modern-media-controls/start-support/start-support-click-to-start.html:36
> > +setTimeout(() => {
> > +    console.log(mediaController.controls.playPauseButton.parent.element.className);
> > +    finishJSTest();
> > +}, 3500);
> 
> Ditto for the play event.

These two should have been gone, I only used them for debugging. Thanks for catching them!

[Antoine Quint]

Committed r227904: <https://trac.webkit.org/changeset/227904>

[Radar WebKit Bug Importer]

<rdar://problem/37073665>