Bug 170120 – [css-grid] Clamp the number of autorepeat tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Clamp the number of autorepeat tracks

[Sergio Villar Senin]

Created attachment 305477 [details]
Patch

[Sergio Villar Senin]

This was causing crashes as the one reported to Chromium https://bugs.chromium.org/p/chromium/issues/detail?id=681381

[Javier Fernandez]

Comment on attachment 305477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305477&action=review

This change looks good to me.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:69
> +    <div style="grid-column: 1;">Item1</div>

Do we really need the "Item1" strings here ? The add unnecessary data in the test's output.

[Alexey Proskuryakov]

Comment on attachment 305477 [details]
Patch

Marking r- due to build failures, as the patch will need to be updated for EWS to run the tests.

[Sergio Villar Senin]

Created attachment 305743 [details]
Patch

Build fix for Mac

[Manuel Rego Casasnovas]

Comment on attachment 305743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305743&action=review

Awesome test, I'm not very used to the internals stuff but it looks good to me.

BTW, now that you're adding the internals API shouldn't we get rid of the unit tests
checking this under Tools/TestWebKitAPI/Tests/WebCore/ and replace them by some JS tests?

> Source/WebCore/ChangeLog:62
> +        (WebCore::GridPosition::setExplicitPosition): Deleted.
> +        (WebCore::GridPosition::setAutoPosition): Deleted.
> +        (WebCore::GridPosition::setSpanPosition): Deleted.
> +        (WebCore::GridPosition::setNamedGridArea): Deleted.
> +        (WebCore::GridPosition::integerPosition): Deleted.
> +        (WebCore::GridPosition::namedGridLine): Deleted.
> +        (WebCore::GridPosition::spanPosition): Deleted.
> +        (WebCore::GridPosition::operator==): Deleted.

Nit: They're not deleted, the implementation is moved to the .cpp file.

> Source/WebCore/rendering/style/GridPosition.cpp:37
> +std::optional<int> GridPosition::gMaxPositionForTesting;
> +static const int kGridMaxTracks = 1000000;

I'm not completely sure about the names of this 2 things.
I believe they should use a similar name: tracks vs position.

Also, I know "kGridMaxTracks" has been around before,
but if I remember correctly we can end up with a grid of "2 * kGridMaxTracks" tracks
because of the implicit grid before the explicit one.
If that's the case, maybe we should try to find a better name.

Last, isn't this number too big? Probably we should have to reduce it
to avoid memory issues like we did in Blink.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:19
> + .lastColumn { grid-column: -2 / -1; }
> + .lastRow { grid-row: -2 / -1; }

Nit: Using "-2" is enough, but feel free to keep "-2 / -1" if it's clearer for you.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:92
> + function testElement(gridElement, property, length, classNames) {

Nit: Could we rename this method to something more self descriptive.
This method returns the track sizes and also checks that the length of the tracks is the expected one.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:93
> +     /* gridElement.style.gridTemplate = 'none';*/

Remove this line.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:98
> +     // Seems like this is not enough to trigger a proper layout;

Mmm, what's this comment?

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:100
> +     var propertyValue = getComputedStyle(gridElement, '').getPropertyValue(property);

Nit: 2nd argument of getComputedStyle() is optional AFAIK.

> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:125
> +     assert_equals(autoFillGrid[maxTracksForTesting - 1], "20px");
> +     assert_equals(autoFillGrid[maxTracksForTesting - 2], "7px");
> +     assert_equals(autoFillGrid[0], "10px");
> +     assert_equals(autoFillGrid[1], "2px");

Nit: Move these checks closer to the call to testElement() for "autoFillGrid".
Also the order is a bit strange, I'd do: 0, 1, max - 2, max - 1.

[Sergio Villar Senin]

Comment on attachment 305743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305743&action=review

>> Source/WebCore/ChangeLog:62
>> +        (WebCore::GridPosition::operator==): Deleted.
> 
> Nit: They're not deleted, the implementation is moved to the .cpp file.

That's automatically added by prepare-ChangeLog

>> Source/WebCore/rendering/style/GridPosition.cpp:37
>> +static const int kGridMaxTracks = 1000000;
> 
> I'm not completely sure about the names of this 2 things.
> I believe they should use a similar name: tracks vs position.
> 
> Also, I know "kGridMaxTracks" has been around before,
> but if I remember correctly we can end up with a grid of "2 * kGridMaxTracks" tracks
> because of the implicit grid before the explicit one.
> If that's the case, maybe we should try to find a better name.
> 
> Last, isn't this number too big? Probably we should have to reduce it
> to avoid memory issues like we did in Blink.

No problem, I can change it to kGridMaxPosition for example.

And yeah the number seems pretty big (it was suggested in the past by specs). Not sure if we should change it now.

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:19
>> + .lastRow { grid-row: -2 / -1; }
> 
> Nit: Using "-2" is enough, but feel free to keep "-2 / -1" if it's clearer for you.

Yeah that's right, but -2 / -1 seems much more understandable to me personally.

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:92
>> + function testElement(gridElement, property, length, classNames) {
> 
> Nit: Could we rename this method to something more self descriptive.
> This method returns the track sizes and also checks that the length of the tracks is the expected one.

OK, I'll try.

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:93
>> +     /* gridElement.style.gridTemplate = 'none';*/
> 
> Remove this line.

ack

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:98
>> +     // Seems like this is not enough to trigger a proper layout;
> 
> Mmm, what's this comment?

Old stuff

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:100
>> +     var propertyValue = getComputedStyle(gridElement, '').getPropertyValue(property);
> 
> Nit: 2nd argument of getComputedStyle() is optional AFAIK.

ack

>> LayoutTests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html:125
>> +     assert_equals(autoFillGrid[1], "2px");
> 
> Nit: Move these checks closer to the call to testElement() for "autoFillGrid".
> Also the order is a bit strange, I'd do: 0, 1, max - 2, max - 1.

As I guess is not an strong opinion, I'll leave them as they are, specially because all the other ones follow the same pattern.

[Sergio Villar Senin]

Committed r214604: <http://trac.webkit.org/changeset/214604>