Move BottomControlsBarHeight to a layout trait
<rdar://problem/79932256>
Created attachment 432525 [details] Patch
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?
Created attachment 432550 [details] Patch
Created attachment 432650 [details] Patch
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 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 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 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.
Committed r279481 (239334@main): <https://commits.webkit.org/239334@main>
Reverted r279481 for reason: Broke a pre-existing test Committed r279489 (239341@main): <https://commits.webkit.org/239341@main>
Created attachment 432837 [details] EWS test
Committed r279547 (239383@main): <https://commits.webkit.org/239383@main>