[Modern Media Controls] Address additional feedback on LayoutTraits refactor
Created attachment 432490 [details] Patch
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.
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 :)
(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.
Created attachment 432494 [details] Patch
<rdar://problem/80215569>
Created attachment 434724 [details] Patch
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].
(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.