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..63d8848972d25ee70938aa451fb5a47cc83498d8 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,70 @@ bool CSSParser::parseTransitionShorthand(CSSPropertyID propId, bool important)
     return true;
 }
 
+bool CSSParser::parseColumnsShorthand(bool important)
+{
+    ShorthandScope scope(this, CSSPropertyWebkitColumns);
+    unsigned propertiesParsed = 0;
+    unsigned autoCount = 0;
+    bool columnCountFound = false;
+    bool columnWidthFound = false;

[Julien - ping for review, 2013/07/24 21:41:11]

Ideally variable names should be good English: columnWidthWasFound or
foundColumnWidth

[mstensho (USE GERRIT), 2013/07/25 12:01:59]

Done.

+
+    while (CSSParserValue* value = m_valueList->current()) {
+        if (propertiesParsed >= 2)
+            return false; // Too many values for this shorthand. Invalid declaration.
+        if (value->id == CSSValueAuto) {
+            // 'auto' is a valid value for any of the two longhands, and at this point we
+            // don't know which one(s) it is meant for. We need to see if there are other
+            // values first. Just count the 'auto' occurrence for now. After having
+            // processed the value list, set unassigned properties to 'auto'.
+            m_valueList->next();
+            autoCount++;
+        } else {
+            if (!columnCountFound && parseValue(CSSPropertyWebkitColumnCount, important)) {
+                columnCountFound = true;
+            } else if (!columnWidthFound && parseValue(CSSPropertyWebkitColumnWidth, important)) {
+                columnWidthFound = true;
+            } else {
+                // If we didn't find at least one match, this is an
+                // invalid shorthand and we have to ignore it.
+                return false;
+            }
+        }
+        propertiesParsed++;
+    }
+
+    if (!autoCount && propertiesParsed == 2)
+        return true;
+

[Julien - ping for review, 2013/07/24 21:41:11]

Probably worth adding this assert:

ASSERT(!columnCountFound || !columnWidthFound);

[mstensho (USE GERRIT), 2013/07/25 12:01:59]

Done.

+    // Fill in any remaining properties with the 'initial' or 'auto' value.
+    RefPtr<CSSValue> value;
+    if (autoCount) {
+        // 'auto' is among the values. Set remaining properties to 'auto'.
+        value = cssValuePool().createIdentifierValue(CSSValueAuto);
+    } else {
+        // 'auto' isn't among the values, but we still have to make sure that the unspecified
+        // longhand gets set to its initial value.
+        value = cssValuePool().createImplicitInitialValue();
+        m_implicitShorthand = true;
+    }
+    if (!columnCountFound) {
+        addProperty(CSSPropertyWebkitColumnCount, value, important);
+        if (autoCount == 1) {
+            // Only one 'auto' value was specified, and we have just assigned it to a longhand. If
+            // the other longhand needs to be set to 'auto' as well, make sure it's marked as
+            // "implicit", so that we don't get too many 'auto' occurrences if we later decide to
+            // reconstruct the shorthand based on these longhands (using getComputedStyle(), for
+            // instance).
+            m_implicitShorthand = true;
+        }
+    }
+    if (!columnWidthFound)
+        addProperty(CSSPropertyWebkitColumnWidth, value, important);
+    m_implicitShorthand = false;
+

[Julien - ping for review, 2013/07/24 21:41:11]

This code is way too complicated and I don't think the complexity is needed.

You need to keep track of which longhand is set and what value those are. The
only tricky case is if the first value is 'auto', the second one (or its
absence) completely determines what's going to happen. If you thought of it in
this way, it should be way more readable IMO.

You could keep the 2 values as RefPtr<CSSValue> when you parse them (0 meaning
implicit) and that would mean you just have to add them once (ie you need to
extract the 'column-width' and 'column-count' parsing code into functions you
can call).

I also don't think the current handles 'auto' properly as you set 'column-count'
to an implicit 'auto' not an implicit 'initial' value.

[mstensho (USE GERRIT), 2013/07/25 12:01:59]

OK, I've tried to do this now. This eliminated the need for creating a
ShorthandScope instance, so I removed it. Fine?
As long as the values are implicit, does it really matter that much whether they
are 'auto' or 'initial', given that the initial value is indeed 'auto'? Anyway,
there was some unnecessary complexity there, and I've tried to clean it up. I've
removed setting of 'initial' completely now. That seems to be what the spec
suggests, too. Also, this way we don't have to randomly choose which longhand
should become 'initial' with 'columns:auto'.

I also added checking of the -webkit-columns shorthand property to the tests.
Seems to behave as desired.

And I generally swapped the order from column-count-then-column-width to
column-width-then-column-count, since this appears to be the canonical order.

+    return true;
+}
+
 bool CSSParser::parseShorthand(CSSPropertyID propId, const StylePropertyShorthand& shorthand, bool important)
 {
     // We try to match as many properties as possible