Move text-shadow/box-shadow properties into CSSPropertyParser

third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp

Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index 7091dc6f1d12df160dd619817674d9ce0530ab45..45dc9bc3238652398b5a4bf79fa49481bf8c53ac 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -12,6 +12,7 @@
 #include "core/css/CSSFontFeatureValue.h"
 #include "core/css/CSSPrimitiveValueMappings.h"
 #include "core/css/CSSQuadValue.h"
+#include "core/css/CSSShadowValue.h"
 #include "core/css/CSSStringValue.h"
 #include "core/css/CSSTimingFunctionValue.h"
 #include "core/css/CSSURIValue.h"
@@ -1332,6 +1333,77 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeWidowsOrOrphans(CSSParserTokenRan
     return consumePositiveInteger(range);
 }
 
+static PassRefPtrWillBeRawPtr<CSSShadowValue> parseSingleShadow(CSSParserTokenRange& range, const CSSParserContext& context, bool allowInset, bool allowSpread)
+{
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> style = nullptr;
+    RefPtrWillBeRawPtr<CSSValue> color = nullptr;
+    WillBeHeapVector<RefPtrWillBeMember<CSSPrimitiveValue>, 4> lengths;

[Timothy Loh, 2015/10/29 02:53:27]

Probably nicer to just have four separate variables horizontalOffset,
verticalOffset, blurRadius, spreadDistance.

[rwlbuis, 2015/10/29 20:48:14]

That actually makes the code much more compact, done.

+
+    if (range.atEnd())
+        return nullptr;
+    if (range.peek().id() == CSSValueInset) {
+        if (!allowInset)
+            return nullptr;
+        style = consumeIdent(range);
+    }
+    color = consumeColor(range, context);
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> length = consumeLength(range, context.mode(), ValueRangeAll);
+    if (!length)
+        return nullptr;
+    lengths.append(length.release());
+
+    length = consumeLength(range, context.mode(), ValueRangeAll);
+    if (!length)
+        return nullptr;
+    lengths.append(length.release());
+
+    length = consumeLength(range, context.mode(), ValueRangeAll);
+    if (length) {
+        // Blur radius must be non-negative.
+        if (length->getDoubleValue() < 0)
+            return nullptr;
+        lengths.append(length.release());
+        length = consumeLength(range, context.mode(), ValueRangeAll);
+        if (length) {
+            if (!allowSpread)

[Timothy Loh, 2015/10/29 02:53:26]

I think it makes more sense to not try and consumeLength at all if allowSpread
is false

[rwlbuis, 2015/10/29 20:48:14]

Done.

+                return nullptr;
+            lengths.append(length.release());
+        }
+    }
+
+    if (!range.atEnd()) {
+        if (RefPtrWillBeRawPtr<CSSValue> colorValue = consumeColor(range, context)) {

[Timothy Loh, 2015/10/29 02:53:27]

Can't we write:
if (!color)
    color = consumeColor(..);

[rwlbuis, 2015/10/29 20:48:14]

Done.

+            if (color)
+                return nullptr;
+            color = colorValue;
+        }
+        if (range.peek().id() == CSSValueInset) {
+            if (!allowInset || style)
+                return nullptr;
+            style = consumeIdent(range);
+        }
+    }
+    unsigned lengthsSeen = lengths.size();
+    return CSSShadowValue::create(lengths.at(0), lengths.at(1), lengthsSeen > 2 ? lengths.at(2) : nullptr,
+        lengthsSeen > 3 ? lengths.at(3) : nullptr, style.release(), color.release());
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeShadow(CSSParserTokenRange& range, const CSSParserContext& context, bool isBoxShadowProperty)
+{
+    if (range.peek().id() == CSSValueNone)
+        return consumeIdent(range);
+
+    RefPtrWillBeRawPtr<CSSValueList> shadowValueList = CSSValueList::createCommaSeparated();
+    do {
+        if (RefPtrWillBeRawPtr<CSSShadowValue> shadowValue = parseSingleShadow(range, context, isBoxShadowProperty, isBoxShadowProperty))

[Timothy Loh, 2015/10/29 02:53:26]

Doesn't this allow extraneous commas everywhere?
text-shadow: <shadow>,,,,,
text-shadow: ,,,<shadow>,,

I think we can just put an else clause which returns nullptr here (this removes
the need for an empty list check later).

[rwlbuis, 2015/10/29 20:48:14]

Ouch! Fixed now.
I noticed box-shadow-interpolation.html using one comma at the end in two cases,
I assume this was an error (Firefox sees this as invalid) so I fixed the test,
adding alancutter to check if the original ending comma's were intended.
Done.

+            shadowValueList->append(shadowValue.release());
+    } while (consumeCommaIncludingWhitespace(range));
+    if (shadowValueList->length() == 0)
+        return nullptr;
+    return shadowValueList;
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -1434,6 +1506,9 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
         return consumeColor(m_range, m_context);
     case CSSPropertyColor:
         return consumeColor(m_range, m_context, inQuirksMode());
+    case CSSPropertyTextShadow: // CSS2 property, dropped in CSS2.1, back in CSS3, so treat as CSS3
+    case CSSPropertyBoxShadow:
+        return consumeShadow(m_range, m_context, property == CSSPropertyBoxShadow);
     default:
         return nullptr;
     }