RESOLVED FIXED Bug 143481
[CSS Grid Layout] Fix raw function pointer usages
https://bugs.webkit.org/show_bug.cgi?id=143481
Summary [CSS Grid Layout] Fix raw function pointer usages
Sergio Villar Senin
Reported 2015-04-07 07:57:56 PDT
Attachments
Patch (15.58 KB, patch)
2015-04-09 01:37 PDT, Sergio Villar Senin
no flags
Patch (15.81 KB, patch)
2015-04-09 02:00 PDT, Sergio Villar Senin
koivisto: review+
Sergio Villar Senin
Comment 1 2015-04-09 01:37:48 PDT
Sergio Villar Senin
Comment 2 2015-04-09 02:00:01 PDT
Created attachment 250428 [details] Patch Build fixes
Antti Koivisto
Comment 3 2015-04-09 03:17:50 PDT
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.
Antti Koivisto
Comment 4 2015-04-09 07:48:18 PDT
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.
Sergio Villar Senin
Comment 5 2015-04-10 01:03:48 PDT
(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.
Antti Koivisto
Comment 6 2015-04-10 01:45:26 PDT
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).
Sergio Villar Senin
Comment 7 2015-04-10 01:58:55 PDT
(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);
Sergio Villar Senin
Comment 8 2015-04-10 02:09:18 PDT
(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.
Antti Koivisto
Comment 9 2015-04-10 02:39:02 PDT
> 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.
Sergio Villar Senin
Comment 10 2015-04-10 03:52:06 PDT
Note You need to log in before you can comment on or make changes to this bug.