[webkit-reviews] review denied: [Bug 200237] Web Inspector: Styles: variable swatch not shown for var() with a fallback : [Attachment 375111] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200237: Web Inspector: Styles: variable swatch not shown for var() with a
fallback
https://bugs.webkit.org/show_bug.cgi?id=200237

Attachment 375111: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375111
  --> https://bugs.webkit.org/attachment.cgi?id=375111
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:98
> +WI.CSSKeywordCompletions.isTimingFunctionAwareProperty = function(name)
> +{
> +    if (name in WI.CSSKeywordCompletions._timingFunctionAwareProperties)
> +	   return true;
> +
> +    let isNotPrefixed = name.charAt(0) !== "-";
> +    if (isNotPrefixed && ("-webkit-" + name) in
WI.CSSKeywordCompletions._timingFunctionAwareProperties)
> +	   return true;

I don't think this works at all. `_timingFunctionAwareProperties` is a Set and
JavaScript's ` in ` syntax does not work with Sets.

They should be like the other code that was updated:

    if (WI.CSSKeywordCompletions._timingFunctionAwareProperties.has(name))
    ...
    if (isNotPrefixed &&
WI.CSSKeywordCompletions._timingFunctionAwareProperties.has("-webkit-" + name))
    ...

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:453
> +	       tokens = this._addVariableTokens(tokens);

Did the order matter? This now creates an inline swatch for the variable
earlier, before colors. I didn't follow this logic all the way to see if that
would be good or bad.