Closed Bug 518669 Opened 13 years ago Closed 13 years ago

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


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: peterv, Assigned: peterv)



(2 files)

Attached patch v1Splinter Review
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+
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 on attachment 402659 [details] [diff] [review]

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

And a fix for comment 3 (idea from bz):
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.