Bug 143481 – [CSS Grid Layout] Fix raw function pointer usages

• Component: CSS
• Status: RESOLVED FIXED

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Created attachment 250426 [details]
Patch

[Sergio Villar Senin]

Created attachment 250428 [details]
Patch

Build fixes

[Antti Koivisto]

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 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]

(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]

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]

(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]

(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]

> 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]

Committed r182628: <http://trac.webkit.org/changeset/182628>