add getElementById on DocumentFragment

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: annevk, Assigned: bzbarsky)

Tracking

({dev-doc-needed})

unspecified
mozilla28
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
At the same time we should probably remove SVGSVGElement.prototype.getElementById.
(Reporter)

Updated

6 years ago
Keywords: dev-doc-needed
I wonder what kind of performance characteristics we need and want for getElementById() when
it is not in document.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
This makes SVGSVGElement's getElementById a good bit faster, both in-tree (due to not needing to do the extra indirection through querySelector) and out-of-tree (due to a much faster tree-walk).
Attachment #825524 - Flags: review?(bugs)
Those two are wins over the status quo no matter what, by the way; the part of this bug that's chancy is in part 3.  ;)
Whiteboard: [need review]
Comment on attachment 825523 [details] [diff] [review]
Factor out the id selector fast-path from querySelector(All) so we can reuse it a bit more broadly.


>+namespace {
>+struct SelectorMatchInfo {
{ goes to the next line


>+    Element *element = static_cast<Element*>(elements->ElementAt(i));
* goes with the type

>+    if (!aRoot->IsElement() ||
>+        (element != aRoot &&
>+           nsContentUtils::ContentIsDescendantOf(element, aRoot))) {
2 extra space before nsContentUtils
Attachment #825523 - Flags: review?(bugs) → review+
Attachment #825531 - Flags: review?(bugs) → review+
Comment on attachment 825524 [details] [diff] [review]
part 2.  Implement nsINode::GetElementById and make SVGSVGElement use it.

># HG changeset patch
># User Boris Zbarsky <bzbarsky@mit.edu>
># Date 1383253113 14400
># Node ID 03b02abd63d3611f7cb94925b41a834c14f7e540
># Parent  069addffd907398f683868428f8a232f4cc26db8
>Bug 933193 part 2.  Implement nsINode::GetElementById and make SVGSVGElement use it.  r=smaug
>
>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
>--- a/content/base/public/nsINode.h
>+++ b/content/base/public/nsINode.h
>@@ -1071,16 +1071,21 @@ public:
>   mozilla::dom::Element* QuerySelector(const nsAString& aSelector,
>                                        mozilla::ErrorResult& aResult);
>   already_AddRefed<nsINodeList> QuerySelectorAll(const nsAString& aSelector,
>                                                  mozilla::ErrorResult& aResult);
> 
>   nsresult QuerySelector(const nsAString& aSelector, nsIDOMElement **aReturn);
>   nsresult QuerySelectorAll(const nsAString& aSelector, nsIDOMNodeList **aReturn);
> 
>+  // nsIDocument overrides this with its own (faster) version.  This
>+  // should only be called for elements, document fragments, and
>+  // documents.
>+  mozilla::dom::Element* GetElementById(const nsAString& aId);
Shouldn't you make this virtual (this is virtual in nsIDocument)

>+Element*
>+nsINode::GetElementById(const nsAString& aId)
>+{
>+  MOZ_ASSERT(IsElement() || IsNodeOfType(eDOCUMENT) ||
>+             IsNodeOfType(eDOCUMENT_FRAGMENT),
>+             "Bogus this object for GetElementById call");
And then remove IsNodeOfType(eDocument) here.


Those fixed, r+

>+  if (IsInDoc()) {
>+    ElementHolder holder;
>+    FindMatchingElementsWithId<true>(aId, this, nullptr, holder);
>+    return holder.mElement;
>+  }
>+
>+  for (nsIContent* kid = GetFirstChild(); kid; kid = kid->GetNextNode(this)) {
I think it is a spec bug that getElementById() does not look for the context object itself if it is an element.
Attachment #825524 - Flags: review?(bugs) → review+
> Shouldn't you make this virtual (this is virtual in nsIDocument)

I don't see a reason to, actually.  Why would it need to be virtual here?

It's virtual in nsIDocument because it uses nsDocument members there....
Flags: needinfo?(bugs)
smaug	bz: so if you have some_nsinode->GetElementById(); which version do you get if some_nsinode is actually nsIDocument.
Flags: needinfo?(bugs)
With my patch, the nsINode version.  It happens to actually work correctly on documents; it's just not quite as fast.  But making it virtual might well make it even slower.

Bindings will never do that, though: they're calling this explicitly on nsIDocument, Element, and DocumentFragment instances, so will pick up the nsIDocument method.
It is just rather odd API that if you do
nsinode_which_is_actually_a_document->GetElementById() you get certain performance, and
static_cast<nsIDocument*>(
nsinode_which_is_actually_a_document)->GetElementById() gives you better perf.

Could we at least assert hard that no one calls the slower version on document.
So another option is that we make the nsINode function protected and add public inline functions on Element and DocumentFragment that just forward to it.  Would you prefer that?
Flags: needinfo?(bugs)
That sounds good. Yes, I prefer that.
Flags: needinfo?(bugs)
So this is exciting.  The failing test is dom/tests/mochitest/ajax/jquery/test_jQuery.html  and the failing subtest looks something like this:

  t( "Descendant escaped ID", "div #foo\\:bar", ["foo:bar"] );

Now this is actually testing jQuery 1.2.6 (yes, ancient, yes, widely deployed).  jQuery 1.2.6 has this code:

  var elem = ret[ret.length-1];
  // Try to do a global search by ID, where we can
  if ( m[1] == "#" && elem && elem.getElementById && !jQuery.isXMLDoc(elem) ) {
    // Optimization for HTML document case
    var oid = elem.getElementById(m[2]);

this used to not get run for cases when "elem" was an element, but now it does.  So for example in this case:

  <div><span id="x"></span></div>
  <div></div>
  <script>
    alert($("div #x").length);
  </script>

ret is the list of all divs, elem ends up being the second div, and then the code above does getElementById on it and ends up with no matches.  :(

I guess I'll post to the standards list for now and see what people say...
For the record, the conclusion is to keep this on SVGSVGElement but add it on DocumentFragment.
Summary: Move getElementById from Document to ParentNode → add getElementById on DocumentFragment
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.