323311 - [FIX] Create an nsINode interface

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Sicking and I have been talking about this on and off.  So I finally did it.

What I need to do now is to run the XSLT and DOM test suites in this build.  Directions on how to do that would be much appreciated.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch (not fully tested yet)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w for ease of review

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Is the idea to move eventually more methods from nsIContent/nsIDocument to
nsINode?
For example Get/Set/Unset/DeleteProperty?
And should also nsIAttribute inherit nsINode?
Or actually, perhaps we could remove nsIAttribute and make nsDOMAttribute
inherit nsINode.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Catch me on irc and i'll help you run the xslt tests.

Making nsIAttribute inherit nsINode seems like a great idea, as is moving the property methods to nsINode.

Not sure if we can entierly get rid of nsIAttribute, though it looks like the only user outside gklayout.dll is xforms so it might be possible. We'd definitely need an IsNodeOfType though to allow safe casting to nsDOMAttribute

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is the idea to move eventually more methods from nsIContent/nsIDocument to
> nsINode?

Maybe.  I moved the obvious things -- places where we were already calling the same method on either a content or document due to shared code.  If moving more things will help, sure.  Just in separate bugs.  ;)

And yeah, there are more games we can play with nsIAttribute, I think.  Again, in followups...

Note that as another followup I want to make AppendChildTo inline and non-virtual on nsINode and kill all subclass impls.

We should have a wiki page somewhere on running tests (with instructions for DOM, XSLT, layout, performance, etc) tests....

Comment 6

[Boris Zbarsky [:bzbarsky]]

That patch crashes on the xslt conformance tests and has lots of failures; I'll look into it tonight, I guess.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Some random thoughts:

We should move nsIContent::IsContentOfType to nsINode::IsNodeOfType with the ability to test for attribute and document. We should also possibly take that opportunity to make IsNodeOfType into:

  PRBool IsNodeOfType(PRUint32 aFlags)
  {
    return !(aFlags & ~NodeType())
  }

and let NodeType be the virtual function. That lets callers run multiple tests on the type by calling NodeType() directly. Possible downside is that binary size might be bigger.


It would be great to have a nsINode* nsINode::GetParent() type function that allows you to get the parent no matter if it's the document or not. Naming is the big problem though because both GetParent and GetParentNode is taken.

We could maybe rename the current nsIContent* nsIContent::GetParent to GetParentContent, and create an nsINode* nsINode::GetParent. Or simply call it GetNodeParent I guess.

I think i like the s/GetParent/GetParentContent/ solution best.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 208386 [details] [diff] [review]
Same as diff -w for ease of review

>Index: content/base/public/nsINode.h

>+  /**
>+   * Insert a content node at a particular index.  This method handles calling
>+   * BindToTree on the child appropriately.
>+   *
>+   * @param aKid the content to insert
>+   * @param aIndex the index it is being inserted at (the index it will have
>+   *        after it is inserted)
>+   * @param aNotify whether to notify the document (current document for
>+   *        nsIContent, and |this| for nsIDocument) that the insert has
>+   *        occurred
>+   *
>+   * @throws NS_ERROR_DOM_HIERARCHY_REQUEST_ERR if one attempts to have more
>+   * than one element node as a child of a document.  Doing this will also
>+   * assert -- you shouldn't be doing it!  Check with
>+   * nsIDocument::GetRootContent() first if you're not sure.  Apart from this
>+   * one constraint, this doesn't do any checking on whether aKid is a valid
>+   * child of |this|.
>+   */
>+  virtual nsresult InsertChildAt(nsIContent* aKid, PRUint32 aIndex,
>+                                 PRBool aNotify) = 0;

Mention that NS_ERROR_OOM can be thrown too so that people always check the result even when they know that NS_ERROR_DOM_HIERARCHY_REQUEST_ERR won't be thrown.

Is there a reason we need to do the double-root checking? Or is it simply cheap enough that it doesn't hurt?


>Index: content/base/public/nsIContent.h

>   nsIContent(nsINodeInfo *aNodeInfo)
>-    : mParentPtrBits(0),
>-      mNodeInfo(aNodeInfo)
>+    : mParentPtrBits(0)
>   {
>+    mNodeInfo = aNodeInfo;
>     NS_ASSERTION(aNodeInfo,
>                  "No nsINodeInfo passed to nsIContent, PREPARE TO CRASH!!!");
>   }

Please make nsINode have a ctor taking an nsINodeInfo (nsIDocument would pass null). I suspect it might save a few bytes and a few cycles.

>Index: content/base/src/nsDocument.h

>   virtual nsresult SetRootContent(nsIContent* aRoot);

This doesn't need to be virtual any more, does it? Or can the function be removed entierly?


>Index: content/base/src/nsDocument.cpp
>+nsDocument::InsertChildAt(nsIContent* aKid, PRUint32 aIndex,
...
>+  if (NS_FAILED(rv) && mRootContent == aKid) {
>+    NS_ASSERTION(mChildren.IndexOfChild(aKid) == -1,
>+                 "Error result and still have same root content but it's in "
>+                 "our child list?");

The text here confuses me a bit. Why "same" root? Is this some reentrency thing?


>+nsDocument::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
>+{
>+  nsCOMPtr<nsIContent> oldKid = GetChildAt(aIndex);
>+  nsresult rv = NS_OK;
>+  if (oldKid) {
>+    if (oldKid == mRootContent) {
>+      NS_ASSERTION(oldKid->IsContentOfType(nsIContent::eELEMENT),
>+                   "Non-element root content?");
>+      // Destroy the link map up front and null out mRootContent before we mess
>+      // with the child list.  Hopefully no one in there will compare the
>+      // content being removed to GetRootContent()....  Need to do it this way
>+      // because otherwise DOM events might fire and want to set a new
>+      // mRootContent after we've removed from the child list but before we've
>+      // set mRootContent to null.  If we ever fix the issue of DOM events
>+      // firing at inconvenient times, consider changing the order here.  Just
>+      // make sure we DestroyLinkMap() before unbinding the content.
>+      DestroyLinkMap();
>+      mRootContent = nsnull;
>+    }

This comment is sort of confusing too. Are we setting mRootContent to null so that fired eventhandlers see a null documentElement?

And what does "in there" refer to in "Hopefully no one in there will compare..."? I take it it doesn't refer to DestroyLinkMap although that's what it sounds like.

> nsDocument::AppendChild(nsIDOMNode* aNewChild, nsIDOMNode** aReturn)
> {
>-  return InsertBefore(aNewChild, nsnull, aReturn);
>+  return nsDocument::InsertBefore(aNewChild, nsnull, aReturn);
> }

Hopefully noone overrides just InsertBefore and expects that to be called from AppendBefore. But I don't see a reason why anyone would override either of them.

>+nsGenericElement::doRemoveChildAt(PRUint32 aIndex, PRBool aNotify,
>+                                  nsIContent* aKid, nsIContent* aParent,
>+                                  nsIDocument* aDocument,
>                 nsAttrAndChildArray& aChildArray)
> {
>   NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!");
>   NS_PRECONDITION(!aParent || aParent->GetCurrentDoc() == aDocument,
>                   "Incorrect aDocument");
> 
>-  nsContentOrDocument container(aParent, aDocument);
>+  nsINode* container = aParent;
>+  if (!container) {
>+    container = aDocument;
>+  }
>+  NS_ASSERTION(container, "Must have a container!");

You've pretty much already asserted this in the first NS_PRECONDITION in the function. Same thing further down.

>Index: content/xslt/src/xslt/txMozillaXMLOutput.cpp
>@@ -297,30 +297,31 @@ void txMozillaXMLOutput::endElement(cons
...
>+#ifdef DEBUG
>+        NS_ASSERTION(!document || !mRootContentCreated,
>+                     "mNonAddedParent shouldn't be a document if we have a "
>+                     "root content");
>+#endif

Remove the ifdef


> void txMozillaXMLOutput::closePrevious(PRInt8 aAction)
...
>-            if (document && currentElement && !mRootContent) {
>-                mRootContent = do_QueryInterface(mCurrentNode);
>-                // XXXbz what to do on failure here?
>-                document->SetRootContent(mRootContent);
>+            if (document) {
>+                mRootContentCreated = PR_TRUE;
>             }

That should be |if (document && currentElement)|, no?

Actually, testing for currentElement is enough since all you're doing is setting the bool.


>@@ -552,60 +549,70 @@ txMozillaXMLOutput::createTxWrapper()
...
>+            case nsIDOMNode::DOCUMENT_TYPE_NODE:
>+                // Make sure to insert the new documentElement after the
>+                // document type.  This is needed for cases when there is no
>+                // existing documentElement in the document.
>+                rootLocation = PR_MAX(rootLocation, j+1);
>+                // Fall through

We shouldn't be in this function if there was no exising documentElement so this seems unneccesary. 

>-    mRootContent = do_QueryInterface(wrapper);
>-    return document->SetRootContent(mRootContent);
>+    mRootContentCreated = PR_TRUE;
>+    nsCOMPtr<nsIContent> wrapperContent = do_QueryInterface(wrapper);
>+    NS_ASSERTION(wrapperContent, "Must have wrapper content");
>+    return document->InsertChildAt(wrapperContent, rootLocation, PR_TRUE);

We shouldn't be in this function unless mRootContentCreated was already true.

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, I the crash was due to bug 297067 being patched incorrectly (see bug 297067 comment 17).  Still debugging the XSLT test failures.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, the failures were because I never initialized mRootContentCreated to PR_FALSE!  With that fixed, I pass the tests just as well as I do without this patch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #8)
> Is there a reason we need to do the double-root checking? Or is it simply
> cheap enough that it doesn't hurt?

I suppose I could just make that an assert...  But it's cheap, and we would really not do well if someone does it.  I'd rather be safe and throw, I think.  But I'm not so firm on that, so if you want this to just assert, let me know.

> >   virtual nsresult SetRootContent(nsIContent* aRoot);
> 
> This doesn't need to be virtual any more, does it? Or can the function be
> removed entierly?

Er, yeah.  It's just gone now.  Good catch!

> >+    NS_ASSERTION(mChildren.IndexOfChild(aKid) == -1,
> >+                 "Error result and still have same root content but it's in "
> >+                 "our child list?");
> 
> The text here confuses me a bit. Why "same" root? Is this some reentrency
> thing?

DOM events can fire from doInsertChildAt.  So the DOM can look nothing like what we think it should look like at this point.

> This comment is sort of confusing too. Are we setting mRootContent to null so
> that fired eventhandlers see a null documentElement?

Yes, since they will in fact fire after the documentElement has been fully removed.

> And what does "in there" refer to in "Hopefully no one in there will
> compare..."? 

"in there" is "in messing with the child list".  As in "in doRemoveChildAt".  Fixed the comment accordingly.

> Hopefully noone overrides just InsertBefore and expects that to be called from
> AppendBefore. But I don't see a reason why anyone would override either of
> them.

People do for elements... They don't for documents, but I wanted to keep the two parallel.  In any case, I want to nuke this code like I said.

>>+  NS_ASSERTION(container, "Must have a container!");
>You've pretty much already asserted this in the first NS_PRECONDITION

True.  Removed.

> That should be |if (document && currentElement)|, no?

Indeed.  Fixed.  I'd like to leave it this way instead of just testing currentElement so that it's clearer what we're doing, I think...

> We shouldn't be in this function if there was no exising documentElement
...
> We shouldn't be in this function unless mRootContentCreated was already true.

I think we can get there via txMozillaXMLOutput::closePrevious with the eFlushText action without there being a documentElement and without mRootContentCreated being true, no?

I'll post an updated patch once I've had a chance to compile it.




> 
> 
> >+nsDocument::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
> >+{
> >+  nsCOMPtr<nsIContent> oldKid = GetChildAt(aIndex);
> >+  nsresult rv = NS_OK;
> >+  if (oldKid) {
> >+    if (oldKid == mRootContent) {
> >+      NS_ASSERTION(oldKid->IsContentOfType(nsIContent::eELEMENT),
> >+                   "Non-element root content?");
> >+      // Destroy the link map up front and null out mRootContent before we mess
> >+      // with the child list.  Hopefully no one in there will compare the
> >+      // content being removed to GetRootContent()....  Need to do it this way
> >+      // because otherwise DOM events might fire and want to set a new
> >+      // mRootContent after we've removed from the child list but before we've
> >+      // set mRootContent to null.  If we ever fix the issue of DOM events
> >+      // firing at inconvenient times, consider changing the order here.  Just
> >+      // make sure we DestroyLinkMap() before unbinding the content.
> >+      DestroyLinkMap();
> >+      mRootContent = nsnull;
> >+    }
> 
> This comment is sort of confusing too. Are we setting mRootContent to null so
> that fired eventhandlers see a null documentElement?
> 
> And what does "in there" refer to in "Hopefully no one in there will
> compare..."? I take it it doesn't refer to DestroyLinkMap although that's what
> it sounds like.
> 
> > nsDocument::AppendChild(nsIDOMNode* aNewChild, nsIDOMNode** aReturn)
> > {
> >-  return InsertBefore(aNewChild, nsnull, aReturn);
> >+  return nsDocument::InsertBefore(aNewChild, nsnull, aReturn);
> > }
> 
> Hopefully noone overrides just InsertBefore and expects that to be called from
> AppendBefore. But I don't see a reason why anyone would override either of
> them.
> 
> >+nsGenericElement::doRemoveChildAt(PRUint32 aIndex, PRBool aNotify,
> >+                                  nsIContent* aKid, nsIContent* aParent,
> >+                                  nsIDocument* aDocument,
> >                 nsAttrAndChildArray& aChildArray)
> > {
> >   NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!");
> >   NS_PRECONDITION(!aParent || aParent->GetCurrentDoc() == aDocument,
> >                   "Incorrect aDocument");
> > 
> >-  nsContentOrDocument container(aParent, aDocument);
> >+  nsINode* container = aParent;
> >+  if (!container) {
> >+    container = aDocument;
> >+  }
> >+  NS_ASSERTION(container, "Must have a container!");
> 
> You've pretty much already asserted this in the first NS_PRECONDITION in the
> function. Same thing further down.
> 
> >Index: content/xslt/src/xslt/txMozillaXMLOutput.cpp
> >@@ -297,30 +297,31 @@ void txMozillaXMLOutput::endElement(cons
> ...
> >+#ifdef DEBUG
> >+        NS_ASSERTION(!document || !mRootContentCreated,
> >+                     "mNonAddedParent shouldn't be a document if we have a "
> >+                     "root content");
> >+#endif
> 
> Remove the ifdef
> 
> 
> > void txMozillaXMLOutput::closePrevious(PRInt8 aAction)
> ...
> >-            if (document && currentElement && !mRootContent) {
> >-                mRootContent = do_QueryInterface(mCurrentNode);
> >-                // XXXbz what to do on failure here?
> >-                document->SetRootContent(mRootContent);
> >+            if (document) {
> >+                mRootContentCreated = PR_TRUE;
> >             }
> 
> That should be |if (document && currentElement)|, no?
> 
> Actually, testing for currentElement is enough since all you're doing is
> setting the bool.
> 
> 
> >@@ -552,60 +549,70 @@ txMozillaXMLOutput::createTxWrapper()
> ...
> >+            case nsIDOMNode::DOCUMENT_TYPE_NODE:
> >+                // Make sure to insert the new documentElement after the
> >+                // document type.  This is needed for cases when there is no
> >+                // existing documentElement in the document.
> >+                rootLocation = PR_MAX(rootLocation, j+1);
> >+                // Fall through
> 
> We shouldn't be in this function if there was no exising documentElement so
> this seems unneccesary. 
> 
> >-    mRootContent = do_QueryInterface(wrapper);
> >-    return document->SetRootContent(mRootContent);
> >+    mRootContentCreated = PR_TRUE;
> >+    nsCOMPtr<nsIContent> wrapperContent = do_QueryInterface(wrapper);
> >+    NS_ASSERTION(wrapperContent, "Must have wrapper content");
> >+    return document->InsertChildAt(wrapperContent, rootLocation, PR_TRUE);
> 
> We shouldn't be in this function unless mRootContentCreated was already true.
> 
> r=me
> 

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #11)
> (In reply to comment #8)
> > Is there a reason we need to do the double-root checking? Or is it simply
> > cheap enough that it doesn't hurt?
> 
> I suppose I could just make that an assert...  But it's cheap, and we would
> really not do well if someone does it.  I'd rather be safe and throw, I think. 
> But I'm not so firm on that, so if you want this to just assert, let me know.

Naah, keep the throw.

> > >+    NS_ASSERTION(mChildren.IndexOfChild(aKid) == -1,
> > >+                 "Error result and still have same root content but it's in "
> > >+                 "our child list?");
> > 
> > The text here confuses me a bit. Why "same" root? Is this some reentrency
> > thing?
> 
> DOM events can fire from doInsertChildAt.  So the DOM can look nothing like
> what we think it should look like at this point.

I'm a bit worried about this. You're relying on that if it throws it will not leave the child in the childlist. This seems to currently be true, but it feels shaky to rely on to never change. I think it might be worth it to check the childlist and update accordingly always when rv indicates failure.

> > Hopefully noone overrides just InsertBefore and expects that to be called from
> > AppendBefore. But I don't see a reason why anyone would override either of
> > them.
> 
> People do for elements... They don't for documents, but I wanted to keep the
> two parallel.  In any case, I want to nuke this code like I said.

They override the nsIContent methods, not the nsIDOMNode ones, no?

> > That should be |if (document && currentElement)|, no?
> 
> Indeed.  Fixed.  I'd like to leave it this way instead of just testing
> currentElement so that it's clearer what we're doing, I think...

Yeah, feel free to. I'm going to simplify some of this stuff later on when I move the code to nsIContent anyway.

> > We shouldn't be in this function if there was no exising documentElement
> ...
> > We shouldn't be in this function unless mRootContentCreated was already true.
> 
> I think we can get there via txMozillaXMLOutput::closePrevious with the
> eFlushText action without there being a documentElement and without
> mRootContentCreated being true, no?

Ah, good point. So I think we should make this behave slightly different then, just append the new wrapper to the end. That should be the end result anyway.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Other candidates for move to nsINode:

GetBaseURI
HandleDOMEvent
GetPrincipal     (doesn't exist on nsIContent, but might be useful there)
Tag/NamespaceID  (doesn't exist on nsIDocument, but might be useful to allow
                  walking the entire node tree)
Range functions  (doesn't exist on nsIDocument, but would probably allow cleaner
                  range implementation)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> but it feels shaky to rely on to never change

We really need that invariant enforced (that failure to insert a node property doesn't actually insert it), and we do enforce it.  If the invariant fails because someone screws up, I'd rather we asserted here than ran quietly and had issues because an improperly bound node is in the child list.

I suppose I could leave the assert but also deal with the case when the assert fires... I might do that.

> They override the nsIContent methods, not the nsIDOMNode ones, no?

Yes.

> Ah, good point. So I think we should make this behave slightly different then,
> just append the new wrapper to the end. That should be the end result anyway.

Hmm.  Good point.  Will do.  I'll leave the existing code anyway, ifdef DEBUG, as a double-check.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Tested (at least on the XSLT tests)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 208592 [details] [diff] [review]
Same as diff -w

>+      // Destroy the link map up front and null out mRootContent before we mess
>+      // with the child list.  Hopefully no one in doRemoveChildAt will compare
>+      // the content being removed to GetRootContent()....  Need to do it this
>+      // way because otherwise DOM events might fire and want to set a new
>+      // mRootContent after we've removed from the child list but before we've
>+      // set mRootContent to null.  If we ever fix the issue of DOM events
>+      // firing at inconvenient times, consider changing the order here.  Just
>+      // make sure we DestroyLinkMap() before unbinding the content.

Aah, now I'm following this better, I originally read "set" as "see". How about changing this to


      // Destroy the link map up front and null out mRootContent before we mess
      // with the child list.  Hopefully no one in doRemoveChildAt will compare
      // the content being removed to GetRootContent()....  Need to do it this
      // before calling doRemoveChildAt since DOM events might fire and want set
      // a new mRootContent.  If we ever fix...

I always prefer to descript why we do something, rather the describe what would happen if we didn't. Less negation that way.

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sure.  I can make that change.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 208592 [details] [diff] [review]
Same as diff -w

sr=jst, looks good!

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip (complete with nsGkAtomList, etc)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed.