[CSS Grid Layout] Fix auto-placement algorithm

[CSS Grid Layout] Fix auto-placement algorithm

In RenderGrid::placeAutoMajorAxisItemOnGrid if the empty grid area does
not fit in the minor axis direction we should discard it. That way we 
will avoid to grow in that direction at this moment, as this was 
previously done at RenderGrid::populateExplicitGridAndOrderIterator().

A simple use case to reproduce the problem would be a 2x2 grid with
grid-auto-flow property set to "row", where the only empty cell is at
row 2 and column 2. If you try to place an item with the following
properties:
    grid-row: auto;
    grid-column: span 2;

It should be inserted in a new row, however before this patch it was
inserted at row 2 using columns 2 and 3. As even when the empty grid
area does not fit in the minor axis direction, it was still being used
later to insert the item.

Updated layout test to include similar cases that don't work without
this patch.

BUG=353789
TEST=fast/css-grid-layout/grid-item-auto-placement-automatic-span.html

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

[svillar, 2014-06-04 09:48:23]

Please check the wording of the description.

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

https://codereview.chromium.org/314993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:937: emptyGridArea = nullptr;
Wouldn't it be possible to call here
createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() directly instead of
nullifying the variable?

[Manuel Rego, 2014-06-04 10:02:52]

Fixed description, I hope it's clearer now.

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

https://codereview.chromium.org/314993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:937: emptyGridArea = nullptr;
On 2014/06/04 09:48:22, svillar wrote:
> Wouldn't it be possible to call here
> createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() directly instead of
> nullifying the variable?

We're looping over the major axis, as you can read in the previous comment:
            // We need to resolve the position for every different index as the
number of cells might vary depending on it.
            // This will happen when we have "span <custom-indent>", which has a
different resolution based on the position.

So we don't have to call createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() if
the empty grid area doesn't fit. As maybe, we found an empty grid area that fits
in the next row/column or whatever.

We just need to call createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() later
if we haven't been able to find any empty grid area.

[Julien - ping for review, 2014-06-10 00:08:05]

lgtm

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html
(right):

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:92:
</div>
I would have added an extra test case instead of patching this one to get more
coverage.

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:140:
</div>
Ditto.

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

https://codereview.chromium.org/314993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:937: emptyGridArea = nullptr;
On 2014/06/04 10:02:51, Manuel Rego wrote:
> On 2014/06/04 09:48:22, svillar wrote:
> > Wouldn't it be possible to call here
> > createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() directly instead of
> > nullifying the variable?
> 
> We're looping over the major axis, as you can read in the previous comment:
>             // We need to resolve the position for every different index as
the
> number of cells might vary depending on it.
>             // This will happen when we have "span <custom-indent>", which has
a
> different resolution based on the position.

It would be good to add such a test case, that would ensure that we don't
regress it by mistake later.

I think this explanation would complement the comment above too.

[Manuel Rego, 2014-06-10 09:49:02]

Thanks for the review. I'm uploading a modified version of the patch for
landing.

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html
(right):

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:92:
</div>
On 2014/06/10 00:08:05, Julien Chaffraix - PST wrote:
> I would have added an extra test case instead of patching this one to get more
> coverage.

Ok, I've kept this as it is now and added a different grid for checking this
specific problem.

https://codereview.chromium.org/314993002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:140:
</div>
On 2014/06/10 00:08:05, Julien Chaffraix - PST wrote:
> Ditto.

The same here.

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

https://codereview.chromium.org/314993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:937: emptyGridArea = nullptr;
On 2014/06/10 00:08:05, Julien Chaffraix - PST wrote:
> It would be good to add such a test case, that would ensure that we don't
> regress it by mistake later.

That was what I was doing in the modification of the original test and now with
the new test case.

> I think this explanation would complement the comment above too.

I've modified the comment to add some more context.

[Manuel Rego, 2014-06-10 09:49:23]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-10 09:50:16]

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

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

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/11414)

[commit-bot: I haz the power, 2014-06-10 11:32:00]

Change committed as 175886