Add support for DocumentFragment.getElementById()

Add support for DocumentFragment.getElementById()

Add support for DocumentFragment.getElementById() as specified in the DOM
specification:
http://dom.spec.whatwg.org/#interface-nonelementparentnode

The patch moves the SVGSVGElement::getElementById() implementation up to
ContainerNode so that it can be shared with DocumentFragment.

Firefox 28 already supports it, IE11 does not.

R=arv@chromium.org, abarth@chromium.org
BUG=366927
TEST=fast/dom/DocumentFragment/get-element-by-id.html

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

[tkent, 2014-04-25 05:14:20]

Would you post intent-to-implement-and-ship email to blink-dev please?
This is just one function addition, but its impact looks not small to me.

[Inactive, 2014-04-25 13:33:39]

On 2014/04/25 05:14:20, tkent wrote:
> Would you post intent-to-implement-and-ship email to blink-dev please?
> This is just one function addition, but its impact looks not small to me.

Fair enough :)

Here it is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D5ZG_JARr68

[arv (Not doing code reviews), 2014-04-25 13:36:18]

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1167: if (inDocument()) {
I don't think this is required. It should be sufficient to get the treeScope.

[Inactive, 2014-04-25 14:41:19]

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1167: if (inDocument()) {
On 2014/04/25 13:36:19, arv wrote:
> I don't think this is required. It should be sufficient to get the treeScope.

Yes, this is not strictly required. I thought it was a nice addition though
because this optimization clearly does not make sense in the case we are not in
a Document (which is always the case for a DocumentFragment).

I think this avoids doing a bit of extra processing (hash look-up + checking if
element is a descendant) in the case the node *is not* in a Document and goes
straight to traversing the subtree.

Also, in the case the node *is* part of a Document, it avoids unnecessarily
traversing the subtree when treeScope().getElementById(id) returned null.

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1171: if (!element)
i.e. Early return here is now possible as we know we are in a Document and there
is no element with the given ID in the document (thus we avoid traversing).

[arv (Not doing code reviews), 2014-04-25 15:04:03]

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1171: if (!element)
On 2014/04/25 14:41:20, Chris Dumez wrote:
> i.e. Early return here is now possible as we know we are in a Document and
there
> is no element with the given ID in the document (thus we avoid traversing).

I was thinking of the case where we are in a disconnected tree but the tree
scope knows about the IDs. Maybe that can never happen?

[Inactive, 2014-04-25 15:13:40]

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1167: if (inDocument()) {
On 2014/04/25 14:41:20, Chris Dumez wrote:
> On 2014/04/25 13:36:19, arv wrote:
> > I don't think this is required. It should be sufficient to get the
treeScope.
> 
> Yes, this is not strictly required. I thought it was a nice addition though
> because this optimization clearly does not make sense in the case we are not
in
> a Document (which is always the case for a DocumentFragment).
> 
> I think this avoids doing a bit of extra processing (hash look-up + checking
if
> element is a descendant) in the case the node *is not* in a Document and goes
> straight to traversing the subtree.
> 
> Also, in the case the node *is* part of a Document, it avoids unnecessarily
> traversing the subtree when treeScope().getElementById(id) returned null.

I think I should be using isInTreeScope() instead of inDocument() since the
DocumentOrderedMap is associated to TreeScopes and not only Documents.

[Inactive, 2014-04-25 15:25:34]

I updated the patch to use isInTreeScope() instead of isInDocument() so that we
can leverage the DocumentOrderedMap for nodes part of a shadow tree.

I don't believe the tree scope can know about IDs of elements that are
disconnected from the tree scope.

[arv (Not doing code reviews), 2014-04-25 15:28:58]

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1173: if (element->isDescendantOf(this))
Can this ever be false?

[Inactive, 2014-04-25 15:31:30]

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1173: if (element->isDescendantOf(this))
On 2014/04/25 15:28:59, arv wrote:
> Can this ever be false?

Yes, just because there is an element in the Document (TreeScope) with the given
ID, doesn't mean it is part of our subtree. As the spec says, we are only
interested in Elements that are in our subtree.

[arv (Not doing code reviews), 2014-04-25 15:37:41]

LGTM

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/251633002/diff/40001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1173: if (element->isDescendantOf(this))
On 2014/04/25 15:31:30, Chris Dumez wrote:
> On 2014/04/25 15:28:59, arv wrote:
> > Can this ever be false?
> 
> Yes, just because there is an element in the Document (TreeScope) with the
given
> ID, doesn't mean it is part of our subtree. As the spec says, we are only
> interested in Elements that are in our subtree.

I see. The SVGSVGElement makes this != tree scope. Good thing isDescendantOf
uses treeScope so we do not have to walk the parent nodes here.

[Inactive, 2014-04-28 18:17:56]

The intent to implement and ship received enough support:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D5ZG_JARr68

Could an API OWNER please review the CL?

[abarth-chromium, 2014-04-28 18:25:25]

LGTM

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:23:
shouldBeNull("fragment.getElementById('doesNotExist')");
Does this test all the paths through the code you've added?  For example, does
it test the path where there are two elements with the ID, one inside the
ContainerNode and one outside that's found by the implementation in the
treescope?

[Inactive, 2014-04-28 18:31:48]

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:23:
shouldBeNull("fragment.getElementById('doesNotExist')");
On 2014/04/28 18:25:25, abarth wrote:
> Does this test all the paths through the code you've added?  For example, does
> it test the path where there are two elements with the ID, one inside the
> ContainerNode and one outside that's found by the implementation in the
> treescope?

No, I will improve the test to cover the duplicate id cases.

[Inactive, 2014-04-28 18:42:01]

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/40001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:23:
shouldBeNull("fragment.getElementById('doesNotExist')");
On 2014/04/28 18:25:25, abarth wrote:
> Does this test all the paths through the code you've added?  For example, does
> it test the path where there are two elements with the ID, one inside the
> ContainerNode and one outside that's found by the implementation in the
> treescope?

Done.

[arv (Not doing code reviews), 2014-04-28 18:44:27]

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:20: var span =
document.createElement("span", "");
Why the second argument?

[Inactive, 2014-04-28 18:46:17]

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:20: var span =
document.createElement("span", "");
On 2014/04/28 18:44:27, arv wrote:
> Why the second argument?

That's a good question :) I'll fix this now.

[Inactive, 2014-04-28 18:50:47]

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
File LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html (right):

https://codereview.chromium.org/251633002/diff/60001/LayoutTests/fast/dom/Doc...
LayoutTests/fast/dom/DocumentFragment/get-element-by-id.html:20: var span =
document.createElement("span", "");
On 2014/04/28 18:46:17, Chris Dumez wrote:
> On 2014/04/28 18:44:27, arv wrote:
> > Why the second argument?
> 
> That's a good question :) I'll fix this now.

Done.

[arv (Not doing code reviews), 2014-04-28 19:03:54]

Still LGTM

[Inactive, 2014-04-28 19:24:46]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-28 19:25:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/251633002/80001

[commit-bot: I haz the power, 2014-04-28 20:00:23]

Change committed as 172796