Parse animation and transition shorthands in CSSPropertyParser with CSSParserTokens

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 29162c36e7cfb03dbc1d97c63b91a95c0f196c5b..37bfd75560865d4868702abb68f6b5142724e4cd 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1202,6 +1202,56 @@ static PassRefPtrWillBeRawPtr<CSSValueList> consumeAnimationPropertyList(CSSProp
     return list.release();
 }
 
+bool CSSPropertyParser::consumeAnimationShorthand(const StylePropertyShorthand& shorthand, bool useLegacyParsing, bool important)
+{
+    const unsigned longhandCount = shorthand.length();
+    RefPtrWillBeRawPtr<CSSValueList> longhands[8];
+    ASSERT(longhandCount <= 8);
+    for (size_t i = 0; i < longhandCount; ++i)
+        longhands[i] = CSSValueList::createCommaSeparated();
+
+    do {
+        bool parsedLonghand[8] = { false };
+        bool foundAnyProperty = false;
+        while (true) {
+            bool foundProperty = false;

[rwlbuis, 2015/10/20 14:32:23]

I really doubt we need two bools. Can't foundAnyProperty be deduced from
longhands? 

[Timothy Loh, 2015/10/21 06:07:39]

Best I can come up with is as follows:

do { // previously while (true) {
    bool foundProperty = false;
    for (..) { .. }
    if (!foundProperty)
        return false;
} while (!range.atEnd() && range.peek().type() != CommaToken);

Do you think this is better? I guess it makes things slightly more efficient
because we won't try parsing each comma as each different animation property.

[rwlbuis, 2015/10/21 20:58:39]

Yes, seems better.

+            for (size_t i = 0; i < longhandCount; ++i) {
+                if (parsedLonghand[i])
+                    continue;
+
+                if (RefPtrWillBeRawPtr<CSSValue> value = consumeAnimationValue(shorthand.properties()[i], m_range, m_context, useLegacyParsing)) {
+                    parsedLonghand[i] = true;
+                    foundProperty = true;
+                    foundAnyProperty = true;
+                    longhands[i]->append(value.release());
+                    break;
+                }
+            }
+            if (!foundProperty)
+                break;
+        }
+        if (!foundAnyProperty)
+            return false;
+
+        // TODO(timloh): This will make invalid longhands, see crbug.com/386459

[rwlbuis, 2015/10/20 14:32:23]

Why not do this in the end like in the legacy code?

[Timothy Loh, 2015/10/21 06:07:39]

It's actually done at the start of the while loop and after the loop in the
legacy code. Moving this outside the loop will make the longhand lists get
misaligned (e.g. "transition: left, 1s" might end up like "transition: left 1s")

+        for (size_t i = 0; i < longhandCount; ++i) {
+            if (!parsedLonghand[i])
+                longhands[i]->append(cssValuePool().createImplicitInitialValue());
+            parsedLonghand[i] = false;
+        }
+    } while (consumeCommaIncludingWhitespace(m_range));
+
+    for (size_t i = 0; i < longhandCount; ++i) {
+        if (!isValidAnimationPropertyList(shorthand.properties()[i], *longhands[i]))

[rwlbuis, 2015/10/20 14:32:23]

Is this done in the legacy code too? If not, is it an improvement and reflected
in the tests?

[Timothy Loh, 2015/10/21 06:07:39]

This is the transition-property check, which is also in the existing code, but I
wrote it this way because I figured it be better to not refer to
transition-property in here.

[rwlbuis, 2015/10/21 20:58:39]

I see. it is just a pity that it is less efficient than before.

+            return false;
+    }
+
+    for (size_t i = 0; i < longhandCount; ++i)
+        addProperty(shorthand.properties()[i], longhands[i].release(), important);
+
+    return m_range.atEnd();
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -1661,13 +1711,15 @@ bool CSSPropertyParser::consumeColumns(bool important)
     return true;
 }
 
-bool CSSPropertyParser::parseShorthand(CSSPropertyID propId, bool important)
+bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool important)
 {
+    CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
+
     m_range.consumeWhitespace();
     CSSPropertyID oldShorthand = m_currentShorthand;
     // TODO(rob.buis): Remove this when the legacy property parser is gone
-    m_currentShorthand = propId;
-    switch (propId) {
+    m_currentShorthand = property;
+    switch (property) {
     case CSSPropertyWebkitMarginCollapse: {
         CSSValueID id = m_range.consumeIncludingWhitespace().id();
         if (!CSSParserFastPaths::isValidKeywordPropertyAndValue(CSSPropertyWebkitMarginBeforeCollapse, id))
@@ -1719,6 +1771,10 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID propId, bool important)
         m_currentShorthand = oldShorthand;
         return consumeColumns(important);
     }
+    case CSSPropertyAnimation:
+        return consumeAnimationShorthand(animationShorthandForParsing(), unresolvedProperty == CSSPropertyAliasWebkitAnimation, important);
+    case CSSPropertyTransition:
+        return consumeAnimationShorthand(transitionShorthandForParsing(), false, important);
     default:
         m_currentShorthand = oldShorthand;
         return false;