[webkit-reviews] review granted: [Bug 95284] Make it possible to use CSS Variables inside Calc expressions. : [Attachment 161392] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 95284: Make it possible to use CSS Variables inside Calc expressions.
https://bugs.webkit.org/show_bug.cgi?id=95284

Attachment 161392: Patch
https://bugs.webkit.org/attachment.cgi?id=161392&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161392&action=review


The change looks fine, but please add more test cases before landing.  There
should be test cases for:
1) Referencing a variable that doesn't exist.
2) Referencing a variable that is not a valid value (e.g., has a string value).

3) --webkit-var() just to codify the behavior.	It's fine that that fails, but
we should still have a test for it.
4) Maybe a test case where the variable itself is a calc expression?  Also
doesn't have to work, but would be nice to have a test case.

> Source/WebCore/css/CSSCalculationValue.cpp:80
> +static String buildCssText(String expression)

Nit: const String& for the input param.  It saves a bit of internal ref
counting.

> Source/WebCore/css/CSSCalculationValue.cpp:330
> +    static String buildCssText(String leftExpression, String
rightExpression, CalcOperator op)

Nit: const String& for input params.