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 2642 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;
 }
 
+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
     // 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");
 }
 
 }