Bug 109100

Created attachment 186944 [details] Proposed patch 1: Added a beefy test that uncovered several bugs in our implementation.

Comment on attachment 186944 [details] Proposed patch 1: Added a beefy test that uncovered several bugs in our implementation. 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) {

Comment on attachment 186944 [details] Proposed patch 1: Added a beefy test that uncovered several bugs in our implementation. 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). The over-growing was wrong. Before the change, we would end up trying to grow the size based on m_maxBreadth == infinity instead of settling on the used breadth. >> 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? Sure, in some cases, even if the column used breadth didn't change, we still need to recompute the (unconstrained) logical height to properly compute the row grid breadth. The bug it hit was to read the previous layout logical height value when the column used breadth didn't change. This wrong check was trying to be conservative instead of just always relayouting. I completely forgot the percent height descendant map which should cover this case better but not sure if it would work 100% of the time. >> 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. :) Sounds good. >> Source/WebCore/rendering/RenderGrid.cpp:409 >> + 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? True, we could fold this loop into the next one.

Created attachment 187719 [details] Updated patch per discussion with Ojan: Always force a layout in minContentForChild / maxContentForChild.

Comment on attachment 187719 [details] Updated patch per discussion with Ojan: Always force a layout in minContentForChild / maxContentForChild. View in context: https://bugs.webkit.org/attachment.cgi?id=187719&action=review > Source/WebCore/rendering/RenderGrid.cpp:297 > + if (!child->needsLayout()) > + child->setNeedsLayout(true, MarkOnlyThis); I don't think we need this bit actually. child->layout() should unconditionally do the layout, no? > Source/WebCore/rendering/RenderGrid.cpp:399 > + // FIXME: This should probably use a RAII pattern to ensure we always update our tracks. I don't understand this FIXME. We always set it in the for-loop below, no?

Comment on attachment 187719 [details] Updated patch per discussion with Ojan: Always force a layout in minContentForChild / maxContentForChild. View in context: https://bugs.webkit.org/attachment.cgi?id=187719&action=review >> Source/WebCore/rendering/RenderGrid.cpp:297 >> + child->setNeedsLayout(true, MarkOnlyThis); > > I don't think we need this bit actually. child->layout() should unconditionally do the layout, no? No, child->layout() will ASSERT if child doesn't need layout (see e.g. RenderBlock::layout) as you shouldn't be calling layout on a renderer that doesn't need it. This part of the change is required and matches what other renderers are doing to force a relayout on their children. >> Source/WebCore/rendering/RenderGrid.cpp:399 >> + // FIXME: This should probably use a RAII pattern to ensure we always update our tracks. > > I don't understand this FIXME. We always set it in the for-loop below, no? It's a stray FIXME, it should have been removed from this change.

Comment on attachment 187719 [details] Updated patch per discussion with Ojan: Always force a layout in minContentForChild / maxContentForChild. View in context: https://bugs.webkit.org/attachment.cgi?id=187719&action=review r=me with the stray FIXME removed. >>> Source/WebCore/rendering/RenderGrid.cpp:297 >>> + child->setNeedsLayout(true, MarkOnlyThis); >> >> I don't think we need this bit actually. child->layout() should unconditionally do the layout, no? > > No, child->layout() will ASSERT if child doesn't need layout (see e.g. RenderBlock::layout) as you shouldn't be calling layout on a renderer that doesn't need it. > > This part of the change is required and matches what other renderers are doing to force a relayout on their children. I see. I wonder how much value we get out of that assert. Seems sort of silly.

Comment on attachment 187719 [details] Updated patch per discussion with Ojan: Always force a layout in minContentForChild / maxContentForChild. View in context: https://bugs.webkit.org/attachment.cgi?id=187719&action=review >>>> Source/WebCore/rendering/RenderGrid.cpp:297 >>>> + child->setNeedsLayout(true, MarkOnlyThis); >>> >>> I don't think we need this bit actually. child->layout() should unconditionally do the layout, no? >> >> No, child->layout() will ASSERT if child doesn't need layout (see e.g. RenderBlock::layout) as you shouldn't be calling layout on a renderer that doesn't need it. >> >> This part of the change is required and matches what other renderers are doing to force a relayout on their children. > > I see. I wonder how much value we get out of that assert. Seems sort of silly. I strongly disagree with your comment. This is a performance ASSERT that guarantees that people don't call layout when it's not needed as layout is an already expensive operation.

Created attachment 188130 [details] Patch for landing

Comment on attachment 188130 [details] Patch for landing Clearing flags on attachment: 188130 Committed r142798: <http://trac.webkit.org/changeset/142798>

All reviewed patches have been landed. Closing bug.