Move background/webkit-mask 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 72031ccf54583a11be8ba63519a1a49a56d2fee7..07ac3cbe47465542c36bd789dbc7426ba79919f6 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -4560,6 +4560,96 @@ static bool consumeRepeatStyle(CSSParserTokenRange& range, RefPtrWillBeRawPtr<CS
     return true;
 }
 
+// Note: consumeBackgroundShorthand assumes y properties (for example background-position-y) follow
+// the x properties in the shorthand array.
+bool CSSPropertyParser::consumeBackgroundShorthand(const StylePropertyShorthand& shorthand, bool important)
+{
+    const unsigned longhandCount = shorthand.length();
+    RefPtrWillBeRawPtr<CSSValue> longhands[10];
+    ASSERT(longhandCount <= 10);
+#if ENABLE(OILPAN)
+    // Zero initialize the array of raw pointers.
+    memset(&longhands, 0, sizeof(longhands));
+#endif
+    bool seenClip = false;

[Timothy Loh, 2016/03/02 04:50:27]

Doesn't seem right, this should be per-layer.

background: url(a) content-box padding-box, url(b) border-box;

should set
background-origin: content-box, border-box;
background-clip: padding-box, border-box;

but we end up setting background-clip: padding-box, initial;

Can we fix this (and add a test)?

[rwlbuis, 2016/03/07 02:50:04]

Done.

+    bool implicit = false;
+    do {
+        bool parsedLonghand[10] = { false };
+        do {
+            bool foundProperty = false;
+            for (size_t i = 0; i < longhandCount; ++i) {
+                if (parsedLonghand[i])
+                    continue;
+
+                RefPtrWillBeRawPtr<CSSValue> value = nullptr;
+                RefPtrWillBeRawPtr<CSSValue> valueY = nullptr;
+                CSSPropertyID property = shorthand.properties()[i];
+                if (property == CSSPropertyBackgroundRepeatX || property == CSSPropertyWebkitMaskRepeatX) {
+                    consumeRepeatStyleComponent(m_range, value, valueY, implicit);
+                } else if (property == CSSPropertyBackgroundPositionX || property == CSSPropertyWebkitMaskPositionX) {
+                    CSSParserTokenRange rangeCopy = m_range;
+                    if (!consumePosition(rangeCopy, m_context.mode(), UnitlessQuirk::Forbid, value, valueY))
+                        continue;
+                    m_range = rangeCopy;
+                } else if (((property == CSSPropertyBackgroundSize || property == CSSPropertyWebkitMaskSize) && !consumeSlashIncludingWhitespace(m_range))

[Timothy Loh, 2016/03/02 04:50:27]

Slash handling looks wrong here, since we could consume a slash and not have a
<bg-size>, but then keep going. Can we just put the size case after the position
code just above? Then we can remove the seenPosition logic below.

[rwlbuis, 2016/03/07 02:50:04]

Not sure if you meant it the way I implemented it in my latest patch :) Anyway
it works, glad to see the seenPosition logic go.

[Timothy Loh, 2016/03/07 03:55:07]

Wasn't really what I meant, but it works.

+                    || property == CSSPropertyBackgroundPositionY || property == CSSPropertyBackgroundRepeatY
+                    || property == CSSPropertyWebkitMaskPositionY || property == CSSPropertyWebkitMaskRepeatY) {
+                    continue;
+                }
+                if (!value)

[Timothy Loh, 2016/03/02 04:50:27]

can this be an else?

[rwlbuis, 2016/03/07 02:50:04]

Done.

+                    value = consumeBackgroundComponent(property, m_range, m_context);
+                if (value) {
+                    if (property == CSSPropertyBackgroundClip || property == CSSPropertyWebkitMaskClip)
+                        seenClip = true;
+                    parsedLonghand[i] = true;
+                    foundProperty = true;
+                    addBackgroundValue(longhands[i], value.release());
+                    if (valueY) {
+                        parsedLonghand[i + 1] = true;
+                        addBackgroundValue(longhands[i + 1], valueY.release());
+                    }
+                }
+            }
+            if (!foundProperty)
+                return false;
+        } while (!m_range.atEnd() && m_range.peek().type() != CommaToken);
+
+        // TODO(timloh): This will make invalid longhands, see crbug.com/386459
+        bool seenPosition = false;
+        for (size_t i = 0; i < longhandCount; ++i) {
+            CSSPropertyID property = shorthand.properties()[i];
+            if (property == CSSPropertyBackgroundColor && m_range.peek().type() == CommaToken) {

[Timothy Loh, 2016/03/02 04:50:27]

maybe clearer to write !m_range.atEnd() instead of checking for a comma?

[rwlbuis, 2016/03/07 02:50:04]

Done.

+                if (parsedLonghand[i])
+                    return false; // Color is not allowed except as the last item in a list for backgrounds.

[Timothy Loh, 2016/03/02 04:50:27]

Sounds like it has to be the very last thing. Maybe something like
// Colors are only allowed in the last layer

[rwlbuis, 2016/03/07 02:50:04]

Done.

+                continue;
+            }
+            if ((property == CSSPropertyBackgroundPositionX || property == CSSPropertyWebkitMaskPositionX) && parsedLonghand[i])
+                seenPosition = true;
+            if ((property == CSSPropertyBackgroundSize || property == CSSPropertyWebkitMaskSize) && parsedLonghand[i] && !seenPosition)
+                return false;
+            if (!parsedLonghand[i])
+                addBackgroundValue(longhands[i], cssValuePool().createImplicitInitialValue());
+            parsedLonghand[i] = false;
+        }
+    } while (consumeCommaIncludingWhitespace(m_range));
+    if (!m_range.atEnd())
+        return false;
+
+    RefPtrWillBeRawPtr<CSSValue> originValue = nullptr;
+    for (size_t i = 0; i < longhandCount; ++i) {
+        CSSPropertyID property = shorthand.properties()[i];
+        if (property == CSSPropertyBackgroundOrigin || property == CSSPropertyWebkitMaskOrigin)
+            originValue = longhands[i];
+        if ((property == CSSPropertyBackgroundClip || property == CSSPropertyWebkitMaskClip) && !seenClip)
+            addProperty(property, originValue.release(), important, implicit);
+        else if (property == CSSPropertyBackgroundSize && longhands[i] && m_context.useLegacyBackgroundSizeShorthandBehavior())
+            continue;
+        else
+            addProperty(property, longhands[i].release(), important, implicit);
+    }
+    return true;
+}
+
 bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool important)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -4722,6 +4812,10 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool im
         addProperty(property == CSSPropertyBackgroundRepeat ? CSSPropertyBackgroundRepeatY : CSSPropertyWebkitMaskRepeatY, resultY.release(), important, implicit);
         return true;
     }
+    case CSSPropertyBackground:
+        return consumeBackgroundShorthand(backgroundShorthand(), important);
+    case CSSPropertyWebkitMask:
+        return consumeBackgroundShorthand(webkitMaskShorthand(), important);
     default:
         m_currentShorthand = oldShorthand;
         CSSParserValueList valueList(m_range);