[css-grid] Computing free space on indefinite sized grids

[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved.

BUG=655390
Committed: https://crrev.com/02c5917e79b05910149b2e6eff680673d6e4a9b0
Cr-Commit-Position: refs/heads/master@{#426464}

[jfernandez, 2016-10-14 09:29:46]

Description was changed from

==========
[css-grid] Free space must be 0 for indefinite sizing computation

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

This makes sense only for  definite sizing computation, since when the
grid container size is indefinite, we assume zero as initial available
space. Hence, the resulting free space should be always zero for
indefinite sizing computation.

BUG=655390
==========

to

==========
[css-grid] Free space must be zero for indefinite sizing computation

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

This makes sense only for  definite sizing computation, since when the
grid container size is indefinite, we assume zero as initial available
space. Hence, the resulting free space should be always zero for
indefinite sizing computation.

BUG=655390
==========

[jfernandez, 2016-10-14 09:29:46]

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

[svillar, 2016-10-17 08:20:33]

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:696:
sizingData.freeSpace(direction) = LayoutUnit();
I disagree with this change for 2 reasons:

1) the statement is false, free space is not 0 for indefinite sizing
computations, it is indefinite. Also you're resetting it _after_ the indefinite
size computation.

2) I don't see the relationship with the other change. This code is executed
only during intrinsic size computation while the other change is on a method
executed during the actual layout so doing this should have 0 influence on the
other change.

[jfernandez, 2016-10-17 08:43:54]

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:696:
sizingData.freeSpace(direction) = LayoutUnit();
On 2016/10/17 08:20:33, svillar wrote:
> I disagree with this change for 2 reasons:
> 
> 1) the statement is false, free space is not 0 for indefinite sizing
> computations, it is indefinite. Also you're resetting it _after_ the
indefinite
> size computation.

Well, since we are initializing it to 0, we are getting a negative value after 
the indefinite size computation. This is confusing, as it's also the case when
grid containers available space is not enough for definite sized tracks; in this
case we assume the negative value as a potential overflow of the grid, which
will be handled by the <overflow-alignment> keyword.

About your  second point, it's not true that  computeTrackSizesForIndefiniteSize

is only called during intrinsic size computation; it's called as part of the
regular 
layoutBlock logic in case of  cachedHasDefiniteLogicalHeight() being false. It's
indeed confusing that we are setting the internal sizingOperation attribute 
to IntrinsicSizeComputation.

> 
> 2) I don't see the relationship with the other change. This code is executed
> only during intrinsic size computation while the other change is on a method
> executed during the actual layout so doing this should have 0 influence on the
> other change.

[svillar, 2016-10-17 09:13:19]

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2415313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:696:
sizingData.freeSpace(direction) = LayoutUnit();
On 2016/10/17 08:43:54, jfernandez wrote:
> On 2016/10/17 08:20:33, svillar wrote:
> > I disagree with this change for 2 reasons:
> > 
> > 1) the statement is false, free space is not 0 for indefinite sizing
> > computations, it is indefinite. Also you're resetting it _after_ the
> indefinite
> > size computation.
> 
> Well, since we are initializing it to 0, we are getting a negative value after

> the indefinite size computation. This is confusing, as it's also the case when
> grid containers available space is not enough for definite sized tracks; in
this
> case we assume the negative value as a potential overflow of the grid, which
> will be handled by the <overflow-alignment> keyword.

I understand but what you're doing here is exactly the same. By setting 0 you
make indistinguishable the cases of using all the available space and having an
indefinite size.

For me the actual fix would be to make both availableSize and freeSpace Optional
in SizingData and only update them if they are not null. That way we could
easily distinguish the cases you mention.

> About your  second point, it's not true that 
computeTrackSizesForIndefiniteSize
> 
> is only called during intrinsic size computation; it's called as part of the
> regular 
> layoutBlock logic in case of  cachedHasDefiniteLogicalHeight() being false.
It's
> indeed confusing that we are setting the internal sizingOperation attribute 
> to IntrinsicSizeComputation.

Correct for heights, I was thinking only about widths

[jfernandez, 2016-10-18 10:45:25]

Description was changed from

==========
[css-grid] Free space must be zero for indefinite sizing computation

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

This makes sense only for  definite sizing computation, since when the
grid container size is indefinite, we assume zero as initial available
space. Hence, the resulting free space should be always zero for
indefinite sizing computation.

BUG=655390
==========

to

==========
[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved. 

BUG=655390
==========

[svillar, 2016-10-18 14:11:39]

Looking good but we need more test cases for this change. We need to check the
behavior of content alignment and overflow when the intrinsic height is
different to the height after performing the layout (for example with
min|max-height restrictions)

[jfernandez, 2016-10-18 14:18:58]

On 2016/10/18 14:11:39, svillar wrote:
> Looking good but we need more test cases for this change. We need to check the
> behavior of content alignment and overflow when the intrinsic height is
> different to the height after performing the layout (for example with
> min|max-height restrictions)

Oh, right. I just wanted to be sure we agreed on the approach to compute
the free space on indefinite height cases. 

I'll provide a new patch with the additional test cases.

[svillar, 2016-10-20 08:34:40]

Patch lgtm. Just a suggestion for the tests. Could you add some margin-bottom to
visually separate the grids in the test, otherwise is quite difficult to know
where they start/end.

[jfernandez, 2016-10-20 12:18:45]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-10-20 12:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/2415313003/#ps60001
(title: "Clearer layout test cases.")

[commit-bot: I haz the power, 2016-10-20 12:19:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 13:28:21]

Description was changed from

==========
[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved. 

BUG=655390
==========

to

==========
[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved. 

BUG=655390
==========

[commit-bot: I haz the power, 2016-10-20 13:28:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:17:53]

Description was changed from

==========
[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved. 

BUG=655390
==========

to

==========
[css-grid] Computing free space on indefinite sized grids

The Grid Tracks sizing algorithm updates receives the available space
to be used as space for tracks. We hold a variable to store the
remaining free space for each dimension.

When the grid container size is indefinite we can't compute the
available free space after computing track sizes until such indefinite
size is resolved.

BUG=655390

Committed: https://crrev.com/02c5917e79b05910149b2e6eff680673d6e4a9b0
Cr-Commit-Position: refs/heads/master@{#426464}
==========

[commit-bot: I haz the power, 2016-10-21 13:17:54]

Patchset 4 (id:??) landed as
https://crrev.com/02c5917e79b05910149b2e6eff680673d6e4a9b0
Cr-Commit-Position: refs/heads/master@{#426464}