WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Attachments
Patch
(15.58 KB, patch)
2015-04-09 01:37 PDT
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(15.81 KB, patch)
2015-04-09 02:00 PDT
,
Sergio Villar Senin
koivisto
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2015-04-09 01:37:48 PDT
Created
attachment 250426
[details]
Patch
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
Committed
r182628
: <
http://trac.webkit.org/changeset/182628
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug