Bug 141431 – [CSS Grid Layout] Support marking/unmarking tracks as infinitely growable

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

As explained here http://lists.w3.org/Archives/Public/www-style/2014Mar/0512.html we sometimes need to consider that some tracks are infinitely growable when they are really not, in order to produce better results.

One example is this case:

* Two "auto" tracks (i.e. "minmax(min-content, max-content) minmax(min-content, max-content)")
* Item 1 is in track 1, and has min-content = max-content = 10.
* Item 2 spans tracks 1 and 2, and has min-content = 30, max-content = 100.

Our current implementation will outcome [45px, 55px]. But with this change that would be [10, 90] which is considered more "natural" from the algorithm author's POV.

[Sergio Villar Senin]

Created attachment 250342 [details]
Patch

[Darin Adler]

Comment on attachment 250342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250342&action=review

> Source/WebCore/rendering/RenderGrid.cpp:76
> +    bool growthLimitIsInfiniteOrTrackIsInfinitelyGrowable() const

I wonder if there’s a shorter name for this that could still be just as clear.

> Source/WebCore/rendering/RenderGrid.cpp:726
> +        auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;

This could be a reference instead of a pointer.

> Source/WebCore/rendering/RenderGrid.cpp:735
> +        if ((track.*trackSize)() == infinity && track.plannedSize() != infinity)
> +            track.setInfinitelyGrowable(true);
> +        else if (track.infinitelyGrowable())
> +            track.setInfinitelyGrowable(false);

This should be written more simply:

    track.setInfinitelyGrowable((track.*trackSize)() == infinity && track.plannedSize() != infinity);

[Sergio Villar Senin]

(In reply to comment #2)
> Comment on attachment 250342 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250342&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:76
> > +    bool growthLimitIsInfiniteOrTrackIsInfinitelyGrowable() const
> 
> I wonder if there’s a shorter name for this that could still be just as
> clear.

I think I can use trackIsInfinitelyGrowable() because that includes the case of growth limit being infinite.

> 
> > Source/WebCore/rendering/RenderGrid.cpp:726
> > +        auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
> 
> This could be a reference instead of a pointer.

Yes it's just used as an argument of a function that requires a pointer (as it could be nullptr)

> > Source/WebCore/rendering/RenderGrid.cpp:735
> > +        if ((track.*trackSize)() == infinity && track.plannedSize() != infinity)
> > +            track.setInfinitelyGrowable(true);
> > +        else if (track.infinitelyGrowable())
> > +            track.setInfinitelyGrowable(false);
> 
> This should be written more simply:
> 
>     track.setInfinitelyGrowable((track.*trackSize)() == infinity &&
> track.plannedSize() != infinity);

Yeah. In any case I'm changing it now that we have explicit phases of the algorithm added in https://bugs.webkit.org/show_bug.cgi?id=143481

[Sergio Villar Senin]

Comment on attachment 250342 [details]
Patch

Clearing the r+ as I'm going to attach a different version of the patch based on the changes added in https://bugs.webkit.org/show_bug.cgi?id=143481. Basically we have now extra information about the step/phase of the algorithm that is being run, so we can take better decisions.

[Sergio Villar Senin]

Created attachment 250519 [details]
Patch

[Darin Adler]

Comment on attachment 250342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250342&action=review

>>> Source/WebCore/rendering/RenderGrid.cpp:726
>>> +        auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
>> 
>> This could be a reference instead of a pointer.
> 
> Yes it's just used as an argument of a function that requires a pointer (as it could be nullptr)

And when passing to that function, you can use & right at that point (&tracksToGrowBeyondGrowthLimits); it’s better to keep locals as references right until they get to the point where they need to become pointers, especially if the code might be refactored later.

[Darin Adler]

Comment on attachment 250519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250519&action=review

> Source/WebCore/rendering/RenderGrid.cpp:839
> +        auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;

Looking at this expression, I wish there was a function to do this that took a sizingData argument, or even a member function.

I still stand by my suggestion that we use a reference here and put the & on the next line of code instead of this one.

[Sergio Villar Senin]

Committed r182704: <http://trac.webkit.org/changeset/182704>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r182704: <http://trac.webkit.org/changeset/182704>

It broke the build everywhere. Could you fix it?

[Csaba Osztrogonác]

ping?

[Sergio Villar Senin]

(In reply to comment #9)
> (In reply to comment #8)
> > Committed r182704: <http://trac.webkit.org/changeset/182704>
> 
> It broke the build everywhere. Could you fix it?

Should be fixed. Sorry for the inconvenience, a last minute change as usual :)