159167 - (isEqualNode) Implement DOM3 isEqualNode()

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Implement isEqualNode().

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Initial Patch

The main reason I'm uploading this is because I don't want to lose this work :)

It compiles, and works mostly, but I haven't run any thorough testcases on all
different combinations of nodes.  It might need a little polish, but if anyone
has comments they are more than welcome to offer them.

Comment 2

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

Comment on attachment 94145 [details] [diff] [review]
Initial Patch

>@@ -61,6 +63,24 @@
>                                          nsIDocument *aOldDocument);
> 
>   static PRBool   IsCallerChrome();
>+
>+  static PRBool   NodeListsAreEqual(nsIDOMNodeList* aNodeList,
>+                                    nsIDOMNodeList* aOtherList);
>+  static PRBool   NamedNodeMapsAreEqual(nsIDOMNamedNodeMap* aNodeMap,
>+                                        nsIDOMNamedNodeMap* aOtherMap);

please add javadoc comments to these new functions

>+  /**
>+   * Checks whether its possible for two DOM nodes to be equal.
>+   * Note that this method returning PR_TRUE does not guarantee
>+   * the two nodes are equal.  Callers can (and should) check for
>+   * sameness before calling this method.
>+   *
>+   * @param aNode one of two nodes to compare
>+   * @param aOther second of two nodes to compare
>+   * @return PR_TRUE if the nodes might be equal, PR_FALSE if its
>+   *         impossible for the two nodes to be equal.
>+   */
>+  static PRBool   NodesCanBeEqual(nsIDOMNode* aNode,
>+                                  nsIDOMNode* aOther);

Exactly what does this function compare? It seems like the caller would need to
do a full equal-check even after calling this method?

>+// static
>+PRBool
>+nsContentUtils::NodeListsAreEqual(nsIDOMNodeList* aNodeList,
>+                                  nsIDOMNodeList* aOtherList)
>+{
>+  if (aNodeList == aOtherList) {
>+    return PR_TRUE;
>+  }
>+
>+  PRUint32 nodeListLength = 0;
>+  PRUint32 otherListLength = 0;
>+  aNodeList->GetLength(&nodeListLength);
>+  aOtherList->GetLength(&otherListLength);
>+  if (nodeListLength != otherListLength) {
>+    return PR_FALSE;
>+  }
>+
>+  if (nodeListLength == 0) {
>+    return PR_TRUE;
>+  }

This check doesn't buy you anything, you'll check for that case in the first
cycle of the loop below anyway.

>+  for (PRUint32 i = 0; i < nodeListLength; ++i) {
>+    nsCOMPtr<nsIDOMNode> node;
>+    nsCOMPtr<nsIDOMNode> other;
>+    aNodeList->Item(i, getter_AddRefs(node));
>+    aOtherList->Item(i, getter_AddRefs(other));
>+    nsCOMPtr<nsIDOM3Node> dom3Node(do_QueryInterface(node));

Check that this QI succeeds

>+// static
>+PRBool
>+nsContentUtils::NamedNodeMapsAreEqual(nsIDOMNamedNodeMap* aNodeMap,
>+                                      nsIDOMNamedNodeMap* aOtherMap)
>+{
>+  if (aNodeMap == aOtherMap) {
>+    return PR_TRUE;
>+  }
>+
>+  nsCOMPtr<nsIContent> nodeMapContent(do_QueryInterface(aNodeMap));
>+  nsCOMPtr<nsIContent> otherMapContent(do_QueryInterface(aOtherMap));

Err.. this won't work, we don't have any NamedNodeMaps that implement
nsIContent.

>+
>+  PRInt32 nodeMapLength = 0;
>+  PRInt32 otherMapLength = 0;
>+  nodeMapContent->ChildCount(nodeMapLength);
>+  otherMapContent->ChildCount(otherMapLength);
>+  if (nodeMapLength != otherMapLength) {
>+    return PR_FALSE;
>+  }
>+
>+  if (nodeMapLength == 0) {
>+    return PR_TRUE;
>+  }
>+
>+  // XXX this algorithm is O(n^2) optimized for the case when attributes
>+  // are in the same order.  We probably need a better algorithm...
>+  for (PRInt32 i = 0; i < nodeMapLength; ++i) {
>+    nsIContent *nodeAttr;
>+    nsIContent *otherAttr;
>+    nodeMapContent->ChildAt(i, nodeAttr);
>+    otherMapContent->ChildAt(i, otherAttr);

This will give you the children of the nsIContent, not the attributes. Back to
the drawingboard dude ;-)

>+// static
>+PRBool
>+nsContentUtils::NodesCanBeEqual(nsIDOMNode* aNode,
>+                                nsIDOMNode* aOther)
>+{
>+  NS_ENSURE_TRUE(aNode, PR_FALSE);
>+  NS_ENSURE_TRUE(aOther, PR_FALSE);
>+
>+  // First, check node types.
>+  PRUint16 nodeType = 0;
>+  PRUint16 otherType = 0;
>+  aNode->GetNodeType(&nodeType);
>+  aOther->GetNodeType(&otherType);
>+  if (nodeType != otherType) {
>+    return PR_FALSE;
>+  }
>+
>+  // Then, compare NodeInfos.

Only compare nodeinfos for stuff that actually have nodeinfos, i.e. elements.
Since attributes don't implement nsIContent you can't get the nodeinfo out of
them :(

>+  nsCOMPtr<nsIContent> nodeContent(do_QueryInterface(aNode));
>+  nsCOMPtr<nsIContent> otherContent(do_QueryInterface(aOther));
>+  nsCOMPtr<nsINodeInfo> nodeInfo;
>+  nsCOMPtr<nsINodeInfo> otherInfo;
>+  if (nodeContent) {
>+    nodeContent->GetNodeInfo(*getter_AddRefs(nodeInfo));
>+  }
>+  if (otherContent) {
>+    otherContent->GetNodeInfo(*getter_AddRefs(otherInfo));
>+  }
>+  if (!nodeInfo->Equals(otherInfo)) {

This is only safe for nodes that actually have nodeinfos.

>+    return PR_FALSE;
>+  }
>+
>+  // Now check the nodeValue

Only do this for stuff that has nodevalues.

>+  nsAutoString nodeString;
>+  nsAutoString otherString;
>+  aNode->GetNodeValue(nodeString);
>+  aOther->GetNodeValue(otherString);
>+  if (!nodeString.Equals(otherString)) {
>+    return PR_FALSE;
>+  }
>+
>+  // Next, check our childNodes.  Normalize first.
>+  aNode->Normalize();
>+  aOther->Normalize();

Waah! you can't modify the nodes!! This is the responsibility of the user.

>+NS_IMETHODIMP
>+nsDOMAttribute::IsEqualNode(nsIDOMNode* aOther,
>+                            PRBool* aReturn)
>+{
>+  *aReturn = PR_FALSE;
>+  NS_ENSURE_ARG_POINTER(aOther);
>+
>+  PRBool sameNode = PR_FALSE;
>+  IsSameNode(aOther, &sameNode);
>+  if (sameNode) {
>+    *aReturn = PR_TRUE;
>+    return NS_OK;
>+  }
>+
>+  if (!nsContentUtils::NodesCanBeEqual(this, aOther)) {
>+    return NS_OK;
>+  }
>+
>+  *aReturn = PR_TRUE;
>+  return NS_OK;
>+}

What about the baseURI?


>Index: content/base/src/nsDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v
>retrieving revision 3.387
>diff -u -r3.387 nsDocument.cpp
>--- content/base/src/nsDocument.cpp	23 Jul 2002 22:16:36 -0000	3.387
>+++ content/base/src/nsDocument.cpp	6 Aug 2002 08:27:42 -0000
>@@ -3443,6 +3443,64 @@
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsDocument::IsEqualNode(nsIDOMNode* aOther,
>+                        PRBool* aReturn)
>+{
...
>+  // Next check the namespaceURI
...
>+  // Next check the prefix

documents don't have namespaceURIs or prefixes. And why not use
nsContentUtils::NodesCanBeEqual?

>+NS_IMETHODIMP
>+nsDocumentFragment::IsEqualNode(nsIDOMNode* aOther,
>+                                PRBool* aReturn)
>+{
>+  *aReturn = PR_FALSE;
>+  NS_ENSURE_ARG_POINTER(aOther);
>+
>+  if (this == aOther) {
>+    *aReturn = PR_TRUE;
>+    return NS_OK;
>+  }
>+
>+  PRUint16 otherNodeType = 0;
>+  aOther->GetNodeType(&otherNodeType);
>+  if (otherNodeType != nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {
>+    return NS_OK;
>+  }
>+
>+  Normalize();
>+  aOther->Normalize();

Waahh!!

>+
>+  *aReturn = PR_TRUE;
>+  return NS_OK;
>+}

you need to check that at least the child nodelists are equal.

>Index: content/base/src/nsGenericDOMDataNode.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericDOMDataNode.cpp,v
>retrieving revision 3.96
>diff -u -r3.96 nsGenericDOMDataNode.cpp
>--- content/base/src/nsGenericDOMDataNode.cpp	10 May 2002 18:21:50 -0000	3.96
>+++ content/base/src/nsGenericDOMDataNode.cpp	6 Aug 2002 08:27:42 -0000
>@@ -385,6 +385,40 @@
> }
> 
> nsresult
>+nsGenericDOMDataNode::IsEqualNode(nsIDOMNode* aOther,
>+                                  PRBool* aReturn)

Please rename this function since it doesn't do a full equality check,
(nodetype check is missing)

>+{
>+  *aReturn = PR_FALSE;
>+
>+  if (this == aOther) {

Hmm.. does this compile? I didn't think nsGenericDOMDataNode inherited
nsIDOMNode.

>+    *aReturn = PR_TRUE;
>+    return NS_OK;
>+  }
>+
>+  // Compare nodeValue attributes
>+  nsAutoString nodeString;
>+  nsAutoString otherString;
>+  GetNodeValue(nodeString);
>+  aOther->GetNodeValue(otherString);
>+  if (!nodeString.Equals(otherString)) {
>+    return NS_OK;
>+  }

Just compare it to mText instead of calling GetNodeValue on yourself

>+  // Compare baseURI attributes
>+  nodeString.Truncate();
>+  otherString.Truncate();

No need to truncate before calling a getter (you do this in a few more places
iirc)

>+  GetBaseURI(nodeString);
>+  nsCOMPtr<nsIDOM3Node> other(do_QueryInterface(aOther));
>+  other->GetBaseURI(otherString);
>+  if (!nodeString.Equals(otherString)) {
>+    return NS_OK;
>+  }
>+
>+  *aReturn = PR_TRUE;
>+  return NS_OK;
>+}



>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.233
>diff -u -r3.233 nsGenericElement.cpp
>--- content/base/src/nsGenericElement.cpp	26 Jul 2002 16:45:45 -0000	3.233
>+++ content/base/src/nsGenericElement.cpp	6 Aug 2002 08:27:42 -0000
>@@ -400,6 +400,21 @@
> }
> 
> NS_IMETHODIMP
>+nsNode3Tearoff::IsEqualNode(nsIDOMNode* aOther,
>+                            PRBool* aReturn)
>+{
>+  nsCOMPtr<nsIDOMNode> node(do_QueryInterface(this));
>+  if (node == aOther) {
>+    *aReturn = PR_TRUE;
>+  }
>+  else {
>+    *aReturn = nsContentUtils::NodesCanBeEqual(node, aOther);
>+  }
>+
>+  return NS_OK;
>+}

BaseURI check is missing

> NS_IMETHODIMP    
> nsHTMLDocument::LookupNamespacePrefix(const nsAString& aNamespaceURI,
>Index: content/xml/content/src/nsXMLProcessingInstruction.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xml/content/src/nsXMLProcessingInstruction.cpp,v
>retrieving revision 1.42
>diff -u -r1.42 nsXMLProcessingInstruction.cpp
>--- content/xml/content/src/nsXMLProcessingInstruction.cpp	5 Apr 2002 11:29:34 -0000	1.42
>+++ content/xml/content/src/nsXMLProcessingInstruction.cpp	6 Aug 2002 08:27:44 -0000
>@@ -147,6 +147,7 @@
>   NS_IMETHOD GetBaseURI(nsAString& aURI) {
>     return nsGenericDOMDataNode::GetBaseURI(aURI);
>   }
>+  NS_IMETHOD IsEqualNode(nsIDOMNode* aOther, PRBool* aReturn);
>   NS_IMETHOD LookupNamespacePrefix(const nsAString& aNamespaceURI,
>                                    nsAString& aPrefix) {
>     return nsGenericDOMDataNode::LookupNamespacePrefix(aNamespaceURI, aPrefix);
>@@ -309,6 +310,29 @@
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXMLProcessingInstruction::IsEqualNode(nsIDOMNode* aOther,
>+                                        PRBool* aReturn)
>+{
>+  *aReturn = PR_FALSE;
>+  NS_ENSURE_ARG_POINTER(aOther);
>+
>+  PRBool canBeEqual = PR_FALSE;
>+  nsGenericDOMDataNode::IsEqualNode(aOther, &canBeEqual);
>+  if (!canBeEqual) {
>+    return NS_OK;
>+  }
>+
>+  nsAutoString domString;
>+  aOther->GetNodeName(domString);
>+  if (!domString.Equals(mTarget)) {
>+    return NS_OK;
>+  }
>+
>+  *aReturn = PR_TRUE;
>+  return NS_OK;
>+}

You need to check that the other node is a PI too.

Comment 3

[Alex Vincent [:WeirdAl]]

The current code has NS_NOTYETIMPLEMENTED("nsNode3Tearoff::IsEqualNode()") in
place.  This means that when assertions go fatal, we will crash here.

caillon, sicking: is this patch still current?  If it is, I can try to apply
sicking's suggested changes to caillon's patch and generate a new patch.

In any case, I dispute the severity: enh value.  :)

Comment 4

[Hixie (not reading bugmail)]

Uh, surely for things that are not implemented we should just throw an
exception, not abort processing. Assertions are for things that cannot happen,
author script calling unimplemented methods is obviously not an impossible
situation.

Comment 5

[Alex Vincent [:WeirdAl]]

caillon's original patch has (unsurprisingly) bitrotted quite badly.

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: Some notes between sicking and myself (as of June 2, 2005)

sicking, anything that should be updated from these notes?

Comment 7

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

The extra thing i was referring to was that cdata nodes and text nodes had the same Tag() at the time. That is not currently the case though, but I would like to change that again I think.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: Remove assertions that user can fire

The user can call IsEqualNode(), and cause these assertions to fire.  Very Bad.

Comment 9

[Alex Vincent [:WeirdAl]]

Accepting this bug.  It's sat for far too long, and I do want to implement this.

Comment 10

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

Actually, even better than this is to remove these functions entierly, or at the very least mark them as unscriptable in the idl

Comment 11

[Alex Vincent [:WeirdAl]]

Updating URI to W3C Recommendation.

Comment 12

[Alex Vincent [:WeirdAl]]

qawanted:  I need some test cases for isEqualNode() support.  I have a first-draft patch which I would like to exhaustively test.

Comment 13

[Alex Vincent [:WeirdAl]]

Attachment: raw data test case

This testcase isn't pretty, but it generates a fair bit of raw data to parse.  In particular, it's worth noting row 6, column 8 and row 24, column 51.  The former is a pair of whitespace-only text nodes, and the latter refers to a copied svg:svg element.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Jonas, if you want to r- this patch, feel free to r/sr the removal-of-assertions patch.  The two conflict, obviously, but this patch actually implements the isEqualNode algorithm to the best of my knowledge.

Comment 15

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

Comment on attachment 222988 [details] [diff] [review]
patch, v1

>Index: content/base/public/nsContentUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsContentUtils.h,v
>retrieving revision 1.95
>diff -p -u -8 -r1.95 nsContentUtils.h
>--- content/base/public/nsContentUtils.h	19 May 2006 10:01:21 -0000	1.95
>+++ content/base/public/nsContentUtils.h	23 May 2006 03:33:12 -0000
>@@ -203,16 +203,41 @@ public:
>    *
>    * @see nsIDOMNode
>    * @see nsIDOM3Node
>    */
>   static PRUint16 ComparePosition(nsINode* aNode1,
>                                   nsINode* aNode2);
> 
>   /**
>+   * Determines whether two named node maps are equal.
>+   *
>+   * @param aMap1 The first map to compare.
>+   * @param aMap2 The second map to compare.
>+   * @param isEqual Whether the maps are equal.
>+   */
>+  static nsresult AreNamedMapsEqual(nsIDOMNamedNodeMap* aMap1,
>+                                    nsIDOMNamedNodeMap* aMap2,
>+                                    PRBool * isEqual);

Why create a generic named-map comparison function when we only have a single map, attributes. Just write specific code to compare attributes of two elements instead.

>+  /**
>+   * Determines whether two nodes are equal.
>+   *
>+   * @param aDOMNode1       The first node to compare.
>+   * @param aDOMNode2       The second node to compare.
>+   * @param checkContainers Check if aDOMNode1 contains or descends from
>+   *                        aDOMNode2.
>+   * @param isEqual         Whether the nodes are equal.
>+   */
>+  static nsresult AreNodesEqual(nsIDOMNode* aNode1,
>+                                nsIDOMNode* aNode2,
>+                                PRBool checkDescendants = PR_TRUE,
>+                                PRBool * isEqual = PR_FALSE);

Make this take two nsINodes instead.

PR_FALSE is *not* a valid pointer value (though it happens to compile).

Make the function return a PRBool.

>Index: content/base/public/nsINode.h
> 
>+  virtual PRBool nodeInfoMatches(nsINode* aOther)
>+  {
>+    return mNodeInfo->Equals(aOther->mNodeInfo);
>+  };
>+

I don't like this function since it will return true if you compare an attribute named 'foo' with an element named 'foo'. This is why I didn't put access to mNodeInfo on nsINode in the first place.

You're going to have to check the type anyway, so you might as well cast to nsIContent/nsIAttribute as needed.

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

>+nsContentUtils::AreNamedMapsEqual(nsIDOMNamedNodeMap* aMap1,
>+                                  nsIDOMNamedNodeMap* aMap2,
>+                                  PRBool * isEqual)
>+{
>+  // This function allows arguments to be null... to a point.
>+  if (!aMap1 && !aMap2)
>+  {
>+    *isEqual = PR_TRUE;

Err.. using the default value for isEqual (null) here is going to make this crash.

Put { on the same line as the |if|.

Anyhow, this function shouldn't exist so I won't review it any further.


>+nsresult
>+nsContentUtils::AreNodesEqual(nsIDOMNode* aDOMNode1,

Write this function using nsINode type interfaces instead (nsINode, nsIContent, nsIDocument). That will be much faster and probably cleaner too.

>+                              nsIDOMNode* aDOMNode2,
>+                              PRBool checkContainers,
>+                              PRBool * isEqual)
>+{
>+  NS_ASSERTION(aDOMNode1, "Who called AreNodesEqual?");
>+  if (!aDOMNode1 || !aDOMNode2)
>+  {
>+    isEqual = PR_FALSE;
>+    return NS_OK;
>+  }

Don't assert on null and then deal with it. If null is allowed don't assert, if it isn't then don't check for it.

>+  // Elements: Attributes check.

Write specific code that uses nsIContent here instead, that'll be a ton faster.


>+    // Entities
...
>+    // Notations

We don't support entities or notations so no need for this.

>+  // Ancestors check:  Make sure one node doesn't contain the other.  This
>+  // saves time on iterating through children.
>+  if (checkContainers &&
>+       (ContentIsDescendantOf(aNode1, aNode2) ||
>+        ContentIsDescendantOf(aNode2, aNode1)))

Why this check? Surely this function will work even without it, so no need to optimize for this specific case.

Comment 16

[Alex Vincent [:WeirdAl]]

(In reply to comment #15)
> Why create a generic named-map comparison function when we only have a single
> map, attributes. Just write specific code to compare attributes of two elements
> instead.

Ironically, that's what I wanted to do.  But not knowing any better about how we do entities and notations for doctypes, I figured a more generic approach would be better.

> Why this check? Surely this function will work even without it, so no need to
> optimize for this specific case.

It will, but if A is a very distant ancestor, and B is just similar enough, I see a perf hit here.  Besides, this check is done ONLY on a very specific condition (from nsGenericElement, not in recursive checks), so it's a one-time deal.  But you want it gone, it's gone.

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2

I've answered the review comments, plus a couple conversations with sicking, in this patch.

Comment 18

[Alex Vincent [:WeirdAl]]

Comment on attachment 224969 [details] [diff] [review]
patch, v2

>Index: content/base/src/nsGenericElement.h
>+  /**
>+   * Determines whether two nodes are equal.
>+   *
>+   * @param aContent1 The first node to compare.
>+   * @param aContent2 The second node to compare.
>+   * @param isEqual   Whether the nodes are equal.
>+   */
>+  static PRBool AreNodesEqual(nsIContent* aContent1,
>+                              nsIContent* aContent2);

Forgot to update the javadoc for this function - oops.

Comment 19

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

Comment on attachment 224969 [details] [diff] [review]
patch, v2

> nsDOMAttribute::IsEqualNode(nsIDOMNode* aOther,
>                             PRBool* aReturn)
> {

>+  // Same node check.
>+  if (this == aOther)
>+  {
>+    *aReturn = PR_TRUE;
>+    return NS_OK;
>+  }

I don't think these checks are worth the extra lines of code. These is no reason to optimize these functions for speed. Same applies in all other implementations.

>+  // Node type check by QI.  We also reuse this later.
>+  nsCOMPtr<nsIAttribute> aOtherAttr = do_QueryInterface(aOther);
>+  if (!aOtherAttr)
>+  {

Follow whitespace conventions please (i.e. put the '{' on the same line as the 'if'). This applies to a lot of other places in the bug.

>+  // Value check
>+  nsAutoString otherValue;
>+  nsAutoString ourValue;

These two can be put on the same line.

>+  nsresult rv = GetValue(ourValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = aOther->GetNodeValue(otherValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!ourValue.Equals(otherValue))
>+  {
>+    *aReturn = PR_FALSE;

It might be worth setting this at the top of the function and change it again at the bottom. That should save a few lines of code. Same probably applies in other implementations.

> nsDocument::IsEqualNode(nsIDOMNode* aOther, PRBool* aReturn)

>+  for (PRUint32 i = 0; i < childCount; i++)
>+  {
>+    nsIContent* aChild1 = GetChildAt(i);
>+    nsIContent* aChild2 = aOtherDoc->GetChildAt(i);
>+    *aReturn = nsNode3Tearoff::AreNodesEqual(aChild1, aChild2);
>+    if (!aReturn)

That should be !*aReturn. But even better would be to set *aReturn to false at the top of the function and just do |if (!nsNode3Tearoff::AreNodesEqual...| here

>+nsNode3Tearoff::AreNodesEqual(nsIContent* aContent1,
>+                              nsIContent* aContent2)
>+{

This function is missing the extra checks that doctypes need. I havn't checked if there is anything else missing from the spec.

>+  // We use nsIContent instead of nsINode for the attributes of elements.
>+
>+  NS_PRECONDITION(aContent1, "Who called AreNodesEqual?");
>+  NS_PRECONDITION(aContent2, "Who called AreNodesEqual?");
>+  NS_PRECONDITION(aContent1 != aContent2, "Someone didn't check for same nodes!");

Remember to remove this precondition when you remove the equality checks elsewhere.

>+  // Node type check.  nsINode can't distinguish between CDATA and text.
>+  nsCOMPtr<nsIDOMNode> aDOMNode1 = do_QueryInterface(aContent1);
>+  nsCOMPtr<nsIDOMNode> aDOMNode2 = do_QueryInterface(aContent2);
>+  NS_ASSERTION(aDOMNode1 && aDOMNode2, "How'd we get nsIContent without nsIDOMNode?");
>+  PRUint16 nodeType1 = 0;
>+  PRUint16 nodeType2 = 0;
>+  aDOMNode1->GetNodeType(&nodeType1);
>+  aDOMNode2->GetNodeType(&nodeType2);
>+  if (nodeType1 != nodeType2)
>+  {
>+    return PR_FALSE;
>+  }

Why is this needed? What will fail if it's not here? Also, never name local variables aSomething, that makes them look like arguments.

>+  // Node value check.
>+  nsAutoString string1;
>+  nsAutoString string2;

Put on the same line.

>+  aDOMNode1->GetNodeValue(string1);
>+  aDOMNode2->GetNodeValue(string2);

Once you get rid of the QIs above, you only should avoid doing this for elements to save the QIs (IsNodeOfType is a fast way to check for elements).

>+  // Child nodes count.
>+  PRUint32 childCount = aContent1->GetChildCount();
>+  if (childCount != aContent2->GetChildCount())
>+  {
>+    return PR_FALSE;
>+  }

Nit: Move this down to the other child-related checks.

>+  // Iterate over attributes.
>+  for (PRUint32 i = 0; i < attrCount; ++i)
>+  {
>+    const nsAttrName* attrName1 = aContent1->GetAttrNameAt(i);
>+    PRBool hasAttr = aContent1->GetAttr(attrName1->NamespaceID(),
>+                                        attrName1->LocalName(),
>+                                        string1);
>+    NS_ASSERTION(hasAttr, "Why don't we have an attr?");

This will produce a warning in optimized builds since the hasAttr variable will be unused.

>+    if (!aContent2->AttrValueIs(attrName1->NamespaceID(),
>+                                attrName1->LocalName(),
>+                                string1,
>+                                eIgnoreCase))

eIgnoreCase seems wrong.

>+  // Iterate over child nodes.
>+  for (PRUint32 i = 0; i < childCount; ++i)
>+  {
>+    nsIContent* child1 = aContent1->GetChildAt(i);
>+    nsIContent* child2 = aContent2->GetChildAt(i);
>+    PRBool isEqual = AreNodesEqual(child1, child2);
>+    if (!isEqual)

Remove the temporary isEqual.

>Index: content/base/src/nsGenericElement.h
>+  /**
>+   * Determines whether two nodes are equal.
>+   *
>+   * @param aContent1 The first node to compare.
>+   * @param aContent2 The second node to compare.
>+   * @param isEqual   Whether the nodes are equal.

This param doesn't exist.


Still some stuff left to do, but getting better.

Comment 20

[Alex Vincent [:WeirdAl]]

(In reply to comment #19)
> >+  // Node type check.  nsINode can't distinguish between CDATA and text.
> >+  nsCOMPtr<nsIDOMNode> aDOMNode1 = do_QueryInterface(aContent1);
> >+  nsCOMPtr<nsIDOMNode> aDOMNode2 = do_QueryInterface(aContent2);
> >+  NS_ASSERTION(aDOMNode1 && aDOMNode2, "How'd we get nsIContent without nsIDOMNode?");
> >+  PRUint16 nodeType1 = 0;
> >+  PRUint16 nodeType2 = 0;
> >+  aDOMNode1->GetNodeType(&nodeType1);
> >+  aDOMNode2->GetNodeType(&nodeType2);
> >+  if (nodeType1 != nodeType2)
> >+  {
> >+    return PR_FALSE;
> >+  }
> 
> Why is this needed? What will fail if it's not here?

PRBool
nsXMLCDATASection::IsNodeOfType(PRUint32 aFlags) const
{
  return !(aFlags & ~(eCONTENT | eTEXT));
}

PRBool
nsTextNode::IsNodeOfType(PRUint32 aFlags) const
{
  return !(aFlags & ~(eCONTENT | eTEXT));
}

What am I not seeing here?  nsINode bit-flags don't cover every node type exclusively.  From the spec:

"Two nodes are equal if and only if the following conditions are satisfied:

    * The two nodes are of the same type."

The rest of this I have no problem with.

Comment 21

[Alex Vincent [:WeirdAl]]

Answering my own question:  CDATA section nodes have a different node name than text nodes, so that test would fail.

Comment 22

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3

Okay, I think I got it now.

Comment 23

[Alex Vincent [:WeirdAl]]

Comment on attachment 225066 [details] [diff] [review]
patch, v3

Whoops.  My document type check was in the wrong spot.

Comment 24

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3.1

Crash fixed.

Comment 25

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

Comment on attachment 225081 [details] [diff] [review]
patch, v3.1

>Index: content/base/src/nsGenericElement.cpp
>+nsNode3Tearoff::AreNodesEqual(nsIContent* aContent1,
>+                              nsIContent* aContent2)
>+{
>+  // We use nsIContent instead of nsINode for the attributes of elements.
>+
>+  NS_PRECONDITION(aContent1, "Who called AreNodesEqual?");
>+  NS_PRECONDITION(aContent2, "Who called AreNodesEqual?");

Combine these to a single NS_PRECONDITION

>+  if (aContent1->Tag() == nsLayoutAtoms::documentTypeNodeName) {
>+    nsCOMPtr<nsIDOMDocumentType> docType1 = do_QueryInterface(aContent1);
>+    nsCOMPtr<nsIDOMDocumentType> docType2 = do_QueryInterface(aContent2);
>+
>+    NS_ASSERTION(docType1, "Why don't we have a document type node?");
>+    NS_ASSERTION(docType2, "Why don't we have a document type node?");

Combine to a single NS_ASSERTION

>+  if (aContent1->IsNodeOfType(nsINode::eELEMENT)) {
>+    // aContent1 is an element.  Do the check on attributes.
>+    PRUint32 attrCount = aContent1->GetAttrCount();
>+    if (attrCount != aContent2->GetAttrCount()) {
>+      return PR_FALSE;
>+    }
>+
>+    // Iterate over attributes.
>+    for (PRUint32 i = 0; i < attrCount; ++i) {
>+      const nsAttrName* attrName1 = aContent1->GetAttrNameAt(i);
>+#ifdef DEBUG
>+      PRBool hasAttr =
>+#endif
>+      aContent1->GetAttr(attrName1->NamespaceID(),
>+                         attrName1->LocalName(),
>+                         string1);
>+#ifdef DEBUG
>+      NS_ASSERTION(hasAttr, "Why don't we have an attr?");
>+#endif

No need to #ifdef DEBUG an assertion, it's #ifdef DEBUG as is.

>+      if (!aContent2->AttrValueIs(attrName1->NamespaceID(),
>+                                  attrName1->LocalName(),
>+                                  string1,
>+                                  eCaseMatters)) {
>+        return PR_FALSE;
>+      }
>+    }
>+  } else {
>+    // aContent1 is not an element.  Node value check.
>+    nsCOMPtr<nsIDOMNode> DOMNode1 = do_QueryInterface(aContent1);
>+    nsCOMPtr<nsIDOMNode> DOMNode2 = do_QueryInterface(aContent2);

Please follow convetion and use camelCase for local variables, e.g. domNode1.

r+sr=me with this fixed

Please attach a new version of the patch though.

Comment 26

[Alex Vincent [:WeirdAl]]

Attachment: final patch, answering review comments

Need check-in please.

Comment 27

[Jeff Walden [:Waldo]]

Patch checked in on trunk.