Node.contains should return true for nodes in Shadow DOM.

Node.contains should return true for nodes in Shadow DOM.

This is discussed on this W3 bug:
<https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141>;

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

[dominicc (has gone to gerrit), 2013-07-30 00:26:21]

PTAL

[esprehn, 2013-07-30 00:29:13]

I don't think this is the right fix.

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

https://codereview.chromium.org/21123005/diff/1/Source/core/dom/Node.idl#newc...
Source/core/dom/Node.idl:94: [ImplementedAs=containsIncludingShadowDOM] boolean
contains([Default=Undefined] optional Node other);
This is not right, containsIncludingShadowDOM walks up the tree every time,
Node::contains has a bunch of optimizations in it.

[Hajime Morrita, 2013-07-30 00:30:58]

LGTM

[Hajime Morrita, 2013-07-30 00:31:30]

Well, not LGTM from as esprehn's comment.

[esprehn, 2013-07-30 01:18:11]

Discussed this through chat, we'll add a few special cases to
containsIncludingShadowDOM so doing document.contains(foo) doesn't walk up the
tree every time (ex. if !inDocument() or in another document, or !hasChildren
etc.) and use bindingContains so it's clear this is bindings exposed behavior
which sounds like a good plan.

[esprehn, 2013-07-30 03:36:26]

lgtm, I think the boolean logic would be clearer if you put the ! in front of
each thing and grouped the OR part.

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

https://codereview.chromium.org/21123005/diff/9001/Source/core/dom/Node.cpp#n...
Source/core/dom/Node.cpp:997: if (!(isContainerNode() &&
toContainerNode(this)->hasChildNodes() || isElementNode() &&
toElement(this)->shadow()))
This is a bit hard to understand, could you propagate the ! through it and group
the || with parens?

https://codereview.chromium.org/21123005/diff/9001/Source/core/dom/Node.cpp#n...
Source/core/dom/Node.cpp:1000: for (; node; node = node->shadowHost()) {
Nice!

[commit-bot: I haz the power, 2013-07-30 07:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/21123005/13001

[commit-bot: I haz the power, 2013-07-30 10:34:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/21123005/13001

[commit-bot: I haz the power, 2013-07-30 13:20:08]

Change committed as 155161