Parse "-webkit-columns: auto <length>" properly.

Source/core/css/CSSParser-in.cpp

Index: Source/core/css/CSSParser-in.cpp
diff --git a/Source/core/css/CSSParser-in.cpp b/Source/core/css/CSSParser-in.cpp
index ef0b3ff3c184acd4d55d8c2edec0638eb0277300..5590126c72d4f520fc24c105e53e4830aa459872 100644
--- a/Source/core/css/CSSParser-in.cpp
+++ b/Source/core/css/CSSParser-in.cpp
@@ -2660,7 +2660,7 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
     case CSSPropertyListStyle:
         return parseShorthand(propId, listStyleShorthand(), important);
     case CSSPropertyWebkitColumns:
-        return parseShorthand(propId, webkitColumnsShorthand(), important);
+        return parseColumnsShorthand(important);
     case CSSPropertyWebkitColumnRule:
         return parseShorthand(propId, webkitColumnRuleShorthand(), important);
     case CSSPropertyWebkitTextStroke:
@@ -3210,6 +3210,77 @@ bool CSSParser::parseTransitionShorthand(CSSPropertyID propId, bool important)
     return true;
 }
 
+bool CSSParser::parseColumnsShorthand(bool important)
+{
+    ShorthandScope scope(this, CSSPropertyWebkitColumns);
+    const StylePropertyShorthand& shorthand = webkitColumnsShorthand();
+    unsigned propertiesParsed = 0;
+    unsigned autoCount = 0;
+    bool propertyFound[]= { false, false };

[Julien - ping for review, 2013/07/23 17:26:50]

This definitely works but this makes the code a lot more complicated than it
ought to be.

You expect only 2 values but the code uses a generic algorithm that could handle
40,000 of them for no good reason.

How about we simplify the code by using 2 values here and avoiding unneeded
loops?

[mstensho (USE GERRIT), 2013/07/23 21:35:33]

Done.

+
+    ASSERT(WTF_ARRAY_LENGTH(propertyFound) == shorthand.length());
+    ASSERT(!shorthand.propertiesForInitialization());
+
+    while (CSSParserValue* value = m_valueList->current()) {
+        if (propertiesParsed >= shorthand.length())
+            return false; // Too many values. Invalid declaration.
+        if (value->id == CSSValueAuto) {
+            // 'auto' is a valid value for any longhand, and at this point we don't know
+            // which one(s) it is meant for. We need to look at the other values first. Just
+            // ignore 'auto' for now. After having processed the value list, set unassigned
+            // properties to 'auto'.
+            m_valueList->next();
+            propertiesParsed++;
+            autoCount++;
+        } else {
+            bool found = false;
+            for (unsigned propIndex = 0; propIndex < shorthand.length(); ++propIndex) {
+                if (!propertyFound[propIndex] && parseValue(shorthand.properties()[propIndex], important)) {
+                    propertyFound[propIndex] = found = true;

[Julien - ping for review, 2013/07/23 17:26:50]

This is a style violation, we use one statement per line.

[mstensho (USE GERRIT), 2013/07/23 21:35:33]

Done.

+                    propertiesParsed++;
+                    break;
+                }
+            }
+
+            // If we didn't find at least one match, this is an
+            // invalid shorthand and we have to ignore it.
+            if (!found)
+                return false;
+        }
+    }
+
+    if (!autoCount && propertiesParsed == shorthand.length())
+        return true;
+
+    // Fill in any remaining properties with the 'initial' or 'auto' value.
+    RefPtr<CSSValue> value;
+    if (autoCount) {
+        value = cssValuePool().createIdentifierValue(CSSValueAuto);
+    } else {
+        value = cssValuePool().createImplicitInitialValue();
+        m_implicitShorthand = true;
+    }
+
+    for (unsigned i = 0; i < shorthand.length(); ++i) {
+        if (propertyFound[i])
+            continue;
+
+        addProperty(shorthand.properties()[i], value, important);

[Julien - ping for review, 2013/07/23 17:26:50]

The parseValue call above already calls addProperty so it's weird that we do it
again here.

[mstensho (USE GERRIT), 2013/07/23 21:35:33]

Here we set the unspecified or auto longhands that the parseValue() call didn't
take care of. See parseShorthand(); it does something very similar. It's where I
stole it from, after all. ;)

+        if (autoCount) {
+            autoCount--;
+            if (!autoCount) {
+                // We have assigned as many 'auto' values as were specified. Now make the remaining
+                // ones implicit, so that we don't get too many of them if we later decide to
+                // reconstruct a shorthand based on longhands.
+                m_implicitShorthand = true;
+            }
+        }
+    }
+    m_implicitShorthand = false;
+
+    return true;
+}
+
 bool CSSParser::parseShorthand(CSSPropertyID propId, const StylePropertyShorthand& shorthand, bool important)
 {
     // We try to match as many properties as possible