[css-grid] Implement grid gutters

[css-grid] Implement grid gutters

Authors can now specify the gutters between grid lines, i.e., the space
between two consecutive grid lines. This can be done using the new
'grid-column-gap 'and 'grid-row-gap' properties (or the 'grid-gap'
shorthand).

From the track sizing algorithm POV, gutters are treated as fixed size
columns. The primary consequence is that grids are enlarged (depending on
the number of tracks). Gutters also affect the sizing of content-sized
tracks and fr tracks as long as the grid have spanning items. Those tracks
will become smaller as gutters will consume part of the item's size, so the
tracks won't need to grow as much as they used to.

Added several new test cases to verify that gutters are properly considered
when sizing and also to check that they do not modify the current behavior.
As many existing tests were reused I took the chance to refactor some
testing code related to alignment so that it could be reused by many
different tests.

Committed: https://crrev.com/ad570a7dcd24b813e0dc2400c0bf080caf30a0ce
Cr-Commit-Position: refs/heads/master@{#352839}

[svillar, 2015-09-08 10:11:23]

Removed two tests which were not testing gutters code

[svillar, 2015-09-08 10:30:42]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, esprehn@chromium.org, jfernandez@igalia.com,
rego@igalia.com

[svillar, 2015-09-08 10:30:42]

Adding reviewers

[jfernandez, 2015-09-09 21:07:45]

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html (right):

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:77: <div
class="grid grid100And200 alignItemsCenter gridRowColumnGaps"
data-expected-width="221" data-expected-height="417">
Nit: can we make numbers easier to compute manually ?  Perhaps using other
row/column gap values.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:77: <div
class="grid grid100And200 alignItemsCenter gridRowColumnGaps"
data-expected-width="221" data-expected-height="417">
Nit: It may be good idea to add some text to be rendered above each case to
visually evaluate what the result would be. There are some alignment tests that
follow that approach.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:79: <div
class="cell firstRowSecondColumn" data-offset-x="121" data-offset-y="80"
data-expected-width="20" data-expected-heçight="40"></div>
heçight ??

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:203:
<p>direction: RTL | justify-content: 'stretch'</p>
Are we managed correctly this case ? I don't see any gutter in the result.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:217:
<p>direction: LTR | justify-content: 'space-evenly'</p>
ditto.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:229:
<p>direction: LTR | justify-content: 'space-between'</p>
ditto.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:243:
<p>direction: LTR | justify-content: 'space-around'</p>
ditto.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:256: <div
class="grid gridWithPaddingBorder directionLTR itemsSelfStart gridRowColumnGaps"
data-expected-width="396" data-expected-height="467">
I see that stretched items fill the gutters space, is that the expected behavior
?

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:415: Vector<GridTrack>& tracks = (direction ==
ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
Why this line has been moved ?

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1333: 
Why these lines are not part of the computeUsedBreadthOfGridTracks logic ?

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1506: gridAreaBreadth +=
guttersSize(direction, span.integerSpan());
Why the gutters size affect the gridArea breadth where an item is placed in ? AS
far as I know, it'd only affect when the item is spanned across several tracks,
since it will fill the gaps between start end end tracks.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1800: endOfRow -= guttersSize(ForRows, 2);
I think we are doing something wrong somewhere, since we shouldn't have to
substrack gutters size to determine the endOfRow position. The content alignment
also is also added to the m_rowPositions value and we don't need to substract it
here. 

It may be that we are adding it somewhere where we shouldn't; perhaps related,
somehow, with my comment on the gridAreaBreadthForChild change.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1826: endOfColumn -= guttersSize(ForRows, 2);
Ditto.

[svillar, 2015-09-10 11:55:45]

Thanks for the review. I'll fix the spotted issues

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html (right):

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:77: <div
class="grid grid100And200 alignItemsCenter gridRowColumnGaps"
data-expected-width="221" data-expected-height="417">
On 2015/09/09 21:07:45, jfernandez wrote:
> Nit: can we make numbers easier to compute manually ?  Perhaps using other
> row/column gap values.

Actually I chose those on purpose in order to more easily identify that gutters
are working as expected.

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:79: <div
class="cell firstRowSecondColumn" data-offset-x="121" data-offset-y="80"
data-expected-width="20" data-expected-heçight="40"></div>
On 2015/09/09 21:07:45, jfernandez wrote:
> heçight ??

Ups. will change it

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:203:
<p>direction: RTL | justify-content: 'stretch'</p>
On 2015/09/09 21:07:45, jfernandez wrote:
> Are we managed correctly this case ? I don't see any gutter in the result.

Oh because I forgot to add them :)

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:217:
<p>direction: LTR | justify-content: 'space-evenly'</p>
On 2015/09/09 21:07:45, jfernandez wrote:
> ditto.

Ditto :).

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:229:
<p>direction: LTR | justify-content: 'space-between'</p>
On 2015/09/09 21:07:45, jfernandez wrote:
> ditto.

Ditto

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:243:
<p>direction: LTR | justify-content: 'space-around'</p>
On 2015/09/09 21:07:44, jfernandez wrote:
> ditto.


Ditto

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:256: <div
class="grid gridWithPaddingBorder directionLTR itemsSelfStart gridRowColumnGaps"
data-expected-width="396" data-expected-height="467">
On 2015/09/09 21:07:45, jfernandez wrote:
> I see that stretched items fill the gutters space, is that the expected
behavior
> ? 

Hmm only spanning items should fill the gutters space, this means that we have a
bug here.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:415: Vector<GridTrack>& tracks = (direction ==
ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
On 2015/09/09 21:07:45, jfernandez wrote:
> Why this line has been moved ? 

I had to move it for a previous version of the patch. Will revert this change.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1333: 
On 2015/09/09 21:07:45, jfernandez wrote:
> Why these lines are not part of the computeUsedBreadthOfGridTracks logic ?

Because I wanted to avoid modifying the algorithm as much as possible.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1506: gridAreaBreadth +=
guttersSize(direction, span.integerSpan());
On 2015/09/09 21:07:45, jfernandez wrote:
> Why the gutters size affect the gridArea breadth where an item is placed in ?
AS
> far as I know, it'd only affect when the item is spanned across several
tracks,
> since it will fill the gaps between start end end tracks. 

If you take a look at guttersSize(), if the span is 1 (meaning non-spanning
item) then this value is 0.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1800: endOfRow -= guttersSize(ForRows, 2);
On 2015/09/09 21:07:45, jfernandez wrote:
> I think we are doing something wrong somewhere, since we shouldn't have to
> substrack gutters size to determine the endOfRow position. The content
alignment
> also is also added to the m_rowPositions value and we don't need to substract
it
> here. 

We're just removing 1 gutter, and just for the case of the endRow not being the
last one (because the last one does not have gutter). We need to do that because
m_rowPositions already has the gaps, so we cannot blindly get the value it has
for the endOfRow because it contains the gutter and we don't want to pick it for
the final resolved position.

> It may be that we are adding it somewhere where we shouldn't; perhaps related,
> somehow, with my comment on the gridAreaBreadthForChild change.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1826: endOfColumn -= guttersSize(ForRows, 2);
On 2015/09/09 21:07:45, jfernandez wrote:
> Ditto.

See above.

[svillar, 2015-09-10 13:49:27]

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html (right):

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:256: <div
class="grid gridWithPaddingBorder directionLTR itemsSelfStart gridRowColumnGaps"
data-expected-width="396" data-expected-height="467">
On 2015/09/09 21:07:45, jfernandez wrote:
> I see that stretched items fill the gutters space, is that the expected
behavior
> ? 


How do you see that? Heights and widths do not include the gutters size and when
checking the final image I don't see them filling the gaps.

[jfernandez, 2015-09-11 08:11:09]

lgtm

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html (right):

https://codereview.chromium.org/1309513008/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-gutters-and-alignment.html:256: <div
class="grid gridWithPaddingBorder directionLTR itemsSelfStart gridRowColumnGaps"
data-expected-width="396" data-expected-height="467">
On 2015/09/10 13:49:26, svillar wrote:
> On 2015/09/09 21:07:45, jfernandez wrote:
> > I see that stretched items fill the gutters space, is that the expected
> behavior
> > ? 
> 
> 
> How do you see that? Heights and widths do not include the gutters size and
when
> checking the final image I don't see them filling the gaps.

Yeah, you are right. It looks correct.

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1800: endOfRow -= guttersSize(ForRows, 2);
On 2015/09/10 11:55:45, svillar wrote:
> On 2015/09/09 21:07:45, jfernandez wrote:
> > I think we are doing something wrong somewhere, since we shouldn't have to
> > substrack gutters size to determine the endOfRow position. The content
> alignment
> > also is also added to the m_rowPositions value and we don't need to
substract
> it
> > here. 
> 
> We're just removing 1 gutter, and just for the case of the endRow not being
the
> last one (because the last one does not have gutter). We need to do that
because
> m_rowPositions already has the gaps, so we cannot blindly get the value it has
> for the endOfRow because it contains the gutter and we don't want to pick it
for
> the final resolved position.
> 
> > It may be that we are adding it somewhere where we shouldn't; perhaps
related,
> > somehow, with my comment on the gridAreaBreadthForChild change.
> 

It seems your implementation may be correct, hence we have some bug somewhere in
the content alignment logic. However, I'm not sure it you have defined a test to
verify gutters logic works fine with alignment and span.

[svillar, 2015-09-16 09:47:10]

@cbiesinger, @esprehn: anything to add?

[cbiesinger, 2015-09-30 15:22:42]

lgtm

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:1797: // // m_rowPositions include gutters so
we need to substract them to get the actual end position for a given
remove one of the //

[esprehn, 2015-10-01 08:46:48]

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:378: DEFINE_STATIC_LOCAL(LayoutUnit, zero,
());
LayoutUnit is basically just an int, don't define a static local for zero

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:380: return valueForLength(trackGap, zero) *
(span - 1);
just pass 0, it'll be made into a LayoutUnit

if snap is negative what happens here?

Also isn't valueForLength an int, did you want to convert to LayoutUnit first?

[svillar, 2015-10-02 10:00:29]

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:378: DEFINE_STATIC_LOCAL(LayoutUnit, zero,
());
On 2015/10/01 08:46:48, esprehn wrote:
> LayoutUnit is basically just an int, don't define a static local for zero

Acknowledged.

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:380: return valueForLength(trackGap, zero) *
(span - 1);
On 2015/10/01 08:46:48, esprehn wrote:
> just pass 0, it'll be made into a LayoutUnit
> 
> if snap is negative what happens here?

span cannot be negative as it is a size_t. In any case I can add an assert that
it must be >= 1.

> Also isn't valueForLength an int, did you want to convert to LayoutUnit first?

Not sure what you mean here, valueForLength() returns a LayoutUnit so the
operator* already converts span-1 to another LayoutUnit

[svillar, 2015-10-02 10:12:00]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-10-02 10:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/1309513008/#ps80001
(title: "Patch for landing v2")

[commit-bot: I haz the power, 2015-10-02 10:12:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309513008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309513008/80001

[commit-bot: I haz the power, 2015-10-02 10:22:02]

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

[commit-bot: I haz the power, 2015-10-02 10:22:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[svillar, 2015-10-05 08:54:35]

svillar@igalia.com changed reviewers:
+ rune@opera.com

[fs, 2015-10-05 10:53:27]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-10-05 10:53:28]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:191: grid-column-gap
runtime_flag=CSSGridLayout, converter=convertLengthSizing
Why convertLengthSizing and not convertLength? (Ditto below)

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210: bool
CSSPropertyParser::parseGridGapShorthand(bool important)
Could you use parseShorthand instead?

[svillar, 2015-10-05 11:58:41]

Thanks for the review

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:191: grid-column-gap
runtime_flag=CSSGridLayout, converter=convertLengthSizing
On 2015/10/05 10:53:28, fs wrote:
> Why convertLengthSizing and not convertLength? (Ditto below)

Yeah it should be convertLength()

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210: bool
CSSPropertyParser::parseGridGapShorthand(bool important)
On 2015/10/05 10:53:28, fs wrote:
> Could you use parseShorthand instead?

I don't think so because although ideally the shorthand gets two values it might
get just one so we have to add that specific behavior. AFAIK that cannot be done
with parseShorthand...

[fs, 2015-10-05 12:01:55]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210: bool
CSSPropertyParser::parseGridGapShorthand(bool important)
On 2015/10/05 at 11:58:40, svillar wrote:
> On 2015/10/05 10:53:28, fs wrote:
> > Could you use parseShorthand instead?
> 
> I don't think so because although ideally the shorthand gets two values it
might get just one so we have to add that specific behavior. AFAIK that cannot
be done with parseShorthand...

Right, I figured it might be the column-gap -> row-gap value expansion (didn't
remember if parseShorthand handled that or not.)

[svillar, 2015-10-05 12:16:49]

On 2015/10/05 12:01:55, fs wrote:
>
https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
> (right):
> 
>
https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210:
bool
> CSSPropertyParser::parseGridGapShorthand(bool important)
> On 2015/10/05 at 11:58:40, svillar wrote:
> > On 2015/10/05 10:53:28, fs wrote:
> > > Could you use parseShorthand instead?
> > 
> > I don't think so because although ideally the shorthand gets two values it
> might get just one so we have to add that specific behavior. AFAIK that cannot
> be done with parseShorthand...
> 
> Right, I figured it might be the column-gap -> row-gap value expansion (didn't
> remember if parseShorthand handled that or not.)

So parseShorthand() is indeed able to fill in the missing values with initial
values but that is not what we want in this case, as I mentioned if just one
value is specified then the other should get the same.

[rune, 2015-10-05 12:23:39]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);
The current spec draft doesn't say it's limited to non-negative lengths, but it
probably should, like the spec for column-gap in multicol does.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);
According to the spec, "normal" is not a valid value for any of the longhands.

Also, the spec doesn't say what normal mean, afaict.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3235:
addProperty(CSSPropertyGridRowGap, columnGap, important);
I think it this code might be easier to read if you have the addProperty calls
at the end and have two variables columnGap and rowGap which is set to the same
value if there's only one value in the declaration.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:799: 
Why the extra empty line?

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleGridData.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleGridData.h:49: return
m_gridTemplateColumns == o.m_gridTemplateColumns && m_gridTemplateRows ==
o.m_gridTemplateRows && m_gridAutoFlow == o.m_gridAutoFlow && m_gridAutoRows ==
o.m_gridAutoRows && m_gridAutoColumns == o.m_gridAutoColumns &&
m_namedGridColumnLines == o.m_namedGridColumnLines && m_namedGridRowLines ==
o.m_namedGridRowLines && m_orderedNamedGridColumnLines ==
o.m_orderedNamedGridColumnLines && m_orderedNamedGridRowLines ==
o.m_orderedNamedGridRowLines && m_namedGridArea == o.m_namedGridArea &&
m_namedGridArea == o.m_namedGridArea && m_namedGridAreaRowCount ==
o.m_namedGridAreaRowCount && m_namedGridAreaColumnCount ==
o.m_namedGridAreaColumnCount && m_gridColumnGap == o.m_gridColumnGap &&
m_gridRowGap == o.m_gridRowGap;
About time to break this into multiple lines.

[svillar, 2015-10-05 12:36:02]

Thanks for the review

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);
On 2015/10/05 12:23:39, rune wrote:
> The current spec draft doesn't say it's limited to non-negative lengths, but
it
> probably should, like the spec for column-gap in multicol does.

True, I think it's just an overlook, actually the Mozilla guy that is going to
implement it in Gecko sent this
https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html to the list
asking for clarifications.

I think that the assumption I made is correct in any case.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);
On 2015/10/05 12:23:39, rune wrote:
> According to the spec, "normal" is not a valid value for any of the longhands.
> 
> Also, the spec doesn't say what normal mean, afaict.

That was specified in the previous draft, see
http://www.w3.org/TR/2015/WD-css-grid-1-20150806/#gutters. My first patch was
from 3 weeks ago when that was the latest version.

Anyway in the email I linked above there is another question for "normal" as
well so they'll clarify it for sure.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3235:
addProperty(CSSPropertyGridRowGap, columnGap, important);
On 2015/10/05 12:23:39, rune wrote:
> I think it this code might be easier to read if you have the addProperty calls
> at the end and have two variables columnGap and rowGap which is set to the
same
> value if there's only one value in the declaration.

Acknowledged.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:799: 
On 2015/10/05 12:23:39, rune wrote:
> Why the extra empty line?

Will remove it.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleGridData.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleGridData.h:49: return
m_gridTemplateColumns == o.m_gridTemplateColumns && m_gridTemplateRows ==
o.m_gridTemplateRows && m_gridAutoFlow == o.m_gridAutoFlow && m_gridAutoRows ==
o.m_gridAutoRows && m_gridAutoColumns == o.m_gridAutoColumns &&
m_namedGridColumnLines == o.m_namedGridColumnLines && m_namedGridRowLines ==
o.m_namedGridRowLines && m_orderedNamedGridColumnLines ==
o.m_orderedNamedGridColumnLines && m_orderedNamedGridRowLines ==
o.m_orderedNamedGridRowLines && m_namedGridArea == o.m_namedGridArea &&
m_namedGridArea == o.m_namedGridArea && m_namedGridAreaRowCount ==
o.m_namedGridAreaRowCount && m_namedGridAreaColumnCount ==
o.m_namedGridAreaColumnCount && m_gridColumnGap == o.m_gridColumnGap &&
m_gridRowGap == o.m_gridRowGap;
On 2015/10/05 12:23:39, rune wrote:
> About time to break this into multiple lines.

Acknowledged.

[rune, 2015-10-05 12:47:10]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);
On 2015/10/05 12:36:01, svillar wrote:
> On 2015/10/05 12:23:39, rune wrote:
> > The current spec draft doesn't say it's limited to non-negative lengths, but
> it
> > probably should, like the spec for column-gap in multicol does.
> 
> True, I think it's just an overlook, actually the Mozilla guy that is going to
> implement it in Gecko sent this
> https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html to the list
> asking for clarifications.
> 
> I think that the assumption I made is correct in any case.

Yes, I just wanted to make sure the spec issue is addressed.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);
On 2015/10/05 12:36:01, svillar wrote:
> On 2015/10/05 12:23:39, rune wrote:
> > According to the spec, "normal" is not a valid value for any of the
longhands.
> > 
> > Also, the spec doesn't say what normal mean, afaict.
> 
> That was specified in the previous draft, see
> http://www.w3.org/TR/2015/WD-css-grid-1-20150806/#gutters. My first patch was
> from 3 weeks ago when that was the latest version.
> 
> Anyway in the email I linked above there is another question for "normal" as
> well so they'll clarify it for sure.

Ah. So the property used to be shared with multicol. Now that explains the need
for 'normal' to mean something. Now that they're separate properties, and the
initial value is 0, and normal used to map to 0, I see no point in keeping
'normal'. I suggest that you just drop normal as a valid value for now and
re-introduce it if the spec for some reason re-introduces it instead of removing
it for the shorthand.

[fs, 2015-10-07 10:25:19]

lgtm

https://codereview.chromium.org/1309513008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3193:
RefPtrWillBeRawPtr<CSSPrimitiveValue> columnGap = nullptr;
Nit: With this new structure, I'd propose that the declaration is moved as close
to/as far down as possible (i.e columnGap to where it's assigned, and rowGap
just before the if.)

[rune, 2015-10-07 12:30:53]

lgtm too

[svillar, 2015-10-07 12:45:54]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-10-07 12:45:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/1309513008/#ps140001
(title: "Changes after rune's review")

[commit-bot: I haz the power, 2015-10-07 12:46:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309513008/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309513008/140001

[commit-bot: I haz the power, 2015-10-07 12:49:25]

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

[commit-bot: I haz the power, 2015-10-07 12:49:25]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svillar, 2015-10-07 13:27:33]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-10-07 13:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, jfernandez@igalia.com, rune@opera.com, fs@opera.com
Link to the patchset: https://codereview.chromium.org/1309513008/#ps160001
(title: "Rebased patch for landing")

[commit-bot: I haz the power, 2015-10-07 13:28:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309513008/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309513008/160001

[commit-bot: I haz the power, 2015-10-07 13:39:02]

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

[commit-bot: I haz the power, 2015-10-07 13:39:04]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[svillar, 2015-10-07 13:43:24]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2015-10-07 13:45:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309513008/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309513008/160001

[commit-bot: I haz the power, 2015-10-07 14:52:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-07 14:53:38]

Patchset 9 (id:??) landed as
https://crrev.com/ad570a7dcd24b813e0dc2400c0bf080caf30a0ce
Cr-Commit-Position: refs/heads/master@{#352839}