[webkit-reviews] review granted: [Bug 193615] Web Inspector: Styles: refactor properties/allProperties/visibleProperties/allVisibleProperties : [Attachment 359626] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 193615: Web Inspector: Styles: refactor
properties/allProperties/visibleProperties/allVisibleProperties
https://bugs.webkit.org/show_bug.cgi?id=193615

Attachment 359626: Patch

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 359626
  --> https://bugs.webkit.org/attachment.cgi?id=359626
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.