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

Source/core/css/CSSParser-in.cpp

 /*
  * Copyright (C) 2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Nicholas Shanks <webkit@nickshanks.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
  * Copyright (C) 2012 Intel Corporation. All rights reserved.
  *
@@ skipping 2466 matching lines @@
     case CSSPropertyTextLineThroughWidth:
     case CSSPropertyTextOverlineWidth:
     case CSSPropertyTextUnderlineWidth:
         if (id == CSSValueAuto || id == CSSValueNormal || id == CSSValueThin ||
             id == CSSValueMedium || id == CSSValueThick)
             validPrimitive = true;
         else
             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)
             validPrimitive = true;
         else
             validPrimitive = validUnit(value, FLength | FNonNeg);
         break;
     case CSSPropertyWebkitColumnAxis:
         if (id == CSSValueHorizontal || id == CSSValueVertical || id == CSSValueAuto)
             validPrimitive = true;
         break;
     case CSSPropertyWebkitColumnProgression:
         if (id == CSSValueNormal || id == CSSValueReverse)
             validPrimitive = true;
         break;
     case CSSPropertyWebkitColumnSpan: // none | all | 1 (will be dropped in the unprefixed property)
         if (id == CSSValueAll || id == CSSValueNone)
             validPrimitive = true;
         else
             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
 
     // Apple specific properties.  These will never be standardized and are purely to
     // support custom WebKit-based Apple applications.
     case CSSPropertyWebkitLineClamp:
         // When specifying number of lines, don't allow 0 as a valid value
         // When specifying either type of unit, require non-negative integers
         validPrimitive = (!id && (value->unit == CSSPrimitiveValue::CSS_PERCENTAGE || value->fValue) && validUnit(value, FInteger | FPercent | FNonNeg, CSSQuirksMode));
         break;
@@ skipping 126 matching lines @@
         // [ [ 'font-style' || 'font-variant' || 'font-weight' ]? 'font-size' [ / 'line-height' ]?
         // 'font-family' ] | caption | icon | menu | message-box | small-caption | status-bar | inherit
         if (id >= CSSValueCaption && id <= CSSValueStatusBar)
             validPrimitive = true;
         else
             return parseFont(important);
         break;
     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:
         return parseShorthand(propId, webkitTextStrokeShorthand(), important);
     case CSSPropertyWebkitAnimation:
         return parseAnimationShorthand(important);
     case CSSPropertyTransition:
     case CSSPropertyWebkitTransition:
         return parseTransitionShorthand(propId, important);
     case CSSPropertyInvalid:
@@ skipping 529 matching lines @@
             addAnimationValue(values[i], cssValuePool().createImplicitInitialValue());
     }
 
     // Now add all of the properties we found.
     for (i = 0; i < numProperties; i++)
         addPropertyWithPrefixingVariant(shorthand.properties()[i], values[i].release(), 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
     // We set up an array of booleans to mark which property has been found,
     // and we try to search for properties until it makes no longer any sense.
     ShorthandScope scope(this, propId);
 
     bool found = false;
     unsigned propertiesParsed = 0;
     bool propertyFound[6]= { false, false, false, false, false, false }; // 6 is enough size.
@@ skipping 8517 matching lines @@
 {
     // The tokenizer checks for the construct of an+b.
     // However, since the {ident} rule precedes the {nth} rule, some of those
     // tokens are identified as string literal. Furthermore we need to accept
     // "odd" and "even" which does not match to an+b.
     return equalIgnoringCase(token, "odd") || equalIgnoringCase(token, "even")
         || equalIgnoringCase(token, "n") || equalIgnoringCase(token, "-n");
 }
 
 }