Closed
Bug 933193
Opened 12 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•12 years ago
|
Keywords: dev-doc-needed
Comment 1•12 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
|
||
![]() |
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•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•