Anne (:annevk) Reporter
	Description • 11 years ago

At the same time we should probably remove SVGSVGElement.prototype.getElementById.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 11 years ago

I wonder what kind of performance characteristics we need and want for getElementById() when
it is not in document.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 11 years ago

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).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

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.  ;)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 11 years ago

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=23694

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 11 years ago

> 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....

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 11 years ago

smaug	bz: so if you have some_nsinode->GetElementById(); which version do you get if some_nsinode is actually nsIDocument.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 11 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 11 years ago

That sounds good. Yes, I prefer that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 11 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4fa51c53e0
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1ec44bb1db
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2b3068888b

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

Backed out for mochitest-3 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca6961857f5

https://tbpl.mozilla.org/php/getParsedLog.php?id=29990338&tree=Mozilla-Inbound

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 11 years ago

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...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 11 years ago

For the record, the conclusion is to keep this on SVGSVGElement but add it on DocumentFragment.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 11 years ago

Relanded just for DocumentFragment:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2b330e02ff
   https://hg.mozilla.org/integration/mozilla-inbound/rev/80070e83f335
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6706b8500d2

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2b2b330e02ff
https://hg.mozilla.org/mozilla-central/rev/80070e83f335
https://hg.mozilla.org/mozilla-central/rev/c6706b8500d2