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

Source/core/css/parser/CSSPropertyParser.cpp

 /*
  * Copyright (C) 2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Nicholas Shanks <webkit@nickshanks.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
  * Copyright (C) 2012 Intel Corporation. All rights reserved.
  *
@@ skipping 1187 matching lines @@
     case CSSPropertyWebkitTransitionDuration:
     case CSSPropertyWebkitTransitionTimingFunction:
     case CSSPropertyWebkitTransitionProperty: {
         if (RefPtrWillBeRawPtr<CSSValueList> val = parseAnimationPropertyList(propId)) {
             addPropertyWithPrefixingVariant(propId, val.release(), important);
             return true;
         }
         return false;
     }
 
+    case CSSPropertyJustifyContent:
+        ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+        return parseContentDistributionOverflowPosition(propId, important);
     case CSSPropertyJustifySelf:
         ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
     case CSSPropertyJustifyItems:
         ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
 
         if (parseLegacyPosition(propId, important))
             return true;
 
         m_valueList->setCurrentIndex(0);
@@ skipping 2867 matching lines @@
         if (!m_valueList->next() || m_valueList->current()->id != CSSValueLegacy)
             return false;
     } else {
         return false;
     }
 
     addProperty(propId, createPrimitiveValuePair(cssValuePool().createIdentifierValue(CSSValueLegacy), cssValuePool().createIdentifierValue(value->id)), 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>? ]
     // <baseline-position> = baseline | last-baseline;
     // <item-position> = center | start | end | self-start | self-end | flex-start | flex-end | left | right;
     // <overflow-position> = true | safe
 
     CSSParserValue* value = m_valueList->current();
     if (!value)
         return false;
@@ skipping 4069 matching lines @@
         parsedValues->append(CSSPrimitiveValue::createIdentifier(CSSValueFill));
     if (!seenStroke)
         parsedValues->append(CSSPrimitiveValue::createIdentifier(CSSValueStroke));
     if (!seenMarkers)
         parsedValues->append(CSSPrimitiveValue::createIdentifier(CSSValueMarkers));
 
     return parsedValues.release();
 }
 
 } // namespace blink