[css-grid] Percentage columns can always be resolved during layout

[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html

Committed: https://crrev.com/3043a48f261b5d1b4f175a2a8f2373ac22ae41f1
Cr-Commit-Position: refs/heads/master@{#398535}

[Manuel Rego, 2016-06-02 11:50:36]

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

[Manuel Rego, 2016-06-02 14:04:37]

I've just realized that AvailableSpaceType is kind of duplicated:
https://codereview.chromium.org/2030803003/

I'll do a new version of this patch using SizingOperation enum instead.

[Manuel Rego, 2016-06-02 14:41:27]

Uploaded new version, on top of this patch:
https://codereview.chromium.org/2030803003/

The new version use SizingOperation enum.

[jfernandez, 2016-06-02 21:58:28]

non-owner LGTM,  but let's fix some typos in the issue description:

The patch pass -> passes.

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: LayoutUnit
marginLogicalWidth = sizingData.sizingOperation == TrackSizing ?
computeMarginLogicalSizeForChild(InlineDirection, child) :
marginIntrinsicLogicalWidthForChild(child);
Is this code orthogonality-independent ?  I know the old code was like that, but
perhaps a TODO here would be a good idea.

[Manuel Rego, 2016-06-03 07:08:08]

Description was changed from

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch pass the AvailableSpaceType to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

to

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

[Manuel Rego, 2016-06-03 07:09:06]

Thanks for the review. Fixed the description.

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: LayoutUnit
marginLogicalWidth = sizingData.sizingOperation == TrackSizing ?
computeMarginLogicalSizeForChild(InlineDirection, child) :
marginIntrinsicLogicalWidthForChild(child);
On 2016/06/02 21:58:28, jfernandez wrote:
> Is this code orthogonality-independent ?  I know the old code was like that,
but
> perhaps a TODO here would be a good idea.

We've a TODO at the beginning of the method:
LayoutGrid::minSizeForChild().
Do we need a new TODO here?

[jfernandez, 2016-06-03 07:20:58]

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: LayoutUnit
marginLogicalWidth = sizingData.sizingOperation == TrackSizing ?
computeMarginLogicalSizeForChild(InlineDirection, child) :
marginIntrinsicLogicalWidthForChild(child);
On 2016/06/03 07:09:06, Manuel Rego wrote:
> On 2016/06/02 21:58:28, jfernandez wrote:
> > Is this code orthogonality-independent ?  I know the old code was like that,
> but
> > perhaps a TODO here would be a good idea.
> 
> We've a TODO at the beginning of the method:
> LayoutGrid::minSizeForChild().
> Do we need a new TODO here?

That TODO is for supporting orthogonal flows, but the code should be already
flow independent, so it should work exactly the same in orthogonal flows, even
not supporting that yet. That patch was already landed, but I guess I forgot
about
this line (specifically, the else part of the condition, because the left side
is already
flow independent).

[Manuel Rego, 2016-06-03 08:56:14]

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2033033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: LayoutUnit
marginLogicalWidth = sizingData.sizingOperation == TrackSizing ?
computeMarginLogicalSizeForChild(InlineDirection, child) :
marginIntrinsicLogicalWidthForChild(child);
I think both parts of the if are the same, as we call
computeMarginLogicalSizeForChild() always using InlineDirection.
The thing is that we're inside "if (isRowAxis)" and isRowAxis is:
bool isRowAxis = direction == childInlineDirection;

So we're already checking the child flow,
so I don't think we need anything special here at this moment.

[cbiesinger, 2016-06-06 22:03:10]

lgtm

[Manuel Rego, 2016-06-07 08:00:11]

Thanks for the reviews!

I'd like that @svillar takes a look anyway,
as he was the one introducing this enum previously, thanks.

[Manuel Rego, 2016-06-07 09:05:26]

Description was changed from

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

to

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

[svillar, 2016-06-08 10:38:04]

Code lgtm but some changes in the tests do not seem good to me

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:15:
PASS window.getComputedStyle(gridWithPercentWithoutSizeWithChildren,
'').getPropertyValue('grid-template-columns') is "3.5px"
decimals...

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html:34:
grid-template-columns: 50% 100%;
why?

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:24:
grid-template-columns: 50% [last];
I guess this is not really needed. It also leads to decimal values for pixels
something I don't like

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:103:
<div class="grid percentageSize justifyContentStart">
why justifyContentStart?

[Manuel Rego, 2016-06-08 11:56:30]

Thanks for the reviews.

What do we do with the decimals then?

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:15:
PASS window.getComputedStyle(gridWithPercentWithoutSizeWithChildren,
'').getPropertyValue('grid-template-columns') is "3.5px"
On 2016/06/08 10:38:03, svillar wrote:
> decimals...

If we don't want decimals, we'd need to do much bigger changes on the test.
Do you think we should change the whole test to get numbers without decimals.

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html:34:
grid-template-columns: 50% 100%;
On 2016/06/08 10:38:03, svillar wrote:
> why?

It was to get simpler results on the tests:
  50% of 7px = 3.5px
  100% of 7px = 7px
vs
  53% of 7px = 3.71px
  99% of 7px = 6.93px

Otherwise we'll have much more decimals.

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:24:
grid-template-columns: 50% [last];
On 2016/06/08 10:38:03, svillar wrote:
> I guess this is not really needed. It also leads to decimal values for pixels
> something I don't like

Again the same reason than before.

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
(right):

https://codereview.chromium.org/2033033002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html:103:
<div class="grid percentageSize justifyContentStart">
On 2016/06/08 10:38:03, svillar wrote:
> why justifyContentStart?

This was not needed, just a copy&paste from above.
I've removed it here and before.

[Manuel Rego, 2016-06-08 12:04:33]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-06-08 12:04:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, cbiesinger@chromium.org, svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/2033033002/#ps100001
(title: "Minor fixes on tests")

[commit-bot: I haz the power, 2016-06-08 12:04:46]

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

[commit-bot: I haz the power, 2016-06-08 13:34:11]

Description was changed from

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

to

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

[commit-bot: I haz the power, 2016-06-08 13:34:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-08 13:37:48]

Description was changed from

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html
==========

to

==========
[css-grid] Percentage columns can always be resolved during layout

The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG=616716
TEST=fast/css-grid-layout/grid-container-percentage-columns.html

Committed: https://crrev.com/3043a48f261b5d1b4f175a2a8f2373ac22ae41f1
Cr-Commit-Position: refs/heads/master@{#398535}
==========

[commit-bot: I haz the power, 2016-06-08 13:37:49]

Patchset 6 (id:??) landed as
https://crrev.com/3043a48f261b5d1b4f175a2a8f2373ac22ae41f1
Cr-Commit-Position: refs/heads/master@{#398535}