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

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

Upgrade the justify-content property to the last CSS3 Box Alignment
specification. It defines a different enumeration for Positional
and Distribution alignment, which requires changes in the FexibleBox
impementation.

BUG=249451
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184660

[jfernandez, 2014-10-07 17:51:52]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org

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

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4100: // auto | <baseline-position>
| [ <content-distribution> <content-position>? | <content-position> ] &&
<overflow-position>?
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.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:225: if
(CSSPrimitiveValue::isContentDistributionKeyword(primitiveValue->getValueID()))
{
I usually say that we control the format that gets passed here, which is why we
should be able to get away without adding the CSSPrimitiveValue functions.

A good way to do that would be to define a custom CSSValue that would wrap the
parsed data.

https://codereview.chromium.org/636993002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/StyleRareNonInheritedData.h (left):

https://codereview.chromium.org/636993002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/StyleRareNonInheritedData.h:162: unsigned
m_justifyContent : 3; // EJustifyContent
Shouldn't we remove EJustifyContent?

[Timothy Loh, 2014-10-21 03:51:10]

Some drive-by comments:

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4098: bool
CSSPropertyParser::parseContentDistributionOverflowPosition(CSSPropertyID
propId, bool important)
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.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4107: if (!value)
We're guaranteed to have at least one value by this point so we don't need this
check

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4111: if (m_valueList->next())
Won't need this check if we return a CSSValue as per above comment

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4139: if (m_valueList->next() ||
!(position || distribution))
Probably don't need to check m_valueList->next() with the above changes.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:220: } else {
Seems weird to not set all three values in these cases.

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

Applied suggested changes.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4098: bool
CSSPropertyParser::parseContentDistributionOverflowPosition(CSSPropertyID
propId, bool important)
On 2014/10/21 03:51:09, Timothy Loh wrote:
> 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.

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.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4100: // auto | <baseline-position>
| [ <content-distribution> <content-position>? | <content-position> ] &&
<overflow-position>?
On 2014/10/20 21:24:02, Julien Chaffraix - PST wrote:
> 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>?
> 

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

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4107: if (!value)
On 2014/10/21 03:51:09, Timothy Loh wrote:
> We're guaranteed to have at least one value by this point so we don't need
this
> check

Done.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4111: if (m_valueList->next())
On 2014/10/21 03:51:09, Timothy Loh wrote:
> Won't need this check if we return a CSSValue as per above comment

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.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4139: if (m_valueList->next() ||
!(position || distribution))
On 2014/10/21 03:51:09, Timothy Loh wrote:
> Probably don't need to check m_valueList->next() with the above changes.

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

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:220: } else {
On 2014/10/21 03:51:09, Timothy Loh wrote:
> Seems weird to not set all three values in these cases.

Well, only explicit values are set. The ones not set remain with the initial
values defined in RenderStyle.

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:225: if
(CSSPrimitiveValue::isContentDistributionKeyword(primitiveValue->getValueID()))
{
On 2014/10/20 21:24:02, Julien Chaffraix - PST wrote:
> I usually say that we control the format that gets passed here, which is why
we
> should be able to get away without adding the CSSPrimitiveValue functions.
> 
> A good way to do that would be to define a custom CSSValue that would wrap the
> parsed data.

Good idea, thanks. Ill try that approach in the next patch.

https://codereview.chromium.org/636993002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/StyleRareNonInheritedData.h (left):

https://codereview.chromium.org/636993002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/StyleRareNonInheritedData.h:162: unsigned
m_justifyContent : 3; // EJustifyContent
On 2014/10/20 21:24:02, Julien Chaffraix - PST wrote:
> Shouldn't we remove EJustifyContent?

Acknowledged.

[Timothy Loh, 2014-10-27 11:55:29]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

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

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4111: if (m_valueList->next())
On 2014/10/27 11:44:37, jfernandez wrote:
> On 2014/10/21 03:51:09, Timothy Loh wrote:
> > Won't need this check if we return a CSSValue as per above comment
> 
> 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.

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

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:220: } else {
On 2014/10/27 11:44:37, jfernandez wrote:
> On 2014/10/21 03:51:09, Timothy Loh wrote:
> > Seems weird to not set all three values in these cases.
> 
> Well, only explicit values are set. The ones not set remain with the initial
> values defined in RenderStyle.

We can go through this function multiple times for a single RenderStyle since we
don't have a cascade pass yet (we'll currently just call this for all matching
declarations in increasing priority order), so they won't necessarily be the
initial values.

[jfernandez, 2014-10-27 14:09:24]

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/636993002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleBuilderCustom.cpp:220: } else {
On 2014/10/27 11:55:30, Timothy Loh wrote:
> On 2014/10/27 11:44:37, jfernandez wrote:
> > On 2014/10/21 03:51:09, Timothy Loh wrote:
> > > Seems weird to not set all three values in these cases.
> > 
> > Well, only explicit values are set. The ones not set remain with the initial
> > values defined in RenderStyle.
> 
> We can go through this function multiple times for a single RenderStyle since
we
> don't have a cascade pass yet (we'll currently just call this for all matching
> declarations in increasing priority order), so they won't necessarily be the
> initial values.

Oh, I see now. I'll assign the 2 values as you suggested in the next patch.
Thanks for the comments.

[Julien - ping for review, 2014-10-28 17:23:05]

lgtm

https://codereview.chromium.org/636993002/diff/20001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-content.html (right):

https://codereview.chromium.org/636993002/diff/20001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-content.html:162:
shouldBe("getComputedStyle(justifyContentAuto,
'').getPropertyValue('justify-content')", "'start'");
We could use shouldBeEqualToString here, which removes the double quoting on the
expected result.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1560:
style.setJustifyContent(ContentPositionStart);
Mutating the RenderStyle is not an option AFAICT: we don't want getComputedStyle
to have a potential side effect (apart from the usual style / layout trigger).

A better approach would be to just return the resolved value (per
http://dev.w3.org/csswg/cssom/#resolved-values) for align-content.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
File Source/core/css/CSSContentDistributionValue.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:3: // found in the LICENSE fil
LICENSE fil again

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:31: result.appendLiteral(" ");
We could simplify this if you just used a space-separated CSSValueList
(::createSpaceSeparated()).

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:32:
result.append(position()->customCSSText());
This creates a temporary object but valueName is statically linked.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
File Source/core/css/CSSContentDistributionValue.h (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.h:3: // found in the LICENSE fil
LICENSE fil? :)

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.h:32: void
traceAfterDispatch(Visitor* visitor) { CSSValue::traceAfterDispatch(visitor); }
Do we need that as it calls the base class?

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4105: if (value->id == CSSValueTrue
|| value->id == CSSValueSafe) {
We should add a helper function for these too!

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4123: // [ <content-distribution>?
&& <content-position>? ]!
It would be better to state it in English:

// The grammar states that we should have at most <content-distribution> or
<content-position> ([ <content-distribution>? && <content-position>? ]!).

[jfernandez, 2014-10-29 10:06:27]

jfernandez@igalia.com changed reviewers:
+ ager@chromium.org

[jfernandez, 2014-10-29 11:03:05]

https://codereview.chromium.org/636993002/diff/20001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-content.html (right):

https://codereview.chromium.org/636993002/diff/20001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-content.html:162:
shouldBe("getComputedStyle(justifyContentAuto,
'').getPropertyValue('justify-content')", "'start'");
On 2014/10/28 17:23:04, Julien Chaffraix - PST wrote:
> We could use shouldBeEqualToString here, which removes the double quoting on
the
> expected result.

Done.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1560:
style.setJustifyContent(ContentPositionStart);
On 2014/10/28 17:23:04, Julien Chaffraix - PST wrote:
> Mutating the RenderStyle is not an option AFAICT: we don't want
getComputedStyle
> to have a potential side effect (apart from the usual style / layout trigger).
> 

Agree. Actually, this logic is not needed since the computed style is already
resolved by the StyleAdjuster during the cascade.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
File Source/core/css/CSSContentDistributionValue.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:3: // found in the LICENSE fil
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> LICENSE fil again

Done.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:31: result.appendLiteral(" ");
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> We could simplify this if you just used a space-separated CSSValueList
> (::createSpaceSeparated()).

Acknowledged.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.cpp:32:
result.append(position()->customCSSText());
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> This creates a temporary object but valueName is statically linked.

Done.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
File Source/core/css/CSSContentDistributionValue.h (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.h:3: // found in the LICENSE fil
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> LICENSE fil? :)

Done.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/CSSConte...
Source/core/css/CSSContentDistributionValue.h:32: void
traceAfterDispatch(Visitor* visitor) { CSSValue::traceAfterDispatch(visitor); }
Well, the class doesn't hold any reference to be handled by the GC, so I think
just calling the empty definition in the base class makes sense. 

The CSSContentDistributionValue implementation will be called by the generic
CSSValue::trace() method, so implementing the prototype for this class make
sense, as far as I could understand how olipan works. Adding Mads Ager to the CC
just in case he could provide any info on this.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4105: if (value->id == CSSValueTrue
|| value->id == CSSValueSafe) {
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> We should add a helper function for these too!

Done.

https://codereview.chromium.org/636993002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4123: // [ <content-distribution>?
&& <content-position>? ]!
On 2014/10/28 17:23:05, Julien Chaffraix - PST wrote:
> It would be better to state it in English:
> 
> // The grammar states that we should have at most <content-distribution> or
> <content-position> ([ <content-distribution>? && <content-position>? ]!).

Done.

[jfernandez, 2014-10-29 11:49:07]

Patchset #3 (id:40001) has been deleted

[jfernandez, 2014-10-29 11:49:48]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-29 11:50:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/60001

[commit-bot: I haz the power, 2014-10-29 12:12:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 12:12:06]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[jfernandez, 2014-10-29 13:17:32]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-29 13:18:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/80001

[commit-bot: I haz the power, 2014-10-29 14:08:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 14:08:55]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[jfernandez, 2014-10-29 14:42:46]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-29 14:43:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/100001

[commit-bot: I haz the power, 2014-10-29 15:29:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 15:29:21]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[jfernandez, 2014-10-29 16:02:36]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-29 16:02:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/120001

[commit-bot: I haz the power, 2014-10-29 17:39:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 17:39:15]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/34217)

[jfernandez, 2014-10-30 12:49:59]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-30 12:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/140001

[commit-bot: I haz the power, 2014-10-30 13:35:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 13:35:43]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[jfernandez, 2014-10-30 15:16:12]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-30 15:17:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/160001

[jfernandez, 2014-10-30 16:03:31]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-10-30 16:03:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636993002/180001

[commit-bot: I haz the power, 2014-10-30 18:04:11]

Committed patchset #9 (id:180001) as 184660