[css-grid] Fix indefinite height detection

[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301, 617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html

Committed: https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769
Cr-Commit-Position: refs/heads/master@{#415478}

[Manuel Rego, 2016-07-15 11:36:08]

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

[Manuel Rego, 2016-07-15 11:36:09]

This is just fixing the initial issue reported on bug #624301.

On a follow-up patch I'll change the rest of the places on Grid Layout
code that check if the height is definite or indefinite.

[eae, 2016-07-15 17:41:04]

OK, LGTM

[cbiesinger, 2016-07-15 19:34:58]

Should hasAutoHeightOrContainingBlockWithAutoHeight use this new function now?

[Manuel Rego, 2016-07-18 06:04:04]

On 2016/07/15 19:34:58, cbiesinger wrote:
> Should hasAutoHeightOrContainingBlockWithAutoHeight use this new function now?

That would be ideal, not only hasAutoHeightOrContainingBlockWithAutoHeight()
but hopefully LayoutBox::computeReplacedLogicalHeightUsing() too.

But in this case, I'd prefer to go step by step. Have an initial patch to
determine
indefinite heights first, and let's see if we can reuse it in more places.
I can add some TODOs on those methods though, if you think it's convenient.

[jfernandez, 2016-07-18 06:13:24]

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

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2699: LayoutUnit
LayoutBox::availableLogicalHeightForPercentageComputation(const LayoutBlock* cb,
bool skippedAutoHeightContainingBlock, bool scrollsOverflowY)
I find it weird how we use the skippedAutoHeightContainingBlock and
scrollsOverflowY arguments. I know these come from the previous
computedPercentageLogicalHeight function that was refactored in this patch, but
I'd prefer to avoid this way of passing info between functions. I think the
arguments may become meaningless when this new function is called in a different
context.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: const LayoutTableCell*
cell = toLayoutTableCell(cb);
Why do you need the cell to be const here ?

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2788: availableHeight =
overrideContainingBlockContentLogicalHeight();
I guess this is new logic. I'd rather do it in a different patch, once the
refactoring has landed and has been proven there are not regressions.

[svillar, 2016-07-18 08:43:30]

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

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1877: return
LayoutBox::availableLogicalHeightForPercentageComputation(this, false,
scrollsOverflowY()) != LayoutUnit(-1);
This is one of the reasons why bools are normally not used for APIs, this false
here is meaningless, it is impossible to know what it means unless I check the
signature of the method.

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

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2699: LayoutUnit
LayoutBox::availableLogicalHeightForPercentageComputation(const LayoutBlock* cb,
bool skippedAutoHeightContainingBlock, bool scrollsOverflowY)
On 2016/07/18 06:13:24, jfernandez wrote:
> I find it weird how we use the skippedAutoHeightContainingBlock and
> scrollsOverflowY arguments. I know these come from the previous
> computedPercentageLogicalHeight function that was refactored in this patch,
but
> I'd prefer to avoid this way of passing info between functions. I think the
> arguments may become meaningless when this new function is called in a
different
> context.

Agree with jfernandez, this is related to the fact of using bools here as well.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2760: availableHeight =
toLayoutView(cb)->viewLogicalHeightForPercentages();
why this?

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2788: availableHeight =
overrideContainingBlockContentLogicalHeight();
On 2016/07/18 06:13:24, jfernandez wrote:
> I guess this is new logic. I'd rather do it in a different patch, once the
> refactoring has landed and has been proven there are not regressions.

I think that's a good idea. First the refactoring, then the fix.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:744: static LayoutUnit
availableLogicalHeightForPercentageComputation(const LayoutBlock*, bool
skippedAutoHeightContainingBlock, bool scrollsOverflowY);
If the first parameter is never null then it should be a reference instead

Same comment about bools.

[cbiesinger, 2016-07-18 20:53:14]

On 2016/07/18 06:04:04, Manuel Rego wrote:
> On 2016/07/15 19:34:58, cbiesinger wrote:
> > Should hasAutoHeightOrContainingBlockWithAutoHeight use this new function
now?
> 
> That would be ideal, not only hasAutoHeightOrContainingBlockWithAutoHeight()
> but hopefully LayoutBox::computeReplacedLogicalHeightUsing() too.
> 
> But in this case, I'd prefer to go step by step. Have an initial patch to
> determine
> indefinite heights first, and let's see if we can reuse it in more places.
> I can add some TODOs on those methods though, if you think it's convenient.

Would be great if you could add those TODOs, thanks!

[Manuel Rego, 2016-07-19 13:12:32]

Thanks for the reviews.
I've redo the patch and I think the result is much better.
I've also added the suggested TODOs.

PTAL.

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

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2699: LayoutUnit
LayoutBox::availableLogicalHeightForPercentageComputation(const LayoutBlock* cb,
bool skippedAutoHeightContainingBlock, bool scrollsOverflowY)
On 2016/07/18 08:43:30, svillar wrote:
> On 2016/07/18 06:13:24, jfernandez wrote:
> > I find it weird how we use the skippedAutoHeightContainingBlock and
> > scrollsOverflowY arguments. I know these come from the previous
> > computedPercentageLogicalHeight function that was refactored in this patch,
> but
> > I'd prefer to avoid this way of passing info between functions. I think the
> > arguments may become meaningless when this new function is called in a
> different
> > context.
> 
> Agree with jfernandez, this is related to the fact of using bools here as
well.

Yeah, I've redo the patch and have removed the bool arguments.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: const LayoutTableCell*
cell = toLayoutTableCell(cb);
On 2016/07/18 06:13:24, jfernandez wrote:
> Why do you need the cell to be const here ? 

This was not needed, just slipped into the patch.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2760: availableHeight =
toLayoutView(cb)->viewLogicalHeightForPercentages();
On 2016/07/18 08:43:30, svillar wrote:
> why this?

This was because it's a static method so I couldn't call view().

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2788: availableHeight =
overrideContainingBlockContentLogicalHeight();
On 2016/07/18 08:43:30, svillar wrote:
> On 2016/07/18 06:13:24, jfernandez wrote:
> > I guess this is new logic. I'd rather do it in a different patch, once the
> > refactoring has landed and has been proven there are not regressions.
> 
> I think that's a good idea. First the refactoring, then the fix.

This is not exactly new logic. There were some similar logic already.

The thing is that current code was wrong in some situations.
E.g. if a grid is a streched grid item, it's height should be considered
definite, however current code fails detecting that.
See: http://jsbin.com/dusoxe/edit?html,css,output

The problem with doing the refactoring first, is that I'd need to do
some weird and wrong stuff, to keep the previous bug and I believe
it's not worth it.

Also the other piece of new logic in this patch is using the new
hasDefiniteLogicalHeight() which is fixing more tests.

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2154593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:744: static LayoutUnit
availableLogicalHeightForPercentageComputation(const LayoutBlock*, bool
skippedAutoHeightContainingBlock, bool scrollsOverflowY);
On 2016/07/18 08:43:30, svillar wrote:
> If the first parameter is never null then it should be a reference instead

Done.

[svillar, 2016-07-20 08:24:01]

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1877: LayoutUnit
availableHeight = LayoutUnit(-1);
LayoutUnit availableHeight(-1);

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2714: LayoutUnit
availableHeight;
My main doubt is, what's the criteria you have used to move the code to the new
function? I'm having a hard time trying to know that.

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2717: } else if
(isGridItem() && hasOverrideContainingBlockLogicalHeight()) {
Why just for grid items? What about !isOutOfFlowPositionedWithSpecifiedHeight ?

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2752: bool
includeBorderPadding = isTable() || (cb->isTableCell() &&
(cb->styleRef().logicalHeight().isFixed() || (!skippedAutoHeightContainingBlock
&& cb->hasOverrideLogicalContentHeight())));
I think this is not correct, for example if cb->isTableCell() and
cb->styleRef().logicalHeight().isFixed() are both true we set this to true, but
in the previous code that was only done whenever 
!skippedAutoHeightContainingBlock.

Apart from that such a complex condition in just 1 line seems a bit too much to
my taste :).

[Manuel Rego, 2016-07-21 09:46:02]

Thanks for the review, some comments inline.

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1877: LayoutUnit
availableHeight = LayoutUnit(-1);
On 2016/07/20 08:24:00, svillar wrote:
> LayoutUnit availableHeight(-1);

Acknowledged.

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2714: LayoutUnit
availableHeight;
On 2016/07/20 08:24:00, svillar wrote:
> My main doubt is, what's the criteria you have used to move the code to the
new
> function? I'm having a hard time trying to know that.

Basically the idea is having a function that I can call from the grid container,
to know if it has a definite or indefinite height.
So I tried to move everything I could to an independent funciton,
so calls to current object like isHorizontalWritingMode() or
hasOverrideContainingBlockLogicalHeight() couldn't be moved.
Also I wanted to avoid to pass weird bool params to that method
(like in previous version).

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2717: } else if
(isGridItem() && hasOverrideContainingBlockLogicalHeight()) {
On 2016/07/20 08:24:00, svillar wrote:
> Why just for grid items? What about !isOutOfFlowPositionedWithSpecifiedHeight
?

True the isGridItem() is not needed.

About "!isOutOfFlowPositionedWithSpecifiedHeight" that was wrong.
It was needed for some cases as we were using it to know if the height is
indefinite.
But it was causing bugs too: http://jsbin.com/hisova/1/edit?html,css,output

And the thing is that it's not possible in the current code to just remove
!isOutOfFlowPositionedWithSpecifiedHeight.
As it'll be making other good tests to fail.
The main issue is how bad we're detecting that the height is indefinite,
using the firstInFlowChildBox()->percentageLogicalHeightIsResolvable().

https://codereview.chromium.org/2154593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2752: bool
includeBorderPadding = isTable() || (cb->isTableCell() &&
(cb->styleRef().logicalHeight().isFixed() || (!skippedAutoHeightContainingBlock
&& cb->hasOverrideLogicalContentHeight())));
On 2016/07/20 08:24:00, svillar wrote:
> I think this is not correct, for example if cb->isTableCell() and
> cb->styleRef().logicalHeight().isFixed() are both true we set this to true,
but
> in the previous code that was only done whenever 
> !skippedAutoHeightContainingBlock.

Are you sure? In the previous code we've:
  else if (cbstyle.logicalHeight().isFixed()) {
    ...
    if (cb->isTableCell())
      includeBorderPadding = true;
  }

So that was enough to make it true.

> Apart from that such a complex condition in just 1 line seems a bit too much
to
> my taste :).

Yeah, I can split it in several lines.

[cbiesinger, 2016-07-21 20:57:28]

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.h:229: static LayoutUnit
availableLogicalHeightForPercentageComputation(const LayoutBlock&);
Why make this a static function and pass in a block, when you could just make
this a member function on that block?

[jfernandez, 2016-07-22 11:26:37]

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-rows.html
(right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-rows.html:42:
<p>This test checks that percentage rows behave as "auto" if the height of the
grid container is indefinite.</p>
None of the test cases defined in this file fail without the patch. Does that
make sense ?

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size.html
(right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size.html:29:
<p>Test passes if you see a 100x100 green box.</p>
This test passes even without applying the patch. Do you think is still useful ?

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1875: LayoutUnit
LayoutBlock::availableLogicalHeightForPercentageComputation(const LayoutBlock&
block)
Shouldn't be const ?

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1898: // We need to recur
and compute the percentage height for our containing block.
I don't understand this comment. Could you please elaborate ?

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2701: LayoutBlock* cb =
containingBlock();
how all this logic behaves under an orthogonal flows scenario ? I don't mean we
should fix those in this patch, if necessary, but perhaps some TODO comment
might be useful.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (left):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1186: inline LayoutBox*
LayoutBox::firstInFlowChildBox() const
Why this removal ?

[Manuel Rego, 2016-07-22 13:05:37]

Thanks for the reviews, more work is needed as you've found
some strange issues.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-rows.html
(right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-rows.html:42:
<p>This test checks that percentage rows behave as "auto" if the height of the
grid container is indefinite.</p>
On 2016/07/22 11:26:37, jfernandez wrote:
> None of the test cases defined in this file fail without the patch. Does that
> make sense ? 

Wow, this has shown a very weird issue we've.

There're actually cases in this test that fail in the current code,
but they fail if they're isolated, if there're so many they don't fail.
And there's a reason for that, right now we're using
percentageLogicalHeightIsResolvable() on the first child.

So the test attached to bug #624301, the first item is in a row
with a fixed height. The problem is that if we call
percentageLogicalHeightIsResolvable() and we haven't done
layout of the item yet, so we didn't set
overrideContainingBlockLogicalHeight yet, we identify that
the height of the grid container is indefinite. However,
if we call percentageLogicalHeightIsResolvable() after
the layout of the item, we get that
overrideContainingBlockLogicalHeight is set, and we
identify the height of the grid as definite.

As you know a simple page without scroll does 2 layouts,
so on the first one we identify it as indefinite and in the
second one as definite.
In the attached example if you avoid 1 layout with:
  html { overflow: scroll; }
You see that the result is the expected one, as we identify
the grid container as indefinite.
Which is the right result in this particular example.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size.html
(right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size.html:29:
<p>Test passes if you see a 100x100 green box.</p>
On 2016/07/22 11:26:37, jfernandez wrote:
> This test passes even without applying the patch. Do you think is still useful
?

Mmmm, here it's 100x200 instead of 100x100. But again it might be the same
issue than explained before, as the grid area overrides are not set before the
first layout.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1875: LayoutUnit
LayoutBlock::availableLogicalHeightForPercentageComputation(const LayoutBlock&
block)
On 2016/07/22 11:26:37, jfernandez wrote:
> Shouldn't be const ?

It's static so you cannot modify anything,
but once I convert it in a member function I'll mark it as const sure.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1898: // We need to recur
and compute the percentage height for our containing block.
On 2016/07/22 11:26:37, jfernandez wrote:
> I don't understand this comment. Could you please elaborate ? 

This comment was already in computePercentageLogicalHeight().
Bur probably it doesn't make sense to move it  here, the thing is
when you're computing the percentage height on a element,
if your containing block has a percentage, you need to compute
that percentage first, before being able to resolve yours.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.h:229: static LayoutUnit
availableLogicalHeightForPercentageComputation(const LayoutBlock&);
On 2016/07/21 20:57:28, cbiesinger wrote:
> Why make this a static function and pass in a block, when you could just make
> this a member function on that block?

Yeah good idea it can be a member const function,
like hasDefiniteLogicalHeight().

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2701: LayoutBlock* cb =
containingBlock();
On 2016/07/22 11:26:37, jfernandez wrote:
> how all this logic behaves under an orthogonal flows scenario ? I don't mean
we
> should fix those in this patch, if necessary, but perhaps some TODO comment
> might be useful.

It should be working fine as there's an if below:
  if (isHorizontalWritingMode() != cb->isHorizontalWritingMode()) {
    availableHeight =
containingBlockChild->containingBlockLogicalWidthForContent();
  }

And I'm not changing anything regarding that.

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (left):

https://codereview.chromium.org/2154593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1186: inline LayoutBox*
LayoutBox::firstInFlowChildBox() const
On 2016/07/22 11:26:37, jfernandez wrote:
> Why this removal ? 

Because it was introduced to be able to call
percentageLogicalHeightIsResolvable() on the first in-flow child.
As that was wrong, we don't need it anymore, and there're no more
users of this method.

[Manuel Rego, 2016-08-25 09:27:44]

Description was changed from

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBox::availableLogicalHeightForPercentageComputation().
That way we can call availableLogicalHeightForPercentageComputation()
directly on an element to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html
==========

[Manuel Rego, 2016-08-25 09:29:38]

I believe the last version addresses the issues detected in the reviews.

Please could you take a new look? Thanks. :-)

[eae, 2016-08-25 18:40:45]

Thank you, LGTM

[cbiesinger, 2016-08-25 21:50:23]

https://codereview.chromium.org/2154593003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2154593003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:229: LayoutUnit
availableLogicalHeightForPercentageComputation() const;
Upon re-reading this patch, I really think this needs a comment that you need to
call this function on the right containing block --
computePercentageLogicalHeight has the function to find the containing block,
but you did not move it to the new function.

https://codereview.chromium.org/2154593003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:230: bool
hasDefiniteLogicalHeight() const;
..and that previous point makes me think this function might not be correct?

[Manuel Rego, 2016-08-29 11:08:54]

Uploaded new version.

https://codereview.chromium.org/2154593003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2154593003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:229: LayoutUnit
availableLogicalHeightForPercentageComputation() const;
On 2016/08/25 21:50:23, cbiesinger wrote:
> Upon re-reading this patch, I really think this needs a comment that you need
to
> call this function on the right containing block --
> computePercentageLogicalHeight has the function to find the containing block,
> but you did not move it to the new function.

We want to be able to call this function directly on any block,
that's why it doesn't have the loop to find the right containing block.
It's true that we've an issue now that we can call this from anywhere
(due to hasDefiniteLogicalHeight()).

I've added a check using skipContainingBlockForPercentHeightCalculation(),
it seems that if this is true we can consider that the element has an indefinite
height.
Basically they're mostly anonymous blocks that are skipped
during percentage height calculations, so I guess it's fine to consider
they have an indefinite height for the purpose of hasDefiniteLogicalHeight().

[cbiesinger, 2016-08-29 21:23:09]

lgtm. I think I didn't fully understand this code previously. This seems good
now. Thanks!

[jfernandez, 2016-08-30 10:32:50]

lgtm

[Manuel Rego, 2016-08-30 21:10:10]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-08-30 21:10:11]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2154593003/#ps160001
(title: "Check skipContainingBlockForPercentHeightCalculation()")

[commit-bot: I haz the power, 2016-08-30 21:10:53]

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-08-30 23:00:53]

Description was changed from

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html
==========

[commit-bot: I haz the power, 2016-08-30 23:00:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-30 23:02:13]

Description was changed from

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG=624301,617876
TEST=fast/css-grid-layout/grid-container-percentage-rows.html

Committed: https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769
Cr-Commit-Position: refs/heads/master@{#415478}
==========

[commit-bot: I haz the power, 2016-08-30 23:02:19]

Patchset 9 (id:??) landed as
https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769
Cr-Commit-Position: refs/heads/master@{#415478}