332148 - extractContents clones nodes when it should just cut them

Status: RESOLVED FIXED

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

Description

[Alex Vincent [:WeirdAl]]

From nsRange.cpp:
// XXX_kin: The spec says that nodes that are completely in the
// XXX_kin: range should be moved into the document fragment, not
// XXX_kin: copied. This method will have to be rewritten using
// XXX_kin: DeleteContents() as a template, with the charData cloning
// XXX_kin: code from CloneContents() merged in.

Testcase coming up.

I'd propose actually creating a new private function (static?) called Cut(nsIDOMDocumentFragment* frag).  DeleteContents would call Cut(nsnull).  ExtractContents would call Cut(*retval).  Cut would largely be the current DeleteContents with the necessary CloneContents code inserted, predicated on if (frag).

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

<sicking> those methods should be rewritten from scratch, they are way ineffecitent and buggy right now
<sicking> so i'd rather not see all of DeleteContents cloned
<sicking> actually, the way you're proposing might not be a bad idea
<sicking> depends on if the result would be too messy or not
<WeirdAl> I'd rather not do any big code rewrites :)

Comment 3

[Alex Vincent [:WeirdAl]]

So I'm looking into this, and I'm bumping into one little problem.  nsCommentNode inherits from nsGenericDOMDataNode, which has this nice SplitText() method from nsIDOMText... but nsCommentNode's QueryInterface doesn't mention nsIDOMText.  This is correct per DOM Level 3 Core, but is also rather inconvenient for me, since it's a great way to break up a data node.

So if I can get a nsIDOMText pointer to point to the comment node, I'd be good.  This is for the case when a comment node is the start parent or end parent of a range.

Comment 4

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

nsCommentNode can use SplitText just fine, because it inherits 
nsGenericDOMDataNode. nsTextNode forward the SplitText calls to nsGenericDOMDataNode, see NS_FORWARD_NSIDOMTEXT(nsGenericDOMDataNode::)

Comment 5

[Alex Vincent [:WeirdAl]]

Smaug: actually, now that I've looked, I think it won't fly, and my own approach wouldn't either.  Here's why:

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericDOMDataNode.h#309 defines a return of nsIDOMText**, which as I said you can't QI nsCommentNode to.

nsGenericDOMDataNode::SplitText does all the right things until the end... when http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericDOMDataNode.cpp#912 strikes:

return CallQueryInterface(newContent, aReturn);

A simple solution would be to overload nsGenericDOMDataNode::SplitText so it has a near-identical method with a second argument of nsGenericDOMDataNode** type.  The current version would then reduce down to calling the newer version and probably re-QI'ing it (or recasting, I guess, but I don't know what I'm talking about here).

Comment 6

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

(In reply to comment #5)
> A simple solution would be to overload nsGenericDOMDataNode::SplitText so it
> has a near-identical method with a second argument of nsGenericDOMDataNode**
> type.  The current version would then reduce down to calling the newer version
> and probably re-QI'ing it (or recasting, I guess, but I don't know what I'm
> talking about here).
> 
Yes, you could just change the current ::SplitText to have nsIContent** as the 
last parameter and then create a new SplitText which would call the original
SplitText and CallQueryInterface (if nsIContent object returned isn't nsnull)
to nsIDOMText*.

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: work in progress

I can't ask for reviews because this patch won't compile:

gkconbase_s.lib(nsRange.obj) : error LNK2019: unresolved external symbol "public: unsigned int __thiscall nsIComment::SplitComment(unsigned int,class nsIComment * *)" (?SplitComment@nsIComment@@QAEIIPAPAV1@@Z) referenced in function "unsigned int __cdecl SplitDataNode(class nsIDOMCharacterData *,unsigned int,unsigned int,class nsIDOMCharacterData * *,class nsIDOMCharacterData * *)" (?SplitDataNode@@YAIPAVnsIDOMCharacterData@@IIPAPAV1@1@Z)
gklayout.dll : fatal error LNK1120: 1 unresolved externals

I don't know what I'm missing - can someone bail me out please?

Comment 8

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

nsIComment should have
virtual nsresult SplitComment(PRUint32 aOffset, nsIComment** aReturn) = 0;
not
nsresult SplitComment(PRUint32 aOffset, nsIComment** aReturn);

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

This patch also fixes bug 384760.

Comment 10

[Alex Vincent [:WeirdAl]]

Now that we have a patch, I think I can request blocking1.9...

Comment 11

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

Why should this block 1.9? When you get reviews, asking approval for 1.9 might be better.

Comment 12

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

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

The patch is full of small white space changes.
Could you not do that, makes using lxr/bonsai slower when
one tries to find when and why something was changed.

>+nsresult SplitDataNode(nsIDOMCharacterData* aStartNode,
>+                       PRUint32 aStartIndex,
>+                       PRUint32 aEndIndex,
>+                       nsIDOMCharacterData** aMiddleNode,
>+                       nsIDOMCharacterData** aEndNode)
>+{
>+  NS_ENSURE_ARG(aStartNode);
>+  NS_ENSURE_ARG_POINTER(aMiddleNode);
>+  nsresult rv;
>+
>+  // Block for text nodes.
>+  {
>+    nsCOMPtr<nsIDOMText> node = do_QueryInterface(aStartNode);
>+    if (node) {
>+      // Split the main node, starting with the end.
>+      if (aEndNode && aEndIndex > aStartIndex) {
>+        nsCOMPtr<nsIDOMText> newNode;
>+        rv = node->SplitText(aEndIndex, getter_AddRefs(newNode));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        rv = CallQueryInterface(newNode, aEndNode);

Why do you need to QI here? nsIDOMText is nsIDOMCharacterData,
so just AddReffing should be enough.

>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+
>+      nsCOMPtr<nsIDOMText> newNode;
>+      rv = node->SplitText(aStartIndex, getter_AddRefs(newNode));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      return CallQueryInterface(newNode, aMiddleNode);
Same here.

>+nsresult nsRange::CutContents(nsIDOMDocumentFragment** aFragment)
>+{
>+  if (IsDetached())
>     return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
>+  nsresult res;
>+
>+  nsCOMPtr<nsIDOMDocument> document =
>+    do_QueryInterface(mStartParent->GetOwnerDoc());
>+  NS_ASSERTION(document, "CutContents needs a document to continue.");
>+  if (!document) return NS_ERROR_FAILURE;

Assertion should be used, IMO, in cases where assertion aborts and
no error handling is really possible.
Here you either want warning, or just the | if (expr) stmt; |

>+
>+  // Create a new document fragment in the context of this document,
>+  // which might be null.
>+
>+  nsCOMPtr<nsIDOMDocumentFragment> retval;
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(document));

Um, first you QI GetOwnerDoc to nsIDOMDocument, which you then QI back to
nsIDocument.

>+      nsCOMPtr<nsIDOMNode> aReturnedNode;
Wrong naming. aXXX is for parameters.


>+      res = retval->InsertBefore(node, lastFragmentNode,
>+                                 getter_AddRefs(aReturnedNode));
>+      lastFragmentNode = aReturnedNode;
>     }
>   }
> 
>-  // XXX_kin: At this point we should be checking for the case
>-  // XXX_kin: where we have 2 adjacent text nodes left, each
>-  // XXX_kin: containing one of the range end points. The spec
>-  // XXX_kin: says the 2 nodes should be merged in that case,
>-  // XXX_kin: and to use Normalize() to do the merging, but
>-  // XXX_kin: calling Normalize() on the common parent to accomplish
>-  // XXX_kin: this might also normalize nodes that are outside the
>-  // XXX_kin: range but under the common parent. Need to verify
>-  // XXX_kin: with the range commitee members that this was the
>-  // XXX_kin: desired behavior. For now we don't merge anything!

So this isn't a problem anymore?


>+++ content/test/unit/test_bug332148.js	24 Sep 2007 00:08:03 -0000

Just wondering, why unittest and why not mochitest? Doesn't really matter though.

I'd like to re-read the code without white space changes, and comments fixed.

Comment 13

[Alex Vincent [:WeirdAl]]

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

I was under the impression reviews at this stage wouldn't happen unless it was blocking or wanted1.9; this bug didn't have either...

Comment 14

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

(In reply to comment #13)
> (From update of attachment 282044 [details] [diff] [review])
> I was under the impression reviews at this stage wouldn't happen unless it was
> blocking or wanted1.9; this bug didn't have either...
> 
Ah, true, sort of, maybe :)
I'd say the focus is on 1.9+ and wanted1.9 bugs.
And anyway, is there any reason why this should *block* 1.9? I'd understand wanted1.9.

Comment 15

[Alex Vincent [:WeirdAl]]

I cannot come up with a great reason for blocking1.9.  But there's no other way to nominate something for wanted1.9 that I know of.  :)

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.1

In an earlier comment, smaug, you asked why I chose xpcshell tests over mochikit tests.  There's two reasons, both personal preference.  (1) xpcshell tests have much less overhead than mochikit tests.  (2) It's easier to run them immediately after a compile:

make tier_gecko check

Comment 17

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

Comment on attachment 282212 [details] [diff] [review]
patch, v1.1

>+nsresult SplitDataNode(nsIDOMCharacterData* aStartNode,
>+                       PRUint32 aStartIndex,
>+                       PRUint32 aEndIndex,
>+                       nsIDOMCharacterData** aMiddleNode,
>+                       nsIDOMCharacterData** aEndNode)
Does this method need to handle the case where aStartIndex is 0?

>+  // Create a new document fragment in the context of this document,
>+  // which might be null.

I don't quite understand "which might be null."

>+
>+  nsCOMPtr<nsIDOMDocumentFragment> retval;
>+
>+  res = NS_NewDocumentFragment(getter_AddRefs(retval),
>+                               doc->NodeInfoManager());

Do you really need to create document fragment always? Even if aFragment
is null. Deleting some big part of a range might get significantly slower because
of InsertBefore etc. calls.
Testing the performance might not be a bad idea; create a range which contains 
10000 siblings and call deleteContents, or something like that.
And add few DOMNodeInserted event listeners there too, so that 
nsContentUtils::HasMutationListeners needs to do more work.

Comment 18

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

Comment on attachment 282212 [details] [diff] [review]
patch, v1.1

marking r- until I see some perf tests or a patch not creating fragment when DeleteContents.

Comment 19

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.2

Sorry for the extended delay.

Comment 20

[Alex Vincent [:WeirdAl]]

(In reply to comment #17)
> Does this method need to handle the case where aStartIndex is 0?

Or where aEndIndex is the length of the node - 1...

I looked in the Range spec, and I couldn't see any special handling for that case, other than kin's comment:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsRange.cpp&rev=1.224&mark=1208-1217#1208

So I regretfully conclude that this currently results in two separate nodes that may need to be merged, one of which is an empty CDATA section.

I'd not object to removing the empty data node entirely.  Unfortunately, it's another spec edge case.  (Once upon a time, I thought the Range spec was exceptionally clear...)

Comment 21

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

Comment on attachment 285911 [details] [diff] [review]
patch, v1.2

>+  if (aFragment) {
>+    rv = NS_NewDocumentFragment(getter_AddRefs(retval),
>+                                doc->NodeInfoManager());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    retval = nsnull;
>+  }

else {} is not needed.

>+
>   // Batch possible DOMSubtreeModified events.
>-  mozAutoSubtreeModified subtree(mRoot ? mRoot->GetOwnerDoc(): nsnull, nsnull);
>+  mozAutoSubtreeModified subtree(mRoot ? mRoot->GetOwnerDoc(): nsnull,
>+                                 nsnull);

Why this change?

>-          // Delete the data between startOffset and endOffset.
>+          // Extract the data between startOffset and endOffset.
Perhaps "Delete or Extract"...


>-          // Delete everything after startOffset.
>+          // Extract everything after startOffset.
Same here

>-        // Delete the data between 0 and endOffset.
>+        // Extract everything before endOffset.
Something similar here too

> 
>   // XXX_kin: At this point we should be checking for the case
>   // XXX_kin: where we have 2 adjacent text nodes left, each
>   // XXX_kin: containing one of the range end points. The spec
>   // XXX_kin: says the 2 nodes should be merged in that case,
>   // XXX_kin: and to use Normalize() to do the merging, but
>   // XXX_kin: calling Normalize() on the common parent to accomplish
>   // XXX_kin: this might also normalize nodes that are outside the
>   // XXX_kin: range but under the common parent. Need to verify
>   // XXX_kin: with the range commitee members that this was the
>   // XXX_kin: desired behavior. For now we don't merge anything!
Please file a bug for this and add XXXbug4----- here.

Unit test could use some comments. How does the xml file format really work, 
what does run_test() do etc..
I'll review it after that.
Please include tests for edge cases, also some expected failures, if possible.
Also test what happens when invalid parameters (null, or out of bounds indexes etc.)
are passed to methods.
If this is hoped to get into 1.9, tests should be pretty extensive, IMO.

So for now r-, just because I want to re-read the tests with comments.

Comment 22

[Alex Vincent [:WeirdAl]]

Filed new spinoff bug 401276 for the merge issue.

Comment 23

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.2.1

With lots more tests thrown in for good measure, and also reorganized to allow the test harness for ranges to grow in the future.

Comment 24

[Jeff Walden [:Waldo]]

In the future you probably want to write tests like these as Mochitests, given that this is behavior that's web-facing; xpcshell generally wants to be for internal-facing stuff, so non-DOM APIs and stuff like that.  If you have the time to convert this to Mochitests (looks like a fair amount of work, so only if you want to), that would be good for better faithfulness to the actual environment where you'd use this stuff.

Comment 25

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

Comment on attachment 286343 [details] [diff] [review]
patch, v1.2.1

>+nsresult nsRange::CutContents(nsIDOMDocumentFragment** aFragment)
If aFragment isn't nsnull, assign *aFragment = nsnull;

>+              // Add to fragment.
>+              rv = retval->InsertBefore(cutNode, lastFragmentNode,
>+                                        getter_AddRefs(returnedNode));
(Note to myself, iteration goes backwards here and so InsertBefore is needed, not AppendChild)

>+++ content/test/unit/test_delete_range.xml	26 Oct 2007 21:55:57 -0000
>@@ -0,0 +1,125 @@
>+<?xml version="1.0" encoding="UTF-8"?>
>+<!--
>+This file holds serialized tests for DOM Range tests on extractContents.
Apparently you're testing also .deleteContents(), so perhaps you could change the comment a bit.

>+function evalXPath(aContextNode, aPath) {
>+  /* XXX ajvincent XPath Evaluator in Mozilla has a weakness:  it chokes on
>+   * document fragments as context nodes.  We're trying to beat that here.
>+   */
Is there a bug open for this? What does the spec say about this?

>+/**
>+ * Run the extraction tests.
>+ */
You're testing deleteContents also. Perhaps comment should indicate that.

>+       After the range's extraction or deletion is done, we use
>+       nsIDOM3Node.isEqualNode() between the altered source fragment and the
>+       result fragment.  We also run isEqualNode() between the extracted
>+       fragment and the fragment from the baseExtract node.  If they are not
>+       equal, we have failed a test.
Not related to this bug, but I hope there are tests to ensure .isEqualNode works properly.

>+  /*
>+  // XXX ajvincent if rv == WRONG_DOCUMENT_ERR, return false?

I think WRONG_DOCUMENT_ERR is fine too. At least you could test that now
using try-catch and if you have some good reasoning to change the behavior, open
a new bug.

>+  // Requested by smaug:  A range involving a comment as a document child.
Remove "Requested by smaug:  "

>+  /* smaug also requested attribute tests.  Sadly, those are not yet supported
>+     in ranges - see https://bugzilla.mozilla.org/show_bug.cgi?id=302775.
>+   */
Change the comment. Something like:
"Add tests for attribute nodes once https://bugzilla.mozilla.org/show_bug.cgi?id=302775
gets fixed."

Comment 26

[Alex Vincent [:WeirdAl]]

Comment on attachment 286343 [details] [diff] [review]
patch, v1.2.1

Regarding isEqualNode:
http://lxr.mozilla.org/seamonkey/source/content/test/unit/test_isequalnode.js

Comment 27

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

Attachment: up-to-date

Because of Bug 405181 the previous patch doesn't apply cleanly.
No functional changes, removed just one unneeded XXX comment and
one extra ';'.

Comment 28

[Alex Vincent [:WeirdAl]]

smaug, if you want to drive this one to completion, be my guest.  I've been unable to contribute to m.o projects for several months now.

Comment 29

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

For what its worth, ideally I'd like to see this code reimplemented using nsINode interfaces rather than the current nsIDOMNode mess. Should make it much smaller and readable.

I'd also like to see some proper codereuse.

(note that I haven't looked at any of the patches here for a while, so it might already be doing that)

Comment 30

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

(In reply to comment #29)
> For what its worth, ideally I'd like to see this code reimplemented using
> nsINode interfaces rather than the current nsIDOMNode mess. Should make it much
> smaller and readable.
Well, in this case nsI interfaces are used when needed and nsIDOM***
when they provide good abstraction - in this case using
nsIDOMCharacterData is quite handy, since it can be a text node or
comment node.

> I'd also like to see some proper codereuse.
Same code is used for DeleteContents and ExtractContents, so I don't
know much better codereuse we can get.

Comment 31

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

Comment on attachment 320735 [details] [diff] [review]
up-to-date

>+          /* The Range spec clearly states clones get cut and original nodes
>+             remain behind, but SplitDataNode places the remaining portion into
>+             cutNode and the extracted portion remains in charData.  Switch the
>+             values.
>+          */
>+          nsString cutData;
>+          nsString retainedData;
These should be nsAutoString objects.

Comment 32

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

Comment on attachment 320735 [details] [diff] [review]
up-to-date

I still suspect that you can get rid of more nsIDOMNode usage. nsIContent works fine for the cases where you just want a generic textnode interface, just call IsNodeOfType(nsINode::eTEXT) or even node->Tag() == nsGkAtoms::textTagName.


>+++ content/base/public/nsIComment.h	2008-05-13 18:59:40.000000000 +0300

Do you really need nsIComment? Can't you just call IsNodeOfType(nsINode::eCOMMENT) (or node->Tag == nsGkAtoms::commentTagName) and then cast to nsGenericDOMDataNode? I guess you could then rename the new SplitText signature to SplitData or some such.


>Index: content/base/src/nsGenericDOMDataNode.cpp
>@@ -929,30 +929,36 @@ nsGenericDOMDataNode::SplitText(PRUint32
...
>-  // No need to handle the case of document being the parent since text
>-  // isn't allowed as direct child of documents
>+  NS_ADDREF(*aReturn = newContent);
>+  return rv;

You could save some addreffing here by doing newContent.swap(*aReturn)

>Index: content/base/src/nsRange.cpp
>===================================================================

>+nsresult SplitDataNode(nsIDOMCharacterData* aStartNode,
>+                       PRUint32 aStartIndex,
>+                       PRUint32 aEndIndex,
>+                       nsIDOMCharacterData** aMiddleNode,
>+                       nsIDOMCharacterData** aEndNode)

I think you can rewrite this function using nsIContent instead, by casting to nsGenericDOMDataNode after checking the nodetype.

>+{
>+  NS_ENSURE_ARG(aStartNode);
>+  NS_ENSURE_ARG_POINTER(aMiddleNode);

Do you really need these nullchecks? At least make them debug-only, but IMHO it's fine to not do them at all to remove clutter. I think you might be able to do this in a few more places.

Also, can you add an assertion to make sure that aEndNode is null/non-null as appropriate based on if aEndIndex is less then the nodes length or not.


>+nsresult nsRange::CutContents(nsIDOMDocumentFragment** aFragment)

>   RangeSubtreeIterator iter;
> 
>-  nsresult res = iter.Init(this);
>-  if (NS_FAILED(res)) return res;
>+  rv = iter.Init(this);
>+  if (NS_FAILED(rv)) return rv;

NS_ENSURE_SUCCESS(rv, rv)? (same applies in a few more places)

>@@ -1161,86 +1260,164 @@ nsresult nsRange::DeleteContents()
...
>+            nsCOMPtr<nsIDOMCharacterData> cutNode;
>+            nsCOMPtr<nsIDOMCharacterData> endNode;
>+            rv = SplitDataNode(charData, startOffset, endOffset,
>+                               getter_AddRefs(cutNode),
>+                               getter_AddRefs(endNode));
>+            if (NS_FAILED(rv)) return rv;
>+            nsCOMPtr<nsIDOMNode> returnedNode;
>+
>+            if (retval) {
>+              // Add to fragment.
>+              rv = retval->InsertBefore(cutNode, lastFragmentNode,
>+                                        getter_AddRefs(returnedNode));
>+              if (NS_FAILED(rv)) return rv;
>+              lastFragmentNode = returnedNode;
>+            } else {
>+              rv = RemoveNode(cutNode);
>+              if (NS_FAILED(rv)) return rv;
>+            }
>           }

Hmm.. this seems somewhat suboptimal. Is this inserting the middle node into the DOM, only to remove it (either into the fragment or remove it entirely).

Though I guess the spec requires it?


>       else if (node == endContainer)
>       {
>-        // Delete the data between 0 and endOffset.
>+        // Delete or extract everything before endOffset.
> 
>         if (endOffset > 0)
>         {
>-          res = charData->DeleteData(0, endOffset);
>-          if (NS_FAILED(res)) return res;
>+          nsCOMPtr<nsIDOMCharacterData> cutNode;
>+          rv = SplitDataNode(charData, endOffset, endOffset,
>+                             getter_AddRefs(cutNode), nsnull);
>+          if (NS_FAILED(rv)) return rv;
>+
>+          /* The Range spec clearly states clones get cut and original nodes
>+             remain behind, but SplitDataNode places the remaining portion into
>+             cutNode and the extracted portion remains in charData.  Switch the
>+             values.
>+          */
>+          nsString cutData;
>+          nsString retainedData;
>+          rv = cutNode->GetData(retainedData);
>+          if (NS_FAILED(rv)) return rv;
>+          rv = charData->GetData(cutData);
>+          if (NS_FAILED(rv)) return rv;
>+          rv = cutNode->SetData(cutData);
>+          if (NS_FAILED(rv)) return rv;
>+          rv = charData->SetData(retainedData);
>+          if (NS_FAILED(rv)) return rv;

Hmm.. this is very ugly, and I don't believe that it's what the spec authors had in mind. Could we instead add a boolean argument to the Split function to say if the clone should get inserted before or after the original node (and thus get the data before or after the split).

Though technically speaking I'm not sure that the spec is actually requiring that the "clones" get cut. I think this only applies to elements as splitText isn't technically "cloning" (that term doesn't seem to ever be used in the context of splitText).


>+function evalXPath(aContextNode, aPath) {
>+  /* XXX ajvincent XPath Evaluator in Mozilla has a weakness:  it chokes on
>+   * document fragments as context nodes.  We're trying to beat that here.
>+   */

Is this whole function there just to deal with that bug in the XPath engine? Seems worthy of fixing asap so we can simplify this test then. Is there a bug filed?

Would like to see another patch to look at the changes to use nsINode/nsIContent more..

Looks great otherwise! Thanks for the patch and sorry I've been so slow with the review :(

Comment 33

[Alex Vincent [:WeirdAl]]

(In reply to comment #32)
> >+function evalXPath(aContextNode, aPath) {
> >+  /* XXX ajvincent XPath Evaluator in Mozilla has a weakness:  it chokes on
> >+   * document fragments as context nodes.  We're trying to beat that here.
> >+   */
> 
> Is this whole function there just to deal with that bug in the XPath engine?
> Seems worthy of fixing asap so we can simplify this test then. Is there a bug
> filed?

Yes, bug 402129, and for the record, I was wrong when I wrote that comment:  document fragment nodes shouldn't be allowed as context nodes.

Comment 34

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

Comment on attachment 286343 [details] [diff] [review]
patch, v1.2.1

Minusing this for now to get it off my queue while waiting for a new patch.

Comment 35

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

Attachment: no nsIComment, less nsIDOM usage

Comment 36

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

Comment on attachment 331081 [details] [diff] [review]
no nsIComment, less nsIDOM usage

I still think we can convert this code to use far less nsIDOM stuff. But lets get this in for now, we can always clean it up more later.

>+RemoveNode(nsIDOMNode* aNode)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIDOMNode> parent;
>+  rv = aNode->GetParentNode(getter_AddRefs(parent));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsIDOMNode> junk;
>+  rv = parent->RemoveChild(aNode, getter_AddRefs(junk));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return rv;
>+}

You need to nullcheck parent here I suspect, in case mutation events have changed the world under us.

Also, this would be faster as:

nsCOMPtr<nsINode> node = do_QI(aNode);
nsINode* parent = node->GetNodeParent();
return parent ? parent->RemoveChildAt(parent->IndexOf(node), PR_TRUE) : NS_OK;

Comment 37

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

ah, right. Though I have to keep parent alive, so nsCOMPtr<nsINode> parent.

Comment 38

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

Attachment: Simpler RemoveNode

Comment 39

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

Attachment: er, this one