Make CSSCalcValue work with CSSParserTokenRange

Source/core/css/CSSCalculationValue.cpp

Index: Source/core/css/CSSCalculationValue.cpp
diff --git a/Source/core/css/CSSCalculationValue.cpp b/Source/core/css/CSSCalculationValue.cpp
index 00244e63127fe562b6e897f6a193d93d04d52306..f59a86f353a04bfc8726a4c2633943f463715e7e 100644
--- a/Source/core/css/CSSCalculationValue.cpp
+++ b/Source/core/css/CSSCalculationValue.cpp
@@ -592,26 +592,24 @@ private:
     const CalcOperator m_operator;
 };
 
-static ParseState checkDepthAndIndex(int* depth, unsigned index, CSSParserValueList* tokens)
+static ParseState checkDepthAndIndex(int* depth, CSSParserTokenRange tokens)
 {
     (*depth)++;
+    if (tokens.atEnd())
+        return NoMoreTokens;
     if (*depth > maxExpressionDepth)
         return TooDeep;
-    if (index >= tokens->size())
-        return NoMoreTokens;
     return OK;
 }
 
 class CSSCalcExpressionNodeParser {
     STACK_ALLOCATED();
 public:
-    PassRefPtrWillBeRawPtr<CSSCalcExpressionNode> parseCalc(CSSParserValueList* tokens)
+    PassRefPtrWillBeRawPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange tokens)
     {
-        unsigned index = 0;
         Value result;
-        bool ok = parseValueExpression(tokens, 0, &index, &result);
-        ASSERT_WITH_SECURITY_IMPLICATION(index <= tokens->size());
-        if (!ok || index != tokens->size())
+        bool ok = parseValueExpression(tokens, 0, &result);
+        if (!ok || !tokens.atEnd())
             return nullptr;
         return result.value;
     }
@@ -623,69 +621,72 @@ private:
         RefPtrWillBeMember<CSSCalcExpressionNode> value;
     };
 
-    char operatorValue(CSSParserValueList* tokens, unsigned index)
-    {
-        if (index >= tokens->size())
-            return 0;
-        CSSParserValue* value = tokens->valueAt(index);
-        if (value->unit != CSSParserValue::Operator)
-            return 0;
-
-        return value->iValue;
+    char operatorValue(CSSParserTokenRange& tokens)

[Timothy Loh, 2015/07/21 07:33:17]

I don't think this function helps, or at least it isn't obvious that it'll
consume whitespace.

[rwlbuis, 2015/07/22 02:19:06]

Perhaps. I did not want to change the original code too much though to play it
safe.

[Timothy Loh, 2015/07/28 08:17:40]

I think this function makes it hard to verify that whitespace is handled
correctly, I'd still prefer we removed this.

+    {
+        tokens.consumeWhitespace();
+        // Use peek since we do not want to consume in case
+        // the operator is not the expected mult/add operation.
+        CSSParserToken value = tokens.peek();
+        if (value.type() == DelimiterToken)
+            return value.delimiter();
+        if (value.type() == LeftParenthesisToken)
+            return '(';
+        if (value.type() == RightParenthesisToken)
+            return ')';
+        return 0;
     }
 
-    bool parseValue(CSSParserValueList* tokens, unsigned* index, Value* result)
+    bool parseValue(CSSParserTokenRange& tokens, Value* result)
     {
-        CSSParserValue* parserValue = tokens->valueAt(*index);
-        if (parserValue->unit >= CSSParserValue::Operator)
+        CSSParserToken token = tokens.consumeSkippingWhitespace();
+        CSSPrimitiveValue::UnitType type = token.unitType();

[alancutter (OOO until 2018), 2015/07/21 07:44:26]

This line should stay after the token.type() check.

[rwlbuis, 2015/07/21 21:29:52]

Done.

+        if (!(token.type() == NumberToken || token.type() == PercentageToken || token.type() == DimensionToken))
             return false;
 
-        CSSPrimitiveValue::UnitType type = static_cast<CSSPrimitiveValue::UnitType>(parserValue->unit);
         if (unitCategory(type) == CalcOther)
             return false;
 
         result->value = CSSCalcPrimitiveValue::create(
-            CSSPrimitiveValue::create(parserValue->fValue, type), parserValue->isInt);
+            CSSPrimitiveValue::create(token.numericValue(), type), token.numericValueType() == IntegerValueType);
 
-        ++*index;
         return true;
     }
 
-    bool parseValueTerm(CSSParserValueList* tokens, int depth, unsigned* index, Value* result)
+    bool parseValueTerm(CSSParserTokenRange& tokens, int depth, Value* result)
     {
-        if (checkDepthAndIndex(&depth, *index, tokens) != OK)
+        if (checkDepthAndIndex(&depth, tokens) != OK)
             return false;
 
-        if (operatorValue(tokens, *index) == '(') {
-            unsigned currentIndex = *index + 1;
-            if (!parseValueExpression(tokens, depth, &currentIndex, result))
+        if (operatorValue(tokens) == '(') {
+            tokens.consume();
+            if (!parseValueExpression(tokens, depth, result))
                 return false;
 
-            if (operatorValue(tokens, currentIndex) != ')')
+            if (operatorValue(tokens) != ')')
                 return false;
-            *index = currentIndex + 1;
+            tokens.consume();
             return true;
         }
 
-        return parseValue(tokens, index, result);
+        return parseValue(tokens, result);
     }
 
-    bool parseValueMultiplicativeExpression(CSSParserValueList* tokens, int depth, unsigned* index, Value* result)
+    bool parseValueMultiplicativeExpression(CSSParserTokenRange& tokens, int depth, Value* result)
     {
-        if (checkDepthAndIndex(&depth, *index, tokens) != OK)
+        if (checkDepthAndIndex(&depth, tokens) != OK)
             return false;
 
-        if (!parseValueTerm(tokens, depth, index, result))
+        if (!parseValueTerm(tokens, depth, result))
             return false;
 
-        while (*index < tokens->size() - 1) {
-            char operatorCharacter = operatorValue(tokens, *index);
+        while (!tokens.atEnd()) {
+            char operatorCharacter = operatorValue(tokens);
             if (operatorCharacter != CalcMultiply && operatorCharacter != CalcDivide)
                 break;
-            ++*index;
+            tokens.consume();
 
             Value rhs;
-            if (!parseValueTerm(tokens, depth, index, &rhs))
+            if (!parseValueTerm(tokens, depth, &rhs))
                 return false;
 
             result->value = CSSCalcBinaryOperation::createSimplified(result->value, rhs.value, static_cast<CalcOperator>(operatorCharacter));
@@ -693,26 +694,27 @@ private:
                 return false;
         }
 
-        ASSERT_WITH_SECURITY_IMPLICATION(*index <= tokens->size());
         return true;
     }
 
-    bool parseAdditiveValueExpression(CSSParserValueList* tokens, int depth, unsigned* index, Value* result)
+    bool parseAdditiveValueExpression(CSSParserTokenRange& tokens, int depth, Value* result)
     {
-        if (checkDepthAndIndex(&depth, *index, tokens) != OK)
+        if (checkDepthAndIndex(&depth, tokens) != OK)
             return false;
 
-        if (!parseValueMultiplicativeExpression(tokens, depth, index, result))
+        if (!parseValueMultiplicativeExpression(tokens, depth, result))
             return false;
 
-        while (*index < tokens->size() - 1) {
-            char operatorCharacter = operatorValue(tokens, *index);
+        while (!tokens.atEnd()) {
+            char operatorCharacter = operatorValue(tokens);
             if (operatorCharacter != CalcAdd && operatorCharacter != CalcSubtract)
                 break;
-            ++*index;
+            if (operatorCharacter == CalcAdd && (&tokens.peek() - 1)->type() != WhitespaceToken)
+                return false; // calc(1px+ 2px) is invalid

[Timothy Loh, 2015/07/21 07:33:17]

Probably need to check the other side too, calc(1px +/**/2px) is also invalid...

[rwlbuis, 2015/07/22 02:19:06]

I think it works, I found a similar test in calc-errors.html(?).

[Timothy Loh, 2015/07/28 08:17:40]

I didn't see any such tests. You need to check the previous and next tokens for
whitespace for both add and subtract, and add tests for the various permutations
of whitespace/comments, e.g. calc(1px/**/- 2px), calc(1px -/**/2px) and so on.

+            tokens.consume();
 
             Value rhs;
-            if (!parseValueMultiplicativeExpression(tokens, depth, index, &rhs))
+            if (!parseValueMultiplicativeExpression(tokens, depth, &rhs))
                 return false;
 
             result->value = CSSCalcBinaryOperation::createSimplified(result->value, rhs.value, static_cast<CalcOperator>(operatorCharacter));
@@ -720,13 +722,12 @@ private:
                 return false;
         }
 
-        ASSERT_WITH_SECURITY_IMPLICATION(*index <= tokens->size());
         return true;
     }
 
-    bool parseValueExpression(CSSParserValueList* tokens, int depth, unsigned* index, Value* result)
+    bool parseValueExpression(CSSParserTokenRange& tokens, int depth, Value* result)
     {
-        return parseAdditiveValueExpression(tokens, depth, index, result);
+        return parseAdditiveValueExpression(tokens, depth, result);
     }
 };
 
@@ -748,7 +749,7 @@ PassRefPtrWillBeRawPtr<CSSCalcExpressionNode> CSSCalcValue::createExpressionNode
         CalcAdd);
 }
 
-PassRefPtrWillBeRawPtr<CSSCalcValue> CSSCalcValue::create(CSSParserValueList* parserValueList, ValueRange range)
+PassRefPtrWillBeRawPtr<CSSCalcValue> CSSCalcValue::create(const CSSParserTokenRange& parserValueList, ValueRange range)

[alancutter (OOO until 2018), 2015/07/21 07:44:26]

s/parserValueList/tokens/

[rwlbuis, 2015/07/21 21:29:52]

Done.

 {
     CSSCalcExpressionNodeParser parser;
     RefPtrWillBeRawPtr<CSSCalcExpressionNode> expression = parser.parseCalc(parserValueList);