[CSS Grid Layout] Support marking tracks as infinitely growable

[CSS Grid Layout] Support marking tracks as infinitely growable

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 even when they are
really not, in order to produce more "natural" results.

For example the following case:
    grid-template-columns: auto auto;

    item 1 in column 1 with min-content = max-content = 10px;
    item 2 in columns 1-2 with min-content = 30, max-content = 100px;

will outcome (45px, 55px) without this patch. But considering the second
column as infinitely growable the result is (10px, 90px), a more "natural"
result because column 1 just needs to be 10px to accommodate item 1.

From now on we can flag GridTracks so that they can infinitely grow even
when the growth limit is finite. In order to completely support the feature
some changes were required:
  * m_plannedIncrease became m_plannedSize because we need to store both the
old and new values of the track size in order to decide if a track should be
considered infinitely growable. Consequently m_increaseDuringDistribution
became m_sizeDuringDistribution.
  * distributeSpaceToTracks() is now unconditionally called even though the
extra space is 0. That's because it computes the plannedSize value.
  * a new templatized function called markAsInfinitelyGrowableForTrackSizeComputationPhase()
marks/unmarks tracks as infinitely growable only during the max functions resolution.

Apart from that all the GridTrack's public attributes were made private so
that accessors are now provided for getting/setting their values. Also
AccumulatorSetterFunction was removed as it is no longer needed.

BUG=455724
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197976

[svillar, 2015-03-17 14:43:34]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jchaffraix@chromium.org

[Julien - ping for review, 2015-03-25 18:40:43]

I would prefer for the specification to be updated before we land this. It's
hard to say if we're doing an interoperable implementation without the algorithm
being updated (unless I missed something in the thread).

[svillar, 2015-03-26 09:10:30]

On 2015/03/25 18:40:43, Julien Chaffraix - PST wrote:
> I would prefer for the specification to be updated before we land this. It's
> hard to say if we're doing an interoperable implementation without the
algorithm
> being updated (unless I missed something in the thread).

Hmm the algorithm doesn't need to be updated. This has been there from the
beginning (it was in MS original algorithm) and then Tab and fantasai added it
to the very first version of the rewritten version.

They were just asking in that thread about the rationale of it because it was
not clear for them what was that for.

[svillar, 2015-03-31 18:32:29]

Julien this is an excerpt from the specs. As you can see what I have implemented
is already there

"For intrinsic maximums: Next increase the growth limit of tracks with a max
track sizing function of min-content or max-content (including auto) by
distributing extra space as needed to account for these items' min-content
contributions. Mark any tracks whose growth limit changed from infinite to
finite in this step as infinitely growable for the next step."

[svillar, 2015-04-07 15:06:20]

@cbiesinger, @jchaffraix ping

[Julien - ping for review, 2015-04-08 18:58:56]

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:97: m_plannedSize = plannedSize;
It seems weird to allow the plannedSize to be infinite. We don't allow that for
the baseSize and we may copy plannedSize into baseSize.

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:130: bool m_infinitelyGrowable;
We are storing one extra member compared to the specification
(m_sizeDuringDistribution doesn't exist), which will add differences in the
algorithm.

Also related is the fact that 'infinitely growable' is either plannedSize or
growthLimit being infinite.

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:442: ASSERT(!track.growthLimitIsInfinite() &&
!track.infinitelyGrowable());
This should be split in 2. Logical AND in ASSERT make it less clear which
predicate failed.

[svillar, 2015-04-10 09:43:09]

Thanks for the review.

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:97: m_plannedSize = plannedSize;
On 2015/04/08 18:58:56, Julien - PST CC moar reviewers wrote:
> It seems weird to allow the plannedSize to be infinite. We don't allow that
for
> the baseSize and we may copy plannedSize into baseSize.

In theory that's true, although not possible with the current code I agree that
we're somehow not enforcing the restriction you mention.

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:130: bool m_infinitelyGrowable;
On 2015/04/08 18:58:56, Julien - PST CC moar reviewers wrote:
> We are storing one extra member compared to the specification
> (m_sizeDuringDistribution doesn't exist), which will add differences in the
> algorithm.

It does exist in the original algorithm, it's called TempBreadth. I changed that
to m_sizeDuringDistribution because you requested it.

In the new algorithm there is no mention to that but it's just because the new
version is descriptive while the previous one it was pseudocode. In any case it
mentions that we need to "Mark any tracks whose growth limit changed from
infinite to finite in this step" which means that we have to store the previous
size in order to compare to the new one. I don't mind if we call it plannedSize
or whatever but we need to store that information anyway.

https://codereview.chromium.org/1010213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:442: ASSERT(!track.growthLimitIsInfinite() &&
!track.infinitelyGrowable());
On 2015/04/08 18:58:56, Julien - PST CC moar reviewers wrote:
> This should be split in 2. Logical AND in ASSERT make it less clear which
> predicate failed.

Acknowledged.

[svillar, 2015-04-30 07:41:57]

svillar@igalia.com changed reviewers:
+ dsinclair@chromium.org

[svillar, 2015-05-11 10:47:27]

Sorry for rebasing the code before posting a new patch but the patch was getting
really rotten after so much time.

[svillar, 2015-06-12 16:03:48]

@jchaffraix: this is a new version of the patch rebased against master. Hope you
could review it.

[svillar, 2015-06-12 16:07:08]

Adding some comments to help the review process

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:814: return growthLimit == infinity ?
track.baseSize() : growthLimit;
This is not strictly required BTW but the growthLimitIfNotInfinite() method was
called just from here so I decided to remove it.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:881: 
This is the implementation of step 2.3 from:
http://dev.w3.org/csswg/css-grid/#distribute-extra-space

[Manuel Rego, 2015-06-26 11:09:46]

rego@igalia.com changed reviewers:
+ rego@igalia.com

[Manuel Rego, 2015-06-26 11:09:48]

Non-owner LGTM.

I've added some minor comments and suggestions inline.

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html (right):

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:23: <div
class="firstRowBothColumn">XXXXXX XXX</div>
I guess that both items are in the first row is not needed.
Not really important but on a first sight, it's a bit weird
that we're placing both items overlapped.

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:27: <div
class="firstRowFirstColumn">XX</div>
Wouldn't be better to have just one "X" here?
That way we could check that the "20px" from minmax()
is actually being considered. 

The result would be the same "20px 80px".

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:83: bool infiniteGrowthPotential() const
Suggestion: maybe call it isInfiniteGrowthPotential()
or growthPotentialIsInfinite().

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:427: track.setInfinitelyGrowable(false);
Do we really need this?
I thoguht this was false by defult.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:881: 
On 2015/06/12 16:07:07, svillar wrote:
> This is the implementation of step 2.3 from:
> http://dev.w3.org/csswg/css-grid/#distribute-extra-space

I think you mean 2.2 here (otherwise I don't get it).

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:984: for (auto* track : tracks)
A pitty that we need an extra loop here. :/

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:992: GridTrack& track = *tracks[i];
Couldn't we use a range-based loop here as well?

[svillar, 2015-06-29 07:08:27]

Many thanks for the review!!!

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html (right):

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:23: <div
class="firstRowBothColumn">XXXXXX XXX</div>
On 2015/06/26 11:09:47, Manuel Rego wrote:
> I guess that both items are in the first row is not needed.
> Not really important but on a first sight, it's a bit weird
> that we're placing both items overlapped.

Why weird? The behavior is exactly the same, the algorithm does not take into
account that so.

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:27: <div
class="firstRowFirstColumn">XX</div>
On 2015/06/26 11:09:47, Manuel Rego wrote:
> Wouldn't be better to have just one "X" here?
> That way we could check that the "20px" from minmax()
> is actually being considered. 
> 
> The result would be the same "20px 80px".

Well this is test was made to check the infinitely growable part of the
algorithm, what you're suggesting is to also check the code that initializes the
used breadths something that IMO is out of scope (almost all the test will fail
in that case). 

Anyway I can change it.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:83: bool infiniteGrowthPotential() const
On 2015/06/26 11:09:48, Manuel Rego wrote:
> Suggestion: maybe call it isInfiniteGrowthPotential()
> or growthPotentialIsInfinite().

If you don't mind I'd like to keep the name as it has already changed before and
also is the same than the one I used in WebKit.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:427: track.setInfinitelyGrowable(false);
On 2015/06/26 11:09:47, Manuel Rego wrote:
> Do we really need this?
> I thoguht this was false by defult.

It's false by default but note that this algorithm can be called multiple times.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:881: 
On 2015/06/26 11:09:47, Manuel Rego wrote:
> On 2015/06/12 16:07:07, svillar wrote:
> > This is the implementation of step 2.3 from:
> > http://dev.w3.org/csswg/css-grid/#distribute-extra-space
> 
> I think you mean 2.2 here (otherwise I don't get it).

Sure 2.2 you're right.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:984: for (auto* track : tracks)
On 2015/06/26 11:09:48, Manuel Rego wrote:
> A pitty that we need an extra loop here. :/

Yes, but this is the best place to do it. Otherwise callers would have to do it.
There are basically 2 callers, one of them already loops over the tracks but the
other don't so we would have to add the loop anyway, just to some other part.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:992: GridTrack& track = *tracks[i];
On 2015/06/26 11:09:48, Manuel Rego wrote:
> Couldn't we use a range-based loop here as well?

It was considered in previous patches, but note that we need the 'i' anyway for
computations so in the end the code was not better.

[Manuel Rego, 2015-06-29 07:46:43]

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html (right):

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:23: <div
class="firstRowBothColumn">XXXXXX XXX</div>
I mean weird from an outsider point of view,
that might think that this is a condition for this specific case.
Also if you open the HTML in the content_shell won't see it very easily.

Anyway, not important at all, so feel free to keep it as it is.

https://codereview.chromium.org/1010213002/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/mark-as-infinitely-growable.html:27: <div
class="firstRowFirstColumn">XX</div>
Yeah, it's not related to this maybe you can've both tests
with 1 and 2 "X". It's always good to increase coverage :-)

Anyway, do whatever you think is better.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:83: bool infiniteGrowthPotential() const
Ok, fair enough.

https://codereview.chromium.org/1010213002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:992: GridTrack& track = *tracks[i];
Yes, I didn't realize about that.

[svillar, 2015-06-29 09:43:27]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2015-06-29 09:43:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010213002/40001

[commit-bot: I haz the power, 2015-06-29 10:54:39]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197976