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`.
Created attachment 359626 [details] Patch
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
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 on attachment 359626 [details] Patch Unrelated test failures.
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 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.
Created attachment 359817 [details] Patch
Comment on attachment 359817 [details] Patch Clearing flags on attachment: 359817 Committed r240314: <https://trac.webkit.org/changeset/240314>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47467443>