Whitespace only text nodes need to be reattached when their siblings are reattached

Whitespace only text nodes need to be reattached when their siblings are reattached

When reversing the style loop, we changed the way whitespace only text nodes are
handled by deferring their processing until later in an attempt to have consistent
whitespace text renderers in our layout test expectations. I also disabled a
whitespace reattach step because this change in behavior masked any bugs from
showing up by doing so in our LayoutTests.

Re-adding the whitespace reattachment now that we have a test that breaks without
it. It also changes some random test expectations by adding whitespace renderers.

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

[leviw_travelin_and_unemployed, 2013-11-04 20:28:10]

The optimization to remove the whitespace node from the WhitespaceChildList when
it's being reattached is probably premature. I'm happy to take it out for
simplicity if preferred.

[leviw_travelin_and_unemployed, 2013-11-04 21:11:30]

Damn, there's a legitimate repaint failure here... looking into it.

[esprehn, 2013-11-04 21:12:24]

On 2013/11/04 20:28:10, Levi wrote:
> The optimization to remove the whitespace node from the WhitespaceChildList
when
> it's being reattached is probably premature. I'm happy to take it out for
> simplicity if preferred.

Yeah lets remove that for now. Does the WhitespaceChildList really serve any
purpose after you add this code back though?

[leviw_travelin_and_unemployed, 2013-11-04 21:20:31]

On 2013/11/04 21:12:24, esprehn wrote:
> On 2013/11/04 20:28:10, Levi wrote:
> > The optimization to remove the whitespace node from the WhitespaceChildList
> when
> > it's being reattached is probably premature. I'm happy to take it out for
> > simplicity if preferred.
> 
> Yeah lets remove that for now. 
Okay.

> Does the WhitespaceChildList really serve any
> purpose after you add this code back though?

Sadly, yes. Substantially more flake is introduced without the
WhitespaceChildList. It's a confluence of issues that makes it flaky :(

[leviw_travelin_and_unemployed, 2013-11-04 22:37:06]

On 2013/11/04 21:20:31, Levi wrote:
> On 2013/11/04 21:12:24, esprehn wrote:
> > On 2013/11/04 20:28:10, Levi wrote:
> > > The optimization to remove the whitespace node from the
WhitespaceChildList
> > when
> > > it's being reattached is probably premature. I'm happy to take it out for
> > > simplicity if preferred.
> > 
> > Yeah lets remove that for now. 
> Okay.
> 
> > Does the WhitespaceChildList really serve any
> > purpose after you add this code back though?
> 
> Sadly, yes. Substantially more flake is introduced without the
> WhitespaceChildList. It's a confluence of issues that makes it flaky :(

Blegh... the one fail case is ugly, but it's not exactly a regression, as the
test was written without this whitespace reattachment code, without which it
would have failed this way previously as well.

The test creates a div with a bunch of layered content inside it. With this
change, it also gets an anonymous RenderBlock with a non-rendered newline. This
causes it to fail a check in RenderLayerScrollableArea that checks to see if the
layer hasVisibleNonLayerContent, which sadly it now does. Our whitespace logic
sucks :(

[leviw_travelin_and_unemployed, 2013-11-04 23:32:04]

On 2013/11/04 22:37:06, Levi wrote:
> On 2013/11/04 21:20:31, Levi wrote:
> > On 2013/11/04 21:12:24, esprehn wrote:
> > > On 2013/11/04 20:28:10, Levi wrote:
> > > > The optimization to remove the whitespace node from the
> WhitespaceChildList
> > > when
> > > > it's being reattached is probably premature. I'm happy to take it out
for
> > > > simplicity if preferred.
> > > 
> > > Yeah lets remove that for now. 
> > Okay.
> > 
> > > Does the WhitespaceChildList really serve any
> > > purpose after you add this code back though?
> > 
> > Sadly, yes. Substantially more flake is introduced without the
> > WhitespaceChildList. It's a confluence of issues that makes it flaky :(
> 
> Blegh... the one fail case is ugly, but it's not exactly a regression, as the
> test was written without this whitespace reattachment code, without which it
> would have failed this way previously as well.
> 
> The test creates a div with a bunch of layered content inside it. With this
> change, it also gets an anonymous RenderBlock with a non-rendered newline.
This
> causes it to fail a check in RenderLayerScrollableArea that checks to see if
the
> layer hasVisibleNonLayerContent, which sadly it now does. Our whitespace logic
> sucks :(

Found the right solution. Upload forthcoming... Happy death to
WhitespaceChildList! :D

[leviw_travelin_and_unemployed, 2013-11-04 23:43:09]

PTAL.

https://codereview.chromium.org/58373002/diff/100001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/58373002/diff/100001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:1010: sibling->reattach();
The previous version wouldn't reattach whitespace nodes with renderers. Getting
this right fixed the flake.

[esprehn, 2013-11-05 00:33:49]

Woah, like a boss. LGTM

[commit-bot: I haz the power, 2013-11-05 21:37:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/58373002/430001

[commit-bot: I haz the power, 2013-11-05 22:56:27]

Change committed as 161389