[webkit-reviews] review granted: [Bug 170120] [css-grid] Clamp the number of autorepeat tracks : [Attachment 305743] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 170120: [css-grid] Clamp the number of autorepeat tracks
https://bugs.webkit.org/show_bug.cgi?id=170120

Attachment 305743: Patch

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




--- Comment #6 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 305743
  --> https://bugs.webkit.org/attachment.cgi?id=305743
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.