Bug 182297

Summary: [Modern Media Controls] Turn media/modern-media-controls/start-support tests back on
Product: WebKit Reporter: Antoine Quint <graouts>
Component: MediaAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, eric.carlson, jonlee, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jonlee: review+

Description Antoine Quint 2018-01-30 09:45:39 PST
[Modern Media Controls] Turn media/modern-media-controls/start-support tests back on
Comment 1 Antoine Quint 2018-01-30 09:51:31 PST
Created attachment 332662 [details]
Patch
Comment 2 Antoine Quint 2018-01-31 01:35:51 PST
Created attachment 332751 [details]
Patch
Comment 3 Antoine Quint 2018-01-31 01:38:05 PST
*** Bug 174683 has been marked as a duplicate of this bug. ***
Comment 4 Eric Carlson 2018-01-31 09:29:30 PST
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.
Comment 5 Antoine Quint 2018-01-31 09:31:25 PST
(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!
Comment 6 Antoine Quint 2018-01-31 09:35:22 PST
Committed r227904: <https://trac.webkit.org/changeset/227904>
Comment 7 Radar WebKit Bug Importer 2018-01-31 09:36:31 PST
<rdar://problem/37073665>