Reattach whitespace siblings only when needed

Reattach whitespace siblings only when needed

When an element's renderer is being reattached, previously we reattach
whitespace siblings, causing full paint invalidation of the parent.

If the whitespace sibling needed renderer and still needs renderer,
don't reattach.

- Renamed Node::reattachWhitespaceSiblings() to
  Node::reattachWhitespaceSlibingsIfNeeded();
- In the function, test if rendererIsNeeded changed before reattaching;
- Modified Text::textRendererIsNeeded() so that it can return correct
  value when the Text has already renderer.

BUG=428997
TEST=fast/repaint/absolute-display-block-to-none.html
TEST=fast/forms/select-listbox-focus-displaynone.html (Existing test
covering the change in Text.cpp).

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

[Xianzhu, 2014-11-03 21:53:52]

wangxianzhu@chromium.org changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Xianzhu, 2014-11-03 21:53:53]

This patch resolves half of the bug (absolute display:block to display:none).

Another patch is needed to resolve another half (absolute display:none to
display:block).

[esprehn, 2014-11-04 22:03:54]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2014-11-04 22:03:55]

This might be okay, but calling textRendererIsNeeded twice is going to be a perf
regression because of http://crbug.com/399816

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

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:961: RenderObject* parentRenderer =
NodeRenderingTraversal::parent(start)->renderer();
This function can probably be called in situations where you're out of
composition, you need to null check the result of parent().

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:962: ASSERT(parentRenderer && parentRenderer->style());
asserting ->style() on a RenderObject doesn't make sense.

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:966: bool rendererIsNeeded =
toText(sibling)->textRendererIsNeeded(*parentRenderer->style(),
*parentRenderer);
This is going to be a perf regression, calling textRendererIsNeeded is slow (and
sometimes n^2), and we're going to call it inside reattach() too.

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Text.cpp#new...
Source/core/dom/Text.cpp:277: while (first && (first == renderer() ||
first->isFloatingOrOutOfFlowPositioned()) && maxSiblingsToVisit--)
renderer() has a branch inside it, you shouldn't call it inside a loop like
this.

[Xianzhu, 2014-11-05 01:30:13]

PTAL.

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

https://codereview.chromium.org/684633006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:966: bool rendererIsNeeded =
toText(sibling)->textRendererIsNeeded(*parentRenderer->style(),
*parentRenderer);
On 2014/11/04 22:03:55, esprehn wrote:
> This is going to be a perf regression, calling textRendererIsNeeded is slow
(and
> sometimes n^2), and we're going to call it inside reattach() too.

To avoid the twice calling to textRendererIsNeeded, the new patch set is based
on https://codereview.chromium.org/699213003/.

Added Text::reattachIfNeeded().

[leviw_travelin_and_unemployed, 2014-11-05 19:23:15]

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:277: while (first && (first == renderer() ||
first->isFloatingOrOutOfFlowPositioned()) && maxSiblingsToVisit--)
You still want to store renderer() in a local variable instead of constantly
calling it in this loop. Otherwise I think this looks good.

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:314: bool hadRenderer = !!renderer();
You shouldn't need the !! when assigning into a bool.

[Xianzhu, 2014-11-05 20:15:33]

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:277: while (first && (first == renderer() ||
first->isFloatingOrOutOfFlowPositioned()) && maxSiblingsToVisit--)
On 2014/11/05 19:23:15, leviw wrote:
> You still want to store renderer() in a local variable instead of constantly
> calling it in this loop. Otherwise I think this looks good.

Moved first==renderer() out of the loop into the following 'if'. If the text has
already renderer and the first non-floating-non-out-of-flow renderer is itself,
return false so that we can detach it.

https://codereview.chromium.org/684633006/diff/20001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:314: bool hadRenderer = !!renderer();
On 2014/11/05 19:23:15, leviw wrote:
> You shouldn't need the !! when assigning into a bool.

Done.

https://codereview.chromium.org/684633006/diff/40001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/684633006/diff/40001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:279: if (!first || first == renderer() ||
NodeRenderingTraversal::nextSiblingRenderer(this) == first)
'!first' is not strictly necessary because first==renderer() can cover it, but
including it seems to improve readability.

[Xianzhu, 2014-11-12 18:58:20]

ping...

[Xianzhu, 2014-11-14 17:05:59]

On 2014/11/12 18:58:20, Xianzhu wrote:
> ping...

ping...

[leviw_travelin_and_unemployed, 2014-11-14 22:23:26]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-14 22:23:26]

LGTM!

[commit-bot: I haz the power, 2014-11-14 22:24:28]

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

[commit-bot: I haz the power, 2014-11-14 22:36:57]

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

[commit-bot: I haz the power, 2014-11-14 22:37:02]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[Xianzhu, 2014-11-16 01:52:47]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-16 01:53:48]

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

[commit-bot: I haz the power, 2014-11-16 02:02:58]

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

[commit-bot: I haz the power, 2014-11-16 02:02:59]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Xianzhu, 2014-11-16 02:21:18]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-16 02:21:54]

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

[commit-bot: I haz the power, 2014-11-16 04:37:44]

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

[commit-bot: I haz the power, 2014-11-16 04:37:46]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/3...)

[Xianzhu, 2014-11-17 01:00:30]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-17 01:00:47]

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

[commit-bot: I haz the power, 2014-11-17 03:13:59]

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

[commit-bot: I haz the power, 2014-11-17 03:14:00]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/3...)

[Xianzhu, 2014-11-17 04:04:23]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-17 04:04:59]

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

[commit-bot: I haz the power, 2014-11-17 06:12:59]

Committed patchset #5 (id:80001) as 185421