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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 29 11:02:33 PDT 2012


Tony Chang <tony at chromium.org> has denied 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 161133: Patch
https://bugs.webkit.org/attachment.cgi?id=161133&action=review

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


> Source/WebCore/ChangeLog:19
> +	   (WebCore::CSSCalcValue::customSerializeResolvingVariables):
> +	   (WebCore::CSSCalcValue::hasVariableReference):
> +	   (CSSCalcPrimitiveValue):
> +	   (WebCore::CSSCalcPrimitiveValue::serializeResolvingVariables):

Please fill in this section of the ChangeLog.

> Source/WebCore/css/CSSCalculationValue.cpp:106
> +    bool expressionHasSingleTerm = expression[0] != '(';
> +    if (expressionHasSingleTerm)
> +	   result.append('(');
> +    result.append(expression);
> +    if (expressionHasSingleTerm)

This code seems to be duplicated with customCSSText.  Can you make a helper
function that takes |expression| as an input param?

> Source/WebCore/css/CSSCalculationValue.cpp:271
> +	   else
> +#endif
>	   switch (op) {

This is hard to follow since the else clause is long and not indented. Maybe
make a new method that returns newCategory (so the variables code would just be
an early return)?

> LayoutTests/fast/css/variables/calc.html:8
> +  border-width: -webkit-calc(-webkit-var(a) + -webkit-var(b))
-webkit-calc(-webkit-var(c) * -webkit-var(d));

Huh, the -webkit makes it look like it's a negative value.  Just for
completeness, can you add a test case with --webkit-var to see if it negates
it?  Also, can you add some test cases for when variables are not defined or
are invalid values?


More information about the webkit-reviews mailing list