[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