Bug 193615 – Web Inspector: Styles: refactor properties/allProperties/visibleProperties/allVisibleProperties

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

[Nikita Vasilyev]

1. properties vs allProperties

`allProperties` is an array of both enabled and disabled properties.
`properties` is an array of only enabled properties:

    this._properties = properties.filter((property) => property.enabled);

I find myself forgetting the difference between the two every 12 months. I suggest to rename:

    properties -> enabledProperties
    allProperties -> properties


2. visibleProperties vs allVisibleProperties

`visibleProperties` is unused and should be removed.
`allVisibleProperties` should be renamed to `visibleProperties`.

[Nikita Vasilyev]

Created attachment 359626 [details]
Patch

[EWS Watchlist]

Comment on attachment 359626 [details]
Patch

Attachment 359626 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10815019

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 359630 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

Comment on attachment 359626 [details]
Patch

Unrelated test failures.

[Devin Rousso]

Comment on attachment 359626 [details]
Patch

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

r=me, nice refactor

Personally, given the fact that any given `WI.CSSStyleDeclaration` is unlikely to have a significantly large number of `WI.CSSProperty`, I'd also be fine with inlining the logic to create `this._enabledProperties` and `this._visibleProperties` inside their respective getters (e.g. don't save the array operation result to a member variable and recalculate it each time, expecting the callers to cache the result).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:47
> +        this._properties = [];

NIT: `this._enabledProperties = [];` should also be defined here

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:118
> +        this._enabledProperties = properties.filter((property) => property.enabled);
> +        this._properties = properties;

NIT: `this._properties` should come before `this._enabledProperties`.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:224
> +        return this._enabledProperties;

NIT: I think this should be a lazy creation (like `get visibleProperties`).  I realize that `this._enabledProperties` is most likely used shortly after `this._properties` is set`, but that isn't always the case (and may not be in the future), and we should try to be more efficient/lazy where we can.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:308
> +    hasEnabledProperties()

NIT: I realize that this is a refactoring change, but I personally don't find much use in functions like this.  I think it'd be easier/simpler to have the caller do `style.enabledProperties.length`.  It's really a not-so-convenient convenience.

[Nikita Vasilyev]

Comment on attachment 359626 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:224
>> +        return this._enabledProperties;
> 
> NIT: I think this should be a lazy creation (like `get visibleProperties`).  I realize that `this._enabledProperties` is most likely used shortly after `this._properties` is set`, but that isn't always the case (and may not be in the future), and we should try to be more efficient/lazy where we can.

I didn't do lazy creation because enabledProperties are used in `update` method.

[Nikita Vasilyev]

Created attachment 359817 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 359817 [details]
Patch

Clearing flags on attachment: 359817

Committed r240314: <https://trac.webkit.org/changeset/240314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47467443>