[CSS Grid Layout] Implement 'justify-items' parsing

[CSS Grid Layout] Implement 'justify-items' parsing

This change adds the property from CSS 3 Box Alignment
and implements the parsing.

As the justify-self property,  for now is protected by the CSS Grid Layout
runtime flag.
 
BUG=249451
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177297

[jfernandez, 2014-06-17 13:02:00]

Pay special attention to the approach I used to implement the
CSSComputedStyleDeclaration logic, which is different to the one used for the
align-items property. 

My understanding of the specification is that the Computed Value should be the
Specified Value, so it will be the specific RenderObject instance which should
decide whether "Auto" value computes to "Strech" or "Start"; the same applies
for the "Legacy" keyword.

[cbiesinger, 2014-06-17 22:53:34]

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
File Source/core/css/CSSPropertyNames.in (right):

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
Source/core/css/CSSPropertyNames.in:140: -webkit-justify-items
alias_for=justify-items
Why add this?

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

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4151: if (value->id ==
CSSValueCenter || value->id == CSSValueLeft || value->id == CSSValueRight) {
Don't you need an m_valueList->current() call / keep around the next() return
value?

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4153: }
else return false?

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4155: return true;
Also, I'm not 100% sure but I think you need an "if m_valueList->next() return
false;" here.

[jfernandez, 2014-06-19 15:19:14]

Applied suggested changed and added additional test cases for the "legacy"
keyword.

I had to define a new function in the StyleBuilder, specifically for the
justify-items property, since it's the only one having to deal with the "legacy"
keyword. 

I had thought on defining "ItemPositionLegacyXXX" values (with the corresponding
CSSValueLegacyXXX" mapping), but since this "legacy" keyword is only used by the
justify-items property I rejected this approach.

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
File Source/core/css/CSSPropertyNames.in (right):

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
Source/core/css/CSSPropertyNames.in:140: -webkit-justify-items
alias_for=justify-items
On 2014/06/17 22:53:33, cbiesinger wrote:
> Why add this?

Well, I had doubts as well, but I saw the alias in the align-xxx properties and
in the justify-self one.

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

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4151: if (value->id ==
CSSValueCenter || value->id == CSSValueLeft || value->id == CSSValueRight) {
On 2014/06/17 22:53:33, cbiesinger wrote:
> Don't you need an m_valueList->current() call / keep around the next() return
> value?

Done.

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4153: }
On 2014/06/17 22:53:33, cbiesinger wrote:
> else return false?

Done.

https://codereview.chromium.org/333423005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:4155: return true;
On 2014/06/17 22:53:33, cbiesinger wrote:
> Also, I'm not 100% sure but I think you need an "if m_valueList->next() return
> false;" here.

Done.

[cbiesinger, 2014-06-21 00:45:24]

lgtm with these changes

Once you upload a new version, an owner will have to lgtm.

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
File Source/core/css/CSSPropertyNames.in (right):

https://codereview.chromium.org/333423005/diff/1/Source/core/css/CSSPropertyN...
Source/core/css/CSSPropertyNames.in:140: -webkit-justify-items
alias_for=justify-items
On 2014/06/19 15:19:14, jfernandez wrote:
> On 2014/06/17 22:53:33, cbiesinger wrote:
> > Why add this?
> 
> Well, I had doubts as well, but I saw the alias in the align-xxx properties
and
> in the justify-self one. 

Please remove it.. the other properties only have it for legacy reasons, because
Flexbox was shipped using those properties before we unprefixed it.

https://codereview.chromium.org/333423005/diff/20001/Source/build/scripts/tem...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/333423005/diff/20001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:526: {% macro
apply_alignment1(property_id, alignment_type) %}
That is a terrible name for this macro. Please rename to
apply_alignment_with_legacy_keyword.

https://codereview.chromium.org/333423005/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/333423005/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/StyleRareNonInheritedData.h:196: bool
m_justifyItemsIsLegacy;
    unsigned m_justifyItemsIsLegacy : 1;

[jfernandez, 2014-06-24 16:08:40]

According to the feedback got from www-style mailing list, the current
especification draft is wrong regarding how to resolve the computed value of
justify-self/items and align-self/items when using "auto" as specified value.

http://lists.w3.org/Archives/Public/www-style/2014Jun/0333.html

[Julien - ping for review, 2014-06-24 19:04:56]

It's going in the right direction but I think it needs an extra round.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-items.html (right):

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:6: justify-items: baseline;
I think we should test last-baseline as it's a valid keyword for
<baseline-position>.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:40: 
I don't see any testing for flex-start / flex-end

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:246:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
Maybe we should test legacy + auto / stretch too.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:253:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
Would it be worth checking flex-container too as it would compute to 'stretch'
in this case?

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:265:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'end'");
That's awesome that you cover getComputedStyle with these. I would either add
testing for element.style as part of this change or file a bug about it as we
usually discover that it's broken down the road if we don't add testing.

https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:542: // FIXME: We
should clear the overflow-alignment mode here and probably
Is this FIXME still required as you have testing for that?

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

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1558: static
PassRefPtrWillBeRawPtr<CSSValueList>
valueForItemPositionWithOverflowAlignment(ItemPosition itemPosition,
OverflowAlignment overflowAlignment, bool isLegacy)
PLEASE NO BOOLEAN PARAMETERS!

You can either add an enum and use it, or just pass in a pointer to an
identifier (that can be NULL). I would lean towards the enum as it is way more
readable at the call site.

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1565:
result->append(CSSPrimitiveValue::create(overflowAlignment));
ASSERT(result.size() <= 2); as you make the assumption above that we can map it
to a Pair?

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1571: return display == FLEX ||
display == INLINE_FLEX || display == GRID || display == INLINE_GRID;
RenderStyle has some similar checks, maybe we could share the code somehow. You
don't have to do it here as I am not sure how this would work but a FIXME would
be nice.

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

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1207: if
(!parseLegacyPosition(propId, important)) {
I would avoid the negative branch as it confused me here (maybe it's just me
though). It also avoids nesting:

if (parseLegacyPosition(propId, important))
    return true;

// Rewind and try the <item-position> && <overflow-position> | auto | stretch.
m_valueList->setCurrentIndex(0);
return parseItemPositionOverflowPosition(propId, important);

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4153: if (!value || value->id !=
CSSValueLegacy || !(value = m_valueList->next()))
Per the grammar: legacy && [ left | right | center ]

That means legacy can appear before *and after* the other keywords. Btw your
test coverage doesn't handle this case.

[jfernandez, 2014-06-26 12:58:44]

Applied suggested changes.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-items.html (right):

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:6: justify-items: baseline;
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I think we should test last-baseline as it's a valid keyword for
> <baseline-position>.

Done.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:40: 
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I don't see any testing for flex-start / flex-end

Done.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:246:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Maybe we should test legacy + auto / stretch too.

Done.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:253:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'start'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Would it be worth checking flex-container too as it would compute to 'stretch'
> in this case?

Done.

https://codereview.chromium.org/333423005/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:265:
shouldBe("getComputedStyle(element, '').getPropertyValue('justify-items')",
"'end'");
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> That's awesome that you cover getComputedStyle with these. I would either add
> testing for element.style as part of this change or file a bug about it as we
> usually discover that it's broken down the road if we don't add testing.

Done.

https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/333423005/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:542: // FIXME: We
should clear the overflow-alignment mode here and probably
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Is this FIXME still required as you have testing for that?

Done.

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

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1558: static
PassRefPtrWillBeRawPtr<CSSValueList>
valueForItemPositionWithOverflowAlignment(ItemPosition itemPosition,
OverflowAlignment overflowAlignment, bool isLegacy)
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> PLEASE NO BOOLEAN PARAMETERS!
> 
> You can either add an enum and use it, or just pass in a pointer to an
> identifier (that can be NULL). I would lean towards the enum as it is way more
> readable at the call site.

I finally implemented an enumeration.

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1565:
result->append(CSSPrimitiveValue::create(overflowAlignment));
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> ASSERT(result.size() <= 2); as you make the assumption above that we can map
it
> to a Pair?

"result->length() <= 2", instead, but I've got the idea. thanks.

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1571: return display == FLEX ||
display == INLINE_FLEX || display == GRID || display == INLINE_GRID;
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> RenderStyle has some similar checks, maybe we could share the code somehow.
You
> don't have to do it here as I am not sure how this would work but a FIXME
would
> be nice.

I've finally implemented a new method "isDisplayFlexibleOrGridBox" in the
RenderStyle class, removed the static one defined in the StyleAdjuster and
adapted the already present calls to the new method.

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

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1207: if
(!parseLegacyPosition(propId, important)) {
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> I would avoid the negative branch as it confused me here (maybe it's just me
> though). It also avoids nesting:
> 
> if (parseLegacyPosition(propId, important))
>     return true;
> 
> // Rewind and try the <item-position> && <overflow-position> | auto | stretch.
> m_valueList->setCurrentIndex(0);
> return parseItemPositionOverflowPosition(propId, important);

Done.

https://codereview.chromium.org/333423005/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4153: if (!value || value->id !=
CSSValueLegacy || !(value = m_valueList->next()))
On 2014/06/24 19:04:56, Julien Chaffraix - PST wrote:
> Per the grammar: legacy && [ left | right | center ]
> 
> That means legacy can appear before *and after* the other keywords. Btw your
> test coverage doesn't handle this case.

Done.

[Julien - ping for review, 2014-06-26 17:37:20]

lgtm with additional testing.

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-items.html (right):

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:95: }
Your test coverage still doesn't cover the case [ right | left | center ] legacy
:(

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:217:
shouldBe("element.style.justifyItems", "'flex-end safe'");
Nit: Refactor these 2 lines repeated a lot into a helper function.

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

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:2051: }
I would like this to be tested. This is not possible with a layout test but it
should be possible using a method similar to
https://codereview.chromium.org/351973004 (you do a setInnerHTML and poke at the
renderer's RenderStyle to check that it is set to what you expect)

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:2068: }
Ditto.

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

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4161: if (!(value =
m_valueList->next()))
Nit: I would split this into 2 lines for readability. It's perfectly valid per
our coding style but it is a line difficult to parse.

https://codereview.chromium.org/333423005/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/333423005/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:531: LegacyPositionTrue
Maybe better naming LegacyPosition vs NotALegacyPosition (or NoLegacyPosition)

[jfernandez, 2014-06-26 22:36:05]

Applied suggested changes.

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-items.html (right):

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:95: }
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> Your test coverage still doesn't cover the case [ right | left | center ]
legacy
> :(

Done.

https://codereview.chromium.org/333423005/diff/80001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-items.html:217:
shouldBe("element.style.justifyItems", "'flex-end safe'");
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> Nit: Refactor these 2 lines repeated a lot into a helper function.

Done.

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

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:2051: }
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> I would like this to be tested. This is not possible with a layout test but it
> should be possible using a method similar to
> https://codereview.chromium.org/351973004 (you do a setInnerHTML and poke at
the
> renderer's RenderStyle to check that it is set to what you expect) 

Such logic is already covered by the provided layout test; look the the test
cases where the display is set to "grid" and "flex". 

Anyway, I think the unit testing idea is interesting; I'll try it out.

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:2068: }
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/333423005/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4161: if (!(value =
m_valueList->next()))
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> Nit: I would split this into 2 lines for readability. It's perfectly valid per
> our coding style but it is a line difficult to parse.

Done.

https://codereview.chromium.org/333423005/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/333423005/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:531: LegacyPositionTrue
On 2014/06/26 17:37:20, Julien Chaffraix - PST wrote:
> Maybe better naming LegacyPosition vs NotALegacyPosition (or NoLegacyPosition)

Done.

[jfernandez, 2014-06-27 17:08:37]

I've added a FIXME to remember the unit testing issue, which I think s not
mandatory since the mentioned cases are already covered by layout tests.

[jfernandez, 2014-06-27 17:08:49]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-06-27 17:09:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/120001

[commit-bot: I haz the power, 2014-06-27 17:10:02]

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

[commit-bot: I haz the power, 2014-06-27 17:10:03]

Failed to apply patch for Source/core/frame/UseCounter.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.cpp
  Hunk #1 FAILED at 509.
  Hunk #2 FAILED at 531.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/frame/UseCounter.cpp.rej

Patch:       Source/core/frame/UseCounter.cpp
Index: Source/core/frame/UseCounter.cpp
diff --git a/Source/core/frame/UseCounter.cpp b/Source/core/frame/UseCounter.cpp
index
a25946e078acdad385773c3e16865a4940cd90f3..fe85008d6bf1711895a17e654c181166e9552758
100644
--- a/Source/core/frame/UseCounter.cpp
+++ b/Source/core/frame/UseCounter.cpp
@@ -509,6 +509,7 @@ int
UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(int id)
     case CSSPropertyBackfaceVisibility: return 451;
     case CSSPropertyGridTemplate: return 452;
     case CSSPropertyGrid: return 453;
+    case CSSPropertyJustifyItems: return 454;
 
     // 1. Add new features above this line (don't change the assigned numbers
of the existing
     // items).
@@ -531,7 +532,7 @@ int
UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(int id)
     return 0;
 }
 
-static int maximumCSSSampleId() { return 453; }
+static int maximumCSSSampleId() { return 454; }
 
 void UseCounter::muteForInspector()
 {

[jfernandez, 2014-06-30 20:55:37]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-06-30 20:57:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/140001

[jfernandez, 2014-06-30 21:38:35]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-06-30 21:39:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/160001

[commit-bot: I haz the power, 2014-06-30 22:48:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14676)

[commit-bot: I haz the power, 2014-06-30 23:56:21]

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

[commit-bot: I haz the power, 2014-06-30 23:56:22]

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

[jfernandez, 2014-07-01 09:49:29]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-01 09:50:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/160001

[commit-bot: I haz the power, 2014-07-01 09:50:46]

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

[commit-bot: I haz the power, 2014-07-01 09:50:47]

Failed to apply patch for Source/core/css/parser/CSSPropertyParser.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/parser/CSSPropertyParser.cpp
  Hunk #1 FAILED at 1197.
  Hunk #2 succeeded at 4180 (offset 45 lines).
  Hunk #3 succeeded at 4192 (offset 45 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/css/parser/CSSPropertyParser.cpp.rej

Patch:       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
3b91cb6a1a777a5ff7a20f110313673470461037..631634d67b523b2b1dfffae7eb93053041a70e4d
100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1197,6 +1197,17 @@ bool CSSPropertyParser::parseValue(CSSPropertyID propId,
bool important)
             return false;
 
         return parseItemPositionOverflowPosition(propId, important);
+
+    case CSSPropertyJustifyItems:
+        if (!RuntimeEnabledFeatures::cssGridLayoutEnabled())
+            return false;
+
+        if (parseLegacyPosition(propId, important))
+            return true;
+
+        m_valueList->setCurrentIndex(0);
+        return parseItemPositionOverflowPosition(propId, important);
+
     case CSSPropertyGridAutoColumns:
     case CSSPropertyGridAutoRows:
         if (!RuntimeEnabledFeatures::cssGridLayoutEnabled())
@@ -4135,6 +4146,11 @@ PassRefPtrWillBeRawPtr<CSSBasicShape>
CSSPropertyParser::parseBasicShapeInset(CS
     return shape;
 }
 
+static bool isBaselinePositionKeyword(CSSValueID id)
+{
+    return id == CSSValueBaseline || id == CSSValueLastBaseline;
+}
+
 static bool isItemPositionKeyword(CSSValueID id)
 {
     return id == CSSValueStart || id == CSSValueEnd || id == CSSValueCenter
@@ -4142,15 +4158,43 @@ static bool isItemPositionKeyword(CSSValueID id)
         || id == CSSValueFlexEnd || id == CSSValueLeft || id == CSSValueRight;
 }
 
+bool CSSPropertyParser::parseLegacyPosition(CSSPropertyID propId, bool
important)
+{
+    // [ legacy && [ left | right | center ]
+
+    CSSParserValue* value = m_valueList->current();
+    if (!value)
+        return false;
+
+    if (value->id == CSSValueLegacy) {
+        value = m_valueList->next();
+        if (!value)
+            return false;
+        if (value->id != CSSValueCenter && value->id != CSSValueLeft &&
value->id != CSSValueRight)
+            return false;
+    } else if (value->id == CSSValueCenter || value->id == CSSValueLeft ||
value->id == CSSValueRight) {
+        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::parseItemPositionOverflowPosition(CSSPropertyID propId,
bool important)
 {
-    // auto | baseline | stretch | [<item-position> && <overflow-position>? ]
+    // 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;
 
-    if (value->id == CSSValueAuto || value->id == CSSValueBaseline || value->id
== CSSValueStretch) {
+    if (value->id == CSSValueAuto || value->id == CSSValueStretch ||
isBaselinePositionKeyword(value->id)) {
         if (m_valueList->next())
             return false;

[jfernandez, 2014-07-01 10:32:19]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-01 10:33:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/333423005/180001

[commit-bot: I haz the power, 2014-07-01 11:42:56]

Change committed as 177297

[Timothy Loh, 2014-07-01 12:44:51]

Just a couple of quick comments on the StyleBuilder changes:

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left):

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:565: // FIXME: We
should clear the overflow-alignment mode here and probably
I'm not sure why you've removed this FIXME, isn't the behaviour still incorrect?
If you specify inherit for one of these properties, and your parent has used a
pair and set the overflow-alignment mode, then won't we fail to inherit the
overflow-alignment mode, since we use default initial/inherit handlers? You can
see the current behaviour for initial/inherit in the generated file
StyleBuilderFunctions.cpp.

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:589:
{{apply_alignment_with_legacy_keyword('CSSPropertyJustifyItems',
'JustifyItems')}}
If you're only going to have a single property with this application rule, you
should probably put the logic in StyleBuilderCustom instead. I understand that
there's a few places where the logic can end up, and writing some documentation
on the right way to do this is on my todo list.

[jfernandez, 2014-07-01 20:54:27]

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left):

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:565: // FIXME: We
should clear the overflow-alignment mode here and probably
On 2014/07/01 12:44:51, Timothy Loh wrote:
> I'm not sure why you've removed this FIXME, isn't the behaviour still
incorrect?
> If you specify inherit for one of these properties, and your parent has used a
> pair and set the overflow-alignment mode, then won't we fail to inherit the
> overflow-alignment mode, since we use default initial/inherit handlers? You
can
> see the current behaviour for initial/inherit in the generated file
> StyleBuilderFunctions.cpp.

I added cases in the layout tests to verify it was working as defined in the
specs, but you are right, it's not working as expected in the case of "inherit"
keyword. It works fine for the "initial", at least, regarding the Computed
Value. I think the FIXME still applies in the case of the Used Value. 

I'm preparing a new patch resolving these issues, according to the deleted FIXME
comment.

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:589:
{{apply_alignment_with_legacy_keyword('CSSPropertyJustifyItems',
'JustifyItems')}}
On 2014/07/01 12:44:51, Timothy Loh wrote:
> If you're only going to have a single property with this application rule, you
> should probably put the logic in StyleBuilderCustom instead. I understand that
> there's a few places where the logic can end up, and writing some
documentation
> on the right way to do this is on my todo list.

I thought that "align-items" would eventually be allowed to use the "legacy", I
still have to clarify that with the specs editor. But you are right that for the
time being, this function is better defined in the StyleBuilderCustom. Ill cook
a new patch to do so.

[jfernandez, 2014-07-01 23:35:19]

On 2014/07/01 20:54:27, jfernandez wrote:
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (left):
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:565: // FIXME:
We
> should clear the overflow-alignment mode here and probably
> On 2014/07/01 12:44:51, Timothy Loh wrote:
> > I'm not sure why you've removed this FIXME, isn't the behaviour still
> incorrect?
> > If you specify inherit for one of these properties, and your parent has used
a
> > pair and set the overflow-alignment mode, then won't we fail to inherit the
> > overflow-alignment mode, since we use default initial/inherit handlers? You
> can
> > see the current behaviour for initial/inherit in the generated file
> > StyleBuilderFunctions.cpp.
> 
> I added cases in the layout tests to verify it was working as defined in the
> specs, but you are right, it's not working as expected in the case of
"inherit"
> keyword. It works fine for the "initial", at least, regarding the Computed
> Value. I think the FIXME still applies in the case of the Used Value. 
> 
> I'm preparing a new patch resolving these issues, according to the deleted
FIXME
> comment.
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/333423005/diff/180001/Source/build/scripts/te...
> Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:589:
> {{apply_alignment_with_legacy_keyword('CSSPropertyJustifyItems',
> 'JustifyItems')}}
> On 2014/07/01 12:44:51, Timothy Loh wrote:
> > If you're only going to have a single property with this application rule,
you
> > should probably put the logic in StyleBuilderCustom instead. I understand
that
> > there's a few places where the logic can end up, and writing some
> documentation
> > on the right way to do this is on my todo list.
> 
> I thought that "align-items" would eventually be allowed to use the "legacy",
I
> still have to clarify that with the specs editor. But you are right that for
the
> time being, this function is better defined in the StyleBuilderCustom. Ill
cook
> a new patch to do so.

The patch with the suggested changes can be reviewed here:

https://codereview.chromium.org/361113002