Bug 227505 – Move BottomControlsBarHeight and InsideMargin to be computed at runtime

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

[Dean Jackson]

Move BottomControlsBarHeight to a layout trait

[Radar WebKit Bug Importer]

<rdar://problem/79932256>

[Dean Jackson]

Created attachment 432525 [details]
Patch

[Sam Weinig]

Comment on attachment 432525 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:220
> +        this.bottomControlsBar.y = Math.max(0, this.height - this.layoutTraits.inlineBottomControlsBarHeight() - InsideMargin);

Not knowing nothing about nothing, can we instead query a CSS variable for something like this and get rid of both BottomControlsBarHeight and InsideMargin and just have the platform's css do it?

[Dean Jackson]

Created attachment 432550 [details]
Patch

[Dean Jackson]

Created attachment 432650 [details]
Patch

[Devin Rousso]

Comment on attachment 432650 [details]
Patch

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

r=mews

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:133
> +        const inlineInsideMargin = this.computedValueForStylePropertyInPx("--inline-controls-inside-margin");

Can we maybe cache this value somewhere so that we don't have to get the computed style more than once?  We don't expect this value to change so there's not much benefit to requesting it more than once.

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:219
> +        const inlineBottomControlsBarHeight = this.computedValueForStylePropertyInPx("--inline-controls-bar-height");

ditto (:133)

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:241
> +            return null;

NIT: maybe the fallback value should be `0` so that it's still valid px value (and therefore able to be used in math operations)?

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:244
> +        return parseFloat(value);

Not sure which is better, but you also could do this with
```
    window.getComputedStyle(this.element).getPropertyCSSValue(propertyName).getFloatValue(CSSPrimitiveValue.CSS_PX)
```

> Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:63
> +        const inlineInsideMargin = this.computedValueForStylePropertyInPx("--inline-controls-inside-margin");
> +        const inlineBottomControlsBarHeight = this.computedValueForStylePropertyInPx("--inline-controls-bar-height");

ditto (inline-media-controls.js:133)

[Dean Jackson]

Comment on attachment 432650 [details]
Patch

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

>> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:133
>> +        const inlineInsideMargin = this.computedValueForStylePropertyInPx("--inline-controls-inside-margin");
> 
> Can we maybe cache this value somewhere so that we don't have to get the computed style more than once?  We don't expect this value to change so there's not much benefit to requesting it more than once.

Good point.

>> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:241
>> +            return null;
> 
> NIT: maybe the fallback value should be `0` so that it's still valid px value (and therefore able to be used in math operations)?

Agreed.

>> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:244
>> +        return parseFloat(value);
> 
> Not sure which is better, but you also could do this with
> ```
>     window.getComputedStyle(this.element).getPropertyCSSValue(propertyName).getFloatValue(CSSPrimitiveValue.CSS_PX)
> ```

OK!

[Sam Weinig]

Comment on attachment 432650 [details]
Patch

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

>>> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:133
>>> +        const inlineInsideMargin = this.computedValueForStylePropertyInPx("--inline-controls-inside-margin");
>> 
>> Can we maybe cache this value somewhere so that we don't have to get the computed style more than once?  We don't expect this value to change so there's not much benefit to requesting it more than once.
> 
> Good point.

It seems like it will only be called once per InlineMediaControls, so I don't see any reason to cache it unless we see some performance concern.

[Devin Rousso]

Comment on attachment 432650 [details]
Patch

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

>>>> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:133
>>>> +        const inlineInsideMargin = this.computedValueForStylePropertyInPx("--inline-controls-inside-margin");
>>> 
>>> Can we maybe cache this value somewhere so that we don't have to get the computed style more than once?  We don't expect this value to change so there's not much benefit to requesting it more than once.
>> 
>> Good point.
> 
> It seems like it will only be called once per InlineMediaControls, so I don't see any reason to cache it unless we see some performance concern.

This is called in every `layout`, which I think can happen pretty frequently.  Though I think it may depend on the state of the video, page, etc.

[Dean Jackson]

Committed r279481 (239334@main): <https://commits.webkit.org/239334@main>

[Amir Mark Jr]

Reverted r279481 for reason:

Broke a pre-existing test

Committed r279489 (239341@main): <https://commits.webkit.org/239341@main>

[Dean Jackson]

Created attachment 432837 [details]
EWS test

[Dean Jackson]

Committed r279547 (239383@main): <https://commits.webkit.org/239383@main>