Add property parser code path based on CSSParserTokenRange

Source/core/css/parser/CSSPropertyParser.cpp

 /*
  * Copyright (C) 2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Nicholas Shanks <webkit@nickshanks.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
  * Copyright (C) 2012 Intel Corporation. All rights reserved.
  *
@@ skipping 61 matching lines @@
     if (a.length() != length)
         return false;
 
     return a.is8Bit() ? WTF::equalIgnoringCase(b, a.characters8(), length) : WTF::equalIgnoringCase(b, a.characters16(), length);
 }
 
 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)
     , m_inParseShorthand(0)
     , m_currentShorthand(CSSPropertyInvalid)
     , m_implicitShorthand(false)
 {
 }
 
 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;
 
     if (ruleType == StyleRule::Viewport) {
         parseSuccess = (RuntimeEnabledFeatures::cssViewportEnabled() || isUASheetBehavior(context.mode()))
             && parser.parseViewportProperty(resolvedProperty, important);
     } else if (ruleType == StyleRule::FontFace) {
         parseSuccess = parser.parseFontFaceDescriptor(resolvedProperty);
     } else {
         parseSuccess = parser.parseValue(unresolvedProperty, important);
@@ skipping 221 matching lines @@
 
 static bool consumeComma(CSSParserValueList* valueList)
 {
     CSSParserValue* value = valueList->current();
     if (!value || !isComma(value))
         return false;
     valueList->next();
     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);
     return value->m_unit == CSSParserValue::Operator && value->iValue == '/';
 }
 
 static bool isGeneratedImageValue(CSSParserValue* val)
 {
     if (val->m_unit != CSSParserValue::Function)
         return false;
@@ skipping 76 matching lines @@
         return;
     }
 
     RefPtrWillBeRawPtr<CSSValue> value = prpValue;
     ShorthandScope scope(this, propId);
     const CSSPropertyID* longhands = shorthand.properties();
     for (unsigned i = 0; i < shorthandLength; ++i)
         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);
 
     CSSParserValue* value = m_valueList->current();
 
     // Note: m_parsedCalculation is used to pass the calc value to validUnit and then cleared at the end of this function.
     // FIXME: This is to avoid having to pass parsedCalc to all validUnit callers.
     ASSERT(!m_parsedCalculation);
 
@@ skipping 18 matching lines @@
 
     if (CSSParserFastPaths::isKeywordPropertyID(propId)) {
         if (!CSSParserFastPaths::isValidKeywordPropertyAndValue(propId, id))
             return false;
         if (m_valueList->next() && !inShorthand())
             return false;
         addProperty(propId, cssValuePool().createIdentifierValue(id), important);
         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] ]
         parsedValue = parseSize();
         break;
     case CSSPropertyQuotes: // [<string> <string>]+ | none
         if (id == CSSValueNone)
             validPrimitive = true;
         else
             parsedValue = parseQuotes();
@@ skipping 824 matching lines @@
             validPrimitive = true;
         else
             validPrimitive = validUnit(value, FLength | FNonNeg);
         break;
     case CSSPropertyWebkitColumnSpan: // none | all | 1 (will be dropped in the unprefixed property)
         validPrimitive = id == CSSValueAll || id == CSSValueNone || (value->unit() == CSSPrimitiveValue::UnitType::Number && value->fValue == 1);
         break;
     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
     // support custom WebKit-based Apple applications.
     case CSSPropertyWebkitLineClamp:
         // When specifying number of lines, don't allow 0 as a valid value
         // When specifying either type of unit, require non-negative integers
         validPrimitive = (!id && !isCalculation(value) && validUnit(value, FInteger | FPercent | FNonNeg) && (value->unit() == CSSPrimitiveValue::UnitType::Percentage || value->fValue));
         break;
 
@@ skipping 5499 matching lines @@
         if (!consumeComma(functionArgs))
             return nullptr;
     }
 
     return imageSet.release();
 }
 
 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();
     }
 
     // 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.
             // Need to return nullptr when currentValue is CSSPropertyAll.
             if (unresolvedProperty == CSSPropertyWillChange || unresolvedProperty == CSSPropertyAll)
                 return nullptr;
             values->append(cssValuePool().createIdentifierValue(unresolvedProperty));
         } else {
-            switch (currentValue->id) {
+            switch (currentToken.id()) {
             case CSSValueNone:
             case CSSValueAll:
             case CSSValueAuto:
             case CSSValueDefault:
             case CSSValueInitial:
             case CSSValueInherit:
                 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();
 }
 
 PassRefPtrWillBeRawPtr<CSSFunctionValue> CSSPropertyParser::parseBuiltinFilterArguments(CSSParserValueList* args, CSSValueID filterType)
 {
     RefPtrWillBeRawPtr<CSSFunctionValue> filterValue = CSSFunctionValue::create(filterType);
     ASSERT(args);
 
@@ skipping 1198 matching lines @@
         }
     }
 
     if (!list->length())
         return nullptr;
 
     return list.release();
 }
 
 } // namespace blink