Skip PositionedMovementLayout when descendants depend on element's height

Skip PositionedMovementLayout when descendants depend on element's height

If we decide based on a style mutation that we only need to do positioned movement
layout we change our mind if it turns out that we have normal flow descendants in need of 
a layout too. One way of noticing this is if our width has changed. Another way
is when our height has changed and we have descendants that depend on it. I believe
the latter are percent height descendants only.

Note that positioned descendants always will always get a layout in positioned 
movement layout (due to ForcedLayoutAfterContainingBlockMoved)- so we don't need to worry about them here.

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

[rhogan, 2015-01-05 21:54:35]

robhogan@gmail.com changed reviewers:
+ jchaffraix@chromium.org

[Julien - ping for review, 2015-01-06 14:35:10]

lgtm

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
File
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-descendant-dependent-on-height-expected.html
(right):

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-descendant-dependent-on-height-expected.html:10:
overflow: hidden;
This doesn't seem needed for the test no?

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-descendant-dependent-on-height-expected.html:19:
height: auto;
height is auto by default.

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
File
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-positioned-descendant-dependent-on-height.html
(right):

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-positioned-descendant-dependent-on-height.html:42:
<div id="top">
#top is unused in both test cases.

https://codereview.chromium.org/802813003/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-height-changes-and-positioned-descendant-dependent-on-height.html:49:
Tests positioned movement layout when its height has changed and a descendant
depends on its height.
You forgot to put the visual condition for passing in both tests.

OR

Those tests looks like they could be check-layout.js test by testing the offset
top and height.

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

https://codereview.chromium.org/802813003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3883: bool
RenderBlock::tryLayoutDoingPositionedMovementOnly()
I can understand why only blocks use this function, however boxes can be
positioned (e.g. table row / row group) so it made sense for the function to be
on RenderBox. Combined with the history churn, I would argue to leave the
function on RenderBox.

https://codereview.chromium.org/802813003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3898: if (hasPercentHeightDescendants() &&
oldHeight != logicalHeight())
The percentage height descendant map is filled during full layout but I think it
works to use it here as we should do a full layout when it is dirtied.

https://codereview.chromium.org/802813003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3900: return true;
The past 3 lines can be combined into a single line, which I find more readable.
If needed you can add a comment that we only care about the logical height if we
have percentage height descendants.

return !hasPercentHeightDescendant() || oldHeight == logicalHeight();

[rhogan, 2015-01-06 21:43:46]

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

https://codereview.chromium.org/802813003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3883: bool
RenderBlock::tryLayoutDoingPositionedMovementOnly()
On 2015/01/06 at 14:35:09, Julien Chaffraix - CET wrote:
> I can understand why only blocks use this function, however boxes can be
positioned (e.g. table row / row group) so it made sense for the function to be
on RenderBox. Combined with the history churn, I would argue to leave the
function on RenderBox.

The other reason I moved it is that hasPercentageHeightDescendants() is
RenderBlock-only. I think the two together make it right to move. What do you
think?

[Julien - ping for review, 2015-01-07 09:41:39]

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

https://codereview.chromium.org/802813003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3883: bool
RenderBlock::tryLayoutDoingPositionedMovementOnly()
On 2015/01/06 21:43:45, rhogan wrote:
> On 2015/01/06 at 14:35:09, Julien Chaffraix - CET wrote:
> > I can understand why only blocks use this function, however boxes can be
> positioned (e.g. table row / row group) so it made sense for the function to
be
> on RenderBox. Combined with the history churn, I would argue to leave the
> function on RenderBox.
> 
> The other reason I moved it is that hasPercentageHeightDescendants() is
> RenderBlock-only. I think the two together make it right to move. What do you
> think?

I think we have no other option then as hasPercentageHeightDescendants() only
makes sense for RenderBlocks (it's only meaningful for containing blocks). What
that means is that positioned movement only is a block concept then, which we
are probably fine with.

[rhogan, 2015-01-07 13:10:30]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-01-07 13:11:27]

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

[commit-bot: I haz the power, 2015-01-07 13:14:48]

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