Closed Bug 139344 Opened 22 years ago Closed 22 years ago

Implement DOM Level 3 compareTreePosition()

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: caillon, Assigned: caillon)

References

(
URL
)

Details

Attachments

(1 file, 7 obsolete files)

Patch v1.5
22.08 KB, patch
john
: review+
caillon
: superreview+
Details | Diff | Splinter Review 
This will be really useful.  Jst and I talked about this on IRC and I'll try to
tackle this sometime soon.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.1alpha
I've got this mostly working in my tree.  There are a few issues I need to iron
out as well as some cleanup, but I should have a patch here RSN.
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
This gets things working.  Comments and suggestions are very much appreciated!
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
Better patch.  Incorporates feedback from fabian and jst.
Attachment #82236 - Attachment is obsolete: true
Comment on attachment 82314 [details] [diff] [review]
Patch v1.1

- In nsDOMAttribute::IsSameNode():

+	   PRInt32 namespace_id;
+	   if (NS_SUCCEEDED(ni->GetNamespaceID(namespace_id)) &&
+	       namespace_id == kNameSpaceID_None) {
+	     ci = PR_TRUE;
+	   }

You can do the above with this oneliner:

  ci = ni->NamespaceEquals(kNameSpaceID_None);

- In nsGenericDOMDataNode, I don't see how any of those changes do anything for
us, nsGenericDOMDataNode gets its nsIDOM3Node methods from nsNode3Tearoff, so
you shouldn't need to make any changes to nsGenericDOMDataNode.

- In nsNode3Tearoff::CompareTreePosition():

+  nsCOMPtr<nsIDOMNode> node(do_QueryInterface(this));

You could QI on mContent in stead of QI'ing on this to avoid going through
nsNode3Tearoff::QI, you know this interface is not implemented by |this|, so
why bother... No big deal, either way does the right thing, one's a bit slower
though...

Also, nodeType in nsNode3Tearoff will never be DOMUMENT_NODE, so no need to
check that there...

- In nsNode3Tearoff::IsSameNode(), same thing, QI on mContent...

Other than that this looks good to me. I'd like to see these issues fixed
before sr'ing though.
Attachment #82314 - Flags: needs-work+
Attachment #82314 - Attachment is obsolete: true
Attached patch Patch v1.3 (obsolete) — — Splinter Review 
Fix a few errors I noticed in nsNode3Tearoff::CompareTreePosition

- When one node hits a document node before the other, keep traversing the
other node up the tree to test for ancestry.

- When tmpa doesn't have a parent, make sure we are at a document node before
giving it the following flag.  (Nodes in a document fragment don't
precede/follow other nodes)

- If the other node is a document fragment, swap the comparison like we do for
attributes.

- Also add a few extra comments for clarity.
Attachment #83006 - Attachment is obsolete: true
Attached patch Patch v1.31 (obsolete) — — Splinter Review 
Minor improvement.  Change two while loops to do...while loops since I know
their first iterations will always run.
Attachment #83028 - Attachment is obsolete: true
Attached file Testcase (obsolete) — 
This is the HTML testcase that I've been using, btw.  It's really not much but
maybe it will help a reviewer who applies the patch.  The one case we fail is
because of the fooAttr.childNodes[0].parentNode problem (bug 73681).
jkeiser, would you review this please?  :-)
Keywords: review
to the watchers of Keith, do we want to use this for XSLT?
If we want to use this in performance critical code we should optimize the
handling of nsNode3Tearoff in nsGenericElement n' friends (i.e. don't hit the
heap for every QI to nsIDOM3Node).
Comment on attachment 83035 [details] [diff] [review]
Patch v1.31

Not that this is necessarily a problem, but I don't see why the new DOM3
constants are in nsIDOMNode but the new methods are in nsIDOM3Node?  It seems
like they should be in the same place.

XMLDocument, HTMLDocument, XULDocument, SVGDocument

>Index: content/base/src/nsDOMAttribute.cpp
>===================================================================
>@@ -484,22 +485,112 @@
>+NS_IMETHODIMP
>+nsDOMAttribute::CompareTreePosition(nsIDOMNode* aOther,
>+                                    PRUint16* aReturn)
>+{
...
>+  PRUint16 nodeType = 0;
>+  aOther->GetNodeType(&nodeType);
>+  if (nodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+    nsCOMPtr<nsIDOMAttr> otherAttr(do_QueryInterface(aOther));
>+    nsCOMPtr<nsIDOMElement> otherEl;
>+    if (el) {
>+      otherAttr->GetOwnerElement(getter_AddRefs(otherEl));

This if (el) does not seem to be needed ... but if it is, the otherAttr and
otherEl definitions should go inside it.

>+
>+NS_IMETHODIMP
>+nsDOMAttribute::IsSameNode(nsIDOMNode* aOther,
>+                           PRBool* aReturn)
>+{
>+  PRBool sameNode = PR_FALSE;
>+
>+  // XXXcaa Ugly hack alert!!!
>+  // Comparing pointers on two attributes is not reliable.  See bug 93614.
>+  // For now, check owner elements and node names.

A stronger comment, like "just compare pointers once bug 93614 is fixed," might
be in order.

Also, this method is long and bunched together.  A couple of short comments
explaining parts of it ("compare owners", "compare attributes" and "check if
it's HTML and compare case insensitive if so" would probably suffice) would be
good.

>Index: content/base/src/nsDocument.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsDocument::IsSameNode(nsIDOMNode* aOther,
>+                       PRBool* aReturn)
>+{
>+  PRBool sameNode = PR_FALSE;
>+
>+  if (NS_STATIC_CAST(nsIDOMNode*, this) == aOther) {

It should be safe to just compare these with ==.  C++ does the cast
automatically.	Same in other places.

>Index: content/base/src/nsDocumentFragment.cpp
>===================================================================
>+
>+NS_IMETHODIMP
>+nsDocumentFragment::CompareTreePosition(nsIDOMNode* aOther,
>+                                        PRUint16* aReturn)
>+{
>+  NS_ENSURE_ARG_POINTER(aOther);
>+  PRUint32 mask = nsIDOMNode::TREE_POSITION_DISCONNECTED;
>+
>+  PRBool sameNode = PR_FALSE;
>+  IsSameNode(aOther, &sameNode);
>+  if (sameNode) {
>+    mask |= nsIDOMNode::TREE_POSITION_SAME_NODE;

You need to | in TREE_POSITION_EQUIVALENT here as well.

>+  }
>+  else {
>+    nsCOMPtr<nsIDOMNode> other(aOther);
>+    while (other) {
>+      IsSameNode(other, &sameNode);
>+      if (sameNode) {
>+        mask |= nsIDOMNode::TREE_POSITION_DESCENDANT;

Anywhere you do this you need FOLLOWING as well.  Also this entire block can be
safely moved to the end of the while().  You don't need to execute it the first
time since you already know it will fail.

>+        break;
>+      }
>+
>+      nsCOMPtr<nsIDOMNode> tmp(do_QueryInterface(other));

Just do tmp = other; here.  No need for do_QueryInterface.  (QueryInterface
should be avoided when possible, as it is slow.)

>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>+  // If the other node is an attribute or a document fragment,
>+  // we can figure out position easier by comparing this node
>+  // relative to the other node, and then reversing positions.

Clever.  Seems like this should apply to document nodes as well.


Stopping review for nsGenericElement::CompareTreePosition issue discussed in
IRC.
I just glanced at the patch, but
nsIContent::getParent is much cheaper than
nsIDOMNode::getParentNode, so it might be good to stick to nsIContent for the 
loops. If that's possible. (IIRC, attribute and document are the only nodes not
implementing nsIContent, not too sure about document fragments right now)
Attached patch Patch v1.4 (obsolete) — — Splinter Review 
Back to normal context now.  The only file that the huge context was needed for
was nsDOMClassInfo.cpp, but just go have a look at an older patch to see those
in full context (or look at the file itself via LXR or something ;-) )

This patch fixes issues so far, and also fixes an annoying compile warning I
get quite frequently in nsDocument.cpp about member variables being re-arranged
to match the declaration order.

>Not that this is necessarily a problem, but I don't see why the new DOM3
>constants are in nsIDOMNode but the new methods are in nsIDOM3Node?  It seems
>like they should be in the same place.

Yeah, I'd like for them to be in the same place.  However, that's done
intentionally.	We need the methods in nsIDOM3Node for binary compat, and the
constants in nsIDOMNode so they can actually be used by people expecting them
in nsIDOMNode, especially in JS.

>This if (el) does not seem to be needed ... but if it is, the otherAttr and
>otherEl definitions should go inside it.

Actually, yeah it is needed since GetOwnerElement can return null, and I don't
want to assign a mask if both are null.  However, it was placed wrongly (it
should have been the top level if statement).  Thats fixed now.

The changes in nsDocumentFragment.cpp wrt the flags aren't needed to be made. 
Items in a document fragment don't precede or follow one another per
conversation with jst.

Everything else should be addressed in this patch.
Attachment #83035 - Attachment is obsolete: true
Comment on attachment 83157 [details] [diff] [review]
Patch v1.4

>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>+  nsAutoVoidArray array1;
>+  nsAutoVoidArray array2;
>+  PRInt32 i = 0;
>+  PRInt32 j = 0;

Need new names. :)

>+      nsCOMPtr<nsIDOMNode> thisAncestor(
>+        do_QueryInterface(NS_STATIC_CAST(nsIDOMNode*, array1.ElementAt(i))));
>+
>+      nsCOMPtr<nsIDOMNode> otherAncestor(
>+        do_QueryInterface(NS_STATIC_CAST(nsIDOMNode*, array2.ElementAt(j))));
>+
>+      nsCOMPtr<nsIDOMNode> commonAncestor(
>+        do_QueryInterface(NS_STATIC_CAST(nsIDOMNode*, array1.ElementAt(++i))));
>+

There is no need to make these COMPtrs.  You're already relying on the weak
references in the array being correct, holding a strong reference here will do
nothing except add an unncessary AddRef and Release.

Also, please use i+1 instead of ++i; it's an unnecessary (and therefore
confusing) step.

>+
>+NS_IMETHODIMP
>+nsNode3Tearoff::IsSameNode(nsIDOMNode* aOther,
>+                           PRBool* aReturn)
>+{
>+  PRBool sameNode = PR_FALSE;
>+
>+  nsCOMPtr<nsISupports> node(do_QueryInterface(mContent));
>+  nsCOMPtr<nsISupports> other(do_QueryInterface(aOther));
>+
>+  if (node == other) {
>+    sameNode = PR_TRUE;
>+  }
>+
>+  *aReturn = sameNode;
>+  return NS_OK;
>+}

I don't see the need for the PRBool sameNode declaration here.

More importantly, fewer QueryInterfaces here would be better.  In fact, if this
is just a tearoff I think it's worth it to have an nsIDOMNode* mContentNode;
member in the tearoff.	Over multiple invocations that would allow *both*
QueryInterfaces to be avoided.	Regardless, you can get rid of one of the two
QueryInterfaces here by comparing using either nsIContent or nsIDOMNode.

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsHTMLDocument::CompareTreePosition(nsIDOMNode* aOther,
>+                                    PRUint16* aReturn)
>+{
>+  return nsDocument::CompareTreePosition(aOther, aReturn);
>+}
>+
>+NS_IMETHODIMP
>+nsHTMLDocument::IsSameNode(nsIDOMNode* aOther,
>+                           PRBool* aReturn)
>+{
>+  return nsDocument::IsSameNode(aOther, aReturn);
>+}
>+

Do you get compile errors if you don't declare these in nsHTMLDocument, or
something?  They both seem redundant since this is a virtual method and you're
just calling the superclass method.

r=jkeiser with these questions answered and changes done.  Excellent work, this
has some clever stuff in it :)
Attachment #83157 - Flags: review+
Blocks: 144014
Attached patch Patch v1.5 — — Splinter Review 
Hopefully the last patch :-)  Also the testcase was based on flawed logic. 
That's now obsolete too.  I'll upload my new testcase shortly.
Attachment #83037 - Attachment is obsolete: true
Attachment #83157 - Attachment is obsolete: true
Comment on attachment 83296 [details] [diff] [review]
Patch v1.5

r=jkeiser, great work!
Attachment #83296 - Flags: review+
Comment on attachment 83296 [details] [diff] [review]
Patch v1.5

Watch out, AFAICT, only SAME nodes don't have either
PRECEDING or FOLLOWING set

>? content/base/src/nsTreePosition.cpp
I didn't see you use that code, so it's probably ok. But scary, nevertheless.
;-)

>Index: dom/src/base/nsDOMClassInfo.cpp
>@@ -1862,6 +1865,7 @@
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMDocument)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSDocument)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNode)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Node)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMDocumentEvent)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMDocumentView)
That's nsSVGDocument, but I don't see that code in the patch :-(

>Index: content/base/src/nsDOMAttribute.cpp

>@@ -492,6 +493,104 @@
>   if (node)
>     rv = node->GetBaseURI(aURI);
>   return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMAttribute::CompareTreePosition(nsIDOMNode* aOther,
>+                                    PRUint16* aReturn)
>+{
>+  NS_ENSURE_ARG_POINTER(aOther);
>+  PRUint16 mask = nsIDOMNode::TREE_POSITION_DISCONNECTED;
>+
>+  nsCOMPtr<nsIDOMElement> el;
>+  GetOwnerElement(getter_AddRefs(el));
just do_QueryInterface(mContent)

<..>
>+      if (el == otherEl) {
>+        // same parent node, the two attributes have equivalent position
>+        mask |= nsIDOMNode::TREE_POSITION_EQUIVALENT;
>+        PRBool sameNode = PR_FALSE;
>+        IsSameNode(aOther, &sameNode);
you checked for same parent already, so you're doing that twice.
I'd either factor the code or duplicate it.

>+        if (sameNode) {
>+          mask |= nsIDOMNode::TREE_POSITION_SAME_NODE;
>+        }
>+      }
>+    }
>+    else {
>+      // The other node isn't an attribute.
>+      // Compare position relative to this attribute's owner element.
>+      nsCOMPtr<nsIDOM3Node> parent(do_QueryInterface(el));
>+      PRUint16 parentMask;
>+      parent->CompareTreePosition(aOther, &parentMask);
>+      if (parentMask & nsIDOMNode::TREE_POSITION_SAME_NODE) {
>+        mask |= nsIDOMNode::TREE_POSITION_PRECEDING;
I'd say DESCENDING, too.

>+      }
>+      else {
>+        mask |= parentMask & (nsIDOMNode::TREE_POSITION_FOLLOWING |
>+                              nsIDOMNode::TREE_POSITION_PRECEDING);
| nsIDOMNode::TREE_POSITION_DESCENDING as well.
>+      }
>+    }
<...>
>+  if (nsIDOMNode::ATTRIBUTE_NODE == otherType) {
>+    nsCOMPtr<nsIDOMElement> nodeOwner;
>+    GetOwnerElement(getter_AddRefs(nodeOwner));
>+    nsCOMPtr<nsIDOMAttr> other(do_QueryInterface(aOther));
>+    nsCOMPtr<nsIDOMElement> otherOwner;
>+    other->GetOwnerElement(getter_AddRefs(otherOwner));
>+    nsCOMPtr<nsIDOM3Node> owner(do_QueryInterface(nodeOwner));
>+    PRBool sameOwners = PR_FALSE;
>+    owner->IsSameNode(otherOwner, &sameOwners);
you're just doing a pointer compare above, do that here, too.

>Index: content/base/src/nsDocument.cpp
> 
>Index: content/base/src/nsDocumentFragment.cpp
>@@ -382,3 +384,62 @@
> 
>   return CallQueryInterface(newFragment, aReturn);
> }
>+
>+NS_IMETHODIMP
>+nsDocumentFragment::CompareTreePosition(nsIDOMNode* aOther,
>+                                        PRUint16* aReturn)
>+{
>+  NS_ENSURE_ARG_POINTER(aOther);
>+  PRUint32 mask = nsIDOMNode::TREE_POSITION_DISCONNECTED;
>+
>+  PRBool sameNode = PR_FALSE;
>+  IsSameNode(aOther, &sameNode);
>+  if (sameNode) {
>+    mask |= nsIDOMNode::TREE_POSITION_SAME_NODE;
>+  }
>+  else {
>+    nsCOMPtr<nsIDOMNode> other(aOther);
>+    while (other) {
>+      IsSameNode(other, &sameNode);
>+      if (sameNode) {
>+        mask |= nsIDOMNode::TREE_POSITION_DESCENDANT;
| nsIDOMNode::TREE_POSITION_FOLLOWING

>+        break;
>+      }
>+
>+      nsCOMPtr<nsIDOMNode> tmp(other);
>+      tmp->GetParentNode(getter_AddRefs(other));
>+      if (!other) {
>+        // No parent.  Check to see if we're at an attribute node.
>+        PRUint16 nodeType = 0;
>+        tmp->GetNodeType(&nodeType);
>+        if (nodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+          // If we are, let's get the owner element and continue up the tree
>+          nsCOMPtr<nsIDOMAttr> attr(do_QueryInterface(tmp));
>+          nsCOMPtr<nsIDOMElement> owner;
>+          attr->GetOwnerElement(getter_AddRefs(owner));
>+          other = do_QueryInterface(owner);
>+          continue;
>+        }
>+        break;
>+      }
How about catching the attribute case right after IsSameNode?
You should then be able to just use nsIContent for going up the tree, too.

>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.224
>diff -u -r3.224 nsGenericElement.cpp
>--- content/base/src/nsGenericElement.cpp	8 May 2002 20:48:19 -0000	3.224
>+++ content/base/src/nsGenericElement.cpp	13 May 2002 01:53:56 -0000
>@@ -247,6 +247,182 @@
> }
> 
> NS_IMETHODIMP
>+nsNode3Tearoff::CompareTreePosition(nsIDOMNode* aOther,
>+                                    PRUint16* aReturn)
>+{
>+  NS_ENSURE_ARG_POINTER(aOther);
>+  PRUint16 mask = nsIDOMNode::TREE_POSITION_DISCONNECTED;
>+
>+  nsCOMPtr<nsIDOMNode> node(do_QueryInterface(mContent));
>+
>+  PRBool sameNode = PR_FALSE;
>+  IsSameNode(aOther, &sameNode);
>+  if (sameNode) {
>+    mask |= nsIDOMNode::TREE_POSITION_SAME_NODE;
>+    nsCOMPtr<nsIDOMDocument> doc;
>+    node->GetOwnerDocument(getter_AddRefs(doc));
>+    // Loose nodes without an owner document are not equivalent
>+    // in tree position since they are not in any tree.
>+    // Add the 'equivalent' flag only if we have an owner document.
>+    if (doc) {
>+      mask |= nsIDOMNode::TREE_POSITION_EQUIVALENT;
>+    }
>+    *aReturn = mask;
>+    return NS_OK;
>+  }
>+
>+  // If the other node is an attribute, document, or document fragment,
>+  // we can find the position easier by comparing this node relative to
>+  // the other node, and then reversing positions.
>+  PRUint16 otherType = 0;
>+  aOther->GetNodeType(&otherType);
>+  if (otherType == nsIDOMNode::ATTRIBUTE_NODE ||
>+      otherType == nsIDOMNode::DOCUMENT_NODE  ||
>+      otherType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {
>+    PRUint16 otherMask = nsIDOMNode::TREE_POSITION_DISCONNECTED;
>+    nsCOMPtr<nsIDOM3Node> other(do_QueryInterface(aOther));
>+    other->CompareTreePosition(node, &otherMask);
>+    if (otherMask & nsIDOMNode::TREE_POSITION_FOLLOWING) {
>+      mask |= nsIDOMNode::TREE_POSITION_PRECEDING;
>+    } else if (otherMask & nsIDOMNode::TREE_POSITION_PRECEDING) {
>+      mask |= nsIDOMNode::TREE_POSITION_FOLLOWING;
>+    }
>+
>+    if (otherMask & nsIDOMNode::TREE_POSITION_ANCESTOR) {
>+      mask |= nsIDOMNode::TREE_POSITION_DESCENDANT;
>+    } else if (otherMask & nsIDOMNode::TREE_POSITION_DESCENDANT) {
>+      mask |= nsIDOMNode::TREE_POSITION_ANCESTOR;
>+    }

mask = otherMask;
if (mask) {
  mask ^= 0x03; // revert FOLLOWING and PRECEDING
  if (mask & 0x0C)
    mask ^= 0x0C; // if ANCESTOR or DESCENDANT, revert
}
should do, right?

<...>
>+  nsAutoVoidArray nodeAncestorList;
>+  nsAutoVoidArray otherAncestorList;
>+  PRInt32 nodeAncestorIdx = 0;
>+  PRInt32 otherAncestorIdx = 0;
>+
>+  nodeAncestorIdx  = nsRange::FillArrayWithAncestors(&nodeAncestorList, node);
>+  otherAncestorIdx = nsRange::FillArrayWithAncestors(&otherAncestorList, aOther);
>+
>+  NS_ASSERTION(nodeAncestorIdx >= 0 && otherAncestorIdx >= 0,
>+               "nsRange::FillArrayWithAncestors() failed!");
>+
>+  nsIDOMNode* nodeRoot =
>+    NS_STATIC_CAST(nsIDOMNode*, nodeAncestorList.ElementAt(nodeAncestorIdx));
>+
>+  nsIDOMNode* otherRoot =
>+    NS_STATIC_CAST(nsIDOMNode*, otherAncestorList.ElementAt(otherAncestorIdx));
>+
>+  if (nodeRoot == otherRoot) {
why not nodeRoot != otherRoot and return early?

<...>
>+    if (nodeAncestorIdx < 0) {
>+      // If we went below this node to find the common ancestor before
>+      // exiting, then this node must be the common ancestor.
>+      mask |= nsIDOMNode::TREE_POSITION_DESCENDANT;
>+      if (rootType == nsIDOMNode::DOCUMENT_NODE) {
>+        mask |= nsIDOMNode::TREE_POSITION_FOLLOWING;
>+      }
either FOLLOWING OR PRECEDING, here

>+    }
>+    else if (otherAncestorIdx < 0) {
>+      // If we went below the other node to find the common ancestor before
>+      // exiting, then the other node must be the common ancestor.
>+      mask |= nsIDOMNode::TREE_POSITION_ANCESTOR;
>+      if (rootType == nsIDOMNode::DOCUMENT_NODE) {
>+        mask |= nsIDOMNode::TREE_POSITION_PRECEDING;
>+      }
and here.

<...>
>+      commonAncestor->GetChildNodes(getter_AddRefs(children));
>+      PRUint32 numKids;
>+      children->GetLength(&numKids);
>+      for (PRUint32 i = 0; i < numKids; ++i) {
>+        // Then go through the children one at a time to see which we hit first.
>+        nsCOMPtr<nsIDOMNode> childNode;
>+        children->Item(i, getter_AddRefs(childNode));
>+        nsCOMPtr<nsIDOM3Node> child(do_QueryInterface(childNode));
>+        sameNode = PR_FALSE;
>+        child->IsSameNode(nodeAncestor, &sameNode);
just pointer compares here

>+        if (sameNode) {
>+          mask |= nsIDOMNode::TREE_POSITION_FOLLOWING;
>+          break;
>+        }
>+
>+        sameNode = PR_FALSE;
>+        child->IsSameNode(otherAncestor, &sameNode);
and here?

>+        if (sameNode) {
>+          mask |= nsIDOMNode::TREE_POSITION_PRECEDING;
>+          break;
>+        }
>+      }
>+    }
>+  }
>+
>+  *aReturn = mask;
>+  return NS_OK;
>+}
>+
> Watch out, AFAICT, only SAME nodes don't have either PRECEDING or FOLLOWING set

Attributes in the same element don't have it set.  All nodes below and including
a document fragment don't have it set, and disconnected nodes don't have it set.
That's how I have it implemented.

>? content/base/src/nsTreePosition.cpp
I didn't see you use that code, so it's probably ok. But scary, nevertheless.;-)

Scary how so?  I'm allowed to use a cpp file for initial designs, aren't I? :-)

>That's nsSVGDocument, but I don't see that code in the patch :-(

Thats because it isn't needed at this time.  The only purpose to my patching
nsDOMClassInfo is so I can call DOM3 methods on ns*Document objects.  SVG should
just call the superclass code.

>you checked for same parent already, so you're doing that twice. I'd either
factor the code or duplicate it.

Yes.  I'm duplicating it as you see.  Right now I'm checking for the same parent
twice.  This algorithm would work best if comparing pointers on attributes were
reliable.  Once that bug is fixed, the duplicate code will be gone.

> I'd say DESCENDING, too.

Unless I grossly misread the spec, I don't think the spec agrees with you.

> why not nodeRoot != otherRoot and return early?

Why?  :-)  All that does is adds a few extra lines of code just to prevent 2
spaces of indent.  Now we're getting a little picky :-)

>>+      if (rootType == nsIDOMNode::DOCUMENT_NODE) {
>>+        mask |= nsIDOMNode::TREE_POSITION_FOLLOWING;
>>+      }
>either FOLLOWING OR PRECEDING, here
<...>
>>+      if (rootType == nsIDOMNode::DOCUMENT_NODE) {
>>+        mask |= nsIDOMNode::TREE_POSITION_PRECEDING;
>>+      }
>and here.

Uhh????  Not sure what you're asking for...

I'll look at the other items.  In particular, do we always know that the pointer
compares will be correct where you ask me to do them?  I'll have a look.
Comment on attachment 83296 [details] [diff] [review]
Patch v1.5

<jst> caillon: yeah, sr=jst
Attachment #83296 - Flags: superreview+
Checked in with a few minor changes jst had me make.  Yay!  Start playing with
these now.  If there are any issues with either of the new methods, please file
a bug against me.  I'm already going to be doing some optimizations under 144014.

FIXED.  (why was this in DOM Style?)
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Component: DOM Style → DOM Core
Resolution: --- → FIXED
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: