[CSS Grid Layout] Tracks growing beyond limits when they should not

[CSS Grid Layout] Tracks growing beyond limits when they should not

In particular it incorrectly grows some tracks beyond limits when handling
min-content and max-content base sizes. In those cases we should only grow
"any affected track that happens to also have an intrinsic max track sizing
function" or all of them if there are none.

The problem is that we're using a vector of indexes pointing to the vector
with the list of affected tracks. Those indexes become easily incorrect
because the first thing we do in distributeSpaceToTracks() is to sort the
vector with the affected tracks by growth potential.

Instead we now pass a vector with pointers to the list of tracks allowed to
grow over the limits. The size changes are now directly stored in the
GridTracks (it's called plannedIncrease) instead of in a separate vector, so
we don't need to worry about tracks being rearranged (and we also get rid of
the ugly vector of indexes pointing to another vector).

Apart from that I took the chance to "modernize" some of our loops with the
use of range-based loops and "auto".

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

[svillar, 2015-01-15 13:52:00]

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

[svillar, 2015-01-15 13:52:02]

Adding reviewers...

[Julien - ping for review, 2015-01-19 16:36:25]

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

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:800: ASSERT(availableLogicalSpace > 0);
If you don't have any |availableLogicalSpace|, it seems wasteful to try to
distribute it below.

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

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:46: , m_plannedIncrease(0)
TBH this field doesn't seem to belong here. It's used only in a single place in
the algorithm ("distribute extra space") soit could be kept outside of
GridTrack.

We used to have |distributeTrackVector| to store the planned increase outside
GridTrack and it's unclear that this approach was completely wrong, just that
our implementation was not totally right.

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:113: m_plannedIncrease += plannedIncrease;
This concerns me because we don't enforce that we don't forget to commit this
planned increase.

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:833: LayoutUnit growthShare =
availableLogicalSpace / (tracksGrowingAboveMaxBreadthSize - i++);
The pattern with the auto-for loop + manual iterator doesn't seem like an
improvement over the old code that was explicit.

[svillar, 2015-01-20 08:23:24]

Thanks for the review.

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

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:800: ASSERT(availableLogicalSpace > 0);
On 2015/01/19 16:36:24, Julien Chaffraix - CET wrote:
> If you don't have any |availableLogicalSpace|, it seems wasteful to try to
> distribute it below.

Correct, I removed it by accident, it was part of a previous downstream instance
of the patch. I'll bring it back.

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

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:46: , m_plannedIncrease(0)
On 2015/01/19 16:36:24, Julien Chaffraix - CET wrote:
> TBH this field doesn't seem to belong here. It's used only in a single place
in
> the algorithm ("distribute extra space") soit could be kept outside of
> GridTrack.
> 
> We used to have |distributeTrackVector| to store the planned increase outside
> GridTrack and it's unclear that this approach was completely wrong, just that
> our implementation was not totally right.

It isn't because the previous approach was wrong, just that this solution seemed
convenient as it allows us not to care about sorts in the Vector of tracks. 

Also that would simplify the fix for another bug I found, which is that we
should not update the sizes of tracks after processing each item, but after
processing all the items with the same span value.

Anyway I'll try some other thing.

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:833: LayoutUnit growthShare =
availableLogicalSpace / (tracksGrowingAboveMaxBreadthSize - i++);
On 2015/01/19 16:36:24, Julien Chaffraix - CET wrote:
> The pattern with the auto-for loop + manual iterator doesn't seem like an
> improvement over the old code that was explicit.

Yeah, I don't have a strong opinion here, I can rollback this change.

[Julien - ping for review, 2015-01-20 13:47:14]

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

https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:46: , m_plannedIncrease(0)
On 2015/01/20 08:23:24, svillar wrote:
> On 2015/01/19 16:36:24, Julien Chaffraix - CET wrote:
> > TBH this field doesn't seem to belong here. It's used only in a single place
> in
> > the algorithm ("distribute extra space") soit could be kept outside of
> > GridTrack.
> > 
> > We used to have |distributeTrackVector| to store the planned increase
outside
> > GridTrack and it's unclear that this approach was completely wrong, just
that
> > our implementation was not totally right.
> 
> It isn't because the previous approach was wrong, just that this solution
seemed
> convenient as it allows us not to care about sorts in the Vector of tracks. 
> 
> Also that would simplify the fix for another bug I found, which is that we
> should not update the sizes of tracks after processing each item, but after
> processing all the items with the same span value.
> 
> Anyway I'll try some other thing.

We discussed this part of the change over IRC and Sergio gave some good
arguments towards keeping that.

We currently keep the planned increase independently but that conflicts with the
different sorting that are done during the algorithm. We could wrap GridTrack
into a class with the planned increase but there would be little benefit to do
so (we don't store GridTrack outside layout so the memory footprint argument is
moot) and that would yield to more complexity.

All in all, we are better off with the field inside GridTrack.

[svillar, 2015-01-20 14:35:31]

On 2015/01/20 13:47:14, Julien Chaffraix - CET wrote:
>
https://codereview.chromium.org/845313003/diff/1/Source/core/rendering/Render...
> All in all, we are better off with the field inside GridTrack.

Patch set 2 uploaded with the changes suggested by Julien.

[Julien - ping for review, 2015-01-23 10:20:12]

lgtm

https://codereview.chromium.org/845313003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution.html
(right):

https://codereview.chromium.org/845313003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution.html:371:
testGridColumnsValues("gridMaxContentAndMaxContentFixedAndMaxContent", "70px
20px 50px");
This result doesn't seem correct. We should spread the extra space over all the
columns AFAICT.

I don't think this should block the patch but we should follow-up.

https://codereview.chromium.org/845313003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/845313003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:797: // Specs mandate to floor
additionalBreadthSpace (extra-space in specs) to 0. Instead we directly avoid
the function
The comment still talks about |additionalBreadthSpace|.

https://codereview.chromium.org/845313003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:824: for (size_t i = 0; i < tracksSize;
++i) {
How about we use a C++11 auto loop here too?

[svillar, 2015-01-23 10:39:47]

https://codereview.chromium.org/845313003/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution.html
(right):

https://codereview.chromium.org/845313003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution.html:371:
testGridColumnsValues("gridMaxContentAndMaxContentFixedAndMaxContent", "70px
20px 50px");
On 2015/01/23 10:20:12, Julien Chaffraix - CET wrote:
> This result doesn't seem correct. We should spread the extra space over all
the
> columns AFAICT.
> 
> I don't think this should block the patch but we should follow-up.

I am not that sure about that. Note that the second track has a fixed size
growth limit. This means that for growing beyond limits, that track won't be
eligible, because for max-content min track functions we should only grow those
tracks with max-content max track sizing functions.

[svillar, 2015-01-23 13:44:10]

https://codereview.chromium.org/845313003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/845313003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:824: for (size_t i = 0; i < tracksSize;
++i) {
On 2015/01/23 10:20:12, Julien Chaffraix - CET wrote:
> How about we use a C++11 auto loop here too?

I did it in the first patch but you said "The pattern with the auto-for loop +
manual iterator doesn't seem like an improvement over the old code that was
explicit." so I removed it in this second version.

[svillar, 2015-01-23 14:01:33]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2015-01-23 14:01:50]

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

[commit-bot: I haz the power, 2015-01-23 15:23:06]

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