[webkit-reviews] review denied: [Bug 109100] [CSS Grid Layout] Removing grid items doesn't properly recompute the track sizes : [Attachment 186944] Proposed patch 1: Added a beefy test that uncovered several bugs in our implementation.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 109100: [CSS Grid Layout] Removing grid items doesn't properly recompute
the track sizes
https://bugs.webkit.org/show_bug.cgi?id=109100

Attachment 186944: Proposed patch 1: Added a beefy test that uncovered several
bugs in our implementation.
https://bugs.webkit.org/attachment.cgi?id=186944&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186944&action=review


> Source/WebCore/ChangeLog:22
> +	   This made us over-grow some cases in the test.

Is the overgrowing correct or incorrect? It's unclear from this description
("over" implies incorrect to me).

> Source/WebCore/rendering/RenderGrid.cpp:306
> +    if (usedBreadth != oldUsedBreadth ||
child->style()->logicalHeight().isPercent())

I don't understand the percent case. It's definitely wrong, but I'm not sure
what the right solution is. Can you explain this case to me?

> Source/WebCore/rendering/RenderGrid.cpp:333
> +    // FIXME: minContentForChild will already call layout on |child|, maybe
there is a way for us to avoid this second layout?

This FIXME is only for the percent case, right? The usedBreadths will be the
same if minContentForChild set it.

That said, we should move this second half of these functions into a helper
functions since they are identical and now becoming non-trivial. Arguably they
were non-trivial before. :)

> Source/WebCore/rendering/RenderGrid.cpp:409
> +    for (size_t i = 0; i < tracksSize; ++i)
> +	   updatedTrackSizes[i] = (tracks[i]->*trackGetter)();

I don't understand what this loop does. Don't we just overwrite all these
values in the next loop?

> Source/WebCore/rendering/RenderGrid.cpp:433
>      tracksSize = tracksForGrowthAboveMaxBreadth->size();
>      for (size_t i = 0; i < tracksSize; ++i) {
> -	   GridTrack& track = *tracksForGrowthAboveMaxBreadth->at(i);
>	   LayoutUnit growthShare = availableLogicalSpace / (tracksSize - i);
> -	   (track.*trackGrowthFunction)(growthShare);
> +	   updatedTrackSizes[i] += growthShare;
>	   availableLogicalSpace -= growthShare;
>      }

You could avoid duplicate code by wrapping this code in the following and then
always doing the for-loop below:
if (availableLogicalSpace > 0 && tracksForGrowthAboveMaxBreadth) {