Bug 141431 - [CSS Grid Layout] Support marking/unmarking tracks as infinitely growable
Summary: [CSS Grid Layout] Support marking/unmarking tracks as infinitely growable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on: 141422
Blocks: 60731
  Show dependency treegraph
 
Reported: 2015-02-10 07:26 PST by Sergio Villar Senin
Modified: 2015-04-13 06:22 PDT (History)
11 users (show)

See Also:


Attachments
Patch (12.96 KB, patch)
2015-04-08 01:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (19.59 KB, patch)
2015-04-10 07:48 PDT, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2015-02-10 07:26:01 PST
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.
Comment 1 Sergio Villar Senin 2015-04-08 01:59:13 PDT
Created attachment 250342 [details]
Patch
Comment 2 Darin Adler 2015-04-09 22:27:20 PDT
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);
Comment 3 Sergio Villar Senin 2015-04-10 07:00:24 PDT
(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
Comment 4 Sergio Villar Senin 2015-04-10 07:45:22 PDT
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.
Comment 5 Sergio Villar Senin 2015-04-10 07:48:19 PDT
Created attachment 250519 [details]
Patch
Comment 6 Darin Adler 2015-04-10 09:26:43 PDT
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.
Comment 7 Darin Adler 2015-04-10 09:28:22 PDT
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.
Comment 8 Sergio Villar Senin 2015-04-13 03:50:50 PDT
Committed r182704: <http://trac.webkit.org/changeset/182704>
Comment 9 Csaba Osztrogonác 2015-04-13 04:10:58 PDT
(In reply to comment #8)
> Committed r182704: <http://trac.webkit.org/changeset/182704>

It broke the build everywhere. Could you fix it?
Comment 10 Csaba Osztrogonác 2015-04-13 04:28:01 PDT
ping?
Comment 11 Sergio Villar Senin 2015-04-13 06:22:52 PDT
(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 :)