Bug 206772

<rdar://problem/57098851>

Created attachment 388729 [details]
Patch

Comment on attachment 388729 [details]
Patch

Clearing flags on attachment: 388729

Committed r255103: <https://trac.webkit.org/changeset/255103>

All reviewed patches have been landed.  Closing bug.

> Committed r255103: <https://trac.webkit.org/changeset/255103>
The newly added test media/modern-media-controls/placard-support/placard-support-airplay-fullscreen-no-controls.html is consistently timing out on iOS. This issue was also indicated by EWS red iOS bubble.

Tracked in Bug 206800.

Comment on attachment 388729 [details]
Patch

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

Some belated review comments and a general comment: why wasn't a LayoutTests/ChangeLog generated?

> Source/WebCore/Modules/modern-media-controls/media/placard-support.js:50
> +        this._isDisabled = false;

You should set a default value for `_isDisabled` in the constructor, right now `!_isDisabled` is true even though `enable()` has never been called after construction. Also, it would probably be best to expose it at the `MediaControllerSupport` level since this is where enable()/disable() are exposed. Then we could have an `enabled` property.

> Source/WebCore/Modules/modern-media-controls/media/placard-support.js:56
> +        // Never disable the plackard, just remeber whether the placard should be visible or not

Typos: "plackard" and "remeber".

> LayoutTests/media/modern-media-controls/placard-support/placard-support-error-recover.html:4
> +<video src="404.mp4" style="width: 320px; height: 240px;" controls></video>

Why was this changed?

> LayoutTests/media/modern-media-controls/placard-support/placard-support-error.html:4
> +<video src="404.mp4" style="width: 320px; height: 240px;" controls></video>

Why was this changed?

(In reply to Antoine Quint from comment #6)
> Comment on attachment 388729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388729&action=review
> 
> Some belated review comments and a general comment: why wasn't a
> LayoutTests/ChangeLog generated?
> 
> > Source/WebCore/Modules/modern-media-controls/media/placard-support.js:50
> > +        this._isDisabled = false;
> 
> You should set a default value for `_isDisabled` in the constructor, right
> now `!_isDisabled` is true even though `enable()` has never been called
> after construction. 

It looked like enable() was always called immediately after construction, which is correct, since the PiP and AirPlay placards should always be enabled.  That said, yes we should probably initialize that property in the constructor.

> Also, it would probably be best to expose it at the
> `MediaControllerSupport` level since this is where enable()/disable() are
> exposed. Then we could have an `enabled` property.

Do any other controllers need specialized enabled/disabled behavior?  No need to add abstractions where they aren't necessary.

> > Source/WebCore/Modules/modern-media-controls/media/placard-support.js:56
> > +        // Never disable the plackard, just remeber whether the placard should be visible or not
> 
> Typos: "plackard" and "remeber".

Whoops.

> > LayoutTests/media/modern-media-controls/placard-support/placard-support-error-recover.html:4
> > +<video src="404.mp4" style="width: 320px; height: 240px;" controls></video>
> 
> Why was this changed?

The prior test was incorrect: error placards should only be displayed when controls are enabled, irregardless of fullscreen state.  In fact, we should have a negative test that the error placards don't get added when the 'controls' attribute is missing.