Make it possible to use CSS Variables inside Calc expressions.
Created attachment 161133 [details] Patch
Comment on attachment 161133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161133&action=review Shadow review. lgtm, please consider nits though > Source/WebCore/css/CSSCalculationValue.cpp:111 > +bool CSSCalcValue::hasVariableReference() const suggestion: could this be 'hasVariables' or 'containsVariables' or 'hasCssVariableReference'? 'hasVariableReference' could be misinterpreted as 'has a reference, and it is variable/mutable', rather than 'there are CSS variables here'. > Source/WebCore/css/CSSCalculationValue.h:108 > + String customSerializeResolvingVariables(const HashMap<AtomicString, String>&) const; Here it is 'customSerializeResolvingVariables', while in CalcExpressionNode it is 'serializeResolvingVariables'. Other methods like 'customCssText' are the same in both - I don't understand why these are different? > Source/WebCore/css/CSSPrimitiveValue.cpp:191 > + return CSSPrimitiveValue::CSS_UNKNOWN; // The type of an calculation containing a variable cannot be known until the value of the variable is determined. typo: 'an caculation' -> 'a calculation'
Comment on attachment 161133 [details] Patch 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?
(In reply to comment #2) > (From update of attachment 161133 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161133&action=review > > Shadow review. > > lgtm, please consider nits though > > > Source/WebCore/css/CSSCalculationValue.cpp:111 > > +bool CSSCalcValue::hasVariableReference() const > > suggestion: could this be 'hasVariables' or 'containsVariables' or 'hasCssVariableReference'? 'hasVariableReference' could be misinterpreted as 'has a reference, and it is variable/mutable', rather than 'there are CSS variables here'. I see your point, but hasVariableReference is already the name used everywhere else in the CSSValue hierarchy. I can change that, but it would be a job for a different patch. > > Source/WebCore/css/CSSCalculationValue.h:108 > > + String customSerializeResolvingVariables(const HashMap<AtomicString, String>&) const; > > Here it is 'customSerializeResolvingVariables', while in CalcExpressionNode it is 'serializeResolvingVariables'. Other methods like 'customCssText' are the same in both - I don't understand why these are different? custom* is used because CSSValue does manual devirtualization, so a different function name is needed. Because CalcExpressionNode does use virtual functions this is not necessary in that case. If in a future patch CSSCalculationValue is changed not to extend CSSValue we can drop the "custom" prefix. > > Source/WebCore/css/CSSPrimitiveValue.cpp:191 > > + return CSSPrimitiveValue::CSS_UNKNOWN; // The type of an calculation containing a variable cannot be known until the value of the variable is determined. > > typo: 'an caculation' -> 'a calculation' Heh, your typo there confused me for a second. s/an/a done.
Created attachment 161374 [details] Patch
(In reply to comment #3) > 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? See: http://www.w3.org/TR/css3-values/#calc - as I understand it (from Mike's verbal description) css calc expressions do not support unary negation, although you can write 0 - x to achieve the same result.
(In reply to comment #6) > (In reply to comment #3) > > 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? > > See: http://www.w3.org/TR/css3-values/#calc - as I understand it (from Mike's verbal description) css calc expressions do not support unary negation, although you can write 0 - x to achieve the same result. The gotcha is that you can't negate a parenthesised expression - ie calc(-(10px * 2)) isn't allowed. You need to do calc(-1 * (10px * 2)) or calc(0 - (10px * 2)).
Created attachment 161392 [details] Patch
Comment on attachment 161392 [details] Patch 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.
Created attachment 161597 [details] Patch for landing
Comment on attachment 161597 [details] Patch for landing Clearing flags on attachment: 161597 Committed r127220: <http://trac.webkit.org/changeset/127220>
All reviewed patches have been landed. Closing bug.