333942 - Implement GetNodeParent

Status: RESOLVED FIXED

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

Description

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

We should move nsIContent::GetParent to nsINode since that might save some casting when nsINode becomes more widely used. Additionally, we should add a function that returns the parent as an nsINode which would return a non-null value even when the parent is a document or an attribute.

Comment 1

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

Attachment: Patch to fix

The biggest problem with this patch is that it adds an |if| check for each GetParent. However I can't think of a way around that and still keeping GetNodeParent as fast as GetParent.

Additionally, it doesn't make GetNodeParent return the attribute for children of attributes (since there is no way to pass that in to BindToTree). However that doesn't feel very important so I'll leave that for a separate bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

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

>+  nsIContent* GetParent() const
>+  {
>+    return mParentPtrBits & PARENT_BIT_PARENT_IS_CONTENT ?
>+           NS_REINTERPRET_CAST(nsIContent*,
>+                               mParentPtrBits & ~kParentBitMask) :
>+           nsnull;

Maybe more like:

if (NS_LIKELY(mParentPtrBits & PARENT_BIT_PARENT_IS_CONTENT)) {
  return ...;
}

return nsnull;

or do we not really think that this will be likely?  I guess I'm ok either way...

>+   * Get the parent nsIContent for this node.
>+   * @return the parent, or null if no parent or the parent is not an nsIContent

For GetNodeParent(), this comment looks wrong.

>Index: content/base/src/nsGenericDOMDataNode.cpp
>+  NS_PRECONDITION(!GetCurrentDoc() &&
>+                  !IsInDoc(), "Already have a document.  Unbind first!");

I'd put the "!IsInDoc()," part before the line break.

> nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
>   mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
>   if (aNullParent) {
>-    mParentPtrBits &= nsIContent::kParentBitMask;
>+    mParentPtrBits = 0;
>   }

Maybe we should do the &= thing only in the else (when !aNullParent)?

Same in nsGenericElement::UnbindFromTree and nsXULElement::UnbindFromTree?  This last needs to have this code tweaked, btw...

r+sr=bzbarsky with those nits.

Comment 3

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

> >+  nsIContent* GetParent() const
> >+  {
> >+    return mParentPtrBits & PARENT_BIT_PARENT_IS_CONTENT ?
> >+           NS_REINTERPRET_CAST(nsIContent*,
> >+                               mParentPtrBits & ~kParentBitMask) :
> >+           nsnull;
> 
> Maybe more like:
> 
> if (NS_LIKELY(mParentPtrBits & PARENT_BIT_PARENT_IS_CONTENT)) {
>   return ...;
> }
> 
> return nsnull;
> 
> or do we not really think that this will be likely?  I guess I'm ok either
> way...

I do think it is likely so I added the NS_LIKELY, however I think some compilers aren't too good at inline functions that contains multiple returns, so I kept the :?

> > nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
> >   mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
> >   if (aNullParent) {
> >-    mParentPtrBits &= nsIContent::kParentBitMask;
> >+    mParentPtrBits = 0;
> >   }
> 
> Maybe we should do the &= thing only in the else (when !aNullParent)?

Switched to a :? syntax

Comment 4

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

So this added 4-4.5k to buildsize. Ideally it wouldn't, but I don't think there's a way around it. And it should save save us in other places where we can simplify code.