[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