Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654533003:
[css-grid] Move the track sizing algorithm to its own class (Closed)
|
Created:
3 years, 11 months ago by svillar Modified:
3 years, 10 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, Manuel Rego, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, svillar, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Move the track sizing algorithm to its own class
This is about moving the track sizing algorithm code out of LayoutGrid to
a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and
easy to use. A side effect of this patch is the removal of GridSizingData as
it is no longer needed. All the data structures in that class were
transferred to GridTrackSizingAlgorithm as attribute members.
The algorithm execution starts with the call to run(). It's mandatory to call
setup() before any call to run() in order to properly configure the behaviour
of the algorithm. You can call setup() & run() multiple times for a single layout
operation (normally twice, one for columns and another one for rows). The
algorithm uses an state machine to verify that the client issues the calls in
the proper order (i.e. first columns and then rows). After finishing the layout,
the client should call reset() to allow the algorithm to perform cleanups and
to prepare itself for another round of calls.
In order to implement the different behaviours of the algorithm depending on
whether the available size is definite or not, a strategy pattern was
implemented in the GridTrackSizingAlgorithmStrategy class. It has two
subclasses, one for definite sizes and another one for indefinite ones.
BUG=687184
Review-Url: https://codereview.chromium.org/2654533003
Cr-Commit-Position: refs/heads/master@{#447489}
Committed: https://chromium.googlesource.com/chromium/src/+/4cb3f97bf30b086d273330d03c92fcc8b6ea7070
Patch Set 1 #
Total comments: 63
Patch Set 2 : Changes after reviews #Patch Set 3 : Changes after review #Patch Set 4 : Build fixes #
Total comments: 15
Patch Set 5 : Build fix #
Total comments: 2
Patch Set 6 : Patch for landing #Patch Set 7 : Win build fix #
Messages
Total messages: 22 (10 generated)
Sending out for review. Although the patch is huge, not the most of it is just code moved from LayoutGrid to the other two new files. The track sizing algorithm code was basically left untouched, except a few method renames in order to adapt them to the new naming in the spec (we were still using MS pseudocode names)
Description was changed from ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. Users of the layout algorithm need to call just to methods, first the setup() to properly configure the algorithm's behaviour and then run(). After finishing the user would be able to access the tracks and some other subproducts of the algorithm like the grid intrinsic sizes. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. ========== to ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. Users of the layout algorithm need to call just to methods, first the setup() to properly configure the algorithm's behaviour and then run(). After finishing the user would be able to access the tracks and some other subproducts of the algorithm like the grid intrinsic sizes. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. ==========
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com
LGTM https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:13: typedef Vector<LayoutBox*, 1> GridCell; Can those LayoutBox pointer be null ? Perhaps we can use references. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:19: // TODO(svillar): move into this class once GridIterator is added. I don't understand this comment. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:22: Grid(const LayoutGrid*); Could we use reference here ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:35: GridArea gridItemArea(const LayoutBox& item) const; We don't need the "item" arg name. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:36: void setGridItemArea(const LayoutBox& item, GridArea); Ditto https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:89: HashMap<const LayoutBox*, size_t> m_gridItemsIndexesMap; Ditton about LayoutBox pointers. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:45: void setPlannedSize(const LayoutUnit&); Why we use a const LayoutUnit reference as input argument ? Does it really worth to avoid that copy ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:48: void setSizeDuringDistribution(const LayoutUnit&); ditto https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:50: void growSizeDuringDistribution(const LayoutUnit&); ditto https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:75: GridTrackSizingAlgorithm(const LayoutGrid* layoutGrid, Grid& grid) Wouldn't be better to use a reference ? I guess layoutGrid can't be null, right ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:101: LayoutUnit& freeSpace(GridTrackSizingDirection direction) { Why we return a reference here ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:183: SizingOperation m_sizingOperation; Wouldn't make sense to initialize this field ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:187: const LayoutGrid* m_layoutGrid; Why not using a reference here ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:190: // The track sizing algorithm is used for both layout and intrinsic size I think these comments need some formatting. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:208: // This is a RAII class used to ensure that the track sizing algorithm is need some formatting ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:222: m_algorithm.advanceNextState(); Why we need to call advanceNextState() when destroying the state machine instance ? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:238: virtual LayoutUnit minContentForChild(LayoutBox& child); the argument name is not needed. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:242: virtual void maximizeTracks(Vector<GridTrack>&, LayoutUnit& freeSpace); Why freeSpace is a reference ?
Many thanks for the review https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:13: typedef Vector<LayoutBox*, 1> GridCell; On 2017/01/30 08:48:48, jfernandez wrote: > Can those LayoutBox pointer be null ? Perhaps we can use references. Right, but I guess we can do it in a follow up change. Otherwise we would have to change many other things instead of moving code around. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:19: // TODO(svillar): move into this class once GridIterator is added. On 2017/01/30 08:48:48, jfernandez wrote: > I don't understand this comment. That's a legacy comment, I'll change it. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:22: Grid(const LayoutGrid*); On 2017/01/30 08:48:48, jfernandez wrote: > Could we use reference here ? I tried but it looks like we cannot because in computeIntrinsicLogicalWidths we need to pass the "this" const pointer. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:35: GridArea gridItemArea(const LayoutBox& item) const; On 2017/01/30 08:48:48, jfernandez wrote: > We don't need the "item" arg name. Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:89: HashMap<const LayoutBox*, size_t> m_gridItemsIndexesMap; On 2017/01/30 08:48:48, jfernandez wrote: > Ditton about LayoutBox pointers. Same comment here, perhaps for a followup change. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:45: void setPlannedSize(const LayoutUnit&); On 2017/01/30 08:48:49, jfernandez wrote: > Why we use a const LayoutUnit reference as input argument ? Does it really worth > to avoid that copy ? Yeah it does not really pay off, but I didn't change that code. I can use a LayoutUnit directly as it's a small change. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:75: GridTrackSizingAlgorithm(const LayoutGrid* layoutGrid, Grid& grid) On 2017/01/30 08:48:49, jfernandez wrote: > Wouldn't be better to use a reference ? I guess layoutGrid can't be null, right > ? Same reason here, we need to use the "this" pointer, which is a const pointer, for the intrinsic size computation. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:101: LayoutUnit& freeSpace(GridTrackSizingDirection direction) { On 2017/01/30 08:48:49, jfernandez wrote: > Why we return a reference here ? This hasn't changed. The reason is because some functions modify the free space. Then you have 2 options, you return by value and then call setFreeSpace(), or you return a reference and modify the value directly. The former was considered more convenient because you don't need to add a setFreeSpace call for every single early return in the code. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:183: SizingOperation m_sizingOperation; On 2017/01/30 08:48:49, jfernandez wrote: > Wouldn't make sense to initialize this field ? I don't think so. You need to specify it in the setup() call as you don't know in advance if you're going to do a layout or a intrinsic size computation. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:187: const LayoutGrid* m_layoutGrid; On 2017/01/30 08:48:49, jfernandez wrote: > Why not using a reference here ? See comments about the "this" pointer. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:190: // The track sizing algorithm is used for both layout and intrinsic size On 2017/01/30 08:48:48, jfernandez wrote: > I think these comments need some formatting. Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:208: // This is a RAII class used to ensure that the track sizing algorithm is On 2017/01/30 08:48:49, jfernandez wrote: > need some formatting ? Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:222: m_algorithm.advanceNextState(); On 2017/01/30 08:48:48, jfernandez wrote: > Why we need to call advanceNextState() when destroying the state machine > instance ? Because that's how this class works. It's created inside the run() method. So once created it checks the validity of the transition, and when the method finishes then it automatically advances the state, that's why I mention it's a RAII class, we use its lifetime to perform actions. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:238: virtual LayoutUnit minContentForChild(LayoutBox& child); On 2017/01/30 08:48:49, jfernandez wrote: > the argument name is not needed. Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:242: virtual void maximizeTracks(Vector<GridTrack>&, LayoutUnit& freeSpace); On 2017/01/30 08:48:48, jfernandez wrote: > Why freeSpace is a reference ? Because the maximize operation reduces the free space. As the free space is an attribute of the GridTrackSizingAlgorithm class we need to pass it here as reference to get it updated.
First one question, is this a single CL or it'd be a bunch of them? Shouldn't we report a bug and associate the CL with it? Probably the bug could describe the plan and the new classes introduced here. Or at least link the generic Grid Layout bug. This CL is doing different things, one is moving Grid to its own class. That seems a straight forward change, could be done in a separate CL? Then the introduction of GridTrackSizingAlgorithm which is a more complex stuff. You're moving a lot of code and at the same time doing some changes, so it's hard to review. Also, it'd be nice to add comments on the different classes, at least on some of them describing their purpose. I think this is a common pattern on Blink that was not so common on WebKit times. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.cpp:61: return m_gridItemsIndexesMap.get(&item); Some of these oneliner methods are implemented on the header file, other here. I don't have a strong preference, but I prefer we try to be consistent in a new file like this. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:1: #ifndef Grid_h I guess you miss the license header here. And in the rest of files of this patch too. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:13: typedef Vector<LayoutBox*, 1> GridCell; On 2017/01/30 09:35:45, svillar wrote: > On 2017/01/30 08:48:48, jfernandez wrote: > > Can those LayoutBox pointer be null ? Perhaps we can use references. > > Right, but I guess we can do it in a follow up change. Otherwise we would have > to change many other things instead of moving code around. Then add a TODO so we don't forget about it. :-) https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:20: class Grid final { Please add a brief comment explaining what's the purpose of this class. People might get confused with LayoutGrid and Grid. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:33: void setHasAnyOrthogonalGridItem(bool); This method could be implemented here, like the previous one. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:89: HashMap<const LayoutBox*, size_t> m_gridItemsIndexesMap; On 2017/01/30 09:35:45, svillar wrote: > On 2017/01/30 08:48:48, jfernandez wrote: > > Ditton about LayoutBox pointers. > > Same comment here, perhaps for a followup change. I think for the HashMap we need pointers, but maybe I'm wrong. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:60: return growthLimitIsInfinite() || m_growthLimit >= m_baseSize; Again oneliner method here and not in the header. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:74: LayoutUnit minContentForChild(LayoutBox& child) override; Name of the argument not needed here, and in the next definitions. The same happens for DefiniteSizeStrategy class. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:110: // FIXME(ALGO): repeated in LayoutGrid What's this comment? It should be something like "TODO(svillar): ...." https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:182: // DCHECK(isOrthogonalChild(child)); Why is this commented out here? Shouldn't we need to add a TODO or remove it or something? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:266: LayoutUnit DefiniteSizeStrategy::minContentForChild(LayoutBox& child) { Wow this DefiniteSizeStrategy::minContentForChild vs IndefiniteSizeStrategy::minContentForChild seem very similar to me. Cannot they share some logic? Also before this patch we only have one minContentForChild(). https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:457: // TODO(ALGO) Again a weird TODO here. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:487: // TODO(ALGO): marginIntrinsicLogicalWidthForChild is protected for You shouldn't use "ALGO", use "svillar" instead. Also missing dot at the end of comment. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:101: LayoutUnit& freeSpace(GridTrackSizingDirection direction) { On 2017/01/30 09:35:45, svillar wrote: > On 2017/01/30 08:48:49, jfernandez wrote: > > Why we return a reference here ? > > This hasn't changed. The reason is because some functions modify the free space. > Then you have 2 options, you return by value and then call setFreeSpace(), or > you return a reference and modify the value directly. The former was considered > more convenient because you don't need to add a setFreeSpace call for every > single early return in the code. Just a suggestion, maybe we could rename the method so it's clear we return a reference and avoid confusion with a "typo" or whatever. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:159: // Track sizing algorithm steps. Here I see 3 steps. But before I see helper methods for step 1, 2 and 4. What happened with step 3? Why we've 4 before and 3 now? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:159: // sizingData.nextState(); Mmmm, what's this? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:164: // DCHECK(sizingData.sizingState > GridSizingData::RowSizingFirstIteration); And this? https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:89: // TODO(ALGO): temporarily making these public. Again, use "svillar", also "T" should be a capital letter.
Thanks for the review! https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.cpp:61: return m_gridItemsIndexesMap.get(&item); On 2017/01/30 11:00:45, Manuel Rego wrote: > Some of these oneliner methods are implemented on the header file, > other here. > I don't have a strong preference, but I prefer we try to be consistent > in a new file like this. I think I'm being consistent, this is not a one-liner it's 3 lines. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:1: #ifndef Grid_h On 2017/01/30 11:00:45, Manuel Rego wrote: > I guess you miss the license header here. And in the rest of files of this patch > too. Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:33: void setHasAnyOrthogonalGridItem(bool); On 2017/01/30 11:00:45, Manuel Rego wrote: > This method could be implemented here, like the previous one. Nop because it'd break the rule of not having implementations except oneliners in the header file. Note that due to the weird chromium style it's difficult to get oneliners with long names. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/Grid.h:89: HashMap<const LayoutBox*, size_t> m_gridItemsIndexesMap; On 2017/01/30 11:00:45, Manuel Rego wrote: > On 2017/01/30 09:35:45, svillar wrote: > > On 2017/01/30 08:48:48, jfernandez wrote: > > > Ditton about LayoutBox pointers. > > > > Same comment here, perhaps for a followup change. > > I think for the HashMap we need pointers, but maybe I'm wrong. Indeed, we cannot have references in the HashMap https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:60: return growthLimitIsInfinite() || m_growthLimit >= m_baseSize; On 2017/01/30 11:00:45, Manuel Rego wrote: > Again oneliner method here and not in the header. Again it isn't a one liner with the new coding style. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:74: LayoutUnit minContentForChild(LayoutBox& child) override; On 2017/01/30 11:00:45, Manuel Rego wrote: > Name of the argument not needed here, and in the next definitions. > The same happens for DefiniteSizeStrategy class. Acknowledged. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:266: LayoutUnit DefiniteSizeStrategy::minContentForChild(LayoutBox& child) { On 2017/01/30 11:00:45, Manuel Rego wrote: > Wow this DefiniteSizeStrategy::minContentForChild vs > IndefiniteSizeStrategy::minContentForChild > seem very similar to me. > Cannot they share some logic? Yes they're pretty similar, we could refactor part of the logic in the parent class if you think it's better. > Also before this patch we only have one minContentForChild(). That's the whole point of having strategies, to separate the code for indefinite and definite size computations. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:487: // TODO(ALGO): marginIntrinsicLogicalWidthForChild is protected for On 2017/01/30 11:00:45, Manuel Rego wrote: > You shouldn't use "ALGO", use "svillar" instead. > Also missing dot at the end of comment. Yeah, I used those for tracking while refactoring the code. I'll remove them all. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:101: LayoutUnit& freeSpace(GridTrackSizingDirection direction) { On 2017/01/30 11:00:46, Manuel Rego wrote: > On 2017/01/30 09:35:45, svillar wrote: > > On 2017/01/30 08:48:49, jfernandez wrote: > > > Why we return a reference here ? > > > > This hasn't changed. The reason is because some functions modify the free > space. > > Then you have 2 options, you return by value and then call setFreeSpace(), or > > you return a reference and modify the value directly. The former was > considered > > more convenient because you don't need to add a setFreeSpace call for every > > single early return in the code. > > Just a suggestion, maybe we could rename the method so it's clear > we return a reference and avoid confusion with a "typo" or whatever. I don't think the problem is the name. As mentioned if we want to change how we use it, it deserves another separate patch. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h:159: // Track sizing algorithm steps. On 2017/01/30 11:00:46, Manuel Rego wrote: > Here I see 3 steps. > But before I see helper methods for step 1, 2 and 4. > What happened with step 3? > Why we've 4 before and 3 now? Because the maximizeTracks step is done entirely in the strategies. I could add a oneliner in the algorithm which calls the strategy but I thought it was not adding anything. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:159: // sizingData.nextState(); On 2017/01/30 11:00:46, Manuel Rego wrote: > Mmmm, what's this? Oh I forgot to remove those. The states are now handled by a separate class. https://codereview.chromium.org/2654533003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:164: // DCHECK(sizingData.sizingState > GridSizingData::RowSizingFirstIteration); On 2017/01/30 11:00:46, Manuel Rego wrote: > And this? Yeah good catch. I forgot to remove them.
Applied the suggested changes. I also refactored minContentForChild, maxContentForChild and minSizeForChild in the parent class. They just require 3 new virtual functions to be implemented by subclasses
LGTM. But I insist I'd report a bug and I'd explain there the new classes and how they're expected to work. You could write a small document, or just a brief explanation in the bug report, so at least there's some documentation about the purpose of this change. Note that I agree it's good to move things out of LayoutGrid, but I'd love to have it clearer all the classes (Algorithm, Strategy) and how to use them. Also that bug could be used to associate the patches for some TODOs: * // TODO(svillar): Repeated in LayoutGrid. static LayoutUnit computeMarginLogicalSizeForChild( * // TODO(svillar): we should use marginIntrinsicLogicalWidthForChild() instead // but it is protected for LayoutObjects. Apparently none of the current tests // fail, so we need a test case for this too. One comment about the description: > Users of the layout algorithm need to call just to methods, first the > setup() to properly configure the algorithm's behaviour and then run(). > After finishing the user would be able to access the tracks and some other > subproducts of the algorithm like the grid intrinsic sizes. Don't they need to call reset() too? Or is not mandatory? https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/Grid.h:27: // by a more memory efficient representation later. This class is used by the Nit: I'm not sure if the "eventually ... later" sentence is 100% right. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/Grid.h:51: size_t gridItemPaintOrder(const LayoutBox& item) const; Nit: "item" is not needed here. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:354: // may Nit: Strange new line here. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:502: // FIXME(svillar): we pass TrackSizing to gridTrackSize() because it does Nit: s/FIXME/TODO https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:503: // not really matter as we know Nit: Wrong line break? https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:571: // Mmmm, this comment is uncomplete or something. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:587: void GridTrackSizingAlgorithm::setup(GridTrackSizingDirection direction, Could we move this setup() method closer to run() and reset(), they seem quite related. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:607: m_flexibleSizedTracksIndex.shrink(0); Could we reuse reset() somehow or not? https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:693: // sizes (AvailableSpaceIndefinite). Nit: I believe this was already like that, but this "AvailableSpaceIndefinite" is not around since some time ago. We should update the comment. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:822: // LayoutGrid::increaseSizesToAccommodateSpanningItems(). Nit: Again probably not related to this patch but this method "increaseSizesToAccommodateSpanningItems()" is no longer present. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1249: // The function is not called if we don't have <flex> grid tracks Nit: Missing dot at the end of comment. https://codereview.chromium.org/2654533003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:78: class IndefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy { Should we call this IndefiniteSizeStrategy or IntrinsicSizesStrategy. For example, the fact that you cannot performa layout is not because the size of the grid container is indefinite. So I'm wondering if we're choosing the right name.
OK, I'll open a bug then https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/Grid.h (right): https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/Grid.h:27: // by a more memory efficient representation later. This class is used by the On 2017/01/31 09:37:09, Manuel Rego wrote: > Nit: I'm not sure if the "eventually ... later" sentence is 100% right. Acknowledged. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/Grid.h:51: size_t gridItemPaintOrder(const LayoutBox& item) const; On 2017/01/31 09:37:09, Manuel Rego wrote: > Nit: "item" is not needed here. Acknowledged. https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:587: void GridTrackSizingAlgorithm::setup(GridTrackSizingDirection direction, On 2017/01/31 09:37:09, Manuel Rego wrote: > Could we move this setup() method closer to run() and reset(), > they seem quite related. Sure https://codereview.chromium.org/2654533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:607: m_flexibleSizedTracksIndex.shrink(0); On 2017/01/31 09:37:09, Manuel Rego wrote: > Could we reuse reset() somehow or not? No because reset() also shrinks m_rows&m_columns, something we don't want to do. It also does set the state to columnSizingFirstIteration which is something we don't want to do. Note that this 2 shrinks were already done in the former computeUsedBreadthOfGridTracks() the old main function of the algorithm. https://codereview.chromium.org/2654533003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right): https://codereview.chromium.org/2654533003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:78: class IndefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy { On 2017/01/31 09:37:09, Manuel Rego wrote: > Should we call this IndefiniteSizeStrategy or IntrinsicSizesStrategy. > For example, the fact that you cannot performa layout is not because > the size of the grid container is indefinite. > So I'm wondering if we're choosing the right name. The name is correct IMO because this class implements the indefinite size branch of the algorithm. Said that, I agree that the fact that we cannot lay out is totally orthogonal to that. I should look for a better way to implement that.
Description was changed from ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. Users of the layout algorithm need to call just to methods, first the setup() to properly configure the algorithm's behaviour and then run(). After finishing the user would be able to access the tracks and some other subproducts of the algorithm like the grid intrinsic sizes. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. ========== to ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. The algorithm execution starts with the call to run(). It's mandatory to call setup() before any call to run() in order to properly configure the behaviour of the algorithm. You can call setup() & run() multiple times for a single layout operation (normally twice, one for columns and another one for rows). The algorithm uses an state machine to verify that the client issues the calls in the proper order (i.e. first columns and then rows). After finishing the layout, the client should call reset() to allow the algorithm to perform cleanups and to prepare itself for another round of calls. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. ==========
Description was changed from ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. The algorithm execution starts with the call to run(). It's mandatory to call setup() before any call to run() in order to properly configure the behaviour of the algorithm. You can call setup() & run() multiple times for a single layout operation (normally twice, one for columns and another one for rows). The algorithm uses an state machine to verify that the client issues the calls in the proper order (i.e. first columns and then rows). After finishing the layout, the client should call reset() to allow the algorithm to perform cleanups and to prepare itself for another round of calls. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. ========== to ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. The algorithm execution starts with the call to run(). It's mandatory to call setup() before any call to run() in order to properly configure the behaviour of the algorithm. You can call setup() & run() multiple times for a single layout operation (normally twice, one for columns and another one for rows). The algorithm uses an state machine to verify that the client issues the calls in the proper order (i.e. first columns and then rows). After finishing the layout, the client should call reset() to allow the algorithm to perform cleanups and to prepare itself for another round of calls. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. BUG=687184 ==========
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jfernandez@igalia.com, rego@igalia.com Link to the patchset: https://codereview.chromium.org/2654533003/#ps120001 (title: "Win build fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485940990381890,
"parent_rev": "98a8e7e16ec315c7b48513edb9b6b5d980ac4a48", "commit_rev":
"4cb3f97bf30b086d273330d03c92fcc8b6ea7070"}
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. The algorithm execution starts with the call to run(). It's mandatory to call setup() before any call to run() in order to properly configure the behaviour of the algorithm. You can call setup() & run() multiple times for a single layout operation (normally twice, one for columns and another one for rows). The algorithm uses an state machine to verify that the client issues the calls in the proper order (i.e. first columns and then rows). After finishing the layout, the client should call reset() to allow the algorithm to perform cleanups and to prepare itself for another round of calls. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. BUG=687184 ========== to ========== [css-grid] Move the track sizing algorithm to its own class This is about moving the track sizing algorithm code out of LayoutGrid to a new class GridTrackSizingAlgorithm, making the LayoutGrid more compact and easy to use. A side effect of this patch is the removal of GridSizingData as it is no longer needed. All the data structures in that class were transferred to GridTrackSizingAlgorithm as attribute members. The algorithm execution starts with the call to run(). It's mandatory to call setup() before any call to run() in order to properly configure the behaviour of the algorithm. You can call setup() & run() multiple times for a single layout operation (normally twice, one for columns and another one for rows). The algorithm uses an state machine to verify that the client issues the calls in the proper order (i.e. first columns and then rows). After finishing the layout, the client should call reset() to allow the algorithm to perform cleanups and to prepare itself for another round of calls. In order to implement the different behaviours of the algorithm depending on whether the available size is definite or not, a strategy pattern was implemented in the GridTrackSizingAlgorithmStrategy class. It has two subclasses, one for definite sizes and another one for indefinite ones. BUG=687184 Review-Url: https://codereview.chromium.org/2654533003 Cr-Commit-Position: refs/heads/master@{#447489} Committed: https://chromium.googlesource.com/chromium/src/+/4cb3f97bf30b086d273330d03c92... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4cb3f97bf30b086d273330d03c92... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
