Chromium Code Reviews
|
|
|
Created:
5 years ago by svillar Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[CSS Grid Layout] Replace the usage of pointers to functions
The argument list of resolveContentBasedTrackSizingFunctionsForItems()
became too long and rather incomprehensible as it included up to 5 function
pointers. This replaces all of them by an enum which describes the phase of
the algorithm that is currently running. With that phase we have enough
information to select the right function to call.
In order not to tangle up too much the method, the new explicit switch
statements where moved to private static helper functions.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196960
Patch Set 1 #
Total comments: 9
Patch Set 2 : Moved helpers to blink namespace #
Messages
Total messages: 18 (4 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jchaffraix@chromium.org
I'd like cbiesinger to review this.
svillar@igalia.com changed reviewers: + dsinclair@chromium.org, eaeltsin@google.com - eae@chromium.org
https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:113: static const LayoutUnit& trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&, TrackSizeRestriction); So the patch looks good except that I don't see any place where you pass in AllowInfinity here? What am I missing?
On 2015/04/30 23:33:02, cbiesinger wrote: > https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... > File Source/core/layout/LayoutGrid.h (right): > > https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... > Source/core/layout/LayoutGrid.h:113: static const LayoutUnit& > trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&, > TrackSizeRestriction); > So the patch looks good except that I don't see any place where you pass in > AllowInfinity here? What am I missing? You're totally right, I'll try to explain why it isn't used (yet). Basically a modified version of this patch (using AllowInfinity) was meant to be uploaded after landing 1010213002, but I decided to upload this first because I have already waited for too long for 1010213002 to be reviewed (1.5 months). So the plan would be to land this, and then upload a new version of 1010213002 using the new helper functions. Does it make sense?
lgtm. FYI I'm not an owner, so if you're not an owner yourself someone else will have to lgtm too. On 2015/05/01 10:40:50, svillar wrote: > On 2015/04/30 23:33:02, cbiesinger wrote: > > > https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... > > File Source/core/layout/LayoutGrid.h (right): > > > > > https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... > > Source/core/layout/LayoutGrid.h:113: static const LayoutUnit& > > trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&, > > TrackSizeRestriction); > > So the patch looks good except that I don't see any place where you pass in > > AllowInfinity here? What am I missing? > > You're totally right, I'll try to explain why it isn't used (yet). > > Basically a modified version of this patch (using AllowInfinity) was meant to be > uploaded after landing 1010213002, but I decided to upload this first because I > have already waited for too long for 1010213002 to be reviewed (1.5 months). So > the plan would be to land this, and then upload a new version of 1010213002 > using the new helper functions. Does it make sense? That makes sense! Thanks for the explanation.
On 2015/05/01 16:34:45, cbiesinger wrote: > lgtm. FYI I'm not an owner, so if you're not an owner yourself someone else will > have to lgtm too. I am not yet, perhaps any of you colleagues?
On 2015/05/01 16:55:48, svillar wrote: > On 2015/05/01 16:34:45, cbiesinger wrote: > > lgtm. FYI I'm not an owner, so if you're not an owner yourself someone else > will > > have to lgtm too. > > I am not yet, perhaps any of you colleagues? @dsinclair maybe you?
https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:113: static const LayoutUnit& trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&, TrackSizeRestriction); Can GridTrack be const? https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:114: static bool shouldProcessTrackForTrackSizeComputationPhase(TrackSizeComputationPhase, const GridTrackSize&); These are only called in this class, and don't seem like they use class knowledge, can they be moved to an anonymous namespace inside the .cpp file? https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:116: static void updateTrackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&); Can GridTrack be const? https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:121: template <TrackSizeComputationPhase> void distributeSpaceToTracks(Vector<GridTrack*>&, const Vector<GridTrack*>* growBeyondGrowthLimitsTracks, GridSizingData&, LayoutUnit& availableLogicalSpace); Why template these and not just pass in TrackSizeComputationPhase as a parameter?
Thanks for the review. https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:113: static const LayoutUnit& trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&, TrackSizeRestriction); On 2015/05/08 15:50:28, (OOO until May 19th) dsinclair wrote: > Can GridTrack be const? Acknowledged. https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:114: static bool shouldProcessTrackForTrackSizeComputationPhase(TrackSizeComputationPhase, const GridTrackSize&); On 2015/05/08 15:50:27, (OOO until May 19th) dsinclair wrote: > These are only called in this class, and don't seem like they use class > knowledge, can they be moved to an anonymous namespace inside the .cpp file? Sure. https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:116: static void updateTrackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase, GridTrack&); On 2015/05/08 15:50:28, (OOO until May 19th) dsinclair wrote: > Can GridTrack be const? Acknowledged. https://codereview.chromium.org/1079563002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:121: template <TrackSizeComputationPhase> void distributeSpaceToTracks(Vector<GridTrack*>&, const Vector<GridTrack*>* growBeyondGrowthLimitsTracks, GridSizingData&, LayoutUnit& availableLogicalSpace); On 2015/05/08 15:50:27, (OOO until May 19th) dsinclair wrote: > Why template these and not just pass in TrackSizeComputationPhase as a > parameter? The reason why I used templates is because we always explicitly call them with a given phase, it isn't that we compute a parameter and pass it to the function. They will obviously be equivalent but having a template makes IMO it more explicit the four phases of the computation.
@jchaffraix is it OK for you?
On 2015/05/27 14:46:00, svillar wrote: > @jchaffraix is it OK for you? Ping Julien...
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/1079563002/#ps20001 (title: "Moved helpers to blink namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1079563002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196960 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
