[css-grid] Move the track sizing algorithm to its own class
Created attachment 300905 [details] Patch
Comment on attachment 300905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300905&action=review I like how the algorithm is implemented now. Please, consider my comment about the Strategy pattern for future refactoring efforts. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:65 > + return (m_growthLimit == infinity) ? m_baseSize : m_growthLimit; Do we need this parenthesis ? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:115 > +static GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid* grid, const RenderBox& child, GridTrackSizingDirection direction) Either grid is defined as a reference of we need a nullcheck before using it. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:186 > + index += index ? autoTrackStylesSize : 0; Can index be a negative value ? If that's the case, wouldn't make more sense as follows ? if (std::abs(index) > 0) index += autoTrackStylesSize; > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:187 > + return autoTrackStyles[index]; I guess that autoTrackStylesSize ensures index is always a positive value, however, should we add an ASSERT here ? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:280 > + GridItemWithSpan(RenderBox& gridItem, GridSpan span) Shouldn't be const RenderBox& gridItem ? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:822 > + : GridTrackSizingAlgorithmStrategy(algorithm) { } Have you considered using Templates for defining the different strategies instead of class inheritance ? GridTrackSizingAlgorithmTemplate<Strategy>::operationXXX() { Strategy::operationXXX(); } > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:864 > + // TODO(svillar): we pass TrackSizing to gridTrackSize() because it does not really matter This comment does not follow our style. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:916 > +class DefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy { Either using templates or class inheritance, perhaps it'd be clearer to define the different strategies in different files.
(In reply to comment #2) > Comment on attachment 300905 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300905&action=review > > I like how the algorithm is implemented now. Please, consider my comment > about the Strategy pattern for future refactoring efforts. > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:65 > > + return (m_growthLimit == infinity) ? m_baseSize : m_growthLimit; > > Do we need this parenthesis ? Nop, although I left that code unchanged, just moved it around. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:115 > > +static GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid* grid, const RenderBox& child, GridTrackSizingDirection direction) > > Either grid is defined as a reference of we need a nullcheck before using it. I'll add an ASSERT > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:186 > > + index += index ? autoTrackStylesSize : 0; > > Can index be a negative value ? If that's the case, wouldn't make more sense > as follows ? > > if (std::abs(index) > 0) > index += autoTrackStylesSize; Haven't changed that code. And the answer is yes, it might be negative, that's why it's defined as int and not as unsigned as some other variables in that method. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:187 > > + return autoTrackStyles[index]; > > I guess that autoTrackStylesSize ensures index is always a positive value, > however, should we add an ASSERT here ? Again I haven't changed that code. But I can add another assert if you want. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:280 > > + GridItemWithSpan(RenderBox& gridItem, GridSpan span) > > Shouldn't be const RenderBox& gridItem ? I haven't checked all the usages of that call, but again unchanged code. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:822 > > + : GridTrackSizingAlgorithmStrategy(algorithm) { } > > Have you considered using Templates for defining the different strategies > instead of class inheritance ? > > GridTrackSizingAlgorithmTemplate<Strategy>::operationXXX() { > Strategy::operationXXX(); > } Nice suggestion. I haven't, not sure what would be the benefit though. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:864 > > + // TODO(svillar): we pass TrackSizing to gridTrackSize() because it does not really matter > > This comment does not follow our style. Ack. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:916 > > +class DefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy { > > Either using templates or class inheritance, perhaps it'd be clearer to > define the different strategies in different files. Not sure it pays off the burden. I'd say that we could consider that if they became large, but from the time being I think they're well located right next to the algorithm mainly because the algorithm and the strategies are very coupled.
Created attachment 301353 [details] Patch
Created attachment 301356 [details] Patch
Created attachment 301500 [details] Patch Build fix
Comment on attachment 301500 [details] Patch Attachment 301500 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3110573 New failing tests: fast/css-grid-layout/grid-item-end-after-get-set.html fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html fast/css-grid-layout/grid-item-named-grid-line-resolution.html fast/css-grid-layout/grid-item-position-changed-dynamic.html fast/css-grid-layout/implicit-tracks-before-explicit.html fast/css-grid-layout/grid-item-negative-position-resolution.html fast/css-grid-layout/grid-item-area-get-set.html fast/css-grid-layout/grid-auto-columns-rows-get-set.html fast/css-grid-layout/grid-auto-flow-resolution.html fast/css-grid-layout/grid-item-start-before-get-set.html fast/css-grid-layout/grid-item-negative-indexes.html fast/css-grid-layout/grid-item-spanning-resolution.html fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html fast/css-grid-layout/grid-positioned-items-within-grid-implicit-track.html
Created attachment 301513 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 301637 [details] Patch Fixed an error in a newly added ASSERT
Ping reviewers. I know that the size of the patch makes it a bit scary but it's mostly moving code out of the RenderGrid to the new file plus some renames.
Comment on attachment 301637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301637&action=review Awesome job, just added a few nits here and there. > Source/WebCore/ChangeLog:9 > + GridTrackSizingAlgorithm, making the RenderGrid more compact and easy to maintain. A nice Nit: s/the RenderGrid/RenderGrid/ > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:93 > +// Static helper methods ////////////////////////////////////////////// Mmmm, is this line properly formatted? I'm fine with having comments to separate the different parts of the class, but I'd use the standard format for comments (finishing the sentence with a "dot" too). > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:157 > +// GridTrackSizingAlgorithm private /////////////////////////////////// Ditto. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:633 > +double GridTrackSizingAlgorithm::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, double flexFactorSum, LayoutUnit& leftOverSpace, const Vector<unsigned, 8>& flexibleTracksIndexes, std::unique_ptr<TrackIndexSet> tracksToTreatAsInflexible) const Nit: This line seems a bit longer than usual. :-) > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:695 > + // The function is not called if we don't have <flex> grid tracks Nit: Missing dot after comment. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:716 > +// GridTrackSizingAlgorithmStrategy /////////////////////////////// Ditto. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:976 > +// GridTrackSizingAlgorithm steps ///////////////////////////////// Ditto. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1109 > +// GridTrackSizingAlgorithm API /////////////////////////////////// Ditto.
Committed r212823: <http://trac.webkit.org/changeset/212823>