Add property parser code path based on CSSParserTokenRange

Source/core/css/parser/CSSPropertyParser.cpp

Index: Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/Source/core/css/parser/CSSPropertyParser.cpp b/Source/core/css/parser/CSSPropertyParser.cpp
index 5c88c54ae81208312709032cb2c521cefb9ec113..bad0867e2638997131556c8ff4ff1e33e1ecc7ff 100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -79,6 +79,7 @@ CSSPropertyParser::CSSPropertyParser(CSSParserValueList* valueList,
     const CSSParserContext& context, WillBeHeapVector<CSSProperty, 256>& parsedProperties,
     StyleRule::Type ruleType)
     : m_valueList(valueList)
+    , m_range(nullptr)
     , m_context(context)
     , m_parsedProperties(parsedProperties)
     , m_ruleType(ruleType)
@@ -89,12 +90,13 @@ CSSPropertyParser::CSSPropertyParser(CSSParserValueList* valueList,
 }
 
 bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool important,
-    CSSParserValueList* valueList, const CSSParserContext& context,
+    CSSParserValueList* valueList, CSSParserTokenRange* range, const CSSParserContext& context,

[Timothy Loh, 2015/09/03 02:05:53]

Having the token range as a pointer is odd, I'd just pass it in by value (it's
only two pointers) and store it by value.

Also it would be nice to remove the CSSParserValueList argument and do the
conversion here instead of CSSParserImpl (this would make CSSParserValues.h only
included here after 1317743006)

[rwlbuis, 2015/09/03 22:27:54]

Done.

     WillBeHeapVector<CSSProperty, 256>& parsedProperties, StyleRule::Type ruleType)
 {
     int parsedPropertiesSize = parsedProperties.size();
 
     CSSPropertyParser parser(valueList, context, parsedProperties, ruleType);
+    parser.m_range = range;
     CSSPropertyID resolvedProperty = resolveCSSPropertyID(unresolvedProperty);
     bool parseSuccess;
 
@@ -336,6 +338,15 @@ static bool consumeComma(CSSParserValueList* valueList)
     return true;
 }
 
+static bool consumeCommaIncludingWhitespace(CSSParserTokenRange& valueList)
+{
+    CSSParserToken value = valueList.peek();
+    if (value.type() != CommaToken)
+        return false;
+    valueList.consumeIncludingWhitespace();
+    return true;
+}
+
 static inline bool isForwardSlashOperator(CSSParserValue* value)
 {
     ASSERT(value);
@@ -432,6 +443,18 @@ void CSSPropertyParser::addExpandedPropertyForValue(CSSPropertyID propId, PassRe
         addProperty(longhands[i], value, important);
 }
 
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID propId)
+{
+    m_range->consumeWhitespace();
+    switch (propId) {
+    case CSSPropertyWillChange:
+        return parseWillChange();
+    default:
+        ASSERT_NOT_REACHED();
+    }
+    return nullptr;
+}
+
 bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool important)
 {
     CSSPropertyID propId = resolveCSSPropertyID(unresolvedProperty);
@@ -470,9 +493,17 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
         return true;
     }
 
+    RefPtrWillBeRawPtr<CSSValue> parsedValue = nullptr;
+    if (propId == CSSPropertyWillChange) {
+        parsedValue = parseSingleValue(propId);
+        if (!parsedValue || !m_range->atEnd())
+            return false;
+        addProperty(propId, parsedValue.release(), important);
+        return true;
+    }
+
     bool validPrimitive = false;
     Units unitless = FUnknown;
-    RefPtrWillBeRawPtr<CSSValue> parsedValue = nullptr;
 
     switch (propId) {
     case CSSPropertySize: // <length>{1,2} | auto | [ <page-size> || [ portrait | landscape] ]
@@ -1317,9 +1348,6 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
     case CSSPropertyWebkitColumnWidth:         // auto | <length>
         parsedValue = parseColumnWidth();
         break;
-    case CSSPropertyWillChange:
-        parsedValue = parseWillChange();
-        break;
     // End of CSS3 properties
 
     // Apple specific properties.  These will never be standardized and are purely to
@@ -6839,7 +6867,8 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseImageSet(CSSParserValue
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseWillChange()
 {
     RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createCommaSeparated();
-    if (m_valueList->current()->id == CSSValueAuto) {
+    CSSParserToken currentToken = m_range->consumeIncludingWhitespace();
+    if (currentToken.id() == CSSValueAuto) {
         // FIXME: This will be read back as an empty string instead of auto
         return values.release();
     }
@@ -6847,11 +6876,10 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseWillChange()
     // Every comma-separated list of identifiers is a valid will-change value,

[Timothy Loh, 2015/09/03 02:05:53]

Heh, the function doesn't actually do this...

[Timothy Loh, 2015/09/03 02:06:58]

That said, let's try and keep behaviour changes separate from moving properties
away from CSSParserValues.

[rwlbuis, 2015/09/03 22:27:54]

I agree. Sadly I don't think this is the only case of a comment not matching
what is happening...

     // unless the list includes an explicitly disallowed identifier.
     while (true) {
-        CSSParserValue* currentValue = m_valueList->current();
-        if (!currentValue || currentValue->m_unit != CSSParserValue::Identifier)
+        if (currentToken.type() != IdentToken)
             return nullptr;
 
-        CSSPropertyID unresolvedProperty = unresolvedCSSPropertyID(currentValue->string);
+        CSSPropertyID unresolvedProperty = unresolvedCSSPropertyID(currentToken.value());
         if (unresolvedProperty) {
             ASSERT(CSSPropertyMetadata::isEnabledProperty(unresolvedProperty));
             // Now "all" is used by both CSSValue and CSSPropertyValue.
@@ -6860,7 +6888,7 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseWillChange()
                 return nullptr;
             values->append(cssValuePool().createIdentifierValue(unresolvedProperty));
         } else {
-            switch (currentValue->id) {
+            switch (currentToken.id()) {
             case CSSValueNone:
             case CSSValueAll:
             case CSSValueAuto:
@@ -6870,17 +6898,18 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseWillChange()
                 return nullptr;
             case CSSValueContents:
             case CSSValueScrollPosition:
-                values->append(cssValuePool().createIdentifierValue(currentValue->id));
+                values->append(cssValuePool().createIdentifierValue(currentToken.id()));

[Timothy Loh, 2015/09/03 02:05:53]

Maybe we should have something like:

static consumeIdent(CSSParserValueRange& range) {
    ASSERT(range.peek().type() == IdentToken);
    return
cssValuePool().createIdentifierValue(range.consumeIncludingWhitespace().id());
}

WDYT? Consuming early like we're doing here seems a bit strange to me.

[rwlbuis, 2015/09/03 22:27:54]

Sounds good, I did that. A bit sad that we need consumeIncludingWhitespace in
two other places.

[Timothy Loh, 2015/09/04 01:24:40]

Yeah, although I think it's more indicative that the code is doing something
weird. At least the first call should probably change to consumeIdent to fix the
FIXME. I suspect most other properties won't need consumeIncludingWhitespace all
over the place.

                 break;
             default:
                 break;
             }
         }
 
-        if (!m_valueList->next())
+        if (m_range->atEnd())
             break;
-        if (!consumeComma(m_valueList))
+        if (!consumeCommaIncludingWhitespace(*m_range))
             return nullptr;
+        currentToken = m_range->consumeIncludingWhitespace();
     }
 
     return values.release();