[css-grid] grid-auto-flow|row should take a <track-size>+

[css-grid] grid-auto-flow|row should take a <track-size>+

Both properties were traditionally accepting just one <track-size> but
a recent change in the spec allows authors to pass a non-empty list of track
sizes. The first implicit track will use the first track size the second
implicit track will use the second track size and so on. For implicit tracks
before the explicit grid we need to transpose the order, i.e, the first
implicit track before the explicit grid will take the last track size.

Updated parsing, styling and track size resolution to support a list of
track sizes instead of just one. The grid shorthand was also updated as it
can set the value of the two grid-auto-* properties.

BUG=618970
Committed: https://crrev.com/ef8ba7e30cc8392bb48ad021e59d3aa496c930f1
Cr-Commit-Position: refs/heads/master@{#407361}

[svillar, 2016-07-21 16:07:41]

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

[svillar, 2016-07-21 16:07:42]

Sending out for review

[Manuel Rego, 2016-07-22 08:24:54]

rego@igalia.com changed reviewers:
+ rob.buis@chromium.org, timloh@chromium.org

[Manuel Rego, 2016-07-22 08:24:55]

Great with this changes we're getting very close to the last spec.
Non-owner LGTM.

+ @timloh & @rob.buis just in case they they want to take a look
to the parsing code.

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html
(right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html:58:
<div class="ahem" style="grid-column: 1; grid-row: 1"></div>
Nit: "ahem" class is not needed here as you don't have any text.

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html:116:
testAutoValues("minmax(min-content, 10px) 48px 5%", "auto 30px minmax(10%,
60%)");
Shouldn't the percentages be resolved when you check the computed style?

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:862: size_t
autoTrackStylesSize = autoTrackStyles.size();
What about defining it as an int to avoid the cast later?

Nit: You can move this out of the if and use it later.

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1928: inline
Vector<GridTrackSize> initialGridAutoTracks()
Why not mark it as static?

[svillar, 2016-07-22 13:08:28]

Thanks for the review!

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html
(right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html:58:
<div class="ahem" style="grid-column: 1; grid-row: 1"></div>
On 2016/07/22 08:24:55, Manuel Rego wrote:
> Nit: "ahem" class is not needed here as you don't have any text.

Yeah, it used to, but I removed it at the very last moment.

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html:116:
testAutoValues("minmax(min-content, 10px) 48px 5%", "auto 30px minmax(10%,
60%)");
On 2016/07/22 08:24:55, Manuel Rego wrote:
> Shouldn't the percentages be resolved when you check the computed style?

Content sized tracks cannot be resolved for obvious reasons, but percentages of
definite sizes should be indeed resolved. I'll check it.

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:862: size_t
autoTrackStylesSize = autoTrackStyles.size();
On 2016/07/22 08:24:55, Manuel Rego wrote:
> What about defining it as an int to avoid the cast later?

We're using it as size_t later anyway.

> Nit: You can move this out of the if and use it later.

OK

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1928: inline
Vector<GridTrackSize> initialGridAutoTracks()
On 2016/07/22 08:24:55, Manuel Rego wrote:
> Why not mark it as static?

No particular reason, some methods in that file are static and some others
don't. I'll add it.

[svillar, 2016-07-22 15:54:39]

On 2016/07/22 13:08:28, svillar wrote:
>
https://codereview.chromium.org/2166393002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-get-set.html:116:
> testAutoValues("minmax(min-content, 10px) 48px 5%", "auto 30px minmax(10%,
> 60%)");
> On 2016/07/22 08:24:55, Manuel Rego wrote:
> > Shouldn't the percentages be resolved when you check the computed style?
> 
> Content sized tracks cannot be resolved for obvious reasons, but percentages
of
> definite sizes should be indeed resolved. I'll check it.

So we were not resolving it and I think we should not resolve them now either.
The specs mention that we should use the "specified value, with lenghts made
absolute" but these are percentages so we should be fine. Actually I think this
should be also applied to grid-template-* ...

[eae, 2016-07-22 17:41:41]

LGTM

[svillar, 2016-07-22 18:32:33]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-07-22 18:32:34]

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

[commit-bot: I haz the power, 2016-07-22 18:33:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 21:16:36]

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

[commit-bot: I haz the power, 2016-07-22 21:16:37]

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

[svillar, 2016-07-23 08:24:31]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-07-23 08:24:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 09:35:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-23 09:36:59]

Description was changed from

==========
[css-grid] grid-auto-flow|row should take a <track-size>+

Both properties were traditionally accepting just one <track-size> but
a recent change in the spec allows authors to pass a non-empty list of track
sizes. The first implicit track will use the first track size the second
implicit track will use the second track size and so on. For implicit tracks
before the explicit grid we need to transpose the order, i.e, the first
implicit track before the explicit grid will take the last track size.

Updated parsing, styling and track size resolution to support a list of
track sizes instead of just one. The grid shorthand was also updated as it
can set the value of the two grid-auto-* properties.

BUG=618970
==========

to

==========
[css-grid] grid-auto-flow|row should take a <track-size>+

Both properties were traditionally accepting just one <track-size> but
a recent change in the spec allows authors to pass a non-empty list of track
sizes. The first implicit track will use the first track size the second
implicit track will use the second track size and so on. For implicit tracks
before the explicit grid we need to transpose the order, i.e, the first
implicit track before the explicit grid will take the last track size.

Updated parsing, styling and track size resolution to support a list of
track sizes instead of just one. The grid shorthand was also updated as it
can set the value of the two grid-auto-* properties.

BUG=618970

Committed: https://crrev.com/ef8ba7e30cc8392bb48ad021e59d3aa496c930f1
Cr-Commit-Position: refs/heads/master@{#407361}
==========

[commit-bot: I haz the power, 2016-07-23 09:37:00]

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