Bug 143481

Summary: [CSS Grid Layout] Fix raw function pointer usages
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, jfernandez, kling, koivisto, kondapallykalyan, rego, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch koivisto: review+

Description Sergio Villar Senin 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
Comment 1 Sergio Villar Senin 2015-04-09 01:37:48 PDT
Created attachment 250426 [details]
Patch
Comment 2 Sergio Villar Senin 2015-04-09 02:00:01 PDT
Created attachment 250428 [details]
Patch

Build fixes
Comment 3 Antti Koivisto 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.
Comment 4 Antti Koivisto 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.
Comment 5 Sergio Villar Senin 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.
Comment 6 Antti Koivisto 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).
Comment 7 Sergio Villar Senin 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);
Comment 8 Sergio Villar Senin 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.
Comment 9 Antti Koivisto 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.
Comment 10 Sergio Villar Senin 2015-04-10 03:52:06 PDT
Committed r182628: <http://trac.webkit.org/changeset/182628>