[CSS Grid Layout] Upgrade justify-content parsing to CSS3 Box Alignment spec.

Source/core/css/parser/CSSPropertyParser.cpp

Index: Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/Source/core/css/parser/CSSPropertyParser.cpp b/Source/core/css/parser/CSSPropertyParser.cpp
index 7cf43db64e2da8e058d18d0613be55d00259dd07..03c3ddcfe15cfb5dbcd0fd8d0a46252a2a09dd9e 100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1205,6 +1205,9 @@ bool CSSPropertyParser::parseValue(CSSPropertyID propId, bool important)
         return false;
     }
 
+    case CSSPropertyJustifyContent:
+        ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+        return parseContentDistributionOverflowPosition(propId, important);
     case CSSPropertyJustifySelf:
         ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
@@ -4092,6 +4095,63 @@ bool CSSPropertyParser::parseLegacyPosition(CSSPropertyID propId, bool important
     return !m_valueList->next();
 }
 
+bool CSSPropertyParser::parseContentDistributionOverflowPosition(CSSPropertyID propId, bool important)

[Timothy Loh, 2014/10/21 03:51:09]

Maybe
"PassRefPtrWillBeRawPtr<CSSValue>
CSSPropertyParser::parseContentDistributionOverflowPosition()" (or
CSSPrimitiveValue) then just return CSSValues instead of calling addProperty. In
the caller just set parsedValue to the returned value.

[jfernandez, 2014/10/27 11:44:37]

I implemented parsing methods of both forms and still not getting a clear idea
of the advantages of one over another. Personally, I'd tend to implement the one
present in this patch. 

I'll give a try, though, since I think it simplifies a bit the code because some
checks are already done by the caller, as you mentioned in other comments.

+{
+    // auto | <baseline-position> | [ <content-distribution> <content-position>? | <content-position> ] && <overflow-position>?

[Julien - ping for review, 2014/10/20 21:24:02]

The editor's draft version has a different syntax that's not totally compatible
with this one:

auto | <baseline-position> | [ <content-distribution>? && <content-position>? ]!
&& <overflow-position>?

Based on the fact that the Working Draft is 5 months old and there is a
requirement to update it every 6 months, I would implement the new syntax.

[jfernandez, 2014/10/27 11:44:37]

The current implementation already follows the new syntax, but I've forgot to
update the comment.

+    // <baseline-position> = baseline | last-baseline;
+    // <content-distribution> = space-between | space-around | space-evenly | stretch;
+    // <content-position> = center | start | end | flex-start | flex-end | left | right;
+    // <overflow-position> = true | safe
+
+    CSSParserValue* value = m_valueList->current();
+    if (!value)

[Timothy Loh, 2014/10/21 03:51:09]

We're guaranteed to have at least one value by this point so we don't need this
check

[jfernandez, 2014/10/27 11:44:37]

Done.

+        return false;
+
+    if (value->id == CSSValueAuto || isBaselinePositionKeyword(value->id)) {
+        if (m_valueList->next())

[Timothy Loh, 2014/10/21 03:51:09]

Won't need this check if we return a CSSValue as per above comment

[jfernandez, 2014/10/27 11:44:37]

I agree that we don't need to check whether next() returns true or not, but is
has to be called, since the caller of the parsing function is checking
"current()" to determine if there are extra values not allowed by the syntax. I
remember that in previous revisions the caller was checking "next()" instead.
However, latest trunk requires to call next() before returning. 

I still doubt whether we could check the next()'s value to return nullptr in
case of false, but not sure if it will bring any advantage, so next patch won't
do that.

[Timothy Loh, 2014/10/27 11:55:30]

Whoops sorry, thought the caller would advance the pointer (but that makes no
sense).

+            return false;
+
+        addProperty(propId, cssValuePool().createIdentifierValue(value->id), important);
+        return true;
+    }
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> distribution = nullptr;
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> position = nullptr;
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> overflow = nullptr;
+    if (value->id == CSSValueTrue || value->id == CSSValueSafe) {
+        overflow = cssValuePool().createIdentifierValue(value->id);
+        value = m_valueList->next();
+    }
+    if (value && CSSPrimitiveValue::isContentDistributionKeyword(value->id)) {
+        distribution = cssValuePool().createIdentifierValue(value->id);
+        value = m_valueList->next();
+    }
+    if (value && CSSPrimitiveValue::isContentPositionKeyword(value->id)) {
+        position = cssValuePool().createIdentifierValue(value->id);
+        value = m_valueList->next();
+    }
+    if (value) {
+        if (overflow || (value->id != CSSValueTrue && value->id != CSSValueSafe))
+            return false;
+        overflow = cssValuePool().createIdentifierValue(value->id);
+    }
+
+    if (m_valueList->next() || !(position || distribution))

[Timothy Loh, 2014/10/21 03:51:09]

Probably don't need to check m_valueList->next() with the above changes.

[jfernandez, 2014/10/27 11:44:37]

It seems next() call is still needed, as I commented in above.

+        return false;
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> content = nullptr;
+    if (position && distribution)
+        content = createPrimitiveValuePair(distribution, position);
+    else
+        content = position ? position : distribution;
+    if (overflow)
+        addProperty(propId, createPrimitiveValuePair(content, overflow), important);
+    else
+        addProperty(propId, content.release(), important);
+
+    return true;
+}
+
 bool CSSPropertyParser::parseItemPositionOverflowPosition(CSSPropertyID propId, bool important)
 {
     // auto | stretch | <baseline-position> | [<item-position> && <overflow-position>? ]