[CSSGridLayout] Pass the valid tracks for |tracksForGrowAboveMaxBreadth|

[CSSGridLayout] Pass the valid tracks for |tracksForGrowAboveMaxBreadth|

Section 10.4 of the specs describe how to resolve content based track sizing
functions. Among others it describes the "distribute extra space" algorithm.
The 3rd bullet of that algorithm specifies how to distribute (and also in which
subset of tracks) the extra space once all the tracks have reached their grow
limits.

Our implementation had 2 problems. First we were not passing a valid subset of
tracks (instead we were using all of them). Secondly the algorithm that was
distributing the extra space was not using that list of passed in tracks,
instead it was using all of them.

Now we use a function that filters the right tracks to be the target of the
extra space distribution depending on whether we're computing the min track
function or max track function.

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

[svillar, 2014-09-18 14:37:45]

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

[svillar, 2014-09-18 14:37:46]

Adding reviewers

[Julien - ping for review, 2014-09-30 17:36:57]

Sorry for the delay in reviewing, last week was pretty bad.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:734:
sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size()
- 1);
This line seems really weird: we are checking the track at |trackPosition| but
are inserting a different index. Why don't we just insert trackPosition.toInt()
? Also why is this logic operating on size_t and not GridTrack& ?

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:791: // affected tracks.
The specification uses 2 passes for growth. Is this implementation guaranteed to
perform like the specification?

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/GridTrackSize.h (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/GridTrackSize.h:113:
m_minTrackBreadthIsMinContentAndMaxTrackBreadthIsMinOrMaxContent =
minTrackBreadthIsMinContent && m_maxTrackBreadthIsMinOrMaxContent;
It seems like we should store "lower-level information" in this class. If we
stored whether we have min-track min-content or max-content and the same for
max-content, we could regenerate all these booleans.

That said, it's a space vs memory trace-off so we may need good data and hold
onto that until we have aligned our algorithm with the specification to see what
we actually need.

[svillar, 2014-10-01 07:48:57]

Thanks for the review.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:734:
sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size()
- 1);
On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> This line seems really weird: we are checking the track at |trackPosition| but
> are inserting a different index. Why don't we just insert
trackPosition.toInt()
> ? Also why is this logic operating on size_t and not GridTrack& ?

The reason is because this function and distributeSpaceToTracks operate on a
different set of tracks. This function filters the tracks between initial and
final position. But distributeSpaceToTracks will use only the set of filtered
tracks. And then we have to select from that set of filtered tracks just the
ones subject to grow beyond growth limits. That's why we collect the indexes of
the tracks inside sizingData.filteredTracks.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:791: // affected tracks.
On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> The specification uses 2 passes for growth. Is this implementation guaranteed
to
> perform like the specification?

Hmm I don't get this. Which 2 passes are you talking about?

I'm basing the implementation on the part that describes the distribution of
extra space http://dev.w3.org/csswg/css-grid/#distribute-extra-space when it
talks about "3. Distribute space beyond growth limits:"

The rest of the algorithm is already there if I am not wrong.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/GridTrackSize.h (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/GridTrackSize.h:113:
m_minTrackBreadthIsMinContentAndMaxTrackBreadthIsMinOrMaxContent =
minTrackBreadthIsMinContent && m_maxTrackBreadthIsMinOrMaxContent;
On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> It seems like we should store "lower-level information" in this class. If we
> stored whether we have min-track min-content or max-content and the same for
> max-content, we could regenerate all these booleans.

Yeah, that's actually what I'm partially doing by defining
minTrackBreadthIsMinContent. Anyway I agree that with the 4 booleans you propose
we should be done.

> That said, it's a space vs memory trace-off so we may need good data and hold
> onto that until we have aligned our algorithm with the specification to see
what
> we actually need.

Good point.

[svillar, 2014-10-08 13:55:25]

@jchaffraix I think I responded to your questions above. WDTY?

[svillar, 2014-10-17 13:14:17]

Gentle ping

[Julien - ping for review, 2014-10-21 16:58:08]

lgtm

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:734:
sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size()
- 1);
On 2014/10/01 at 07:48:56, svillar wrote:
> On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> > This line seems really weird: we are checking the track at |trackPosition|
but
> > are inserting a different index. Why don't we just insert
trackPosition.toInt()
> > ? Also why is this logic operating on size_t and not GridTrack& ?
> 
> The reason is because this function and distributeSpaceToTracks operate on a
different set of tracks. This function filters the tracks between initial and
final position. But distributeSpaceToTracks will use only the set of filtered
tracks. And then we have to select from that set of filtered tracks just the
ones subject to grow beyond growth limits. That's why we collect the indexes of
the tracks inside sizingData.filteredTracks.

Now I understand the rationale for the logic but storing the indexes into
another vector is super dangerous. Ideally we should push the GridTrack
directly. As the code stands, it won't work as we would need to get this index
back but if we were to introduce some wrappers around GridTrack to hold the
temporary growing information, we could make it work. It's a good follow-up
change.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:791: // affected tracks.
On 2014/10/01 07:48:57, svillar wrote:
> On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> > The specification uses 2 passes for growth. Is this implementation
guaranteed
> to
> > perform like the specification?
> 
> Hmm I don't get this. Which 2 passes are you talking about?

I think I was referring to this text from step 2: "(and continuing to grow the
unfrozen tracks as needed)". I thought we would need some extra passes to do it
but on second thoughts we should be good as we order the tracks by grow
potential.

> I'm basing the implementation on the part that describes the distribution of
> extra space http://dev.w3.org/csswg/css-grid/#distribute-extra-space when it
> talks about "3. Distribute space beyond growth limits:"
> 
> The rest of the algorithm is already there if I am not wrong.

You're right, most of the algorithm is here.

[svillar, 2014-10-22 07:16:17]

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:734:
sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size()
- 1);
On 2014/10/21 16:58:08, Julien Chaffraix - PST wrote:
> On 2014/10/01 at 07:48:56, svillar wrote:
> > On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> > > This line seems really weird: we are checking the track at |trackPosition|
> but
> > > are inserting a different index. Why don't we just insert
> trackPosition.toInt()
> > > ? Also why is this logic operating on size_t and not GridTrack& ?
> > 
> > The reason is because this function and distributeSpaceToTracks operate on a
> different set of tracks. This function filters the tracks between initial and
> final position. But distributeSpaceToTracks will use only the set of filtered
> tracks. And then we have to select from that set of filtered tracks just the
> ones subject to grow beyond growth limits. That's why we collect the indexes
of
> the tracks inside sizingData.filteredTracks.
> 
> Now I understand the rationale for the logic but storing the indexes into
> another vector is super dangerous. Ideally we should push the GridTrack
> directly. As the code stands, it won't work as we would need to get this index
> back but if we were to introduce some wrappers around GridTrack to hold the
> temporary growing information, we could make it work. It's a good follow-up
> change.

Exactly that's the reason why I used the indexes, but I agree that we might
switch to some wrapper ala GridItemWithSpan in a follow-up change.

https://codereview.chromium.org/582733003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:791: // affected tracks.
On 2014/10/21 16:58:08, Julien Chaffraix - PST wrote:
> On 2014/10/01 07:48:57, svillar wrote:
> > On 2014/09/30 17:36:57, Julien Chaffraix - PST wrote:
> > > The specification uses 2 passes for growth. Is this implementation
> guaranteed
> > to
> > > perform like the specification?
> > 
> > Hmm I don't get this. Which 2 passes are you talking about?
> 
> I think I was referring to this text from step 2: "(and continuing to grow the
> unfrozen tracks as needed)". I thought we would need some extra passes to do
it
> but on second thoughts we should be good as we order the tracks by grow
> potential.

Yeah it isn't obvious after a first read but the fact that the algorithm does
the sorting before growing the tracks guarantees that we equally grow all of
them while respecting the upper limits.

My opinion is that the MS algorithm was designed to be efficient and avoid as
many passes as possible. The counterpart is that the resulting code is far for
self-explanatory. Switching to the rewritten algo will improve the situation
although we should be very careful with the implementation.

[svillar, 2014-10-22 08:55:39]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 08:56:16]

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

[commit-bot: I haz the power, 2014-10-22 08:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 08:56:37]

Failed to apply patch for
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
  Hunk #1 FAILED at 10.
  Hunk #2 FAILED at 25.
  2 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt.rej

Patch:      
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
Index:
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
diff --git
a/LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
b/LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
index
39a779a179b3dd96bf0c9a461354c410169fb2c5..3deac4e450eacfc3367a66efadf1ecdc6e16d20d
100644
---
a/LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
+++
b/LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt
@@ -10,6 +10,10 @@ PASS window.getComputedStyle(gridAutoMinContent,
'').getPropertyValue('grid-temp
 PASS window.getComputedStyle(gridAutoMaxContent,
'').getPropertyValue('grid-template-columns') is "20px 70px"
 PASS window.getComputedStyle(gridMaxContentAndMinContentFixed,
'').getPropertyValue('grid-template-columns') is "70px 20px"
 PASS window.getComputedStyle(gridMaxContentAndMaxContentFixed,
'').getPropertyValue('grid-template-columns') is "55px 35px"
+PASS window.getComputedStyle(gridMinContentFixedAndAutoAboveLimits,
'').getPropertyValue('grid-template-columns') is "15px 95px"
+PASS window.getComputedStyle(gridMaxContentFixedAndAutoAboveLimits,
'').getPropertyValue('grid-template-columns') is "65px 85px"
+PASS window.getComputedStyle(gridMinContentFixedAndFixedFixedAndAuto,
'').getPropertyValue('grid-template-columns') is "20px 20px 60px"
+PASS window.getComputedStyle(gridAutoAndFixedFixedAndMaxContentFixed,
'').getPropertyValue('grid-template-columns') is "40px 20px 90px"
 PASS successfullyParsed is true
 
 TEST COMPLETE
@@ -25,3 +29,11 @@ XXXX XXXX
 XXXX XXXX
 XXXX XXXX
 XXXX XXXX
+XXXX XXXX
+XXXXXXXXXXX
+XXXX XXXX
+XXXXXXXXXXXXXXX
+XXXX XXXX
+XXXXXXXXXX
+XXXX XXXX
+XXXXXXXXXXXXXXX

[svillar, 2014-10-22 09:09:24]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 09:10:27]

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

[commit-bot: I haz the power, 2014-10-22 09:24:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 09:25:00]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/3...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[svillar, 2014-10-22 10:09:57]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 10:10:47]

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

[commit-bot: I haz the power, 2014-10-22 11:56:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 11:56:54]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/33045)

[svillar, 2014-10-22 12:51:25]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 12:52:28]

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

[commit-bot: I haz the power, 2014-10-22 14:02:12]

Committed patchset #3 (id:40001) as 184181