Move background related shorthands 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 f453d18a1cd72b38a55718161f2166bb9915c030..2aa718afcbb1db88ba0bbfea48da8593ce18a461 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -4522,6 +4522,60 @@ bool CSSPropertyParser::consumeLegacyBreakProperty(CSSPropertyID property, bool
     return true;
 }
 
+static bool consumeBackgroundPosition(CSSParserTokenRange& range, const CSSParserContext& context, UnitlessQuirk unitless, RefPtrWillBeRawPtr<CSSValue>& resultX, RefPtrWillBeRawPtr<CSSValue>& resultY)
+{
+    do {
+        RefPtrWillBeRawPtr<CSSValue> positionX = nullptr;
+        RefPtrWillBeRawPtr<CSSValue> positionY = nullptr;
+        if (!consumePosition(range, context.mode(), unitless, positionX, positionY))
+            return false;
+        addBackgroundValue(resultX, positionX);
+        if (positionY)

[Timothy Loh, 2016/02/23 05:53:15]

I think both always get set if consumePosition returns true

[rwlbuis, 2016/02/23 20:16:42]

Done.

+            addBackgroundValue(resultY, positionY);
+    } while (consumeCommaIncludingWhitespace(range));
+    return resultX;

[Timothy Loh, 2016/02/23 05:53:14]

return true?

[rwlbuis, 2016/02/23 20:16:42]

Done, also in consumeRepeatStyle.

+}
+
+static bool consumeRepeatStyleComponent(CSSParserTokenRange& range, RefPtrWillBeRawPtr<CSSValue>& value1, RefPtrWillBeRawPtr<CSSValue>& value2, bool& implicit)
+{
+    if (consumeIdent<CSSValueRepeatX>(range)) {
+        value1 = cssValuePool().createIdentifierValue(CSSValueRepeat);
+        value2 = cssValuePool().createIdentifierValue(CSSValueNoRepeat);
+        implicit = true;
+        return true;
+    }
+    if (consumeIdent<CSSValueRepeatY>(range)) {
+        value1 = cssValuePool().createIdentifierValue(CSSValueNoRepeat);
+        value2 = cssValuePool().createIdentifierValue(CSSValueRepeat);
+        implicit = true;
+        return true;
+    }
+    CSSValueID id = range.peek().id();
+    value1 = consumeIdent<CSSValueRepeat, CSSValueNoRepeat, CSSValueRound, CSSValueSpace>(range);
+    if (!value1)
+        return false;
+
+    value2 = consumeIdent<CSSValueRepeat, CSSValueNoRepeat, CSSValueRound, CSSValueSpace>(range);
+    if (!value2) {
+        value2 = cssValuePool().createIdentifierValue(id);

[Timothy Loh, 2016/02/23 05:53:14]

isn't this just value1?

[rwlbuis, 2016/02/23 20:16:41]

True, I changed it.

+        implicit = true;
+    }
+    return true;
+}
+
+static bool consumeRepeatStyle(CSSParserTokenRange& range, RefPtrWillBeRawPtr<CSSValue>& resultX, RefPtrWillBeRawPtr<CSSValue>& resultY, bool& implicit)
+{
+    do {
+        RefPtrWillBeRawPtr<CSSValue> repeatX = nullptr;
+        RefPtrWillBeRawPtr<CSSValue> repeatY = nullptr;
+        if (!consumeRepeatStyleComponent(range, repeatX, repeatY, implicit))
+            break;

[Timothy Loh, 2016/02/23 05:53:15]

Any reason for this to be different than consumeBackgroundPosition?

[rwlbuis, 2016/02/23 20:16:42]

Maybe at some point, but probably not anymore, fixed.

+        addBackgroundValue(resultX, repeatX);
+        addBackgroundValue(resultY, repeatY);
+    } while (consumeCommaIncludingWhitespace(range));
+    return resultX;
+}
+
 bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool important)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -4663,6 +4717,33 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool im
     case CSSPropertyWebkitColumnBreakBefore:
     case CSSPropertyWebkitColumnBreakInside:
         return consumeLegacyBreakProperty(property, important);
+    case CSSPropertyWebkitMaskPosition:
+    case CSSPropertyBackgroundPosition: {
+        RefPtrWillBeRawPtr<CSSValue> resultX = nullptr;
+        RefPtrWillBeRawPtr<CSSValue> resultY = nullptr;
+        CSSPropertyID positionX = property == CSSPropertyBackgroundPosition ? CSSPropertyBackgroundPositionX : CSSPropertyWebkitMaskPositionX;
+        CSSPropertyID positionY = property == CSSPropertyBackgroundPosition ? CSSPropertyBackgroundPositionY : CSSPropertyWebkitMaskPositionY;
+        if (!consumeBackgroundPosition(m_range, m_context, UnitlessQuirk::Allow, resultX, resultY) || !m_range.atEnd())
+            return false;
+        addProperty(positionX, resultX.release(), important);
+        if (resultY)

[Timothy Loh, 2016/02/23 05:53:14]

There shouldn't be any case where a shorthand only sets some of its longhands
(except for that one quirk that we have, see bug 277157). Same below.

[rwlbuis, 2016/02/23 20:16:42]

Good point! Fixed.

+            addProperty(positionY, resultY.release(), important);
+        return true;
+    }
+    case CSSPropertyBackgroundRepeat:
+    case CSSPropertyWebkitMaskRepeat: {
+        RefPtrWillBeRawPtr<CSSValue> resultX = nullptr;
+        RefPtrWillBeRawPtr<CSSValue> resultY = nullptr;
+        CSSPropertyID repeatX = property == CSSPropertyBackgroundRepeat ? CSSPropertyBackgroundRepeatX : CSSPropertyWebkitMaskRepeatX;
+        CSSPropertyID repeatY = property == CSSPropertyBackgroundRepeat ? CSSPropertyBackgroundRepeatY : CSSPropertyWebkitMaskRepeatY;
+        bool implicit = false;
+        if (!consumeRepeatStyle(m_range, resultX, resultY, implicit) || !m_range.atEnd())
+            return false;
+        addProperty(repeatX, resultX.release(), important, implicit);
+        if (resultY)
+            addProperty(repeatY, resultY.release(), important, implicit);
+        return true;
+    }
     default:
         m_currentShorthand = oldShorthand;
         CSSParserValueList valueList(m_range);