Bug 227489 – [Modern Media Controls] Address additional feedback on LayoutTraits refactor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[Modern Media Controls] Address additional feedback on LayoutTraits refactor

[Sam Weinig]

Created attachment 432490 [details]
Patch

[Sam Weinig]

This is addressing the additional post land feedback in https://bugs.webkit.org/show_bug.cgi?id=227423.

It does not address the feedback on the overridenSupportingObjectClasses() delegation point, as doing so would require some file placement factoring that seems outside the scope.

[Devin Rousso]

Comment on attachment 432490 [details]
Patch

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

r=mews, nice!  Thanks for making these changes :)

> Source/WebCore/Modules/modern-media-controls/controls/ios-layout-traits.js:43
> +    controlsAvailabilityOverride()

I think this is much clearer :)

> Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:98
> +    FallThrough  : 2,

NIT: I feel like maybe `FallThrough` should be `0` so that its the only non-override (as well as being the only falsy value)

> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:253
> -        this._supportingObjects = this._supportingObjectClasses().map(SupportClass => new SupportClass(this), this);
> +        this._supportingObjects = this.layoutTraits.supportingObjectClasses().map(SupportClass => new SupportClass(this), this);

comment #2 suggests that you weren't going to do this.  FWIW this is the idea that I had in mind, so I'm on-board with this approach if it is workable :)

[Sam Weinig]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 432490 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432490&action=review
> 
> r=mews, nice!  Thanks for making these changes :)
> 
> > Source/WebCore/Modules/modern-media-controls/controls/ios-layout-traits.js:43
> > +    controlsAvailabilityOverride()
> 
> I think this is much clearer :)
> 
> > Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:98
> > +    FallThrough  : 2,
> 
> NIT: I feel like maybe `FallThrough` should be `0` so that its the only
> non-override (as well as being the only falsy value)

Ok.

> 
> > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:253
> > -        this._supportingObjects = this._supportingObjectClasses().map(SupportClass => new SupportClass(this), this);
> > +        this._supportingObjects = this.layoutTraits.supportingObjectClasses().map(SupportClass => new SupportClass(this), this);
> 
> comment #2 suggests that you weren't going to do this.  FWIW this is the
> idea that I had in mind, so I'm on-board with this approach if it is
> workable :)

Yeah, this is wrong. I borked the commit. Will fix.

[Sam Weinig]

Created attachment 432494 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80215569>

[Sam Weinig]

Created attachment 434724 [details]
Patch

[EWS]

Committed r280534 (240166@main): <https://commits.webkit.org/240166@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434724 [details].

[Sam Weinig]

(In reply to EWS from comment #8)
> Committed r280534 (240166@main): <https://commits.webkit.org/240166@main>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 434724 [details].

Well, drat. Will land the rest in new bugs.