[CSS Grid Layout] Remove stack from grid-auto-flow syntax

[CSS Grid Layout] Remove stack from grid-auto-flow syntax

"stack" mode for grid-auto-flow property has been removed from the grid
layout spec:
http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow

New syntax is:
[ row | column ] || dense

Modified parsing in order to adapt it to the new syntax.

It introduces a new flag in order to detect if the grid has or not
auto-placed items and avoid to dirty the grid in some situations.

Also the current behavior relying on "stack" has been updated following
the spec. Now it won't be possible to mimic the old "none" (or "stack")
unless you specify manually the grid-placement properties.

BUG=384099

[Manuel Rego, 2014-12-26 15:35:15]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Manuel Rego, 2014-12-26 15:35:16]

This is related to another CL:
https://codereview.chromium.org/826893002/

Here I'm trying to avoid marking the grid as dirty when it doesn't have
auto-placed items.
However, I've run the performance tests and I got similar values than the patch
in the other CL.

So I'm not sure if it's worth to add a flag like this and all the complex code
we have in RenderGrid::addChild() and RenderGrid::removeChild().

[Julien - ping for review, 2015-01-02 10:04:18]

On 2014/12/26 at 15:35:16, rego wrote:
> This is related to another CL:
> https://codereview.chromium.org/826893002/
> 
> Here I'm trying to avoid marking the grid as dirty when it doesn't have
auto-placed items.
> However, I've run the performance tests and I got similar values than the
patch in the other CL.
> 
> So I'm not sure if it's worth to add a flag like this and all the complex code
we have in RenderGrid::addChild() and RenderGrid::removeChild().

I don't think we have good coverage of this case as the PerformanceTests are
stressing layout, not grid mutation. The best course would be adding some tests,
without that it's difficult to justify the extra logic for the flag.

[Manuel Rego, 2015-01-02 14:18:42]

On 2015/01/02 10:04:18, Julien Chaffraix - CET wrote:
> On 2014/12/26 at 15:35:16, rego wrote:
> > This is related to another CL:
> > https://codereview.chromium.org/826893002/
> > 
> > Here I'm trying to avoid marking the grid as dirty when it doesn't have
> auto-placed items.
> > However, I've run the performance tests and I got similar values than the
> patch in the other CL.
> > 
> > So I'm not sure if it's worth to add a flag like this and all the complex
code
> we have in RenderGrid::addChild() and RenderGrid::removeChild().
> 
> I don't think we have good coverage of this case as the PerformanceTests are
> stressing layout, not grid mutation. The best course would be adding some
tests,
> without that it's difficult to justify the extra logic for the flag.

Thanks for the suggestion.

I've created 2 perftests that add items on a 100x100 grid
(uploaded in a new version of the patch for reference).
In the auto-positioned items test this patch has a result 6% better
than the patch without the flag
(https://codereview.chromium.org/826893002).

The difference between both patches is: http://sprunge.us/eILF?diff

I'm not sure if this use case is important enough to keep all that
complex code in add/removeChild().
What do you think?

[Manuel Rego, 2015-01-02 22:13:34]

On 2015/01/02 14:18:42, Manuel Rego wrote:
> In the auto-positioned items test this patch has a result 6% better

Of course this is a typo, the auto-positioned items has
the same result with or without the flag.
The one improving 6% is the positioned items test.

[Julien - ping for review, 2015-01-05 13:11:35]

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (left):

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:59: grid-auto-flow:
stack row row;
We could keep some of these tests to make sure they are not supported.

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
File LayoutTests/ietestcenter/css3/grid/grid-column-002.htm (right):

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
LayoutTests/ietestcenter/css3/grid/grid-column-002.htm:91: <div id="container">
Those are external tests so we avoid making big changes to them.

Furthermore I don't think the change is correct, even if it will test and
provide a similar result. Both elements ended up in the same grid cell and thus
the the second was painting above as it is last. Here you mix a positioned and a
non-positioned object to achieve the same result (ie a lot more unneeded
complexity)

https://codereview.chromium.org/826893003/diff/20001/PerformanceTests/Layout/...
File PerformanceTests/Layout/grid-add-auto-positioned-items.html (right):

https://codereview.chromium.org/826893003/diff/20001/PerformanceTests/Layout/...
PerformanceTests/Layout/grid-add-auto-positioned-items.html:44:
PerfTestRunner.measureTime(createGridTest(100));
It's better to measure runs/sec instead of time. This is because we can be gated
by the vsync unless we do a forced layout but more importantly we can run into
time measurement issues if the test is fast enough.

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

https://codereview.chromium.org/826893003/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3814: switch (firstId) {
We could probably simplify this code now that we basically have 2 real cases to
handle (row or column with and without dense).

https://codereview.chromium.org/826893003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.h (right):

https://codereview.chromium.org/826893003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.h:69: void addChildToIndexesMap(RenderBox&
child);
Unneeded parameter (the name of the function makes it obvious what it is).

[Julien - ping for review, 2015-01-05 13:14:20]

On 2015/01/02 at 22:13:34, rego wrote:
> On 2015/01/02 14:18:42, Manuel Rego wrote:
> > In the auto-positioned items test this patch has a result 6% better
> 
> Of course this is a typo, the auto-positioned items has
> the same result with or without the flag.
> The one improving 6% is the positioned items test.

I can't say if 6% is statistically significant without knowing your margin of
error. Assuming it is, 6% on only one of the tests seems pretty small IMHO to
justify the extra code complexity. We can always revise this decision in light
of new evidence.

[Manuel Rego, 2015-01-08 12:32:51]

On 2015/01/05 13:14:20, Julien Chaffraix - CET wrote:
> On 2015/01/02 at 22:13:34, rego wrote:
> > On 2015/01/02 14:18:42, Manuel Rego wrote:
> > > In the auto-positioned items test this patch has a result 6% better
> > 
> > Of course this is a typo, the auto-positioned items has
> > the same result with or without the flag.
> > The one improving 6% is the positioned items test.
> 
> I can't say if 6% is statistically significant without knowing your margin of
> error. Assuming it is, 6% on only one of the tests seems pretty small IMHO to
> justify the extra code complexity. We can always revise this decision in light
> of new evidence.

I've used PerfTestRunner.measureRunsPerSecond() and the results are
different now.

I've used the following tests:
http://people.igalia.com/mrego/css/grid/add-items-perftests/grid-add-auto-pos...
http://people.igalia.com/mrego/css/grid/add-items-perftests/grid-add-position...

And run them 3 times with CL 826893002 (without flag)
and 3 times this CL 826893003 (with flag).

You can check the results at:
http://people.igalia.com/mrego/css/grid/add-items-perftests/

The "grid-add-positioned-items" test improved ~2.2% with the flag,
but the "grid-add-auto-positioned-items" has a ~1.8% worse result.

So, I guess that this data could be enough to discard the flag at this moment.

If you agree, I'll close this CL and apply the changes from your review in
https://codereview.chromium.org/826893002/. Uploading a new patch
for review there.

[Manuel Rego, 2015-01-08 12:50:29]

Thanks for the review. Just a few comments.

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (left):

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:59: grid-auto-flow:
stack row row;
On 2015/01/05 13:11:34, Julien Chaffraix - CET wrote:
> We could keep some of these tests to make sure they are not supported.

Sure, I'll keep some of them.

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
File LayoutTests/ietestcenter/css3/grid/grid-column-002.htm (right):

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
LayoutTests/ietestcenter/css3/grid/grid-column-002.htm:91: <div id="container">
On 2015/01/05 13:11:34, Julien Chaffraix - CET wrote:
> Those are external tests so we avoid making big changes to them.

The problem here is that they're wrong according to the spec now.
So we could remove them or update them.
We already updated them in the past to add "stack" for example.

> Furthermore I don't think the change is correct, even if it will test and
> provide a similar result. Both elements ended up in the same grid cell and
thus
> the the second was painting above as it is last. Here you mix a positioned and
a
> non-positioned object to achieve the same result (ie a lot more unneeded
> complexity)

Yeah, probably a best change is to make both items to be on the first cell
using the grid placement properties.
Otherwise, we cannot reproduce this behavior.

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

https://codereview.chromium.org/826893003/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3814: switch (firstId) {
On 2015/01/05 13:11:34, Julien Chaffraix - CET wrote:
> We could probably simplify this code now that we basically have 2 real cases
to
> handle (row or column with and without dense).

I don't see very clear how to simplify this code.

Maybe you're missing that we could have "row dense" and "dense row",
so we need to check both parameters depending on the other.

[Julien - ping for review, 2015-01-09 09:13:03]

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
File LayoutTests/ietestcenter/css3/grid/grid-column-002.htm (right):

https://codereview.chromium.org/826893003/diff/20001/LayoutTests/ietestcenter...
LayoutTests/ietestcenter/css3/grid/grid-column-002.htm:91: <div id="container">
On 2015/01/08 12:50:28, Manuel Rego wrote:
> On 2015/01/05 13:11:34, Julien Chaffraix - CET wrote:
> > Those are external tests so we avoid making big changes to them.
> 
> The problem here is that they're wrong according to the spec now.
> So we could remove them or update them.
> We already updated them in the past to add "stack" for example.

We could probably get both grid items into the first grid area by specifying the
grid-{column|row}-end but I am not convinced it's worth it consider the churn on
this test. Thus I would advise to just remove it.
 
> > Furthermore I don't think the change is correct, even if it will test and
> > provide a similar result. Both elements ended up in the same grid cell and
> thus
> > the the second was painting above as it is last. Here you mix a positioned
and
> a
> > non-positioned object to achieve the same result (ie a lot more unneeded
> > complexity)
> 
> Yeah, probably a best change is to make both items to be on the first cell
> using the grid placement properties.
> Otherwise, we cannot reproduce this behavior.

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

https://codereview.chromium.org/826893003/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3814: switch (firstId) {
On 2015/01/08 at 12:50:28, Manuel Rego wrote:
> On 2015/01/05 13:11:34, Julien Chaffraix - CET wrote:
> > We could probably simplify this code now that we basically have 2 real cases
to
> > handle (row or column with and without dense).
> 
> I don't see very clear how to simplify this code.
> 
> Maybe you're missing that we could have "row dense" and "dense row",
> so we need to check both parameters depending on the other.

OK!

[Manuel Rego, 2015-01-09 11:43:30]

So, I'm closing this CL as we discard the flag for the moment.
And apply the suggested changes directly in the other CL:
https://codereview.chromium.org/826893002/