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..4d7e00b8581e4804d0a1b1b2b1e9ef65a732feb5 100644
--- a/Source/core/css/CSSParser-in.cpp
+++ b/Source/core/css/CSSParser-in.cpp
@@ -2484,10 +2484,7 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
             validPrimitive = !id && validUnit(value, FNumber | FLength | FPercent);
         break;
     case CSSPropertyWebkitColumnCount:
-        if (id == CSSValueAuto)
-            validPrimitive = true;
-        else
-            validPrimitive = !id && validUnit(value, FPositiveInteger, CSSQuirksMode);
+        parsedValue = parseColumnCount();
         break;
     case CSSPropertyWebkitColumnGap:         // normal | <length>
         if (id == CSSValueNormal)
@@ -2510,10 +2507,7 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
             validPrimitive = validUnit(value, FNumber | FNonNeg) && value->fValue == 1;
         break;
     case CSSPropertyWebkitColumnWidth:         // auto | <length>
-        if (id == CSSValueAuto)
-            validPrimitive = true;
-        else // Always parse this property in strict mode, since it would be ambiguous otherwise when used in the 'columns' shorthand property.
-            validPrimitive = validUnit(value, FLength | FNonNeg, CSSStrictMode) && value->fValue;
+        parsedValue = parseColumnWidth();
         break;
     // End of CSS3 properties
 
@@ -2660,7 +2654,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 +3204,86 @@ bool CSSParser::parseTransitionShorthand(CSSPropertyID propId, bool important)
     return true;
 }
 
+PassRefPtr<CSSValue> CSSParser::parseColumnWidth()
+{
+    CSSParserValue* value = m_valueList->current();
+    // Always parse lengths in strict mode here, since it would be ambiguous otherwise when used in
+    // the 'columns' shorthand property.
+    if (value->id == CSSValueAuto
+        || validUnit(value, FLength | FNonNeg, CSSStrictMode) && value->fValue) {
+        RefPtr<CSSValue> parsedValue = parseValidPrimitive(value->id, value);
+        m_valueList->next();
+        return parsedValue;
+    }
+    return 0;
+}
+
+PassRefPtr<CSSValue> CSSParser::parseColumnCount()
+{
+    CSSParserValue* value = m_valueList->current();
+    if (value->id == CSSValueAuto
+        || (!value->id && validUnit(value, FPositiveInteger, CSSQuirksMode))) {
+        RefPtr<CSSValue> parsedValue = parseValidPrimitive(value->id, value);
+        m_valueList->next();
+        return parsedValue;
+    }
+    return 0;
+}
+
+bool CSSParser::parseColumnsShorthand(bool important)
+{
+    RefPtr <CSSValue> columnWidth;
+    RefPtr <CSSValue> columnCount;
+    bool hasPendingExplicitAuto = false;
+
+    for (unsigned propertiesParsed = 0; CSSParserValue* value = m_valueList->current(); propertiesParsed++) {
+        if (propertiesParsed >= 2)
+            return false; // Too many values for this shorthand. Invalid declaration.
+        if (!propertiesParsed && 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.
+            m_valueList->next();
+            hasPendingExplicitAuto = true;
+        } else {
+            if (columnWidth || !(columnWidth = parseColumnWidth())) {

[Julien - ping for review, 2013/07/26 20:14:39]

The second part of the 'if' is unusual in Blink. So I would prefer a slight
refactoring on the way to do the parsing:

if (!columnWidth) {
    if (columnWidth = parseColumnWidth())
        continue;
} else if (!columnCount) {
    if (columnCount = parseColumnCount())
        continue;
} else {
    // If we didn't find at least one match, this is an
    // invalid shorthand and we have to ignore it.
    return false;
}

It is a bit longer but it does underline the code flow a bit better IMHO.

[mstensho (USE GERRIT), 2013/07/26 22:20:19]

Done.

I omitted the "else"s, though. Otherwise it would fail miserably. :)

+                if (columnCount || !(columnCount = parseColumnCount())) {
+                    // If we didn't find at least one match, this is an
+                    // invalid shorthand and we have to ignore it.
+                    return false;
+                }
+            }
+        }
+    }
+    if (hasPendingExplicitAuto) {
+        // Time to assign the previously skipped 'auto' value to a property. If both properties are
+        // unassigned at this point (i.e. 'columns:auto'), it doesn't really matter which one we
+        // set. The one we don't set will get an implicit 'auto' value further down.

[Julien - ping for review, 2013/07/26 20:14:39]

Note that it does make a difference when using the web-inspector as one of the
other will be considered implicit. That said, it's definitely not super
critical.

[mstensho (USE GERRIT), 2013/07/26 22:20:19]

Right. That's undefined behavior. I guess it's problematic to have a shorthand
with arbitrary order of the longhands when a certain value is legal for both
longhands...

I'll improve the comment, since it turns out that "doesn't really matter" is a
bit on the assertive side.

+        if (!columnWidth) {
+            columnWidth = cssValuePool().createIdentifierValue(CSSValueAuto);
+        } else {
+            ASSERT(!columnCount);
+            columnCount = cssValuePool().createIdentifierValue(CSSValueAuto);
+        }
+    }
+    ASSERT(columnCount || columnWidth);
+
+    // Any unassigned property at this point will become implicit 'auto'.
+    if (columnWidth) {
+        addProperty(CSSPropertyWebkitColumnWidth, columnWidth, important);
+    } else {
+        ImplicitScope implicitScope(this, PropertyImplicit);

[Julien - ping for review, 2013/07/26 20:14:39]

Note that you don't necessarily need the ImplicitScope anymore. Just call:

addProperty(CSSPropertyWebkitColumnWidth,
cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit
*/)

[mstensho (USE GERRIT), 2013/07/26 22:20:19]

Nice! I failed to notice that. Got totally blinded by the m_implicitShorthand
thing, I suppose. Thanks. Parameters FTW!

Done.

+        addProperty(CSSPropertyWebkitColumnWidth, cssValuePool().createIdentifierValue(CSSValueAuto), important);
+    }
+    if (columnCount) {
+        addProperty(CSSPropertyWebkitColumnCount, columnCount, important);
+    } else {
+        ImplicitScope implicitScope(this, PropertyImplicit);
+        addProperty(CSSPropertyWebkitColumnCount, cssValuePool().createIdentifierValue(CSSValueAuto), important);
+    }
+    return true;
+}
+
 bool CSSParser::parseShorthand(CSSPropertyID propId, const StylePropertyShorthand& shorthand, bool important)
 {
     // We try to match as many properties as possible