324572 - More nsINode work

Status: RESOLVED FIXED

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

Description

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

Patch coming up that makes nsIAttribute inherit nsINode and that moves the property methods to nsINode.

I added a proper implementation for the property methods to nsGenericDOMDataNode so that all nsINodes support properties. Almost all code was already there since data nodes already supported user-data.

The childnode of attributes behave sort of badly. First of all the childnode will not claim to have a parent. This is because we can't set an nsIContent parent. Second, if someone places the childnode elsewhere in the tree we won't remove it as a child of the attribute.

However both these problems existed before this patch too. We should fix this, but that's for a different bug.

I also fixed some unefficient code in nsDOMAttribute::GetValue to speed up the child iteration methods.

Comment 1

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

Attachment: Patch to fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209517 [details] [diff] [review]
Patch to fix

So per IRC discussion, we should consider just implementing the property stuff on nsINode (as virtual methods, but ones with impls) and not having 4 copies of it in the tree.  generic elements and generic DOM data nodes will need to override SetProperty to set their bits, but that's it.

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

Worth including nsINode.h here too?  Right now it comes in via nsIContent.h, but to be safe?

Also, change the IID.

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

This needs a new IID.  So do all interfaces inheriting from this (specifically, nsIContent and nsIDocument).

> #include "nsCOMPtr.h"
>+#include "nsPropertyTable.h"

Hmm.... Should both those includes perhaps be inside the #ifdef MOZILLA_INTERNAL_API?  I think they should, since neither is needed if that's not defined.

>+   * Get a property associated with this node.
....
>+   * @return               the property.

"null if the property is not set (though a null return value does not imply the property was not set)"?

>+   * Set a property to be associated with this node. Will overwrite existing

"This will overwrite an existing ..."

>+   * value if one exists. The existing value is destroyed using the descrution

s/descrution/destructor/, I would say.

>+   * @param aDtor          descrutor function to be used when this property

s/descrutor/destructor/

>+   * Destroys a property associated with this node. The value is destroyed
>+   * using the descrution function given when that value was set.

s/descrution/destruction/

>+   * Unset a property associated with this node. The value will not be
>+   * destroyed but rather returned.

"It is the caller's responsibility to destroy the value after that point" perhaps?

>+   * @return               the property

"null if the property is not set (though a null return value does not imply the property was not set)"?
"?

>Index: content/base/src/nsDOMAttribute.cpp
>+nsDOMAttribute::EnsureChild(PRBool aSetText, PRBool &aHasChild) const

This should not bother creating mChild if it's null and the value is empty.

And maybe "EnsureChildState()" is a better name?

The rest looks ok.  I'd like to see the updated patch, though.

Comment 3

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

> And maybe "EnsureChildState()" is a better name?

I prefer EnsureChild since this doesn't touch any other state.

Comment 4

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

Attachment: patch v2

This fixes bzs comments. I did also notice that we're inconsistent in which pointer we use to store/remove stuff in the properties table. Sometimes we use the nsIDOMNode pointer, sometimes the nsIContent pointer, and sometimes just |this|.

The end result is that we can "leak" properties until the document goes away.

So made us always use the nsINode pointer by making the userdata apis on nsIDocument use |nsINode*| rather then |nsISupports*|. Sorry about the extra changes in the new patch :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I prefer EnsureChild since this doesn't touch any other state.

But it does more than ensure that mChild exists -- it ensures that the text value in it is correct (in some cases).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209557 [details] [diff] [review]
patch v2

So... where were we using inconsistent pointers for the user data stuff?  I agree that it's better to use nsINode, but I'm curious to see where it was wrong before.  Note that nsIDOMAttr is the primary nsISupports pointer for nsDOMAttribute.

The userdata stuff is trunk-only, right?

>Index: base/src/nsDOMAttribute.cpp
>+nsDOMAttribute::EnsureChild(PRBool aSetText, PRBool &aHasChild) const

>+  if (aSetText) {
>+    mChild->SetText(value, PR_TRUE);
>+  }

mChild can be null here if value is empty.  Need to check.

I still think EnsureChildState is a better name, but either way.

r+sr=bzbarsky with that fixed.

Comment 7

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

Turns out we're never using the wrong pointer on trunk as is. The inconsitencies I was seing were all either from the fact that we use different primary nsISupports for different node-types or from the new code.

Ideally we should make propertytable use typed interfaces rather then just void* as key to avoid people from making the same misstake I did. I have an idea to use c++ magic to accomplish that in an extensible manner, but that's for a different bug.

Changed the name to EnsureChildState.

Comment 8

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

Oh, I removed a the same casts from nsDocument UserData functions that I had removed from the other classes.

Comment 9

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

Attachment: final version

This is what i just checked in

Comment 10

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

Attachment: Remove property methods from nsDOMAttribute

Doh! I didn't remove the property method implementations from nsDOMAttribute.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209635 [details] [diff] [review]
Remove property methods from nsDOMAttribute

Bring on the code removal!

Comment 12

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

Checked in, thanks for the reviews