Bug 108110

Created attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing.

Comment on attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. View in context: https://bugs.webkit.org/attachment.cgi?id=185069&action=review > Source/WebCore/rendering/RenderGrid.cpp:202 > + Vector<GridTrack*> tracksForDistribution(tracksSize); > + for (size_t i = 0; i < tracksSize; ++i) > + tracksForDistribution[i] = tracks.data() + i; It seems unfortunate that we need to make a copy of the Vector here. Would it work to have the first param of distributeSpaceToTracks be Vector<GridTrack> and in resolveContentBasedTrackSizingFunctionsForItems, copy the result to |track|? > Source/WebCore/rendering/RenderGrid.cpp:332 > +void RenderGrid::distributeSpaceToTracks(Vector<GridTrack*>& tracks, Vector<GridTrack*>* tracksForGrowthAboveMaxBreadth, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, LayoutUnit& availableLogicalSpace) Can we make tracksForGrowthAboveMaxBreadth const?

Comment on attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. View in context: https://bugs.webkit.org/attachment.cgi?id=185069&action=review >> Source/WebCore/rendering/RenderGrid.cpp:202 >> + tracksForDistribution[i] = tracks.data() + i; > > It seems unfortunate that we need to make a copy of the Vector here. Would it work to have the first param of distributeSpaceToTracks be Vector<GridTrack> and in resolveContentBasedTrackSizingFunctionsForItems, copy the result to |track|? Your approach could work but would require us to create and use a permutation inside distributeSpaceToTracks to iterate over the tracks in the right order. Creating the permutation will need an array of |trackSize| integers. A copy of the tracks is 2 * |trackSize| integers so it's a win. If you think a bit forward, we will need to pass arbitrary groups of tracks to distributeSpaceToTracks (due to column / row spans), which we will need to order, set and copy back so I don't think the memory win will be that big in the end. Not to mention that you need to be careful that you are handling *copies* of the tracks. > Source/WebCore/rendering/RenderGrid.cpp:328 > + distributeSpaceToTracks(tracks, &tracks, trackGetter, trackGrowthFunction, additionalBreadthSpace); This needs a FIXME as it's wrong in the general case, depends on who is calling resolveContentBasedTrackSizingFunctionsForItems and works only because we have a limited testing.

Comment on attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. View in context: https://bugs.webkit.org/attachment.cgi?id=185069&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:202 >>> + tracksForDistribution[i] = tracks.data() + i; >> >> It seems unfortunate that we need to make a copy of the Vector here. Would it work to have the first param of distributeSpaceToTracks be Vector<GridTrack> and in resolveContentBasedTrackSizingFunctionsForItems, copy the result to |track|? > > Your approach could work but would require us to create and use a permutation inside distributeSpaceToTracks to iterate over the tracks in the right order. Creating the permutation will need an array of |trackSize| integers. A copy of the tracks is 2 * |trackSize| integers so it's a win. > > If you think a bit forward, we will need to pass arbitrary groups of tracks to distributeSpaceToTracks (due to column / row spans), which we will need to order, set and copy back so I don't think the memory win will be that big in the end. Not to mention that you need to be careful that you are handling *copies* of the tracks. Actually, let me amend that: by using pointers, the Vector is used to build the permutation over the tracks. This uses |trackSize| pointers (and not 2 * |trackSize| as we don't do a deep copy of the GridTrack) so it would be equivalent to your proposal in terms of memory. The upside I see is that it allows for much more flexibility when we will come around and implement column / row span.

Comment on attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. I think this is OK, but it's very confusing who mutates GridTracks and who doesn't. Using a mix of Vector<GridTrack> and Vector<GridTrack*> makes things more confusing.

Comment on attachment 185069 [details] Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. Attachment 185069 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16182254 New failing tests: fast/workers/worker-document-leak.html

Created attachment 185296 [details] Patch for landing

Comment on attachment 185296 [details] Patch for landing Clearing flags on attachment: 185296 Committed r141163: <http://trac.webkit.org/changeset/141163>

All reviewed patches have been landed. Closing bug.