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