[css-grid] repeat() syntax should take a <track-list> argument

[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
Committed: https://crrev.com/5e06bcde925d051ebdcb42243dc15084d762e17b
Cr-Commit-Position: refs/heads/master@{#406540}

[svillar, 2016-07-19 13:23:06]

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

[svillar, 2016-07-19 13:23:07]

Sending out for review

[svillar, 2016-07-19 13:23:32]

Description was changed from

==========
[css-grid] repeat() syntax takes a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
==========

to

==========
[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
==========

[jfernandez, 2016-07-19 14:43:18]

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:747:
OrderedNamedLinesCollector(const ComputedStyle& style, bool isRowAxis, size_t
autoRepeatTracks)
I'd add some quantification sufix/prefix, like num or size.

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:790:
DCHECK(m_autoRepeatTotalTracks);
Wouldn't be better to check whether it's > 0 ?

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

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:883: return
trackStyles[untranslatedIndex - autoRepeatTracksCount];
Are you sure this can't lead to a negative value as vector index ?

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545:
DCHECK_GE(gridRowCount(), GridPositionsResolver::explicitGridRowCount(*style(),
autoRepeatTracksCount(ForRows)));
It seems that we are using the autoRepeatTracksCount(ForRows/ForColumns) mutiple
times. Wouldn't make sense to cache it, in some local variable, to avoid
repeating the arithmetic operations ?

[eae, 2016-07-19 20:59:48]

LGTM w/nit but please make sure you address all of jfernandezs concerns before
landing.

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

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:883: return
trackStyles[untranslatedIndex - autoRepeatTracksCount];
On 2016/07/19 14:43:18, jfernandez wrote:
> Are you sure this can't lead to a negative value as vector index ?

Please add a DCHECK here to ensure it cannot go negative.

[svillar, 2016-07-20 07:22:30]

Thanks for both reviews.

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:747:
OrderedNamedLinesCollector(const ComputedStyle& style, bool isRowAxis, size_t
autoRepeatTracks)
On 2016/07/19 14:43:18, jfernandez wrote:
> I'd add some quantification sufix/prefix, like num or size.

Acknowledged.

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:790:
DCHECK(m_autoRepeatTotalTracks);
On 2016/07/19 14:43:18, jfernandez wrote:
> Wouldn't be better to check whether it's > 0 ?

That's exactly what we do here, m_autoRepeatTotalTracks is an unsigned.

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

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:883: return
trackStyles[untranslatedIndex - autoRepeatTracksCount];
On 2016/07/19 20:59:48, eae wrote:
> On 2016/07/19 14:43:18, jfernandez wrote:
> > Are you sure this can't lead to a negative value as vector index ?
> 
> Please add a DCHECK here to ensure it cannot go negative.

Replying to both, I am not changing the code here, just renaming the variable.

That said we know from the if above that

    untraslatedIndex >= insertionPoint + autoRepeatTracksCount

so if my math is correct

    untranslatedIndex >= autoRepeatTracksCount

This means that the code flow guarantees the condition (which my patch is not
changing). IMHO we should not add a check there because it's pretty explicit in
the code.

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545:
DCHECK_GE(gridRowCount(), GridPositionsResolver::explicitGridRowCount(*style(),
autoRepeatTracksCount(ForRows)));
On 2016/07/19 14:43:18, jfernandez wrote:
> It seems that we are using the autoRepeatTracksCount(ForRows/ForColumns)
mutiple
> times. Wouldn't make sense to cache it, in some local variable, to avoid
> repeating the arithmetic operations ?

I've been checking where we use m_autoRepeatColumns|Rows and it seems that we
just use them now for autoRepeatTracksCount() computation. So I'm going to
change their semantics and store in those attributes the total number of auto
repeat tracks, because we don't use the actual number of repetitions for
anything else.

[dollieward40_gmail.com, 2016-07-20 07:53:54]

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jfernandez, 2016-07-20 08:51:50]

LGTM

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

https://codereview.chromium.org/2161003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:883: return
trackStyles[untranslatedIndex - autoRepeatTracksCount];
On 2016/07/20 07:22:30, svillar wrote:
> On 2016/07/19 20:59:48, eae wrote:
> > On 2016/07/19 14:43:18, jfernandez wrote:
> > > Are you sure this can't lead to a negative value as vector index ?
> > 
> > Please add a DCHECK here to ensure it cannot go negative.
> 
> Replying to both, I am not changing the code here, just renaming the variable.
> 
> That said we know from the if above that
> 
>     untraslatedIndex >= insertionPoint + autoRepeatTracksCount
> 
> so if my math is correct
> 
>     untranslatedIndex >= autoRepeatTracksCount
> 
> This means that the code flow guarantees the condition (which my patch is not
> changing). IMHO we should not add a check there because it's pretty explicit
in
> the code.

Yes, you're right.

[Manuel Rego, 2016-07-20 09:20:29]

I'm late to the party but it looks good to me too.
Maybe next patch can be http://crbug.com/618970/ :-)

The auto-repeat logic is pretty complex, but that's what the spec is giving us.

[svillar, 2016-07-20 09:32:02]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-07-20 09:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2161003003/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-07-20 09:32:25]

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-20 13:06:03]

Description was changed from

==========
[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
==========

to

==========
[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
==========

[commit-bot: I haz the power, 2016-07-20 13:06:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-20 13:06:11]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 13:07:56]

Description was changed from

==========
[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930
==========

to

==========
[css-grid] repeat() syntax should take a <track-list> argument

The repeat() notation used to allow just 1 <track-size> as second argument.
It was recently changed to support <track-list> instead, meaning that we can
pass an arbitrary number of track sizes and line numbers.

It has been working for some time for repeat() if the first argument was
a positive integer, but it requires some changes for the auto repeat cases
(auto-fill and auto-fit).

BUG=619930

Committed: https://crrev.com/5e06bcde925d051ebdcb42243dc15084d762e17b
Cr-Commit-Position: refs/heads/master@{#406540}
==========

[commit-bot: I haz the power, 2016-07-20 13:07:57]

Patchset 2 (id:??) landed as
https://crrev.com/5e06bcde925d051ebdcb42243dc15084d762e17b
Cr-Commit-Position: refs/heads/master@{#406540}