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

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: 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%.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 402659 [details] [diff] [review]
v1

Looks good.

Comment 2

[Peter Van der Beken [:peterv]]

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

Comment 3

[[e-mail]]

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.

Comment 4

[Peter Van der Beken [:peterv]]

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