[webkit-reviews] review granted: [Bug 233372] Web Inspector: Add CSS variable names to property name completion list : [Attachment 446782] Patch 1.3

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 233372: Web Inspector: Add CSS variable names to property name completion
list
https://bugs.webkit.org/show_bug.cgi?id=233372

Attachment 446782: Patch 1.3

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




--- Comment #15 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 446782
  --> https://bugs.webkit.org/attachment.cgi?id=446782
Patch 1.3

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:179
> +	   console.assert(Array.isArray(values), values);

NIT: Do we also want the other assertion in the `constructor` too?  In fact,
you could just call this in the `constructor` and avoid some repeated code :)

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:182
> +	   this._queryController?.reset();

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:60
> +	   return super.executeQuery(query);

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:66
> +	   return super.startsWith(prefix);

ditto (:60)

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:71
> +	   throw new Error("Adding values will overwrite the list of supported
CSS property names");

NIT: I'd just make this into a `console.assert(false, "Adding values will
overwrite the list of supported CSS property names")` so that this doesn't
cause issues in production.  You've already made sure it's not an issue by NOT
calling `super.addValues(...)`.

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:91
> +	   let variables = Array.from(nodeStyles.allCSSVariables);
> +	   let values = variables;

NIT: I'd just do `let values = Array.from(nodeStyles.allCSSVariables);` and
avoid the extra `variables` variable that's not used otherwise.

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:93
> +

Style: I'd remove this newline

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:95
> +	   this._needsVariablesFromInspectedNode = false;

Style: I'd add a newline before this