[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..877b50a67c3b0f54cef1f784f6af0e6869a46066 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -4472,6 +4472,26 @@ bool CSSPropertyParser::consumeGridTemplateShorthand(CSSPropertyID shorthandId,
     return consumeGridTemplateRowsAndAreasAndColumns(shorthandId, important);
 }
 
+enum AutoFlowType { GridRow, GridColumn };
+
+static CSSValueList* consumeImplicitAutoFlow(CSSParserTokenRange& range, AutoFlowType direction)

[Manuel Rego, 2016/09/14 21:14:35]

You're passing the AutoFlowType, just to set the flowDirection.
Maybe we can avoid this AutoFlowType enum.
In that case you could change this method to something like:
  static bool consumeImplicitAutoFlow(CSSParserTokenRange& range, CSSValue*&
denseAlogrithm)
You parse "auto-flow && dense?" and then set or not the "denseAlgorithm"
argument.
Of course it'll return false if there's something invalid.
Then in consumeGridShorthand() you'll set the flowDirection manually.

Another idea I'm wondering if we could parse in this method the track list too.
Maybe that would make more complex the return value and so on.

[Timothy Loh, 2016/09/16 05:40:46]

I think the suggested function header here is good.

[jfernandez, 2016/09/16 09:48:49]

I understand the point of avoiding the enum. However, I don't agree with the
benefits of the proposed function because that would imply managing the
CSSValueList outside this utility function. Notice that the solely purpose of
this function is to refactoring code that it's shared among both branches of
the main parsing logic. 

What I can do is to pass the resolved PrimitiveValue instead of the enum.
However,
I'd try to avoid creating any CSSValue until we know we are going to use it,
since
an invalid parsing would lead to an early return of the main parsing logic.

+{
+    // [ auto-flow && dense? ]
+    CSSValue* flowDirection = direction == GridRow ? CSSPrimitiveValue::createIdentifier(CSSValueRow) : CSSPrimitiveValue::createIdentifier(CSSValueColumn);
+    CSSValue* denseAlgorithm = nullptr;
+    if ((consumeIdent<CSSValueAutoFlow>(range))) {
+        denseAlgorithm = consumeIdent<CSSValueDense>(range);
+    } else {
+        if (!((denseAlgorithm = consumeIdent<CSSValueDense>(range)) && consumeIdent<CSSValueAutoFlow>(range)))
+            return nullptr;
+    }
+    CSSValueList* list = CSSValueList::createSpaceSeparated();
+    list->append(*flowDirection);
+    if (denseAlgorithm)
+        list->append(*denseAlgorithm);
+    return list;
+}
+
 bool CSSPropertyParser::consumeGridShorthand(bool important)
 {
     ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
@@ -4493,39 +4513,40 @@ 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)
+    CSSValue* templateRows = nullptr;
+    CSSValue* templateColumns = nullptr;
+    CSSValueList* gridAutoFlow = nullptr;
+    if ((gridAutoFlow = consumeImplicitAutoFlow(m_range, GridRow))) {
+        // 2- [ auto-flow && dense? ] <grid-auto-rows>? / <grid-template-columns>
+        if (consumeSlashIncludingWhitespace(m_range))
+            autoRowsValue = CSSInitialValue::createLegacyImplicit();
+        else if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range)))

[Manuel Rego, 2016/09/14 21:14:34]

For me this kind of lines are complex to understand.
In several lines this could be:
  else {
    autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto));
    if (!consumeSlashIncludingWhitespace(m_range))
      return false;
  }

[jfernandez, 2016/09/16 09:48:50]

Your proposed code is not equivalent, and incorrect BTW. The autoRowsValue must
be
nonNull if there is not a slash after the auto-flow component. I perfectly
understand
that having more lines and perhaps extra variables helps to have a more readable
code, however, I think it's better to keep the parsing code as compact as
possible and
I really think the current design is perfectly understandable. It matches better
what
the syntax state; like "if NOT a slash, then we MUST have a auto-size AND a
slash". 

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

[Manuel Rego, 2016/09/14 21:14:35]

And this one is even more complex:
3 conditions and 2 assignments in just one line.

[Timothy Loh, 2016/09/16 05:40:45]

This one I'd definitely split up

[jfernandez, 2016/09/16 09:48:50]

Again, I understand the point behind splitting up conditions, but this is 
the same case than before, but adding an additional condition. Again, it
perfectly matches what syntax states; "<grid-template-rows> / [ auto-flow " 
All these 3 components MUST be present and it that order. I don't see the
point of having 3 conditions, all of them returning false if they are not
satisfied.

[Timothy Loh, 2016/09/19 03:48:18]

OK, but let's at least use some more whitespace here:

if (!(....
    && ....
    && ....))

+            return false;
+        if (m_range.atEnd())
+            autoColumnsValue = CSSInitialValue::createLegacyImplicit();
+        else if (!(autoColumnsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)))
+            return false;
+        templateColumns = CSSInitialValue::createLegacyImplicit();
         autoRowsValue = CSSInitialValue::createLegacyImplicit();
     }
 
-    // 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;
 
     // 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, important);
+    addProperty(CSSPropertyGridTemplateRows, CSSPropertyGrid, *templateRows, important);
     addProperty(CSSPropertyGridTemplateAreas, CSSPropertyGrid, *CSSInitialValue::createLegacyImplicit(), important);
     addProperty(CSSPropertyGridAutoFlow, CSSPropertyGrid, *gridAutoFlow, important);
     addProperty(CSSPropertyGridAutoColumns, CSSPropertyGrid, *autoColumnsValue, important);