[webkit-reviews] review granted: [Bug 236729] [Modern Media Controls] Add an alternative overflow icon : [Attachment 452233] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 236729: [Modern Media Controls] Add an alternative overflow icon
https://bugs.webkit.org/show_bug.cgi?id=236729

Attachment 452233: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 452233
  --> https://bugs.webkit.org/attachment.cgi?id=452233
Patch

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

r=me

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:38
> +    Ellipsis        : { name: "Ellipsis", type: "svg", label:
UIString("More\u2026") },

please alphabetize this by moving it right above `EllipsisCircle`

> Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:94
> +    overflowButtonHasCircle() {

Style: `{` goes on a separate line :)

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:46
> +	   if (layoutTraits.overflowButtonHasCircle())

I think you can actually get to the `layoutTraits` inside `OverflowButton` via
something like `layoutDelegate?.layoutTraits`.	So I'd leave this as-is and
then change the `constructor` of `OverflowButton` to be like so:
```
super({
    cssClassName: "overflow",
    iconName: layoutDelegate?.layoutTraits.overflowButtonHasCircle() ?
Icons.EllipsisCircle : Icons.Ellipsis,
    layoutDelegate,
});
```