Bug 170120 - [css-grid] Clamp the number of autorepeat tracks
Summary: [css-grid] Clamp the number of autorepeat tracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-27 09:57 PDT by Sergio Villar Senin
Modified: 2017-03-30 07:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (49.14 KB, patch)
2017-03-27 10:23 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (51.25 KB, patch)
2017-03-29 09:15 PDT, Sergio Villar Senin
rego: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2017-03-27 09:57:55 PDT
[css-grid] Clamp the number of autorepeat tracks
Comment 1 Sergio Villar Senin 2017-03-27 10:23:01 PDT
Created attachment 305477 [details]
Patch
Comment 2 Sergio Villar Senin 2017-03-27 10:24:12 PDT
This was causing crashes as the one reported to Chromium https://bugs.chromium.org/p/chromium/issues/detail?id=681381
Comment 3 Javier Fernandez 2017-03-27 13:24:14 PDT
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.
Comment 4 Alexey Proskuryakov 2017-03-27 17:23:15 PDT
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.
Comment 5 Sergio Villar Senin 2017-03-29 09:15:01 PDT
Created attachment 305743 [details]
Patch

Build fix for Mac
Comment 6 Manuel Rego Casasnovas 2017-03-30 02:13:38 PDT
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.
Comment 7 Sergio Villar Senin 2017-03-30 07:38:46 PDT
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.
Comment 8 Sergio Villar Senin 2017-03-30 07:40:56 PDT
Committed r214604: <http://trac.webkit.org/changeset/214604>