Both old and new tracks sizing algorithms instruct us to update the sizes of the content sized tracks only after distributing the extra space for all the items in the same span group. So far we have been doing it inside distributeSpaceToTracks(), i.e., once for every single item. That is wrong because it makes the algorithm order dependent. The problem is that our code is like this: resolveContentBasedTrackSizingFunctions() for each item resolveContentBasedTrackSizingFunctionsForItems() (x4) distributeSpaceToTracks() updateTrackSizes(); We must move the update out of the distributeSpaceToTracks(). We also need to call resolveContentBasedTrackSizingFunctionsForItems() with all the items with a particular span instead of one by one.
Created attachment 247328 [details] Patch
Ping
Knock, knock reviewers :)
Any chance to get this reviewed? It's more than 1 month old
Comment on attachment 247328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247328&action=review Ok but... > Source/WebCore/rendering/RenderGrid.cpp:619 > +struct GridItemsSpanGroupRange { > + Vector<GridItemWithSpan>::iterator rangeStart; > + Vector<GridItemWithSpan>::iterator rangeEnd; > +}; Maybe you could use WTF::IteratorRange? > Source/WebCore/rendering/RenderGrid.cpp:694 > + GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPosition.toInt()] : sizingData.rowTracks[trackPosition.toInt()]; Indexing to array with unchecked. > Source/WebCore/rendering/RenderGrid.cpp:696 > + spanningTracksSize += (track.*correctedTrackSize)(); > + if (!(trackSize.*filterFunction)()) Raw function pointers. > Source/WebCore/rendering/RenderGrid.cpp:699 > + sizingData.filteredTracks.append(&track); Raw pointer. All this stuff looks scary and potentially insecure. I don't doubt that the logic is sound but is there any way to make this code look less like a disaster waiting to happen?
Comment on attachment 247328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247328&action=review > Source/WebCore/rendering/RenderGrid.cpp:645 > + resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth); The argument list is rather incomprehensible. Maybe you should consider passing them as a struct.
Comment on attachment 247328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247328&action=review > Source/WebCore/rendering/RenderGrid.cpp:82 > + const LayoutUnit& plannedSize() const { return m_plannedSize; } Any particular reason to return a reference here? > Source/WebCore/rendering/RenderGrid.cpp:84 > + void setPlannedSize(const LayoutUnit& plannedSize) const LayoutUnit& plannedSize -> LayoutUnit plannedSize > Source/WebCore/rendering/RenderGrid.cpp:89 > + LayoutUnit& tempSize() { return m_tempSize; } Any particular reason to return a reference here?
Comment on attachment 247328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247328&action=review >> Source/WebCore/rendering/RenderGrid.cpp:82 >> + const LayoutUnit& plannedSize() const { return m_plannedSize; } > > Any particular reason to return a reference here? It was suggested by Darin in a previous patch to avoid giving public access to the attribute. But since we have setPlannedSize() now we can remove the reference. >> Source/WebCore/rendering/RenderGrid.cpp:84 >> + void setPlannedSize(const LayoutUnit& plannedSize) > > const LayoutUnit& plannedSize -> LayoutUnit plannedSize OK >> Source/WebCore/rendering/RenderGrid.cpp:89 >> + LayoutUnit& tempSize() { return m_tempSize; } > > Any particular reason to return a reference here? That's because that particular attribute is basically initialized and then incremented several times. So in order to avoid setTempSize(tempSize() + increment) I decided to return a reference and directly do tempSize() += increment >> Source/WebCore/rendering/RenderGrid.cpp:619 >> +}; > > Maybe you could use WTF::IteratorRange? Interesting, I'll try indeed. >> Source/WebCore/rendering/RenderGrid.cpp:645 >> + resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth); > > The argument list is rather incomprehensible. Maybe you should consider passing them as a struct. Agree, I'll do it in a follow-up patch. >> Source/WebCore/rendering/RenderGrid.cpp:699 >> + sizingData.filteredTracks.append(&track); > > Raw pointer. > > All this stuff looks scary and potentially insecure. I don't doubt that the logic is sound but is there any way to make this code look less like a disaster waiting to happen? This particular change does not add any of the issues you are mentioning. So although I agree with your smart comments I think we should fix those in a different bug in order to isolate these changes in the algorithm.
> This particular change does not add any of the issues you are mentioning. So > although I agree with your smart comments I think we should fix those in a > different bug in order to isolate these changes in the algorithm. Right, that's why I r+'d this. It was more of a general comment.
Committed r182472: <http://trac.webkit.org/changeset/182472>
(In reply to comment #10) > Committed r182472: <http://trac.webkit.org/changeset/182472> Some time travel in the changelog entry dates. I dunno if we normally fix that or not.
We usually don’t bother, but any committer can fix it if they like. Not worth posting a patch just to do so.