540848 - Add appendChild/insertBefore/replaceChild/removeChild on nsINode

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

These will be primarily used from quickstubs.

Comment 1

[Peter Van der Beken [:peterv]]

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

I also added GetNodeType on nsINode. Ideally we'd change the signature (to |PRUint16 NodeType()|?), but for now this allows us to change some variables and arguments from nsIDOMNode to nsINode without changing the calls to GetNodeType and just reuse the implementations of GetNodeType.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Didn't save before qref :-/.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

>+++ b/content/base/public/nsINode.h
>+  NS_IMETHOD GetNodeType(PRUint16* aNodeType) = 0;

Do file a bug on a better signature for this, please.

>+  nsINode* ReplaceOrInsertBefore(PRBool aReplace, nsINode *aNewChild,
>+                                 nsINode *aRefChild, nsresult *aReturn)
>+  {
>+    *aReturn = ReplaceOrInsertBefore(PR_FALSE, aNewChild, aRefChild);

s/PR_FALSE/aReplace/, right?  If none of our tests caught this, add one that would?

>+++ b/content/base/src/nsGenericElement.cpp
>+nsINode::ReplaceOrInsertBefore(PRBool aReplace, nsINode* aNewChild,
>+    refContent = static_cast<nsIContent*>(aRefChild);

Assert that it's an nsIContent, please (using IsNodeOfType)?

Is it me, or was the old code's use of GetCurrentDoc when calling do_ReplaceOrInsertBefore just buggy (e.g. wouldn't lead to adoption when it was needed)?

>+          mozAutoRemovableBlockerRemover blockerRemover(GetOwnerDoc());

Can you not use |doc| here?  Is it just that it might have changed?  If so, please document?

Also, can GetOwnerDoc() change between when you set |document| and when you set |doc|?  If not, why do you need |doc|?  Please document as needed?

>+          mozAutoSubtreeModified subtree(GetOwnerDoc(), this);

Again, can this use |doc|?

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to comment #3)
> >+  NS_IMETHOD GetNodeType(PRUint16* aNodeType) = 0;
> 
> Do file a bug on a better signature for this, please.

Bug 548579.

> s/PR_FALSE/aReplace/, right?  If none of our tests caught this, add one that
> would?

Yes, sorry. This will be caught by existing tests once we start using these methods from quickstubs.

> Is it me, or was the old code's use of GetCurrentDoc when calling
> do_ReplaceOrInsertBefore just buggy (e.g. wouldn't lead to adoption when it was
> needed)?

I tried to write a testcase for this, but it turns out that it's not an issue. GetCurrentDoc can be null but then aParent won't be null, and we'll use that to check for same ownerDocument.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 6

[Peter Van der Beken [:peterv]]

v1.1 with a followup patch to use it from quickstubs passes mochitests.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Peter, can you post an interdiff from v1 to v1.1?  For some reason the nsGenericElement.cpp changes in 1.1 don't apply to my m-c tree....

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Interdiff

Interdiff of v1 merged on top of the patch from bug 548463 and v1.1. I think I got the merge conflicts sorted out the right way.

I'm not sure why I changed the argument of the mozAutoDocConditionalContentUpdateBatch to GetCurrentDoc. Also, I think the piece of code that throws NS_ERROR_DOM_WRONG_DOCUMENT_ERR can go too, I think that situation can't happen. I had split that off into bug 548463 (only to remove it later) but somehow it made it back into this patch :-/.

Comment 9

[Peter Van der Beken [:peterv]]

Ah, and the testcase changes. hc_attrremovechild1 is passing with the patch. bug503926.xul and test_bug503926.xul relied on appendChild QI'ing its argument, after the QS patch lands we won't anymore :-D.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 428961 [details] [diff] [review]
v1.1

r=bzbarsky

Comment 11

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/068150b06d11

Comment 12

[Timothy Nikkel (:tnikkel)]

Shouldn't this have rev'd the iid on nsINode.h?

Comment 13

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/13571a9165ea