Summary: | [CSS Grid Layout] Make resolveContentBasedTrackSizingFunctionsForItems reuse distributeSpaceToTracks | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | buildbot, eric, ojan.autocc, ojan, rniwa, tony, webkit.review.bot, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 103311 | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2013-01-28 13:39:08 PST
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. |