[webkit-reviews] review granted: [Bug 178354] Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget : [Attachment 323974] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 16 21:51:31 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178354: Web Inspector: [PARITY] Styles Redesign: Add color picker inline
widget
https://bugs.webkit.org/show_bug.cgi?id=178354

Attachment 323974: Patch

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




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

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

r=me, my comments are just style and nits.

> Source/WebInspectorUI/UserInterface/Models/Color.js:645
> +    "hsla"

Style: Include the trailing comma. It makes future diffs nicer. We might add a
color() for p3 later.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:285
> +	       let colorTokenElement = document.createElement("span");
> +	       colorTokenElement.classList.add("token-color");
> +
> +	       let innerElement = document.createElement("span");
> +	       innerElement.classList.add("token-color-value");

Style: For elements we create where we are setting the class names ourselves I
think we should use `elem.className = x` instead of `elem.classList.add(x)`. So
here:

    colorTokenElement.className = "token-color";
    ...
    innerElement.className = "token-color-value";

In practice it probably doesn't matter, but it is slightly faster and easier to
reason about (not having to worry about other potential class names).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:296
> +		       let swatch = event && event.target;
> +		       console.assert(swatch, "Color swatch is missing.");
> +		       if (!swatch)
> +			   return;

This is not possible. If the event happens `event` and `event.target` cannot be
missing.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:310
> +		   swatch.element.addEventListener("mousedown", (event) =>
event.stop());

Style: We prefer using braces with arrow functions if the return value is
unimportant / unused. So:

    swatch.element.addEventListener("mousedown", (event) => { event.stop(); });

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:351
> +		   let possiblyColor =
this._stringFromTokens(tokens.slice(colorFunctionStartIndex, i + 1));
> +		   let color = WI.Color.fromString(possiblyColor);
> +		   if (color)
> +		       newTokens.push(createColorTokenElement(possiblyColor,
color));
> +		   else
> +		       newTokens.push(...tokens.slice(colorFunctionStartIndex,
i + 1));

Nit: `possiblyColor` => `possibleColor`
Nit: You should stash the slice into a local to avoid re-computing it later if
the fromString fails:

    let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
    let possibleColor = this._stringFromTokens(rawTokens);
    let color = WI.Color.fromString(possibleColor);
    if (color)
	newTokens.push(createColorTokenElement(possibleColor, color));
    else
	newTokens.push(...rawTokens);

Finally, in general these blocks can share some code:

    function pushPossibleColorToken(text, ...tokens) {
	let color = WI.Color.fromString(text);
	if (color)
	    newTokens.push(createColorTokenElement(text, color);
	else
	    newTokens.push(...tokens);
    }

    for (let i = 0; i < tokens.length; ++i) {
	let token = tokens[i];
	if (...) {
	    // Hex
	    pushPossibleColorToken(token.value, token);
	} else if (...) {
	    // Color Function start
	    ...
	} else if (...) {
	    // Nickname
	    pushPossibleColorToken(token.value, token);
	} else if (...) {
	    // Color Function end
	    let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
	    let text = this._stringFromTokens(rawTokens);
	    pushPossibleColorToken(text, rawTokens);
	    colorFunctionStartIndex = NaN;
	}
    }

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:366
> +	   return tokens.map((token) => {
> +	       return token.value;
> +	   }).join("");

Nit: Simplify:

    return tokens.map((token) => token.value).join("");


More information about the webkit-reviews mailing list