[css-grid] Clamp the number of auto repeat tracks in all cases

[css-grid] Clamp the number of auto repeat tracks in all cases

In #446646 we added clamping of auto repetitions so that they respected
kGridMaxTracks. The problem is that we were doing it only if the number of
auto repeat tracks was >kGridMaxtracks. When the amount of auto-repeat
tracks was exactly kGridMaxTracks there was no clamping and thus, the sole
existence of one non auto repeat track was enough to surpass the limits.

Apart from that there were other non-handled cases, like having more than
kGridMaxTracks (both non auto repeat and auto repeat tracks) or having
a <track-list> in the auto repeat syntax with more than kGridMaxTracks
tracks.

BUG=687941
Review-Url: https://codereview.chromium.org/2670363003
Cr-Commit-Position: refs/heads/master@{#448602}
Committed: https://chromium.googlesource.com/chromium/src/+/bccd320563864b14db492b37954bac2d9d63d548

[svillar, 2017-02-06 08:49:55]

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

[Manuel Rego, 2017-02-06 09:51:12]

LGTM.

One question, what happens with r"egular" repeat()?
For example, are we testing things like?
  repeat(1, 1px 2px 3px... 2000px);

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html
(right):

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:106:
<div id="AThousandAutoFillRows" class="grid height25k autoFillRows25px"></div>
Nit: Previous IDs start with lowercase, but the new ones start with uppercase.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:292:
// There shouldn't be any auto-repeat tracks
Nit: Missing dot after comment.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:301:
// There shouldn't be any auto-repeat tracks
Nit: Ditto.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:310:
// There shouldn't be any auto-repeat tracks
Nit: Ditto.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:319:
// There shouldn't be any auto-repeat tracks
Nit: Ditto.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:328:
// There shouldn't be any auto-repeat tracks
Nit: Ditto.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:337:
// There shouldn't be any auto-repeat tracks
Nit: Ditto.

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

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:603: if (otherTracks >=
kGridMaxTracks)
We can move this if above.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:613: return repetitions *
autoRepeatTrackListLength;
Might be worth to explain here that we've decided to reduce the auto-repeat
tracks.

Also do we have tests for this?

For example if we've:
  width: 2000px;
  grid-template-columns: 10px 5px repeat(auto-fill, 1px);

If I understand it properly the final result would be:
  grid-template-columns: 10px 5px repeat(998, 1px);

[svillar, 2017-02-06 10:06:04]

Thanks for the review.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html
(right):

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:106:
<div id="AThousandAutoFillRows" class="grid height25k autoFillRows25px"></div>
On 2017/02/06 09:51:12, Manuel Rego wrote:
> Nit: Previous IDs start with lowercase, but the new ones start with uppercase.

Right, seems like we always use lowercase, so I'll change them all.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:292:
// There shouldn't be any auto-repeat tracks
On 2017/02/06 09:51:12, Manuel Rego wrote:
> Nit: Missing dot after comment.

ACK to all

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

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:603: if (otherTracks >=
kGridMaxTracks)
On 2017/02/06 09:51:12, Manuel Rego wrote:
> We can move this if above.

Acknowledged.

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:613: return repetitions *
autoRepeatTrackListLength;
On 2017/02/06 09:51:12, Manuel Rego wrote:
> Might be worth to explain here that we've decided to reduce the auto-repeat
> tracks.

Yeah, you're right. As this is not in the specs we should document it.

> Also do we have tests for this?
> 
> For example if we've:
>   width: 2000px;
>   grid-template-columns: 10px 5px repeat(auto-fill, 1px);
> 
> If I understand it properly the final result would be:
>   grid-template-columns: 10px 5px repeat(998, 1px);

Pretty similar ones were already present in the test I'm modifying. For example:

.lotsOfAutoRepeatWithAutoFitCols { grid-template-columns: repeat(auto-fit, 10px
2px 8px) repeat(10, 1px); }

[Manuel Rego, 2017-02-06 10:09:52]

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

https://codereview.chromium.org/2670363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:613: return repetitions *
autoRepeatTrackListLength;
On 2017/02/06 10:06:03, svillar wrote:
> On 2017/02/06 09:51:12, Manuel Rego wrote:
> > Might be worth to explain here that we've decided to reduce the auto-repeat
> > tracks.
> 
> Yeah, you're right. As this is not in the specs we should document it.

Oops, sorry but I believe this is against the spec:
 "Since memory is limited, UAs may clamp the possible size
  of the grid to be within a UA-defined limit,
  dropping all lines outside that limit."

So in the following example:
  width: 2000px;
  grid-template-columns: repeat(auto-fill, 1px) 10px 5px;
The result right now would be (considering a limit of 1000 tracks):
  grid-template-columns: repeat(998, 1px) 10px 5px;
But I think per spec it should be:
  grid-template-columns: repeat(1000, 1px);

So no-lgtm until we change this.

> Pretty similar ones were already present in the test I'm modifying. For
example:
> 
> .lotsOfAutoRepeatWithAutoFitCols { grid-template-columns: repeat(auto-fit,
10px
> 2px 8px) repeat(10, 1px); }

Nice, I missed them as they were not added in this CL.

[eae, 2017-02-06 17:56:49]

LGTM

[Manuel Rego, 2017-02-07 09:36:36]

LGTM too.

[svillar, 2017-02-07 10:23:06]

Description was changed from

==========
[css-grid] Clamp the number of auto repeat tracks in all cases

In #446646 we added clamping of auto repetitions so that they respected
kGridMaxTracks. The problem is that we were doing it only if the number of
auto repeat tracks was >kGridMaxtracks. When the amount of auto-repeat
tracks was exactly kGridMaxTracks there was no clamping and thus, the sole
existence of one non auto repeat track was enough to surpass the limits.

Apart from that there were other non-handled cases, like having more than
kGridMaxTracks (both non auto repeat and auto repeat tracks) or having
a <track-list> in the auto repeat syntax with more than kGridMaxTracks
tracks.

The specs do not specify what to do in these cases. We have decided to
simply compute as 0 the number of auto repeat tracks whenever a single
repetition overflows the maximum number of allowed tracks.

BUG=687941
==========

to

==========
[css-grid] Clamp the number of auto repeat tracks in all cases

In #446646 we added clamping of auto repetitions so that they respected
kGridMaxTracks. The problem is that we were doing it only if the number of
auto repeat tracks was >kGridMaxtracks. When the amount of auto-repeat
tracks was exactly kGridMaxTracks there was no clamping and thus, the sole
existence of one non auto repeat track was enough to surpass the limits.

Apart from that there were other non-handled cases, like having more than
kGridMaxTracks (both non auto repeat and auto repeat tracks) or having
a <track-list> in the auto repeat syntax with more than kGridMaxTracks
tracks.

BUG=687941
==========

[svillar, 2017-02-07 10:23:11]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2017-02-07 10:23:32]

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, 2017-02-07 11:49:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486462991806460,
"parent_rev": "a7856eef7d2f632f777a82a8af8eec76315c58b8", "commit_rev":
"bccd320563864b14db492b37954bac2d9d63d548"}

[commit-bot: I haz the power, 2017-02-07 11:50:22]

Description was changed from

==========
[css-grid] Clamp the number of auto repeat tracks in all cases

In #446646 we added clamping of auto repetitions so that they respected
kGridMaxTracks. The problem is that we were doing it only if the number of
auto repeat tracks was >kGridMaxtracks. When the amount of auto-repeat
tracks was exactly kGridMaxTracks there was no clamping and thus, the sole
existence of one non auto repeat track was enough to surpass the limits.

Apart from that there were other non-handled cases, like having more than
kGridMaxTracks (both non auto repeat and auto repeat tracks) or having
a <track-list> in the auto repeat syntax with more than kGridMaxTracks
tracks.

BUG=687941
==========

to

==========
[css-grid] Clamp the number of auto repeat tracks in all cases

In #446646 we added clamping of auto repetitions so that they respected
kGridMaxTracks. The problem is that we were doing it only if the number of
auto repeat tracks was >kGridMaxtracks. When the amount of auto-repeat
tracks was exactly kGridMaxTracks there was no clamping and thus, the sole
existence of one non auto repeat track was enough to surpass the limits.

Apart from that there were other non-handled cases, like having more than
kGridMaxTracks (both non auto repeat and auto repeat tracks) or having
a <track-list> in the auto repeat syntax with more than kGridMaxTracks
tracks.

BUG=687941

Review-Url: https://codereview.chromium.org/2670363003
Cr-Commit-Position: refs/heads/master@{#448602}
Committed:
https://chromium.googlesource.com/chromium/src/+/bccd320563864b14db492b37954b...
==========

[commit-bot: I haz the power, 2017-02-07 11:50:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bccd320563864b14db492b37954b...