This comes from anttik's review here: https://bugs.webkit.org/show_bug.cgi?id=141422#c5 and https://bugs.webkit.org/show_bug.cgi?id=141422#c6
Created attachment 250426 [details] Patch
Created attachment 250428 [details] Patch Build fixes
Comment on attachment 250428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250428&action=review > Source/WebCore/rendering/RenderGrid.cpp:648 > GridItemsSpanGroupRange spanGroupRange = { it, std::upper_bound(it, end, *it) }; > - resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth); > - resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMaxContentMinTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth); > - resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMaxTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::growthLimit, &GridTrack::growthLimitIfNotInfinite, &GridTrack::setGrowthLimit); > - resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMaxContentMaxTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::growthLimit, &GridTrack::growthLimitIfNotInfinite, &GridTrack::setGrowthLimit); > + resolveContentBasedTrackSizingFunctionsForItems(ResolveIntrinsicMinimums, direction, sizingData, spanGroupRange); > + resolveContentBasedTrackSizingFunctionsForItems(ResolveMaxContentMinimums, direction, sizingData, spanGroupRange); > + resolveContentBasedTrackSizingFunctionsForItems(ResolveIntrinsicMaximums, direction, sizingData, spanGroupRange); > + resolveContentBasedTrackSizingFunctionsForItems(ResolveMaxContentMaximums, direction, sizingData, spanGroupRange); So much more readable! > Source/WebCore/rendering/RenderGrid.cpp:675 > +enum TrackSizeRestriction { enum class would be slightly nicer. > Source/WebCore/rendering/RenderGrid.cpp:757 > + switch (phase) { > + case RenderGrid::ResolveIntrinsicMinimums: > + case RenderGrid::ResolveMaxContentMinimums: > + track.setBaseSize(track.plannedSize()); > + break; > + case RenderGrid::ResolveIntrinsicMaximums: > + case RenderGrid::ResolveMaxContentMaximums: > + track.setGrowthLimit(track.plannedSize()); > + break; > + } > +} You could return instead of break and ASSERT_NOT_REACHED() at the end. > Source/WebCore/rendering/RenderGrid.cpp:802 > + LayoutUnit itemSize; > + switch (phase) { > + case ResolveIntrinsicMinimums: > + case ResolveIntrinsicMaximums: > + itemSize = minContentForChild(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks); > + break; > + case ResolveMaxContentMinimums: > + case ResolveMaxContentMaximums: > + itemSize = maxContentForChild(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks); > + break; > + } Maybe this could have helper function as well? > Source/WebCore/rendering/RenderGrid.h:64 > + enum TrackSizeComputationPhase { > + ResolveIntrinsicMinimums, > + ResolveMaxContentMinimums, > + ResolveIntrinsicMaximums, > + ResolveMaxContentMaximums, > + }; > + typedef enum TrackSizeComputationPhase TrackSizeComputationPhase; enum class would be nice: enum class TrackSizeResolvePhase { IntrinsicMinimums, ... } for example. The typedef shoudn't be necessary. > Source/WebCore/rendering/RenderGrid.h:70 > + enum DistributeSpacePhase { > + DistributeSpaceToBaseSizes, > + DistributeSpaceToGrowthLimits, > + }; > + typedef enum DistributeSpacePhase DistributeSpacePhase; Here too: enum class SpaceDistributionPhase { ToBaseSizes, ... } for example.
Comment on attachment 250428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250428&action=review > Source/WebCore/rendering/RenderGrid.cpp:759 > +void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(TrackSizeComputationPhase phase, GridTrackSizingDirection direction, GridSizingData& sizingData, const GridItemsSpanGroupRange& gridItemsWithSpan) This could be renamed to something that doesn't include "TrackSizingFunctions" since you no longer pass any functions. Since this is only ever called with fixed TrackSizeComputationPhase values you could make it a template argument instead. Maybe not very important.
(In reply to comment #4) > Comment on attachment 250428 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250428&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:759 > > +void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(TrackSizeComputationPhase phase, GridTrackSizingDirection direction, GridSizingData& sizingData, const GridItemsSpanGroupRange& gridItemsWithSpan) > > This could be renamed to something that doesn't include > "TrackSizingFunctions" since you no longer pass any functions. The functions that this method is resolving are not the ones passed as arguments but the minimum size and maximum size functions specified by authors when defining the grid, so I think the name is still accurate. > Since this is only ever called with fixed TrackSizeComputationPhase values > you could make it a template argument instead. Maybe not very important. Sure, I actually also had the same idea after uploading the patch.
Use of enum classes is not actually that important as these enums are not used outside the class. TrackSizeComputationPhase and DistributeSpacePhase probably don't need to be public (unless you plan to use them somewhere else too).
(In reply to comment #6) > Use of enum classes is not actually that important as these enums are not > used outside the class. > > TrackSizeComputationPhase and DistributeSpacePhase probably don't need to be > public (unless you plan to use them somewhere else too). Agreed, I'm right now changing the functions to have a template parameter, something like: template <RenderGrid::TrackSizeComputationPhase phase> void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection, GridSizingData&, const GridItemsSpanGroupRange&) which gives us a nice set of calls: resolveContentBasedTrackSizingFunctionsForItems<ResolveIntrinsicMinimums>(direction, sizingData, spanGroupRange); resolveContentBasedTrackSizingFunctionsForItems<ResolveMaxContentMinimums>(direction, sizingData, spanGroupRange); resolveContentBasedTrackSizingFunctionsForItems<ResolveIntrinsicMaximums>(direction, sizingData, spanGroupRange); resolveContentBasedTrackSizingFunctionsForItems<ResolveMaxContentMaximums>(direction, sizingData, spanGroupRange);
(In reply to comment #7) > (In reply to comment #6) > > Use of enum classes is not actually that important as these enums are not > > used outside the class. > > > > TrackSizeComputationPhase and DistributeSpacePhase probably don't need to be > > public (unless you plan to use them somewhere else too). > > Agreed, I'm right now changing the functions to have a template parameter, > something like: Actually I think I'll do the same for the helper functions as the same rationale applies for them.
> Actually I think I'll do the same for the helper functions as the same > rationale applies for them. I don't think you need to do that, the compiler will figure it out.
Committed r182628: <http://trac.webkit.org/changeset/182628>