Remove a QI and AddRef/Release from nsNodeUtils::CloneAndAdopt

RESOLVED FIXED in mozilla1.9.3a1

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
Created attachment 402659 [details] [diff] [review]
v1

CloneAndAdopt calls itself recursively, it has the result as an nsINode but the result argument is an nsIDOMNode**, so we QI to nsIDOMNode for every call. Only the outer call needs the result, so there's a lot of pointless QIs. I split it up in two functions, one that has a nsINode** argument, and one that has a nsIDOMNode** argument that just calls the nsINode** one and then QIs. The recursive calls just use the nsINode** one.
It also QIs an nsINode to nsIContent, but we already know at that point that the nsINode is an nsIContent (it only happens when the caller passes in a parent, we only pass in a parent when we're walking its children, all the children are nsIContent). It also QIs the clone of a childnode to nsIContent, that should always be an nsIContent so it doesn't really need to QI (that code is only for cloning a document though).
This speeds up a testcase that I have that clones a bunch of nodes and appends them by about 2%.
Attachment #402659 - Flags: review?(bzbarsky)
Attachment #402659 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 2

9 years ago
Created attachment 402678 [details] [diff] [review]
Remove another QI

Turns out we can remove another QI for elements with attributes, if we clone an nsGenericElement we can assume the clone is also an nsGenericElement. We can also avoid the QI to nsIDOMNode for attribute nodes, since we already QI to nsIDOMAttr immediately after (this doesn't matter much, no one uses attribute nodes).
Attachment #402678 - Flags: review?(bzbarsky)
Attachment #402678 - Flags: review?(bzbarsky) → review+

Comment 3

9 years ago
Comment on attachment 402659 [details] [diff] [review]
v1

>+  {
>+    nsCOMPtr<nsINode> clone;
>+    nsresult rv = CloneAndAdopt(aNode, aClone, aDeep, aNewNodeInfoManager,
>+                                aCx, aOldScope, aNewScope, aNodesWithProperties,
>+                                nsnull, getter_AddRefs(clone));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return clone ? CallQueryInterface(clone, aResult) : NS_OK;
>+  }
As I understand it, if aClone is true then a successful call will always return a result, and if aClone is false then the callers will not expect a result, but it looks odd that you don't write aResult in that case.
(Assignee)

Comment 4

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9c2a79c5373d
http://hg.mozilla.org/mozilla-central/rev/7f865337ce94

And a fix for comment 3 (idea from bz):
http://hg.mozilla.org/mozilla-central/rev/1e0d1e1cdbef
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.