[CSS Grid Layout] Update grid-auto-flow to the new syntax

[CSS Grid Layout] Update grid-auto-flow to the new syntax

In last versions of the spec grid-auto-flow syntax has changed. New
syntax is:
[ row | column ] && dense? | stack && [ row | column ]?

Implemented parsing for new syntax and added/modified test cases in
current layout tests.

Adapted code to keep the current behavior using "stack" as if it was
"none" in the previous syntax. This should be fixed in a follow-up patch
once "stack" is implemented.

Also "dense" needs to be reviewed. Right now auto-placement algorithm is
always "dense" and never "sparse".

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

[Julien - ping for review, 2014-06-27 17:44:54]

https://codereview.chromium.org/333563003/diff/40001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (right):

https://codereview.chromium.org/333563003/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:32: grid-auto-flow:
inherit;
Inherit is always a valid keyword for CSS properties.

What the 'inherited' section means is that it's not inherited by default.

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

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3921: if (value->id != CSSValueRow
&& value->id != CSSValueColumn && value->id != CSSValueDense && value->id !=
CSSValueStack)
Nit: firstId->id !=

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3935: return parsedValues;
I assume this is needed for the 'grid' shorthand but won't this risk allowing
anything for 'grid-auto-flow' (I couldn't find any testing for "row foo" as an
invalid value)

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3940: return parsedValues;
Same comment.

https://codereview.chromium.org/333563003/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/333563003/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:486: enum GridAutoFlow {
AutoFlowRow, AutoFlowColumn, AutoFlowRowDense, AutoFlowColumnDense,
AutoFlowStackRow, AutoFlowStackColumn };
I wonder if we couldn't choose the value to make for easier comparisons.

You know that you have a base value which is either 'flow', 'dense' or 'stack'
suffixed by 'row' or 'column' so we could do:

enum InternalGridAutoFlowAlgorithm {
   InternalGridAutoFlowFlow = 0x1,
    InternalGridAutoFlowDense = 0x2,
    InternalGridAutoFlowStack = 0x4
};


enum InternalGridAutoFlowDirection {
    InternalGridAutoFlowColumn = 0x10,
    InternalGridAutoFlowRow = 0x20
};

enum GridAutoFlow {
    AutoFlowRow = InternalGridAutoFlowFlow | InternalGridAutoFlowRow,
    AutoFlowColumn = InternalGridAutoFlowFlow | InternalGridAutoFlowColumn,
    AutoFlowRowDense = InternalGridAutoFlowDense | InternalGridAutoFlowRow,
    [...]
};

(It's a bit too verbose for my taste but you get the idea)

With these you can easily know if something is 'dense' or flows along the 'row'
by some simple bitwise arithmetic. We can wrap these into helper function so
that we don't expose these details to RenderGrid.

[Manuel Rego, 2014-06-27 22:43:51]

Thanks for the review. I'm uploading a new patch addressing your comments.

https://codereview.chromium.org/333563003/diff/40001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (right):

https://codereview.chromium.org/333563003/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:32: grid-auto-flow:
inherit;
Ok, thanks for the clarification. Changed it to allow "inherit" too.

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

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1634: if (!m_valueList->current()
|| inShorthand()) {
This line makes that things like "column foo" are consider as invalid.
Basically, if there's some value pending to be parsed, only if we're in a
shorthand
it'll return true, otherwise it'll return false.

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3921: if (value->id != CSSValueRow
&& value->id != CSSValueColumn && value->id != CSSValueDense && value->id !=
CSSValueStack)
Yeah, I changed value->id by firstId.

https://codereview.chromium.org/333563003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3935: return parsedValues;
I had one "column column" that was an invalid value.
I've added now a "column foo" and it's already being detected as invalid value.

As explained before the reason is the if at the end of
CSSPropertyParser::parseValue().

https://codereview.chromium.org/333563003/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/333563003/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:486: enum GridAutoFlow {
AutoFlowRow, AutoFlowColumn, AutoFlowRowDense, AutoFlowColumnDense,
AutoFlowStackRow, AutoFlowStackColumn };
Thanks for the suggestion. I've changed it and I've added some helper functions
in RenderStyle.

Now the code in RenderGrid that needs to know if you're in row or column
direction, or if the algorithm is stack or not is much simpler.

[Julien - ping for review, 2014-06-29 01:02:25]

lgtm

https://codereview.chromium.org/333563003/diff/60001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (right):

https://codereview.chromium.org/333563003/diff/60001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:103:
shouldBe("window.getComputedStyle(gridAutoFlowColumnDense,
'').getPropertyValue('grid-auto-flow')", "'column dense'");
Nit: We should test both getComputedStyle and element.style. This doesn't have
to be done in this change but I have been bitten when implementing
grid-row-start and friends as one was working but not the only one.

https://codereview.chromium.org/333563003/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/333563003/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:493: enum
GridAutoFlowDirection {
I would put internal in the names to underline that they are not mapping to any
property.

[Manuel Rego, 2014-06-30 09:44:52]

Thanks for the review.

https://codereview.chromium.org/333563003/diff/60001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html (right):

https://codereview.chromium.org/333563003/diff/60001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:103:
shouldBe("window.getComputedStyle(gridAutoFlowColumnDense,
'').getPropertyValue('grid-auto-flow')", "'column dense'");
Ok, I'll do it in a different patch, as I'm just adapting the test to the new
syntax now,
but previous version was already lacking test for style.

[Manuel Rego, 2014-06-30 09:44:58]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-30 09:45:25]

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

[Manuel Rego, 2014-06-30 10:29:09]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-30 10:29:39]

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

[commit-bot: I haz the power, 2014-06-30 11:33:46]

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

[commit-bot: I haz the power, 2014-06-30 12:14:29]

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

[commit-bot: I haz the power, 2014-06-30 12:14:30]

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

[Manuel Rego, 2014-06-30 22:45:37]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-30 22:46:28]

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

[commit-bot: I haz the power, 2014-06-30 23:55:47]

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

[commit-bot: I haz the power, 2014-07-01 00:46:05]

Change committed as 177258