Closed
Bug 933193
Opened 11 years ago
Closed 11 years ago
add getElementById on DocumentFragment
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: annevk, Assigned: bzbarsky)
References
()
Details
(Keywords: dev-doc-needed)
Attachments
(3 files)
5.46 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.52 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.02 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
At the same time we should probably remove SVGSVGElement.prototype.getElementById.
Reporter | ||
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 1•11 years ago
|
||
I wonder what kind of performance characteristics we need and want for getElementById() when it is not in document.
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #825523 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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).
Attachment #825524 -
Flags: review?(bugs)
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. ;)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #825531 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need review]
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
Attachment #825523 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #825531 -
Flags: review?(bugs) → review+
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.
Attachment #825524 -
Flags: review?(bugs) → review+
Comment 8•11 years ago
|
||
Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=23694
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....
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
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.
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?
Flags: needinfo?(bugs)
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
Whiteboard: [need review]
Target Milestone: --- → mozilla28
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
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...
Assignee | ||
Comment 18•11 years ago
|
||
For the record, the conclusion is to keep this on SVGSVGElement but add it on DocumentFragment.
Assignee | ||
Updated•11 years ago
|
Summary: Move getElementById from Document to ParentNode → add getElementById on DocumentFragment
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
Flags: in-testsuite+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•