Bug 108110 - [CSS Grid Layout] Make resolveContentBasedTrackSizingFunctionsForItems reuse distributeSpaceToTracks
Summary: [CSS Grid Layout] Make resolveContentBasedTrackSizingFunctionsForItems reuse ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 103311
  Show dependency treegraph
 
Reported: 2013-01-28 13:39 PST by Julien Chaffraix
Modified: 2013-01-29 14:12 PST (History)
8 users (show)

See Also:


Attachments
Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing. (8.15 KB, patch)
2013-01-28 14:36 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (8.29 KB, patch)
2013-01-29 13:30 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2013-01-28 13:39:08 PST
The specification makes resolveContentBasedTrackSizingFunctionsForItems reuse distributeSpaceToTracks.

This makes sense for us to do the same as it will ease maintenance (less code duplication) but also because being close to the specified algorithm prevents any deviation from it.
Comment 1 Julien Chaffraix 2013-01-28 14:36:57 PST
Created attachment 185069 [details]
Proposed refactoring #1: change distributeSpaceToTracks arguments to match intent and allow for more sharing.
Comment 2 Tony Chang 2013-01-28 15:22:42 PST
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 3 Julien Chaffraix 2013-01-28 16:08:05 PST
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 4 Julien Chaffraix 2013-01-28 16:24:04 PST
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 5 Tony Chang 2013-01-28 16:49:26 PST
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 6 Build Bot 2013-01-28 18:59:45 PST
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
Comment 7 Julien Chaffraix 2013-01-29 13:30:09 PST
Created attachment 185296 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-01-29 14:12:47 PST
Comment on attachment 185296 [details]
Patch for landing

Clearing flags on attachment: 185296

Committed r141163: <http://trac.webkit.org/changeset/141163>
Comment 9 WebKit Review Bot 2013-01-29 14:12:52 PST
All reviewed patches have been landed.  Closing bug.