Bug 227505 - Move BottomControlsBarHeight and InsideMargin to be computed at runtime
Summary: Move BottomControlsBarHeight and InsideMargin to be computed at runtime
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-29 13:59 PDT by Dean Jackson
Modified: 2021-07-03 14:29 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.37 KB, patch)
2021-06-29 14:05 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (5.28 KB, patch)
2021-06-29 16:18 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2021-06-30 19:21 PDT, Dean Jackson
drousso: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS test (12.37 KB, patch)
2021-07-02 18:14 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-06-29 13:59:50 PDT
Move BottomControlsBarHeight to a layout trait
Comment 1 Radar WebKit Bug Importer 2021-06-29 14:02:12 PDT
<rdar://problem/79932256>
Comment 2 Dean Jackson 2021-06-29 14:05:15 PDT
Created attachment 432525 [details]
Patch
Comment 3 Sam Weinig 2021-06-29 16:06:29 PDT
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?
Comment 4 Dean Jackson 2021-06-29 16:18:27 PDT
Created attachment 432550 [details]
Patch
Comment 5 Dean Jackson 2021-06-30 19:21:58 PDT
Created attachment 432650 [details]
Patch
Comment 6 Devin Rousso 2021-06-30 23:48:49 PDT
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)
Comment 7 Dean Jackson 2021-07-01 12:28:47 PDT
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!
Comment 8 Sam Weinig 2021-07-01 12:34:12 PDT
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.
Comment 9 Devin Rousso 2021-07-01 13:01:17 PDT
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.
Comment 10 Dean Jackson 2021-07-01 13:28:57 PDT
Committed r279481 (239334@main): <https://commits.webkit.org/239334@main>
Comment 11 Amir Mark Jr 2021-07-01 17:22:38 PDT
Reverted r279481 for reason:

Broke a pre-existing test

Committed r279489 (239341@main): <https://commits.webkit.org/239341@main>
Comment 12 Dean Jackson 2021-07-02 18:14:18 PDT
Created attachment 432837 [details]
EWS test
Comment 13 Dean Jackson 2021-07-03 14:29:41 PDT
Committed r279547 (239383@main): <https://commits.webkit.org/239383@main>