Move grid-template-columns/grid-template-rows into CSSPropertyParser

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

Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index c0049bd1d317bebfea3b367c02af5217b56fc15f..7091a33609add0c23fa5eb9844db8599f1c533d9 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -16,6 +16,8 @@
 #include "core/css/CSSFontFeatureValue.h"
 #include "core/css/CSSFunctionValue.h"
 #include "core/css/CSSGradientValue.h"
+#include "core/css/CSSGridAutoRepeatValue.h"
+#include "core/css/CSSGridLineNamesValue.h"
 #include "core/css/CSSImageSetValue.h"
 #include "core/css/CSSPaintValue.h"
 #include "core/css/CSSPathValue.h"
@@ -3102,6 +3104,117 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeGridTrackSize(CSSParserTokenRange
     return consumeGridBreadth(range, cssParserMode, restriction);
 }
 
+static PassRefPtrWillBeRawPtr<CSSGridLineNamesValue> consumeGridLineNames(CSSParserTokenRange& range)
+{
+    if (range.peek().type() != LeftBracketToken)
+        return nullptr;
+    CSSParserTokenRange rangeCopy = range;
+    rangeCopy.consumeIncludingWhitespace(); // Skip '['.

[Timothy Loh, 2016/03/29 02:47:39]

Not a fan of the two comments here, at least because they're using different
words. Maybe just declare rangeCopy first, and then change both checks to

if (range.consumeIncludingWhitespace().type() != XBracketToken)
    return nullptr;

[rwlbuis, 2016/03/29 21:15:51]

Done.

+    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = CSSGridLineNamesValue::create();
+    while (RefPtrWillBeRawPtr<CSSCustomIdentValue> lineName = consumeCustomIdentForGridLine(rangeCopy))
+        lineNames->append(lineName.release());
+    if (rangeCopy.peek().type() != RightBracketToken)
+        return nullptr;
+    rangeCopy.consumeIncludingWhitespace(); // Consume ']'.
+    range = rangeCopy;
+    return lineNames.release();
+}
+
+static bool consumeGridTrackRepeatFunction(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValueList& list, bool& isAutoRepeat)
+{
+    CSSParserTokenRange args = consumeFunction(range);
+    // 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 = 1;
+    isAutoRepeat = identMatches<CSSValueAutoFill, CSSValueAutoFit>(args.peek().id());
+    RefPtrWillBeRawPtr<CSSValueList> repeatedValues;
+    if (isAutoRepeat) {
+        repeatedValues = CSSGridAutoRepeatValue::create(args.consumeIncludingWhitespace().id());
+    } else {
+        if (args.peek().type() != NumberToken || args.peek().numericValueType() != IntegerValueType)
+            return false;
+        double repetition = args.consumeIncludingWhitespace().numericValue();
+        if (repetition < 1)
+            return false;
+        repetitions = clampTo<size_t>(repetition, 0, kGridMaxTracks);
+        repeatedValues = CSSValueList::createSpaceSeparated();
+    }
+    if (!consumeCommaIncludingWhitespace(args))
+        return false;
+    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = consumeGridLineNames(args);
+    if (lineNames)
+        repeatedValues->append(lineNames.release());
+
+    size_t numberOfTracks = 0;
+    TrackSizeRestriction restriction = isAutoRepeat ? FixedSizeOnly : AllowAll;
+    while (!args.atEnd()) {
+        if (isAutoRepeat && numberOfTracks)
+            return false;
+        RefPtrWillBeRawPtr<CSSValue> trackSize = consumeGridTrackSize(args, cssParserMode, restriction);
+        if (!trackSize)
+            return false;
+        repeatedValues->append(trackSize.release());
+        ++numberOfTracks;
+        lineNames = consumeGridLineNames(args);
+        if (lineNames)
+            repeatedValues->append(lineNames.release());
+    }
+    // We should have found at least one <track-size> or else it is not a valid <track-list>.
+    if (!numberOfTracks)
+        return false;
+
+    if (isAutoRepeat) {
+        list.append(repeatedValues.release());
+    } else {
+        // We clamp the repetitions to a multiple of the repeat() track list's size, while staying below the max grid size.
+        repetitions = std::min(repetitions, kGridMaxTracks / numberOfTracks);
+        for (size_t i = 0; i < repetitions; ++i) {
+            for (size_t j = 0; j < repeatedValues->length(); ++j)
+                list.append(repeatedValues->item(j));
+        }
+    }
+    return true;
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeGridTrackList(CSSParserTokenRange& range, CSSParserMode cssParserMode)
+{
+    if (range.peek().id() == CSSValueNone)

[Timothy Loh, 2016/03/29 02:47:39]

I think you forgot to remove this from here since you added it to
consumeGridTemplatesRowsOrColumns

[rwlbuis, 2016/03/29 21:15:50]

Done.

+        return consumeIdent(range);
+    RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
+    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = consumeGridLineNames(range);
+    if (lineNames)
+        values->append(lineNames.release());
+
+    bool seenAutoRepeat = false;
+    do {

[Timothy Loh, 2016/03/29 02:47:39]

This allows <line-names> directly before <auto-repeat>, and allows a leading [
<fixed-size> | <fixed-repeat> ] to be omitted. Looks like the existing code also
gets these cases wrong so we could just add a TODO for them.

[svillar, 2016/03/29 08:27:36]

That's right. If we specify <line-names> then there should be a track size
declaration as well. I agree it should be fixed after the move.

[rwlbuis, 2016/03/29 21:15:51]

Done.

[Timothy Loh, 2016/03/30 00:15:39]

Not sure the TODO is accurate, or I'm misunderstanding it. I would've written
something like "<line-names> should not be able to directly precede
<auto-repeat>"

+        bool isAutoRepeat;
+        if (range.peek().functionId() == CSSValueRepeat) {
+            if (!consumeGridTrackRepeatFunction(range, cssParserMode, *values, isAutoRepeat) || (isAutoRepeat && seenAutoRepeat))

[svillar, 2016/03/29 08:27:36]

I would prefer this to be in two different conditions because
consumeGridTrackRepeatFunction() modifies the value of isAutoRepeat (someone
"optimizing" the code could wrongly decide to put the second condition in the
first place with the hope of having cheaper checks)

[rwlbuis, 2016/03/29 21:15:50]

Done.

+                return nullptr;
+            seenAutoRepeat = seenAutoRepeat || isAutoRepeat;
+        } else if (RefPtrWillBeRawPtr<CSSValue> value = consumeGridTrackSize(range, cssParserMode, seenAutoRepeat ? FixedSizeOnly : AllowAll)) {
+            values->append(value.release());
+        } else {
+            return nullptr;
+        }
+        lineNames = consumeGridLineNames(range);
+        if (lineNames)
+            values->append(lineNames.release());
+    } while (!range.atEnd() && range.peek().type() != DelimiterToken);
+    // <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 && !allTracksAreFixedSized(*values))
+        return nullptr;
+    return values.release();
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeGridTemplatesRowsOrColumns(CSSParserTokenRange& range, CSSParserMode cssParserMode)
+{
+    if (range.peek().id() == CSSValueNone)
+        return consumeIdent(range);
+    return consumeGridTrackList(range, cssParserMode);
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -3474,6 +3587,10 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
     case CSSPropertyGridAutoRows:
         ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
         return consumeGridTrackSize(m_range, m_context.mode());
+    case CSSPropertyGridTemplateColumns:
+    case CSSPropertyGridTemplateRows:
+        ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+        return consumeGridTemplatesRowsOrColumns(m_range, m_context.mode());
     default:
         CSSParserValueList valueList(m_range);
         if (valueList.size()) {