[CSS Grid Layout] Support for justify-items property.

[CSS Grid Layout] Support for justify-items property.

Partial implementation of justify-items handling in grid.

Some values still missing, like stretch, baseline and last-baseline.

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

[cbiesinger, 2014-08-22 00:39:22]

lgtm

[jfernandez, 2014-08-22 20:11:00]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-08-22 20:11:16]

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

[commit-bot: I haz the power, 2014-08-22 21:12:48]

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

[commit-bot: I haz the power, 2014-08-22 21:15:43]

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

[commit-bot: I haz the power, 2014-08-22 21:15:43]

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

[Julien - ping for review, 2014-08-22 21:22:15]

lgtm (sorry I missed this change)


The description should be clearer as it looks like it implements *all* the value
for justify-items but it's not. It should reflect the change intent.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (left):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1141: return
columnPositionAlignedWithGridContainerStart(child);
Unneeded change. We can agree / disagree over which format is more readable but
this touches the history for no good reason.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1135: switch (justifySelf) {
Unneeded variable:

switch(resolveJustification(style(), child->style()) {

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1137: // If orthogonal writing-modes, this
computes to 'Start'.
'start' (lowercase s) to match the specification.

Nit: This reads better for me but you may disagree -> For orthogonal
writing-modes, this computes to 'start'

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1138: // FIXME: grid track sizing and
positioning does not support orthogonal modes yet.
grid track sizing and position *do* not support (it's plural :))

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1149: // FIXME: grid track sizing and
positioning does not support orthogonal modes yet.
Same comments.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1190: break;
ASSERT_NOT_REACHED();

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:1622: static OverflowAlignment
initialJustifyItemsOverflowAlignment() { return OverflowAlignmentDefault; }
Again why is that change better than the existing code? (I would argue the other
way FWIW)

If we don't have a strong reason for the change, it scrambles the history for no
good reason.

[jfernandez, 2014-08-26 11:23:14]

Applied suggested changes.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (left):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1141: return
columnPositionAlignedWithGridContainerStart(child);
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> Unneeded change. We can agree / disagree over which format is more readable
but
> this touches the history for no good reason.

Acknowledged.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1135: switch (justifySelf) {
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> Unneeded variable:
> 
> switch(resolveJustification(style(), child->style()) {

Done.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1137: // If orthogonal writing-modes, this
computes to 'Start'.
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> 'start' (lowercase s) to match the specification.
> 
> Nit: This reads better for me but you may disagree -> For orthogonal
> writing-modes, this computes to 'start'

Done.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1138: // FIXME: grid track sizing and
positioning does not support orthogonal modes yet.
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> grid track sizing and position *do* not support (it's plural :))

Done.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1149: // FIXME: grid track sizing and
positioning does not support orthogonal modes yet.
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> Same comments.

Done.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1190: break;
On 2014/08/22 21:22:14, Julien Chaffraix - PST wrote:
> ASSERT_NOT_REACHED();

There is such ASSERT at the end of the function, which is also used by the
"default" case.

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/438043002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:1622: static OverflowAlignment
initialJustifyItemsOverflowAlignment() { return OverflowAlignmentDefault; }
On 2014/08/22 21:22:15, Julien Chaffraix - PST wrote:
> Again why is that change better than the existing code? (I would argue the
other
> way FWIW)
> 
> If we don't have a strong reason for the change, it scrambles the history for
no
> good reason.

Acknowledged.

[jfernandez, 2014-08-26 11:27:02]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-08-26 11:27:57]

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

[commit-bot: I haz the power, 2014-08-26 12:51:31]

Committed patchset #2 (20001) as 180899