Added another guard to the NodeRenderingContext::nextSibling() fast path and its twin.

Added another guard to the NodeRenderingContext::nextSibling() fast path and its twin.

Currently, NRC::nextSibling() checks Node::needsShadowTreeWalker() before taking a shortcut.
The flag is on when the node or its parent is a host, a shadowroot or an insertion point.
This check works for traversals from children to parents, there is a hole where
a sibling of the node is an insertion point. In such a case, we need to skip the insertion
point for the traversal and we cannot take the shortcut.

This changes adds guards for such cases and lets the traversal take its slow path.

BUG=229245
TEST=content-element-crash.html

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

[Hajime Morrita, 2013-05-10 08:31:25]

Hi Dimitri, could you take a look? This fixes an assertion failure which is hit
on pica.

[dglazkov, 2013-05-10 16:44:51]

LGTM.

[commit-bot: I haz the power, 2013-05-10 23:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/14731014/1

[commit-bot: I haz the power, 2013-05-11 00:41:15]

Change committed as 150157

[esprehn, 2013-05-12 08:10:53]

This patch adds a virtual call in the "fast path" which kind of defeats the
whole purpose of the fast path and all the bitfield fidling for
needsShadowTreeWalker. Can we just remove the "fast path" entirely? It seems
like a source of bugs, and getting correctness from it and keeping it fast seems
at odds. I'm also unconvinced that the perf difference is really that much to be
important with keeping this complexity.

Anyone mind if I post a patch to remove this stuff?

[Hajime Morrita, 2013-05-12 14:38:01]

isInsertionPoint() isn't a virtual call.
It is an inline function which has a fast path seeing another bitfield.
It has a minor slow path with a virtual call when the check fails though.

[Hajime Morrita, 2013-05-13 01:03:17]

By the way, I don't mind to get rid of the whole needsShadowTreeWalker and
possibly NodeRenderingTraversal thing.
As you know, it is introduced to speedup on of Dromaeo test, and 
the result is observable only on Apple port.
So I don't think killing it really suffers.

[dominicc (has gone to gerrit), 2013-05-13 01:29:58]

On 2013/05/13 01:03:17, morrita1 wrote:
> By the way, I don't mind to get rid of the whole needsShadowTreeWalker and
> possibly NodeRenderingTraversal thing.
> As you know, it is introduced to speedup on of Dromaeo test, and 
> the result is observable only on Apple port.
> So I don't think killing it really suffers.

If there's negligible performance impact let's KILL IT. It had a good life. It
outlived its purpose. Removing it cleanly is the humane solution.