[webkit-reviews] review denied: [Bug 178404] Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier curve, and spring inline widgets : [Attachment 324071] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 18 12:02:38 PDT 2017


Devin Rousso <webkit at devinrousso.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178404: Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier
curve, and spring inline widgets
https://bugs.webkit.org/show_bug.cgi?id=178404

Attachment 324071: Patch

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




--- Comment #3 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 324071
  --> https://bugs.webkit.org/attachment.cgi?id=324071
Patch

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

r- for the issue with nested var()

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:338
> +		   let text = rawTokens.map((token) => token.value).join("");
> +
> +		   let gradient = WI.Gradient.fromString(text);

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
> +	       } else if (!isNaN(cubicBezierStartIndex) && token.value === ")")
{

Simply checking for ")" is not valid, as it is possible to nest `var()` inside
`cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")"
and only perform this logic once they are both even.  Gradient already has this
logic.

This will also be the case for Color and Spring.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:422
> +    _addSpringTokens(tokens)

The logic in `_addSpringTokens` is almost identical to `_addCubicBezierTokens`,
with the exception of:

    (429): token.value === "spring"
    (437): let spring = WI.Spring.fromString(text);
    (439): newTokens.push(this._createInlineSwatch(WI.InlineSwatch.Type.Spring,
text, spring));

Considering that CubicBezier and Spring are both used for the same CSS
properties, can you combine them and avoid this logic?


More information about the webkit-reviews mailing list