[CSS Grid Layout] Support sparse in auto-placement algorithm

[CSS Grid Layout] Support sparse in auto-placement algorithm

Sparse mode for auto-placement algorithm was not implemented yet.

This patch implements sparse mode for auto-placement algorithm. It keeps
track of the auto-placement cursor in
RenderGrid::placeAutoMajorAxisItemsOnGrid() and updates it accordingly
when auto-positioned items are placed.
If we're in dense mode it resets the cursor after each item.

GridIterator has been adapted to look for empty areas from a given
position in both directions.

Test cases have been adapted accordingly, adding new cases to cover both
sparse and dense options.

TEST=fast/css-grid-layout/grid-auto-flow-sparse.html

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

[svillar, 2014-07-11 09:29:02]

On 2014/07/01 10:58:55, Manuel Rego wrote:

Informally reviewing the change looks awesome to me.

[Julien - ping for review, 2014-07-14 16:22:39]

lgtm

https://codereview.chromium.org/362733002/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/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:104:
<div class="grid gridAutoFlowRow">
Shouldn't this be gridAutoFlowRowDense as the results were generated for the
dense packing algorithm?

If I am missing something, I would rather have a strict separation between
sparse and dense testing.

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:282:
<div class="grid gridAutoFlowColumn gridNamedGridLinesRows">
Same comment here.

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

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:94:
<div class="grid gridAutoFlowColumn">
Ditto.

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/resources/grid.css (right):

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/resources/grid.css:162: .gridAutoFlowRow {
I would put a FIXME to rename that to gridAutoFlowRowSparse to make it obvious
we are using the sparse algorithm.

[Manuel Rego, 2014-07-14 20:10:03]

Thanks for the review.

I've some doubts regarding the tests comments, could you take another look?

https://codereview.chromium.org/362733002/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/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:104:
<div class="grid gridAutoFlowRow">
On 2014/07/14 16:22:39, Julien Chaffraix - PST wrote:
> Shouldn't this be gridAutoFlowRowDense as the results were generated for the
> dense packing algorithm?

No, as I'm adding sparse cases to this test, so we'll have both sparse and
dense cases in the same test.

This test is checking that spaning is suppored in auto-placement, so I've just
kept the "old" cases using dense now, and adding the corresponding examples
if we were using sparse instead.

> If I am missing something, I would rather have a strict separation between
> sparse and dense testing.

Actually I'm doing different changes in the tests:

1) Keeping the "old" behaviour in the unrelated tests like (just replacing
gridAutoFlowRow by gridAutoFlowRowDense):
* grid-container-change-explicit-grid-recompute-child.html
* grid-item-removal-auto-placement-update.html
* named-grid-lines-with-named-grid-areas-resolution.html

2) Improve tests adding new cases for sparse mode, keeping "old" dense
cases too:
* grid-auto-flow-resolution.html
* grid-item-auto-placement-definite-span.html
* grid-item-auto-placement-automatic-span.html

3) Add new explicit test for sparse mode:
* grid-auto-flow-sparse.html

If you think it's better, I could remove the new cases added in 2) and simply
use dense always and add the new test in 3).
What do you think?

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/resources/grid.css (right):

https://codereview.chromium.org/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/resources/grid.css:162: .gridAutoFlowRow {
On 2014/07/14 16:22:39, Julien Chaffraix - PST wrote:
> I would put a FIXME to rename that to gridAutoFlowRowSparse to make it obvious
> we are using the sparse algorithm.

I agree, I'll add the FIXME and provide a patch to do the rename.

[Julien - ping for review, 2014-07-14 21:15:07]

https://codereview.chromium.org/362733002/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/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:104:
<div class="grid gridAutoFlowRow">
On 2014/07/14 20:10:03, Manuel Rego wrote:
> On 2014/07/14 16:22:39, Julien Chaffraix - PST wrote:
> > Shouldn't this be gridAutoFlowRowDense as the results were generated for the
> > dense packing algorithm?
> 
> No, as I'm adding sparse cases to this test, so we'll have both sparse and
> dense cases in the same test.
> 
> This test is checking that spaning is suppored in auto-placement, so I've just
> kept the "old" cases using dense now, and adding the corresponding examples
> if we were using sparse instead.
> 
> > If I am missing something, I would rather have a strict separation between
> > sparse and dense testing.
> 
> Actually I'm doing different changes in the tests:
> 
> 1) Keeping the "old" behaviour in the unrelated tests like (just replacing
> gridAutoFlowRow by gridAutoFlowRowDense):
> * grid-container-change-explicit-grid-recompute-child.html
> * grid-item-removal-auto-placement-update.html
> * named-grid-lines-with-named-grid-areas-resolution.html
> 
> 2) Improve tests adding new cases for sparse mode, keeping "old" dense
> cases too:
> * grid-auto-flow-resolution.html
> * grid-item-auto-placement-definite-span.html
> * grid-item-auto-placement-automatic-span.html
> 
> 3) Add new explicit test for sparse mode:
> * grid-auto-flow-sparse.html
> 
> If you think it's better, I could remove the new cases added in 2) and simply
> use dense always and add the new test in 3).
> What do you think?

I prefer to keep them separated so add the new cases in 2) into the new test 3).

[Manuel Rego, 2014-07-14 21:45:36]

https://codereview.chromium.org/362733002/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/362733002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:104:
<div class="grid gridAutoFlowRow">
On 2014/07/14 21:15:06, Julien Chaffraix - PST wrote:
> I prefer to keep them separated so add the new cases in 2) into the new test
3).

Moving them to 3) seems not very clean, because of some of them have a
quite specific CSS that should be duplicated in the new test or moved to
resources/grid.css (even when it's not generic at all).

So, probably if you prefer to keep them separated, I'd remove the new cases
from 2) and then only move to 3) the ones that are really adding new coverage
if any.

[Manuel Rego, 2014-07-16 08:17:01]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-07-16 08:17:45]

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

[commit-bot: I haz the power, 2014-07-16 09:47:56]

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

[commit-bot: I haz the power, 2014-07-16 10:48:02]

Change committed as 178262