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..d10e483db8cf6c81fed99fa337489d2ca3da8e3c 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,113 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeGridTrackSize(CSSParserTokenRange
     return consumeGridBreadth(range, cssParserMode, restriction);
 }
 
+static bool consumeGridLineNames(CSSParserTokenRange& range, CSSValueList& valueList)

[Timothy Loh, 2016/03/24 04:29:31]

I think this would be clearer it returned a CSSGridLineNamesValue (we'll need to
make sure we don't update the range if we fail and callsites will need to append
the value to their lists). The function signature right now suggests that it it
will return false if it doesn't consume anything.

[rwlbuis, 2016/03/24 20:40:32]

Done.

+{
+    if (range.peek().type() != LeftBracketToken)
+        return true;
+    range.consumeIncludingWhitespace(); // Skip '['.
+    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = CSSGridLineNamesValue::create();
+    while (RefPtrWillBeRawPtr<CSSCustomIdentValue> lineName = consumeCustomIdentForGridLine(range))
+        lineNames->append(lineName.release());
+    if (range.peek().type() != RightBracketToken)
+        return false;
+    range.consumeIncludingWhitespace(); // Consume ']'.
+    valueList.append(lineNames.release());
+    return true;
+}
+
+static bool consumeGridTrackRepeatFunction(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValueList& list, bool& isAutoRepeat)

[Timothy Loh, 2016/03/24 04:29:31]

Maybe better / more consistent with everything else if the caller has to check
functionId() instead. Otherwise we'll end up consuming repeat(random junk) and
it would behave the same as if the function wasn't there.

[rwlbuis, 2016/03/24 20:40:32]

Done.

+{
+    if (range.peek().functionId() != CSSValueRepeat)
+        return false;
+
+    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) {

[Timothy Loh, 2016/03/24 04:29:31]

clearer to not negate here and swap the blocks around

[rwlbuis, 2016/03/24 20:40:32]

Done.

+        if (args.peek().type() != NumberToken || args.peek().numericValueType() != IntegerValueType)

[Timothy Loh, 2016/03/24 04:29:31]

consumePositiveInteger??

[rwlbuis, 2016/03/24 20:40:32]

That would work, but it will create a CSSValue for nothing IMHO.

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

We need to use it to handle calc() correctly. Maybe add a TODO to make a
consumeIntegerRaw or something (similar to how we have consumeNumberRaw)

+            return false;
+        double repetition = args.consumeIncludingWhitespace().numericValue();
+        if (repetition < 1)
+            return false;
+        repetitions = clampTo<size_t>(repetition, 0, kGridMaxTracks);
+        repeatedValues = CSSValueList::createSpaceSeparated();
+    } else {
+        repeatedValues = CSSGridAutoRepeatValue::create(args.consumeIncludingWhitespace().id());
+    }
+    if (!consumeCommaIncludingWhitespace(args) || !consumeGridLineNames(args, *repeatedValues))
+        return false;
+
+    size_t numberOfTracks = 0;
+    TrackSizeRestriction restriction = isAutoRepeat ? FixedSizeOnly : AllowAll;
+    while (!args.atEnd()) {

[Timothy Loh, 2016/03/24 04:29:31]

I think you can do:

do {
    ..
} while (!args.atEnd() && !isAutoRepeat);

and then remove the numberOfTracks variable and just use
repeatedValues->length() when doing the division below.

[rwlbuis, 2016/03/24 20:40:32]

I don't think this will work since then it would include grid lines as well.

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

Not sure what you mean exactly :/

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

Sorry, I meant I think we still need numberOfTracks, since it does not include
gridline names, whereas repeatedValues->length() could include them.

+        if (isAutoRepeat && numberOfTracks)
+            return false;
+        RefPtrWillBeRawPtr<CSSValue> trackSize = consumeGridTrackSize(args, cssParserMode, restriction);
+        if (!trackSize)
+            return false;
+        repeatedValues->append(trackSize.release());
+        ++numberOfTracks;
+        if (!consumeGridLineNames(args, *repeatedValues))
+            return false;
+    }
+    // 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/24 04:29:31]

I think consumeGridTrackList (or just consumeTrackList?) shouldn't include this
bit and just be responsible for parsing <track-size>|<auto-track-size>. We could
have some function like consumeGridTemplatesRowsOrColumns which handles none.
The grid shorthand is going to need to re-use this code for <track-size>.

[rwlbuis, 2016/03/24 20:40:32]

Agreed, good point, done.

+        return consumeIdent(range);
+    RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
+    if (!consumeGridLineNames(range, *values))
+        return nullptr;
+
+    bool seenTrackSizeOrRepeatFunction = false;
+    bool seenAutoRepeat = false;
+    while (!range.atEnd()) {

[Timothy Loh, 2016/03/24 04:29:31]

Maybe like this, and then you don't need seenTrackSizeOrRepeatFunction. I
generally prefer a do-while loop to indicate that you must have at least one
iteration.

do {
    ..
} while (!range.atEnd() && range.peek().type() != DelimiterToken);

[rwlbuis, 2016/03/24 20:40:32]

Done.

+        if (range.peek().type() == DelimiterToken && range.peek().delimiter() == '/')
+            break;
+        bool isAutoRepeat;
+        if (consumeGridTrackRepeatFunction(range, cssParserMode, *values, isAutoRepeat)) {
+            if (isAutoRepeat && seenAutoRepeat)
+                return nullptr;
+            seenAutoRepeat = seenAutoRepeat || isAutoRepeat;
+        } else if (RefPtrWillBeRawPtr<CSSValue> value = consumeGridTrackSize(range, cssParserMode, seenAutoRepeat ? FixedSizeOnly : AllowAll)) {
+            values->append(value.release());
+        } else {
+            return nullptr;
+        }
+        seenTrackSizeOrRepeatFunction = true;
+        if (!consumeGridLineNames(range, *values))
+            return nullptr;
+    }
+    // We should have found a <track-size> or else it is not a valid <track-list>.
+    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 && !allTracksAreFixedSized(*values))
+        return nullptr;
+    return values.release();
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -3474,6 +3583,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 consumeGridTrackList(m_range, m_context.mode());
     default:
         CSSParserValueList valueList(m_range);
         if (valueList.size()) {