[CSS Grid Layout] Fix missing layout in flexible and content sized columns

[CSS Grid Layout] Fix missing layout in flexible and content sized columns

RenderGrid::logicalContentHeightForChild() is called for some items at 
the beginning of RenderGrid::layoutGridItems() from 
RenderGrid::computeUsedBreadthOfGridTracks(). This causes that the
comparison inside the for loop in RenderGrid::layoutGridItems() does not
detect width changes, so elements won't be marked as needsLayout.

So the comparison is done in RenderGrid::logicalContentHeightForChild()
and the element is marked to perform a layout if the width has changed.

The issue can be reproduced easily with a simple grid with one flexible 
or content sized column, all the available width is not used. On top
of that, when you resize the window the flexible or content sized 
columns are not updating their size properly.

Perftest Layout/auto-grid-lots-of-data.html is around 4% worse, which is
expected as we're adding a missing layout.

TEST=fast/css-grid-layout/flex-content-sized-column-use-available-width.html,fast/css-grid-layout/flex-content-sized-columns-resize.html

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

[Julien - ping for review, 2014-01-29 02:14:07]

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

https://codereview.chromium.org/146833018/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:568: if
(child->style()->logicalHeight().isPercent() ||
oldOverrideContainingBlockContentLogicalWidth !=
overrideContainingBlockContentLogicalWidth)
Quick question: why is this check in logicalContentHeight when we check the
containingBlockContentLogicalWidth?

Also I think this check is too broad. Only for specific cases would this change
matter (you hit one) but there are other cases who don't need that check (e.g.
fixed sized grid item).

[Manuel Rego, 2014-01-29 22:18:22]

On 2014/01/29 02:14:07, Julien Chaffraix - PST wrote:
> Quick question: why is this check in logicalContentHeight when we check the
> containingBlockContentLogicalWidth?

As you can see in the current file we have the following line:
child->setOverrideContainingBlockContentLogicalWidth(gridAreaBreadthForChild(child,
ForColumns, columnTracks));

So for the elements that passed through this method, the same check in
RenderView::layoutGridItems() was never true, even if the width had really
changed so the element was not marked as needsLayout.

Doing it here, before the setOverrideContainingBlockContentLogicalWidth() allows
us to mark the element as needsLayout when width changes.

> Also I think this check is too broad. Only for specific cases would this
change
> matter (you hit one) but there are other cases who don't need that check (e.g.
> fixed sized grid item).

I agree, I'll upload a new patch only marking elements as needsLayout if the
width has changed and it's in a flex length column.

[Manuel Rego, 2014-01-29 23:42:05]

New patch has been uploaded for review only issuing a layout for items in
flexible length columns.

[Manuel Rego, 2014-01-31 14:04:45]

On 2014/01/29 02:14:07, Julien Chaffraix - PST wrote:
> Quick question: why is this check in logicalContentHeight when we check the
> containingBlockContentLogicalWidth?

In the last patch I've moved this to
RenderGrid::resolveContentBasedTrackSizingFunctions().

> Also I think this check is too broad. Only for specific cases would this
change
> matter (you hit one) but there are other cases who don't need that check (e.g.
> fixed sized grid item).

And added a function to limit it to flexible and content sized columns.

Updated the tests to include content sized columns examples too.

[Julien - ping for review, 2014-02-07 00:13:57]

lgtm on the original approach.

https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html
(right):

https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:33:
testRunner.useUnfortunateSynchronousResizeMode();
O_o

https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:42:
shouldBe("getComputedStyle(oneColumnGrid, '').getPropertyValue('width')",
"getComputedStyle(oneColumnRegular, '').getPropertyValue('width')");
Can we have a check-layout.js test instead of manual checks or ref-tests that
only test layout information?

https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:49:
<p>Grid flex and content sized columns width should be updated properly when you
resize the window (you should not see the grid background in grey color). It
should work like in the float and regular cases.</p>
Which float and regular case?

https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:625: gridItem->layoutIfNeeded();
I think I understand the problem now. The original code was better as it ensured
that any call to logicalContentHeight would return the right answer.

This code seemed parachuted here for no good reason.

https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:639: bool
RenderGrid::breathIsFlexOrContentSizedForChild(RenderBox* child,
GridTrackSizingDirection direction) const
If you think of it, this check is unneeded as we only check content sized grid
tracks.

[Manuel Rego, 2014-02-07 23:13:59]

On 2014/02/07 00:13:57, Julien Chaffraix - PST wrote:
> lgtm on the original approach.

Uploaded a new patch coming back to the original approach.

>
https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
> File LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html
> (right):
> 
>
https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:33:
> testRunner.useUnfortunateSynchronousResizeMode();
> O_o

Without this the window is not resized. I've done a simple check printing
window.getComputedStyle(document.body).width and without this line the window
was not resized.

>
https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:42:
> shouldBe("getComputedStyle(oneColumnGrid, '').getPropertyValue('width')",
> "getComputedStyle(oneColumnRegular, '').getPropertyValue('width')");
> Can we have a check-layout.js test instead of manual checks or ref-tests that
> only test layout information?

I've changed this for a ref-test.

>
https://codereview.chromium.org/146833018/diff/40001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/flex-content-sized-columns-resize.html:49:
> <p>Grid flex and content sized columns width should be updated properly when
you
> resize the window (you should not see the grid background in grey color). It
> should work like in the float and regular cases.</p>
> Which float and regular case?

As it's now a ref-test I've removed the float and regular case. The regular case
was the one under "Regular 1 column".

>
https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderGrid.cpp (right):
> 
>
https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:625: gridItem->layoutIfNeeded();
> I think I understand the problem now. The original code was better as it
ensured
> that any call to logicalContentHeight would return the right answer.
> 
> This code seemed parachuted here for no good reason.

I agree, I've moved it to the original place.

>
https://codereview.chromium.org/146833018/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:639: bool
> RenderGrid::breathIsFlexOrContentSizedForChild(RenderBox* child,
> GridTrackSizingDirection direction) const
> If you think of it, this check is unneeded as we only check content sized grid
> tracks.

You're right again. logicalContentHeight() is only being called from
minContentForChild() and maxContentForChild(), which means that we're already in
a content sized grid track.

Thanks for the review.

[Manuel Rego, 2014-02-11 09:39:46]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-02-11 09:40:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rego@igalia.com/146833018/110001

[commit-bot: I haz the power, 2014-02-11 11:47:04]

Change committed as 166914