[webkit-reviews] review granted: [Bug 227489] [Modern Media Controls] Address additional feedback on LayoutTraits refactor : [Attachment 432490] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 227489: [Modern Media Controls] Address additional feedback on LayoutTraits
refactor
https://bugs.webkit.org/show_bug.cgi?id=227489

Attachment 432490: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432490
  --> https://bugs.webkit.org/attachment.cgi?id=432490
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 :)