[css-grid] Add parsing support for <auto-repeat> syntax

third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp

Index: third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
index 18cd6df66230f8985c8f9e28c2f6ea0d877f2eb7..6e79de8907f1fa1bbb4814a69237d69fc050e0a4 100644
--- a/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
@@ -2343,15 +2343,22 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList()
         return nullptr;
 
     bool seenTrackSizeOrRepeatFunction = false;
+    bool seenAutoRepeat = false;
     while (CSSParserValue* currentValue = m_valueList->current()) {
         if (isForwardSlashOperator(currentValue))
             break;
         if (currentValue->m_unit == CSSParserValue::Function && currentValue->function->id == CSSValueRepeat) {
-            if (!parseGridTrackRepeatFunction(*values))
+            bool isAutoRepeat;
+            if (!parseGridTrackRepeatFunction(*values, isAutoRepeat))
+                return nullptr;
+            // <auto-repeat> can only appear once in the <track-list>. It also requires definite
+            // minimum track sizes in order to compute the number of repetitions.

[Manuel Rego, 2016/01/19 16:37:47]

Nit: The 2nd sentence doesn't apply here.

The comment that it can only appear once seems enough.

[svillar, 2016/01/21 12:25:15]

Acknowledged.

+            if (isAutoRepeat && seenAutoRepeat)
                 return nullptr;
             seenTrackSizeOrRepeatFunction = true;
+            seenAutoRepeat = seenAutoRepeat || isAutoRepeat;
         } else {
-            RefPtrWillBeRawPtr<CSSValue> value = parseGridTrackSize(*m_valueList);
+            RefPtrWillBeRawPtr<CSSValue> value = parseGridTrackSize(*m_valueList, seenAutoRepeat ? FixedSizeOnly : AllowAll);
             if (!value)
                 return nullptr;
             values->append(value);
@@ -2366,17 +2373,40 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList()
     if (!seenTrackSizeOrRepeatFunction)
         return nullptr;
 
+    // <auto-repeat> requires definite minimum track sizes in order to compute the number of repetitions.
+    // The above while loop detects those appearances after the <auto-repeat> but not the ones before.
+    if (seenAutoRepeat) {
+        for (auto value : *values) {
+            if (value->isGridLineNamesValue())
+                continue;
+            ASSERT(value->isPrimitiveValue() || value->isFunctionValue());
+            const CSSPrimitiveValue& primitiveValue = value->isPrimitiveValue()
+                ? toCSSPrimitiveValue(*value)
+                : *toCSSPrimitiveValue(toCSSFunctionValue(*value).item(0));

[eae, 2016/01/19 22:47:48]

Is this guaranteed to be non-null? Mixing references and pointers like this
makes me nervous.

[svillar, 2016/01/21 12:25:15]

For the if branch we have an assertion just above the statement. For the else
part perhaps I could assert on the existance of item(0) (which is guaranteed,
there are no functions without at least 1 item).

I'll rewrite it anyway.

+            CSSValueID valueID = primitiveValue.getValueID();
+            if (valueID == CSSValueMinContent || valueID == CSSValueMaxContent || valueID == CSSValueAuto || primitiveValue.isFlex())
+                return nullptr;
+        }
+    }
+
     return values;
 }
 
-bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
+bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list, bool& isAutoRepeat)
 {
     CSSParserValueList* arguments = m_valueList->current()->function->args.get();
-    if (!arguments || arguments->size() < 3 || !validUnit(arguments->valueAt(0), FPositiveInteger) || !isComma(arguments->valueAt(1)))
+    if (!arguments || arguments->size() < 3 || !isComma(arguments->valueAt(1)))
         return false;
 
-    ASSERT(arguments->valueAt(0)->fValue > 0);
-    size_t repetitions = clampTo<size_t>(arguments->valueAt(0)->fValue, 0, kGridMaxTracks);
+    CSSParserValue* currentValue = arguments->valueAt(0);
+    isAutoRepeat = currentValue->id == CSSValueAutoFill || currentValue->id == CSSValueAutoFit;
+    if (!validUnit(currentValue, FPositiveInteger) && !isAutoRepeat)
+        return false;
+
+    // The number of repetitions for <auto-repeat> is not important at parsing level
+    // because it will be computed later, let's set it to 1.
+    size_t repetitions = isAutoRepeat ? 1 : currentValue->fValue;
+    repetitions = clampTo<size_t>(repetitions, 1, kGridMaxTracks);
 
     RefPtrWillBeRawPtr<CSSValueList> repeatedValues = CSSValueList::createSpaceSeparated();
     arguments->next(); // Skip the repetition count.
@@ -2387,8 +2417,12 @@ bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
         return false;
 
     size_t numberOfTracks = 0;
+    TrackSizeRestriction restriction = isAutoRepeat ? FixedSizeOnly : AllowAll;
     while (arguments->current()) {
-        RefPtrWillBeRawPtr<CSSValue> trackSize = parseGridTrackSize(*arguments);
+        if (isAutoRepeat && numberOfTracks)

[Manuel Rego, 2016/01/19 16:37:47]

Maybe a comment explaining that <auto-repeat>
can only define 1 track.

[svillar, 2016/01/21 12:25:15]

If you don't mind, in this case I think the code is pretty clearly saying it. I
don't think we need to justify every line of code citing the specs. Maintaining
comments is hard so let's try to avoid adding them if the code is
self-explanatory

+            return false;
+
+        RefPtrWillBeRawPtr<CSSValue> trackSize = parseGridTrackSize(*arguments, restriction);
         if (!trackSize)
             return false;
 
@@ -2419,7 +2453,7 @@ bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
 }
 
 
-PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackSize(CSSParserValueList& inputList)
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackSize(CSSParserValueList& inputList, TrackSizeRestriction restriction)
 {
     ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
 
@@ -2427,7 +2461,7 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackSize(CSSParser
     inputList.next();
 
     if (currentValue->id == CSSValueAuto)
-        return cssValuePool().createIdentifierValue(CSSValueAuto);
+        return restriction == AllowAll ? cssValuePool().createIdentifierValue(CSSValueAuto) : nullptr;
 
     if (currentValue->m_unit == CSSParserValue::Function && currentValue->function->id == CSSValueMinmax) {
         // The spec defines the following grammar: minmax( <track-breadth> , <track-breadth> )
@@ -2435,7 +2469,7 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackSize(CSSParser
         if (!arguments || arguments->size() != 3 || !isComma(arguments->valueAt(1)))
             return nullptr;
 
-        RefPtrWillBeRawPtr<CSSPrimitiveValue> minTrackBreadth = parseGridBreadth(arguments->valueAt(0));
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> minTrackBreadth = parseGridBreadth(arguments->valueAt(0), restriction);
         if (!minTrackBreadth)
             return nullptr;
 
@@ -2449,15 +2483,18 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackSize(CSSParser
         return result.release();
     }
 
-    return parseGridBreadth(currentValue);
+    return parseGridBreadth(currentValue, restriction);
 }
 
-PassRefPtrWillBeRawPtr<CSSPrimitiveValue> CSSPropertyParser::parseGridBreadth(CSSParserValue* currentValue)
+PassRefPtrWillBeRawPtr<CSSPrimitiveValue> CSSPropertyParser::parseGridBreadth(CSSParserValue* currentValue, TrackSizeRestriction restriction)
 {
     if (currentValue->id == CSSValueMinContent || currentValue->id == CSSValueMaxContent || currentValue->id == CSSValueAuto)
-        return cssValuePool().createIdentifierValue(currentValue->id);
+        return restriction == AllowAll ? cssValuePool().createIdentifierValue(currentValue->id) : nullptr;
 
     if (currentValue->unit() == CSSPrimitiveValue::UnitType::Fraction) {
+        if (restriction == FixedSizeOnly)
+            return nullptr;
+
         double flexValue = currentValue->fValue;
 
         // Fractional unit is a non-negative dimension.