[css-grid] New syntax for the 'grid' shorthand

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 ff919153a8c07d1fbf25947e0600df8c1ce3e0a2..762ee08fd49da2c4be9f45b2454d8d9e4a2c1918 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -4493,43 +4493,53 @@ bool CSSPropertyParser::consumeGridShorthand(bool important)
 
     m_range = rangeCopy;
 
-    // 2- <grid-auto-flow> [ <grid-auto-rows> [ / <grid-auto-columns> ]? ]
-    CSSValueList* gridAutoFlow = consumeGridAutoFlow(m_range);
-    if (!gridAutoFlow)
-        return false;
-
     CSSValue* autoColumnsValue = nullptr;
     CSSValue* autoRowsValue = nullptr;
-
-    if (!m_range.atEnd()) {
-        autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto);
-        if (!autoRowsValue)
-            return false;
-        if (consumeSlashIncludingWhitespace(m_range)) {
-            autoColumnsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto);
-            if (!autoColumnsValue)
+    CSSValue* templateRows = nullptr;
+    CSSValue* templateColumns = nullptr;
+    CSSValue* flowDirection = nullptr;
+    CSSValue* denseAlgorithm = nullptr;
+    if (identMatches<CSSValueAutoFlow>(m_range.peek().id())) {
+        // 2- [ auto-flow && dense? ] <grid-auto-rows>? / <grid-template-columns>

[Manuel Rego, 2016/09/12 20:48:17]

Maybe we could refactor the part that consumes:
  [ auto-flow && dense? ] <grid-auto-rows>?
And use it in both parts, first for rows and later for columns.

[jfernandez, 2016/09/13 12:50:30]

I thought about that, but it made parsing a bit more complicated and it's just 2
lines of code, so it didn't pay off. 

However, not that I have to modify the logic to allow 'dense' and 'auto-flow'
keywords in any order it might be useful to define an independent function.

+        consumeIdent<CSSValueAutoFlow>(m_range);
+        flowDirection = CSSPrimitiveValue::createIdentifier(CSSValueRow);
+        if (identMatches<CSSValueDense>(m_range.peek().id()))

[Manuel Rego, 2016/09/12 20:48:18]

As noted on the tests "dense" could be before "auto-flow".

[jfernandez, 2016/09/13 12:50:30]

Acknowledged.

+            denseAlgorithm = consumeIdent<CSSValueDense>(m_range);
+        if (!consumeSlashIncludingWhitespace(m_range)) {
+            if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range)))

[Manuel Rego, 2016/09/12 20:48:18]

Probably it'd be easier to read in several lines.

[jfernandez, 2016/09/13 12:50:30]

That was my first implementation, actually, and I changed it to reduce the
number of different 'if' sentences. I understand it could be a bit more legible,
but I found the whole function simpler using this approach.

                 return false;
         }
-        if (!m_range.atEnd())
+        if (!(templateColumns = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())))
             return false;

[Manuel Rego, 2016/09/12 20:48:18]

We could do after this if:
  autoColumnsValue = CSSInitialValue::createLegacyImplicit();
  templateRows = CSSInitialValue::createLegacyImplicit();

So we simplify the code later, and we don't need to check
if they're or not null.

The same could be done in the other part for autoRowsValue
and templateColumns.

[jfernandez, 2016/09/13 12:50:30]

Done.

     } else {
-        // Other omitted values are set to their initial values.
-        autoColumnsValue = CSSInitialValue::createLegacyImplicit();
-        autoRowsValue = CSSInitialValue::createLegacyImplicit();
+        // 3- <grid-template-rows> / [ auto-flow && dense? ] <grid-auto-columns>?
+        if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && consumeIdent<CSSValueAutoFlow>(m_range)))
+        return false;

[Manuel Rego, 2016/09/12 20:48:17]

Wrong indentation?

[jfernandez, 2016/09/13 12:50:30]

Done.

+        flowDirection = CSSPrimitiveValue::createIdentifier(CSSValueColumn);
+        if (identMatches<CSSValueDense>(m_range.peek().id()))
+            denseAlgorithm = consumeIdent<CSSValueDense>(m_range);
+        if (!m_range.atEnd()) {
+            if (!(autoColumnsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)))
+                return false;
+        }
     }
 
-    // if <grid-auto-columns> value is omitted, it is set to the value specified for grid-auto-rows.
-    if (!autoColumnsValue)
-        autoColumnsValue = autoRowsValue;
+    if (!m_range.atEnd())
+        return false;
+
+    CSSValueList* gridAutoFlow = CSSValueList::createSpaceSeparated();
+    gridAutoFlow->append(*flowDirection);
+    if (denseAlgorithm)
+        gridAutoFlow->append(*denseAlgorithm);
 
     // It can only be specified the explicit or the implicit grid properties in a single grid declaration.
     // The sub-properties not specified are set to their initial value, as normal for shorthands.
-    addProperty(CSSPropertyGridTemplateColumns, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
-    addProperty(CSSPropertyGridTemplateRows, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
+    addProperty(CSSPropertyGridTemplateColumns, CSSPropertyGrid, templateColumns ? *templateColumns : *CSSInitialValue::createLegacyImplicit(), important);
+    addProperty(CSSPropertyGridTemplateRows, CSSPropertyGrid, templateRows ? *templateRows : *CSSInitialValue::createLegacyImplicit(), important);
     addProperty(CSSPropertyGridTemplateAreas, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
     addProperty(CSSPropertyGridAutoFlow, CSSPropertyGrid, *gridAutoFlow, important);
-    addProperty(CSSPropertyGridAutoColumns, CSSPropertyGrid, *autoColumnsValue, important);
-    addProperty(CSSPropertyGridAutoRows, CSSPropertyGrid, *autoRowsValue, important);
+    addProperty(CSSPropertyGridAutoColumns, CSSPropertyGrid, autoColumnsValue ? *autoColumnsValue : *CSSInitialValue::createLegacyImplicit(), important);
+    addProperty(CSSPropertyGridAutoRows, CSSPropertyGrid, autoRowsValue ? *autoRowsValue : *CSSInitialValue::createLegacyImplicit(), important);
     addProperty(CSSPropertyGridColumnGap, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
     addProperty(CSSPropertyGridRowGap, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
     return true;