[webkit-reviews] review denied: [Bug 135701] [CSS Grid Layout] Sort items by span when resolving content-based track sizing functions : [Attachment 236190] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 135701: [CSS Grid Layout] Sort items by span when resolving content-based
track sizing functions
https://bugs.webkit.org/show_bug.cgi?id=135701

Attachment 236190: Patch
https://bugs.webkit.org/attachment.cgi?id=236190&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236190&action=review


Looks fine.

Test coverage doesn’t seem to be sufficient; it missed the fact that the sort
is not stable, and instead incorrectly is dependent on the pointer values of
the grid item renderers. That tells me that we did not check the stability
constraint for complex enough grids.

> Source/WebCore/rendering/RenderGrid.cpp:534
> +size_t RenderGrid::gridItemSpan(const RenderBox* child,
GridTrackSizingDirection direction)

This should take a RenderBox& rather than a RenderBox* since it takes an
argument that can’t be null.

> Source/WebCore/rendering/RenderGrid.cpp:536
> +    GridCoordinate childCoordinate = cachedGridCoordinate(child);

Seems this could just be called coordinate, since this is a local variable and
the child context is clear.

> Source/WebCore/rendering/RenderGrid.cpp:537
> +    GridSpan childSpan = (direction == ForRows) ? childCoordinate.rows :
childCoordinate.columns;

I suggest using the type GridSpan& or auto& for this instead of making a copy.
And also call it just span instead of childSpan.

> Source/WebCore/rendering/RenderGrid.cpp:539
> +    return childSpan.resolvedFinalPosition.toInt() -
childSpan.resolvedInitialPosition.toInt() + 1;

Do we have a guarantee this will not be negative?

> Source/WebCore/rendering/RenderGrid.cpp:542
> +typedef std::pair<RenderBox*, size_t> GridItemWithSpan;

Might be easier to read the code if this was a class or struct instead of a
pair. It’s not at all clear that “first” is the grid item and “second” is the
span.

It doesn’t make sense to use size_t for a span, since a span is not a
memory-object-size concept. I’m not sure why the existing code does this; it
should not. The type should be unsigned unless there is some reason it needs to
be 64-bit. I think that generally this file is overusing the size_t type a lot.
But especially for the span value, since we are using toInt to compute the
values here, this is an unwanted mix of types, since int is 32-bit and size_t
is often 64-bit.

> Source/WebCore/rendering/RenderGrid.cpp:546
> +static bool gridItemWithSpanSorter(const GridItemWithSpan& item1, const
GridItemWithSpan& item2)

This function should only compare the span values. It should not compare the
RenderBox pointers. It should be named isSpanLessThan, not “grid item with span
sorter”, because this comparison function is neither something that returns a
“grid item” nor a “sorter”; the current item makes it sound like it returns a
“grid item” that has a “span sorter”. Or we could use the naming scheme we use
elsewhere with stable_sort and call this compareSpans; not great but better
than this name.

> Source/WebCore/rendering/RenderGrid.cpp:551
> +    return item1.first < item2.first;

It is incorrect to sort by comparing pointers. In fact, it can even be a
security problem to sort things by pointer value.

> Source/WebCore/rendering/RenderGrid.cpp:554
> +static bool uniquePointerInPair(const GridItemWithSpan& item1, const
GridItemWithSpan& item2)

A function named “unique pointer in pair” should return a “unique pointer”.
Since it’s a predicate it should have a different name, an adjective phrase
instead of noun phrase. Maybe arePointersEqual.

> Source/WebCore/rendering/RenderGrid.cpp:563
>      for (size_t i = 0; i < sizingData.contentSizedTracksIndex.size(); ++i) {


Should change this to a modern for loop.

> Source/WebCore/rendering/RenderGrid.cpp:569
> +	   std::stable_sort(itemsSortedByIncreasingSpan.begin(),
itemsSortedByIncreasingSpan.end(), gridItemWithSpanSorter);

With the comparison function in this patch, stable_sort is not needed because
the comparison function fully orders the values. So if that function was
correct, I’d say we should switch to std::sort, since it can be more efficient
and we gain nothing from the more complex stable sort.

However, I think the intent here may in fact be to sort in a stable fashion. We
want to preserve the ordering of the grid items from the iterator when the
spans are equal, so I suppose it’s OK to use stable_sort and fix the comparison
function as described above.

> Source/WebCore/rendering/RenderGrid.cpp:570
> +	   Vector<GridItemWithSpan>::iterator end =
std::unique(itemsSortedByIncreasingSpan.begin(),
itemsSortedByIncreasingSpan.end(), uniquePointerInPair);

The type here should be auto as with the iterator in the loop below.

> Source/WebCore/rendering/RenderGrid.cpp:572
> +	   for (auto it = itemsSortedByIncreasingSpan.begin(); it != end; ++it)
{

If I was writing this I would probably use a shrink call and then a modern for
loop instead, but this way is also OK.

> Source/WebCore/rendering/RenderGrid.cpp:573
> +	       RenderBox* gridItem = it->first;

Since this can never be null, I suggest we make it a RenderBox& instead.

>
LayoutTests/fast/css-grid-layout/grid-item-order-in-content-sized-columns-resol
ution-expected.txt:8
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.
> +PASS grid-template-columns are identical after grid row swap.

It’s better to write tests so they make clear in the output what is being
tested. It’s not so good to just right out “success” over and over again.
Easier to see what’s failing. That’s why the standard functions write out what
is being tested as well as the test results.