RenderBlock::isSelfCollapsingBlock() should early exit on a relayout boundary

RenderBlock::isSelfCollapsingBlock() should early exit on a relayout boundary

RenderBox::layoutOverflowRectForPropagation() needs to know whether the current
block is self-collapsing to pad layout overflow with after-margin. This can
trigger an assertion failure that it expects clean layout. This breaks the
contract of relayout boundary that its internal layout shouldn't matter to the
parent.

This CL simply reorders the assertion so we can give a quick answer if the box
is a relayout boundary before we assume clean layout.

This CL fixes an existing test failure cause by another CL.
CL affected by this bug: https://codereview.chromium.org/203463007

TEST=fast/overflow/overflow-dirty-relayout-boundary-no-crash.html
BUG=365507
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172577

[ojan, 2014-04-24 17:22:08]

Is there a layout test for this failure? If not, I think it's worth writing one.
We don't want to depend on ChromeOS tests for testing blink layout behavior.

This patch is definitely fine. It makes sense to have the assert actually be
before the code that depends on layout having happened. It's not clear to me
that this fixes 100% of the potentially problematic asserts though. Does the
problematic case only happen when the element has overflow clip?

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

https://codereview.chromium.org/255433003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1191: // Being a relayout boundary implies
a block formatting context, and also
This comment is a little confusing. It talks about relayout boundaries, but the
code is only checking block formatting context.

[ojan, 2014-04-24 17:27:36]

On 2014/04/24 17:22:08, ojan wrote:
> Is there a layout test for this failure? If not, I think it's worth writing
one.
> We don't want to depend on ChromeOS tests for testing blink layout behavior.

To be clear, I understand that the test wouldn't fail at tip of tree, but that's
OK. It can give us confidence when you reland that patch that it doesn't regress
the test.

Also, can you include a link to your other patch in the change description? That
way, someone looking at this patch years from now will have the context to
understand this change. I realize it's in the bug, but that bug is hard to
follow since there are a bunch of disparate issues there.

[trchen, 2014-04-24 20:25:23]

On 2014/04/24 17:27:36, ojan wrote:
> On 2014/04/24 17:22:08, ojan wrote:
> > Is there a layout test for this failure? If not, I think it's worth writing
> one.
> > We don't want to depend on ChromeOS tests for testing blink layout behavior.
> 
> To be clear, I understand that the test wouldn't fail at tip of tree, but
that's
> OK. It can give us confidence when you reland that patch that it doesn't
regress
> the test.

Good plan! Producing a minimal test case now.

> Also, can you include a link to your other patch in the change description?
That
> way, someone looking at this patch years from now will have the context to
> understand this change. I realize it's in the bug, but that bug is hard to
> follow since there are a bunch of disparate issues there.

Done.

[trchen, 2014-04-25 01:16:50]

test uploaded

[ojan, 2014-04-25 02:30:18]

lgtm

Clarified the root cause of the bug on chat.

Ojan Vafai: so, the overflow:hidden is needed to hit the assert?
Tien-Ren Chen: yes, we need overflow clip to establish relayout boundary
Ojan Vafai: i still don't fully understand the root cause of the bug...what's
the relationship between being a relayout boundary and causing the bug?
Tien-Ren Chen: being the relayout boundary means that we can have
box->needsLayout() but not box->container->needsLayout()
Tien-Ren Chen: when we recalc overflow on box->container, the container will
request layoutOverflowForPropagation from the box.

[commit-bot: I haz the power, 2014-04-25 02:30:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/255433003/20001

[commit-bot: I haz the power, 2014-04-25 03:40:55]

Change committed as 172577