Simplify parseShadowValue

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 1e56c0fc106751d24769bdb34a22df2352018252..c26b40e6beb75fc3aea2d04f8b7889aee283b0ea 100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1004,15 +1004,8 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
     case CSSPropertyBoxShadow:
         if (id == CSSValueNone)
             validPrimitive = true;
-        else {
-            RefPtrWillBeRawPtr<CSSValueList> shadowValueList = parseShadow(m_valueList, propId);
-            if (shadowValueList) {
-                addProperty(propId, shadowValueList.release(), important);
-                m_valueList->next();
-                return true;
-            }
-            return false;
-        }
+        else
+            return parseShadow(m_valueList, propId, important);

[Timothy Loh, 2015/09/01 07:18:08]

parsedValue = parseShadow(..) and remove the important argument? :-)

[rwlbuis, 2015/09/02 22:38:58]

Done.

         break;
     case CSSPropertyWebkitBoxReflect:
         if (id == CSSValueNone)
@@ -5143,175 +5136,94 @@ bool CSSPropertyParser::parseColorFromValue(const CSSParserValue* value, RGBA32&
     return true;
 }
 
-// This class tracks parsing state for shadow values.  If it goes out of scope (e.g., due to an early return)
-// without the allowBreak bit being set, then it will clean up all of the objects and destroy them.
-class ShadowParseContext {
-    STACK_ALLOCATED();
-public:
-    ShadowParseContext(CSSPropertyID prop)
-        : property(prop)
-        , allowX(true)
-        , allowY(false)
-        , allowBlur(false)
-        , allowSpread(false)
-        , allowColor(true)
-        , allowStyle(prop == CSSPropertyBoxShadow)
-        , allowBreak(true)
-    {
-    }
-
-    bool allowLength() { return allowX || allowY || allowBlur || allowSpread; }
-
-    void commitValue()
-    {
-        // Handle the ,, case gracefully by doing nothing.
-        if (x || y || blur || spread || color || style) {
-            if (!values)
-                values = CSSValueList::createCommaSeparated();
-
-            // Construct the current shadow value and add it to the list.
-            values->append(CSSShadowValue::create(x.release(), y.release(), blur.release(), spread.release(), style.release(), color.release()));
-        }
-
-        // Now reset for the next shadow value.
-        x = nullptr;
-        y = nullptr;
-        blur = nullptr;
-        spread = nullptr;
-        style = nullptr;
-        color = nullptr;
-
-        allowX = true;
-        allowColor = true;
-        allowBreak = true;
-        allowY = false;
-        allowBlur = false;
-        allowSpread = false;
-        allowStyle = property == CSSPropertyBoxShadow;
-    }
-
-    void commitLength(PassRefPtrWillBeRawPtr<CSSPrimitiveValue> val)
-    {
-        if (allowX) {
-            x = val;
-            allowX = false;
-            allowY = true;
-            allowColor = false;
-            allowStyle = false;
-            allowBreak = false;
-        } else if (allowY) {
-            y = val;
-            allowY = false;
-            allowBlur = true;
-            allowColor = true;
-            allowStyle = property == CSSPropertyBoxShadow;
-            allowBreak = true;
-        } else if (allowBlur) {
-            blur = val;
-            allowBlur = false;
-            allowSpread = property == CSSPropertyBoxShadow;
-        } else if (allowSpread) {
-            spread = val;
-            allowSpread = false;
-        }
-    }
-
-    void commitColor(PassRefPtrWillBeRawPtr<CSSPrimitiveValue> val)
-    {
-        color = val;
-        allowColor = false;
-        if (allowX) {
-            allowStyle = false;
-            allowBreak = false;
-        } else {
-            allowBlur = false;
-            allowSpread = false;
-            allowStyle = property == CSSPropertyBoxShadow;
-        }
-    }
-
-    void commitStyle(CSSParserValue* v)
-    {
-        style = cssValuePool().createIdentifierValue(v->id);
-        allowStyle = false;
-        if (allowX)
-            allowBreak = false;
-        else {
-            allowBlur = false;
-            allowSpread = false;
-            allowColor = false;
-        }
+bool CSSPropertyParser::parseShadow(CSSParserValueList* valueList, CSSPropertyID propID, bool important)
+{
+    RefPtrWillBeRawPtr<CSSValueList> shadowValueList = CSSValueList::createCommaSeparated();
+    while (RefPtrWillBeRawPtr<CSSShadowValue> shadowValue = parseSingleShadow(valueList, propID == CSSPropertyBoxShadow, true, propID == CSSPropertyBoxShadow ? 4 : 3)) {
+        shadowValueList->append(shadowValue);
+        if (!valueList->current())
+            break;
+        if (!isComma(valueList->current()))

[Timothy Loh, 2015/09/01 07:18:08]

!consumeComma and then you don't need the next() below

[rwlbuis, 2015/09/02 22:38:58]

Done.

+            return false;
+        valueList->next();
     }
+    if (shadowValueList->length() == 0 || valueList->current())
+        return false;
+    addProperty(propID, shadowValueList.release(), important);
+    return true;
+}
 
-    CSSPropertyID property;
-
-    RefPtrWillBeMember<CSSValueList> values;
-    RefPtrWillBeMember<CSSPrimitiveValue> x;
-    RefPtrWillBeMember<CSSPrimitiveValue> y;
-    RefPtrWillBeMember<CSSPrimitiveValue> blur;
-    RefPtrWillBeMember<CSSPrimitiveValue> spread;
+PassRefPtrWillBeRawPtr<CSSShadowValue> CSSPropertyParser::parseSingleShadow(CSSParserValueList* valueList, bool allowInset, bool allowBreak, unsigned maxLength)
+{
     RefPtrWillBeMember<CSSPrimitiveValue> style;
     RefPtrWillBeMember<CSSPrimitiveValue> color;
+    RefPtrWillBeRawPtr<CSSValueList> lengths = CSSValueList::createSpaceSeparated();

[Timothy Loh, 2015/09/01 07:18:08]

Probably just use a vector (or have four CSSValues?)

[rwlbuis, 2015/09/02 22:38:58]

Done.

 
-    bool allowX;
-    bool allowY;
-    bool allowBlur;
-    bool allowSpread;
-    bool allowColor;
-    bool allowStyle; // inset or not.
-    bool allowBreak;
-};
+    CSSParserValue* val = valueList->current();
+    if (!val)
+        return nullptr;
+    if (val->id == CSSValueInset) {
+        if (!allowInset)
+            return nullptr;
+        style = cssValuePool().createIdentifierValue(val->id);
+        val = valueList->next();
+        if (!val)
+            return nullptr;
+    }
+    if ((color = parseColor(val)))
+        val = valueList->next();
 
-PassRefPtrWillBeRawPtr<CSSValueList> CSSPropertyParser::parseShadow(CSSParserValueList* valueList, CSSPropertyID propId)
-{
-    ShadowParseContext context(propId);
-    for (CSSParserValue* val = valueList->current(); val; val = valueList->next()) {
-        // Check for a comma break first.
-        if (val->m_unit == CSSParserValue::Operator) {
-            if (val->iValue != ',' || !context.allowBreak) {
-                // Other operators aren't legal or we aren't done with the current shadow
-                // value.  Treat as invalid.
-                return nullptr;
-            }
-            // The value is good.  Commit it.
-            context.commitValue();
-        } else if (validUnit(val, FLength, HTMLStandardMode)) {
-            // We required a length and didn't get one. Invalid.
-            if (!context.allowLength())
-                return nullptr;
+    if (!val || !validUnit(val, FLength, HTMLStandardMode))

[Timothy Loh, 2015/09/01 07:18:08]

Doesn't this fail if we have: color inset length length

I was thinking this would be something like:
while(true) {
    if (!style && allowInset && val->id == CSSValueInset)
        // set style, continue
    if (!color && (color = parseColor())
        // continue
    if (lengths.isEmpty()) {
        // consume lengths..
    }
    break;
}

[rwlbuis, 2015/09/02 22:38:58]

In fact that syntax is not allowed, see fast/box-shadow/inset.html, same in
Firefox.

+        return nullptr;
+    lengths->append(createPrimitiveNumericValue(val));
+    val = valueList->next();
 
-            // Blur radius must be non-negative.
-            if (context.allowBlur && (m_parsedCalculation ? m_parsedCalculation->isNegative() : !validUnit(val, FLength | FNonNeg, HTMLStandardMode)))
-                return nullptr;
+    if (!val || !validUnit(val, FLength, HTMLStandardMode))
+        return nullptr;
+    lengths->append(createPrimitiveNumericValue(val));
+    val = valueList->next();
 
-            // A length is allowed here.  Construct the value and add it.
-            RefPtrWillBeRawPtr<CSSPrimitiveValue> length = createPrimitiveNumericValue(val);
-            context.commitLength(length.release());
-        } else if (val->id == CSSValueInset) {
-            if (!context.allowStyle)
-                return nullptr;
+    if (val && validUnit(val, FLength, HTMLStandardMode)) {
+        // Blur radius must be non-negative.
+        if (m_parsedCalculation ? m_parsedCalculation->isNegative() : !validUnit(val, FLength | FNonNeg, HTMLStandardMode))
+            return nullptr;
+        lengths->append(createPrimitiveNumericValue(val));
+        val = valueList->next();
+        if (val && validUnit(val, FLength, HTMLStandardMode)) {
+            lengths->append(createPrimitiveNumericValue(val));
+            val = valueList->next();
+        }
+    }
 
-            context.commitStyle(val);
-        } else {
-            if (!context.allowColor)
+    if (val) {
+        if (RefPtrWillBeMember<CSSPrimitiveValue> colorValue = parseColor(val)) {
+            if (color)
                 return nullptr;
-
-            // The only other type of value that's ok is a color value.
-            RefPtrWillBeRawPtr<CSSPrimitiveValue> parsedColor = parseColor(val);
-            if (!parsedColor)
+            color = colorValue;
+            val = valueList->next();
+        }
+        if (val && val->id == CSSValueInset) {
+            if (!allowInset || style)
                 return nullptr;
-
-            context.commitColor(parsedColor.release());
+            style = cssValuePool().createIdentifierValue(val->id);
+            val = valueList->next();
         }
     }
 
-    if (context.allowBreak) {
-        context.commitValue();
-        if (context.values && context.values->length())
-            return context.values.release();
+    if (val && val->m_unit == CSSParserValue::Operator) {

[Timothy Loh, 2015/09/01 07:18:08]

We don't really need a check like this here (or the allowBreak flag). It should
be fine if this just consumes as many values as it can and returns a
CSSShadowValue out of them (or nullptr). The huge switch statement checks at the
bottom for trailing tokens, and the list parsing routing can bail if it sees a
non-comma.

[rwlbuis, 2015/09/02 22:38:58]

Done.

+        if (val->iValue != ',' || !allowBreak) {
+            // Other operators aren't legal or we aren't done with the current shadow
+            // value.  Treat as invalid.
+            return nullptr;
+        }
     }
-
-    return nullptr;
+    unsigned lengthsSeen = lengths->length();
+    if (lengthsSeen > maxLength)
+        return nullptr;
+    return CSSShadowValue::create(toCSSPrimitiveValue(lengths->item(0)), toCSSPrimitiveValue(lengths->item(1)),
+                                     lengthsSeen > 2 ? toCSSPrimitiveValue(lengths->item(2)) : nullptr,
+                                     lengthsSeen > 3 ? toCSSPrimitiveValue(lengths->item(3)) : nullptr,
+                                     style.release(), color.release());
 }
 
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseReflect()
@@ -6958,11 +6870,10 @@ PassRefPtrWillBeRawPtr<CSSFunctionValue> CSSPropertyParser::parseBuiltinFilterAr
     }
     case CSSValueDropShadow: {
         // drop-shadow() takes a single shadow.
-        RefPtrWillBeRawPtr<CSSValueList> shadowValueList = parseShadow(args, CSSPropertyWebkitFilter);
-        if (!shadowValueList || shadowValueList->length() != 1)
+        RefPtrWillBeRawPtr<CSSShadowValue> shadowValue = parseSingleShadow(args);
+        if (!shadowValue)
             return nullptr;
-
-        filterValue->append((shadowValueList.release())->item(0));
+        filterValue->append(shadowValue.release());
         break;
     }
     default: