Bug 193615 - Web Inspector: Styles: refactor properties/allProperties/visibleProperties/allVisibleProperties
Summary: Web Inspector: Styles: refactor properties/allProperties/visibleProperties/al...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-19 17:43 PST by Nikita Vasilyev
Modified: 2019-01-22 17:47 PST (History)
5 users (show)

See Also:


Attachments
Patch (27.36 KB, patch)
2019-01-19 17:51 PST, Nikita Vasilyev
drousso: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.44 MB, application/zip)
2019-01-19 19:45 PST, EWS Watchlist
no flags Details
Patch (27.13 KB, patch)
2019-01-22 17:17 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-01-19 17:43:55 PST
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`.
Comment 1 Nikita Vasilyev 2019-01-19 17:51:12 PST
Created attachment 359626 [details]
Patch
Comment 2 EWS Watchlist 2019-01-19 19:45:52 PST
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
Comment 3 EWS Watchlist 2019-01-19 19:45:54 PST
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
Comment 4 Nikita Vasilyev 2019-01-19 20:02:29 PST
Comment on attachment 359626 [details]
Patch

Unrelated test failures.
Comment 5 Devin Rousso 2019-01-22 16:43:56 PST
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.
Comment 6 Nikita Vasilyev 2019-01-22 16:55:14 PST
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.
Comment 7 Nikita Vasilyev 2019-01-22 17:17:28 PST
Created attachment 359817 [details]
Patch
Comment 8 WebKit Commit Bot 2019-01-22 17:46:07 PST
Comment on attachment 359817 [details]
Patch

Clearing flags on attachment: 359817

Committed r240314: <https://trac.webkit.org/changeset/240314>
Comment 9 WebKit Commit Bot 2019-01-22 17:46:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-01-22 17:47:31 PST
<rdar://problem/47467443>