[CSS Grid Layout] Determine definite/indefinite size properly in nested grids

[CSS Grid Layout] Determine definite/indefinite size properly in nested grids

If a grid item is a grid itself, you shouldn't check if its size is
definite or indefinite against the containing block (the parent), but
rather against the overridden containing block (the grid area).

Modified hasDefiniteLogicalWidth/Height() in order to take into account
the overrideContainingBlockContentLogicalWidth/Height() values.
Otherwise we were considering as definite situations in which it was not
possible to determine the size of the grid.

BUG=457849
TEST=fast/css-grid-layout/percent-intrinsic-track-breadth.html

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

[Manuel Rego, 2015-02-16 18:05:46]

rego@igalia.com changed reviewers:
+ eae@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Julien - ping for review, 2015-02-18 22:13:16]

https://codereview.chromium.org/914993003/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/percent-intrinsic-track-breadth-crash.html
(right):

https://codereview.chromium.org/914993003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/percent-intrinsic-track-breadth-crash.html:28:
<div class="grid percentage">
I assume we were also not resolving this correctly so how about a
check-layout.js test.

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

https://codereview.chromium.org/914993003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4179: bool
RenderBox::logicalWidthIsResolvableFromBlock(const RenderBlock* containingBlock)
This function looks like it could have static linkage.

https://codereview.chromium.org/914993003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4184: return
cb->overrideContainingBlockContentLogicalWidth() != -1;
I am concerned about this logic needing the bits in hasDefiniteLogicalWidth
below. This means that we're bleeding some details around.

Passing the descendant into logicalWidthIsResolvableFromBlock would remove this
limitation, but you're not guaranteed to manipulate a block anymore (but at
least a RenderBox). I think that's a fine tradeoff as it would make the function
self-contained.

[Manuel Rego, 2015-02-20 15:28:26]

Patchset #2 (id:20001) has been deleted

[Manuel Rego, 2015-02-25 16:58:00]

Thanks for the review. New version uploaded.

https://codereview.chromium.org/914993003/diff/1/LayoutTests/fast/css-grid-la...
File LayoutTests/fast/css-grid-layout/percent-intrinsic-track-breadth-crash.html
(right):

https://codereview.chromium.org/914993003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/percent-intrinsic-track-breadth-crash.html:28:
<div class="grid percentage">
On 2015/02/18 22:13:16, Julien Chaffraix - PST wrote:
> I assume we were also not resolving this correctly so how about a
> check-layout.js test.

Yeah, I've changed to a check-layout test and realized that width
was not working as expected yet.

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

https://codereview.chromium.org/914993003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4179: bool
RenderBox::logicalWidthIsResolvableFromBlock(const RenderBlock* containingBlock)
On 2015/02/18 22:13:16, Julien Chaffraix - PST wrote:
> This function looks like it could have static linkage.

This is marked as static in LayoutBox.h like
percentageLogicalHeightIsResolvableFromBlock().

https://codereview.chromium.org/914993003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4184: return
cb->overrideContainingBlockContentLogicalWidth() != -1;
On 2015/02/18 22:13:16, Julien Chaffraix - PST wrote:
> I am concerned about this logic needing the bits in hasDefiniteLogicalWidth
> below. This means that we're bleeding some details around.
> 
> Passing the descendant into logicalWidthIsResolvableFromBlock would remove
this
> limitation, but you're not guaranteed to manipulate a block anymore (but at
> least a RenderBox). I think that's a fine tradeoff as it would make the
function
> self-contained.

Done.

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

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
Source/core/layout/LayoutGrid.cpp:645: 
In addition to previous changes I needed to add this here
(similar to what we have in logicalHeightForChild()).

[Julien - ping for review, 2015-02-27 02:07:04]

lgtm

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
Source/core/layout/LayoutBox.h:458: static bool
logicalWidthIsResolvableFromBlock(const LayoutBox* containingBlock);
Let's make this a reference if we're touching the signature.

Also there is no point in exposing this function, it could have static linkage.

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

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
Source/core/layout/LayoutGrid.cpp:644:
child.setOverrideContainingBlockContentLogicalWidth(-1);
Our need to change the override constantly means that we won't be able to skip
any of those forced relayouts. It's not a comment on your change specifically
but on something that's deeper rooted in our code.

[Manuel Rego, 2015-02-27 08:48:50]

Thanks for the review!

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
Source/core/layout/LayoutBox.h:458: static bool
logicalWidthIsResolvableFromBlock(const LayoutBox* containingBlock);
On 2015/02/27 02:07:03, Julien Chaffraix - PST wrote:
> Let's make this a reference if we're touching the signature.
> 
> Also there is no point in exposing this function, it could have static
linkage.

Done.

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

https://codereview.chromium.org/914993003/diff/40001/Source/core/layout/Layou...
Source/core/layout/LayoutGrid.cpp:644:
child.setOverrideContainingBlockContentLogicalWidth(-1);
On 2015/02/27 02:07:03, Julien Chaffraix - PST wrote:
> Our need to change the override constantly means that we won't be able to skip
> any of those forced relayouts. It's not a comment on your change specifically
> but on something that's deeper rooted in our code.

I agree. We should try to find a best alternative at some point.

[Manuel Rego, 2015-02-27 08:48:56]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2015-02-27 08:48:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/914993003/#ps60001 (title:
"Patch for landing")

[commit-bot: I haz the power, 2015-02-27 08:49:45]

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

[commit-bot: I haz the power, 2015-02-27 08:54:15]

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

[commit-bot: I haz the power, 2015-02-27 08:54:16]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)

[Manuel Rego, 2015-02-27 08:56:56]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2015-02-27 08:56:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/914993003/#ps80001 (title:
"Rebased patch")

[commit-bot: I haz the power, 2015-02-27 08:58:02]

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

[Manuel Rego, 2015-02-27 09:05:01]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2015-02-27 09:05:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/914993003/#ps100001
(title: "Rename isRenderView() calls to isLayoutView()")

[commit-bot: I haz the power, 2015-02-27 09:05:06]

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

[commit-bot: I haz the power, 2015-02-27 10:21:14]

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

[commit-bot: I haz the power, 2015-02-27 10:21:15]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Manuel Rego, 2015-02-27 11:02:51]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2015-02-27 11:03:18]

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

[commit-bot: I haz the power, 2015-02-27 11:46:51]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190988