Bug 238661 – Web Inspector: Use computedStyleMap() instead of deprecated getPropertyCSSValue()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Carlos Garcia Campos]

.

[Carlos Garcia Campos]

Created attachment 456340 [details]
Patch

[Carlos Garcia Campos]

Ping reviewers.

[Devin Rousso]

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;   
    },
});
```

[Carlos Garcia Campos]

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?

[Devin Rousso]

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.

[Carlos Garcia Campos]

(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.

[Devin Rousso]

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 :)

[Carlos Garcia Campos]

Created attachment 456910 [details]
Patch

[Devin Rousso]

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 :)

[Radar WebKit Bug Importer]

<rdar://problem/91472876>

[Carlos Garcia Campos]

Committed r292597 (249430@trunk): <https://commits.webkit.org/249430@trunk>