.
Created attachment 456340 [details] Patch
Ping reviewers.
Comment on attachment 456340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456340&action=review Can you please add a comment in the ChangeLog explaining why this is desired/necessary? > Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:65 > + page.corePage()->settings().setCSSTypedOMEnabled(true); Since this is not something enabled by default, we should ensure that the frontend doesn't break if this is disabled. Could we create a helper function that still uses the old behavior if this isn't enabled? Something like: ``` Object.defineProperty(Element.prototype, "getComputedCSSPropertyNumberValue", { value(property) { let result = undefined; result ??= this.computedStyleMap?.().get(property)?.value; result ??= window.getComputedStyle(this).getPropertyCSSValue(property)?.getFloatValue(CSSPrimitiveValue.CSS_PX); return result; }, }); ```
Comment on attachment 456340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456340&action=review >> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:65 >> + page.corePage()->settings().setCSSTypedOMEnabled(true); > > Since this is not something enabled by default, we should ensure that the frontend doesn't break if this is disabled. Could we create a helper function that still uses the old behavior if this isn't enabled? Something like: > ``` > Object.defineProperty(Element.prototype, "getComputedCSSPropertyNumberValue", { > value(property) { > let result = undefined; > result ??= this.computedStyleMap?.().get(property)?.value; > result ??= window.getComputedStyle(this).getPropertyCSSValue(property)?.getFloatValue(CSSPrimitiveValue.CSS_PX); > return result; > }, > }); > ``` Doesn't the setting ensure it's always enabled in the inspector frontend? or do you mean adding the helper instead of changing the setting?
Comment on attachment 456340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456340&action=review >>> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:65 >>> + page.corePage()->settings().setCSSTypedOMEnabled(true); >> >> Since this is not something enabled by default, we should ensure that the frontend doesn't break if this is disabled. Could we create a helper function that still uses the old behavior if this isn't enabled? Something like: >> ``` >> Object.defineProperty(Element.prototype, "getComputedCSSPropertyNumberValue", { >> value(property) { >> let result = undefined; >> result ??= this.computedStyleMap?.().get(property)?.value; >> result ??= window.getComputedStyle(this).getPropertyCSSValue(property)?.getFloatValue(CSSPrimitiveValue.CSS_PX); >> return result; >> }, >> }); >> ``` > > Doesn't the setting ensure it's always enabled in the inspector frontend? or do you mean adding the helper instead of changing the setting? Oh sorry I wasn't entirely clear. This relies on *both* the setting and `ENABLE_CSS_TYPED_OM`. I was more concerned with about if `ENABLE_CSS_TYPED_OM` is not enabled (for whatever reason). If/When the build flag is removed, then we can remove the fallback code.
(In reply to Devin Rousso from comment #5) > Comment on attachment 456340 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456340&action=review > > >>> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:65 > >>> + page.corePage()->settings().setCSSTypedOMEnabled(true); > >> > >> Since this is not something enabled by default, we should ensure that the frontend doesn't break if this is disabled. Could we create a helper function that still uses the old behavior if this isn't enabled? Something like: > >> ``` > >> Object.defineProperty(Element.prototype, "getComputedCSSPropertyNumberValue", { > >> value(property) { > >> let result = undefined; > >> result ??= this.computedStyleMap?.().get(property)?.value; > >> result ??= window.getComputedStyle(this).getPropertyCSSValue(property)?.getFloatValue(CSSPrimitiveValue.CSS_PX); > >> return result; > >> }, > >> }); > >> ``` > > > > Doesn't the setting ensure it's always enabled in the inspector frontend? or do you mean adding the helper instead of changing the setting? > > Oh sorry I wasn't entirely clear. This relies on *both* the setting and > `ENABLE_CSS_TYPED_OM`. I was more concerned with about if > `ENABLE_CSS_TYPED_OM` is not enabled (for whatever reason). If/When the > build flag is removed, then we can remove the fallback code. I'm not familiar with xcode build system, but I think CSS_TYPED_OM is enabled unconditionally in PlatformEnableCocoa.h (or can that be overridden with xcode magic?). And I enabled it by default for GTK and WPE ports too.
Comment on attachment 456340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456340&action=review >>>>> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:65 >>>>> + page.corePage()->settings().setCSSTypedOMEnabled(true); >>>> >>>> Since this is not something enabled by default, we should ensure that the frontend doesn't break if this is disabled. Could we create a helper function that still uses the old behavior if this isn't enabled? Something like: >>>> ``` >>>> Object.defineProperty(Element.prototype, "getComputedCSSPropertyNumberValue", { >>>> value(property) { >>>> let result = undefined; >>>> result ??= this.computedStyleMap?.().get(property)?.value; >>>> result ??= window.getComputedStyle(this).getPropertyCSSValue(property)?.getFloatValue(CSSPrimitiveValue.CSS_PX); >>>> return result; >>>> }, >>>> }); >>>> ``` >>> >>> Doesn't the setting ensure it's always enabled in the inspector frontend? or do you mean adding the helper instead of changing the setting? >> >> Oh sorry I wasn't entirely clear. This relies on *both* the setting and `ENABLE_CSS_TYPED_OM`. I was more concerned with about if `ENABLE_CSS_TYPED_OM` is not enabled (for whatever reason). If/When the build flag is removed, then we can remove the fallback code. > > I'm not familiar with xcode build system, but I think CSS_TYPED_OM is enabled unconditionally in PlatformEnableCocoa.h (or can that be overridden with xcode magic?). And I enabled it by default for GTK and WPE ports too. Yes, but on cocoa platforms that's predicated by `#if !defined(ENABLE_CSS_TYPED_OM)`. I admit it's probably unlikely that some build system would override this, but in the off chance that something does I'd hate for Web Inspector to be broken in that situation. We do something similar for other things that are guarded by flags (e.g. `WebGLRenderingContext`, `indexedDB`, etc.), just in case :)
Created attachment 456910 [details] Patch
Comment on attachment 456910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456910&action=review r=me, thanks! > Source/WebInspectorUI/ChangeLog:8 > + * UserInterface/Base/Utilities.js: Can you add some explanation as to why this change is needed/desired? Would be helpful for future engineers who may encounter this change and wonder why it was made :)
<rdar://problem/91472876>
Committed r292597 (249430@trunk): <https://commits.webkit.org/249430@trunk>