421765 - Implement DOM 3 replaceWholeText and wholeText

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

Doing so would pick up a point on acid3.

I have a patch that's tested only far enough to say that the following tests pass:

http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextwholetext01.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextwholetext02.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextwholetext03.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext01.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext02.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext03.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext04.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext05.html

These tests fail because we don't support entity references, but we hardly care about that:

http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext06.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext07.html
http://test.bclary.com/tests/w3.org/2001/DOM-Test-Suite/build/ecmascript/jsunit/testRunner.html?implementation=iframe&skipIncompatibleTests=true&autorun=true&contentType=text%2Fxml&testpage=http%3A%2F%2Ftest.bclary.com%2Ftests%2Fw3.org%2F2001%2FDOM-Test-Suite%2Fbuild%2Fecmascript%2Flevel3%2Fcore%2Ftextreplacewholetext08.html

However, I'm getting shutdown crashes and once leaked 1.7M doing little more than running the tests, so I'm pretty sure the patch is buggy still.

I'm hoping for 3, but given the trickiness here with mutation it may not happen.  If it does, the change deserves the strictest scrutiny, even tho it's probably only going to be 10K or so ignoring the tests I have yet to write.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Turns out not addrefing out parameters is a good way to leak/crash

The patch is >35K, but do note that it's mostly the tests taking up space.  If I cut out the test-related lines, it drops to 16K.

The part that most worried me with this patch was getting replaceWholeText to play nice with mutation listeners.  As I understand it the line at the top of the method is sufficient to do this.  I tried writing some tests to break it, but the only thing making the listeners do mutations to try to throw off index counts did was cause lots and lots of recursion as my DOMNodeRemoved listener got called lots and lots of times; removing the listener-caused mutation would prevent the recursion.  This behavior seemed incomprehensible, and it didn't agree with what WebKit did, so I removed it from the patch.  The experience is enough to convince me that I'm doing the mutation-prevention thing correctly enough to avoid crashing, which is what I think really mattered.

Comment 2

[Jeff Walden [:Waldo]]

While re-skimming the diff view of the patch, I noticed the IDL docs for replaceWholeText were inaccurate.  Please consider the following docs instead:

/**
 * If aContent is empty, removes all logically adjacent text nodes (including
 * this node) from the DOM tree; otherwise, replaces the contents of this node
 * with aContent and removes all other logically adjacent text nodes from the
 * DOM tree.
 */
nsIDOMText replaceWholeText(in DOMString content) raises(DOMException);

Comment 3

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

I'm not convinced we should do this for FF3

Comment 4

[Jeff Walden [:Waldo]]

If it weren't self-contained, didn't have tests, or weren't part of Acid3, I'd agree.  As it is, assuming the correctness of the tests and methods can be vetted (test results say it's good, and all the tests also pass a WebKit nightly if I replace the <![CDATA[]]> with <![CDATA[d]]>; WebKit seems to ignore empty CDATA sections), I think it's reasonable to do this.  It's a relatively cheap point, it's nearly the last such point remaining, and there was nothing fundamentally hard, tricky, or wide-reaching about the changes.

Comment 5

[Jeff Walden [:Waldo]]

Okay, I'd like something more definitive than "not convinced" here -- we should either take this because it gains another Acid3 point with relatively little effort and complexity, or we should not take this and explain why.  Either way, I'd like to see some sort of discussion prior to a decision being made.  I'll start:

Reasons to take:
- gains another point on Acid3, good because we'll be stuck with the score we
  have in Firefox 3 for awhile
- the algorithmic changes are small (~50 lines) and relatively uncomplicated
- the algorithmic changes are accompanied by a large number of tests
  (notably but not exclusively for edge-case first-in-children and
  last-in-children situations, to demonstrate index-calculation correctness)
  which have been run against another implementation to verify their
  correctness
- none of the changes (excluding interface-map changes, whose effects are well
  understood) affect existing code paths
- this plus bug 260106 (the only other Acid3 fix I think we can reasonably
  take for v3) would get us to a final, aesthetically-pleasing score of 70/100
  *even if all GC-dependent tests fail*
- probability of regressions is low due to separate codepath and extensive
  tests

Reasons not to take:
- replaceWholeText is a mutating (== scary) method
- too late
- ???

Rebuttals:
- how does mozAutoSubtreeModified not suffice to address mutation concerns?
  Element.normalize() relies upon no more or less than it
- see reasons to take; also, this answer just begs the question

In total, I think we should take this because all of the following hold:

1. It gains us an Acid3 point.
2. The non-trivial changes do not affect existing code paths.
3. It is well-tested.
4. (corollary to 2/3) It is very unlikely to cause regressions.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I can give these reasons not to take this now:

1)  There's little win for anything other than the Acid3 pissing match.  So if
    there's any regression risk at all, I would say it's not acceptable this
    late in the game.
2)  It would take away reviewer time from things that actually need to happen
    before release.
3)  If it's as safe and self-contained as you say, it can land in a point release
    of for 1.9.1 or whatever we end up doing.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> 1)  There's little win for anything other than the Acid3 pissing match.  So if
>     there's any regression risk at all, I would say it's not acceptable this
>     late in the game.

I'll go further and say there's no win outside Acid3, but I think that's enough, given that I don't think there's any regression risk.

> 2)  It would take away reviewer time from things that actually need to happen
>     before release.

I think we can afford ten or fifteen minutes of reviewer time for this, modulo nitpicking; if it takes more than that, I've done something wrong.

> 3)  If it's as safe and self-contained as you say, it can land in a point
>     release of for 1.9.1 or whatever we end up doing.

Since it's ready earlier and won't regress anything, I think we should land it now.  It'll land eventually; my concern is just that that happen as soon as reasonably possible, which I think is now.

Comment 8

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

I'd rather not spend time on this. There is always a risk of regressions, or at least of not getting all edge cases right.

There is IMHO very little value in passing ACID3 for the sake of passing ACID3. I do think that we should work on passing it, but we should do it in order to improve the web.

I definitely think we should land this as soon as we've branched though.

Comment 9

[Boris Zbarsky [:bzbarsky]]

fwiw, it'd take me a lot more than 15 minutes to review that patch...  Just as a data point.  Then again, I tend to be pretty careful about my reviews.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Updated to latest mozilla-central

Comment 11

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

Comment on attachment 321298 [details] [diff] [review]
Updated to latest mozilla-central

>+nsGenericDOMDataNode::GetWholeText(nsAString& aWholeText)
...
>+  // Handle parent-less nodes
>+  if (!parent) {
>+    nsCOMPtr<nsIDOMText> text = do_QueryInterface(this);
>+    NS_ASSERTION(text, "don't call GetWholeText on a non-text node!");
>+    return text->GetData(aWholeText);
No need to QI, just
if (!parent) {
  return GetData(aWholeText);
}

>+  nsCOMPtr<nsIDOMText> node;
>+  nsAutoString tmp;
>+  do {
>+    node = do_QueryInterface(parent->GetChildAt(first));
>+    node->GetData(tmp);
>+    aWholeText.Append(tmp);
>+  } while (first++ < last);
You should Truncate aWholeText before modifying it.

>+nsresult
>+nsGenericDOMDataNode::ReplaceWholeText(const nsAFlatString& aContent,
>+                                       nsIDOMText **aReturn)
>+{
>+  // Batch possible DOMSubtreeModified events.
>+  mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull);
You could add also 
mozAutoDocUpdate updateBatch(GetCurrentDoc(), UPDATE_CONTENT_MODEL, PR_TRUE);
(that could be added to Normalize() too, but that is different bug)

>+  nsIContent* parent = GetParent();
Make this nsCOMPtr<nsIContent>. Mutation events may kill
the parent. 

>+/**
>+ * A tearoff class for the nsIDOM3Text portion of nsTextNode.
>+ */
>+class nsText3Tearoff : public nsIDOM3Text
Couldn't you move this to nsGenericDOMDataNode.h and
make the member variable nsRefPtr<nsGenericDOMDataNode>.
Then you could reuse the same class for nsTextNode and nsXMLCDATASection, right?

I could still look at the updated patch, so r-.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Comments addressed

I think this is everything you wanted, assuming I understood your instructions on the mutation part correctly.

Doesn't mozAutoSubtreeModified defer mutation events until its dtor is called?  Why wouldn't that protect the weak parent pointer?  Or am I misunderstanding something?

Comment 13

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

mozAutoSubtreeModified batches DOMSubtreeModified events. Not other mutation events.

Comment 14

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

Comment on attachment 323344 [details] [diff] [review]
Comments addressed

> public:
>   nsTextNode(nsINodeInfo *aNodeInfo);
>   virtual ~nsTextNode();
>-
>+  
Nit, no need for this whitespace change.

>+  /**
>+   * If aContent is empty, removes all logically adjacent text nodes (including
Nit, replaceWholeText doesn't have parameter "aContent" but "content"
>+   * this node) from the DOM tree, returning null; otherwise, replaces the
>+   * contents of this node with aContent and removes all other logically
>+   * adjacent text nodes from the DOM tree, returning this node.
>+   */
>+  nsIDOMText replaceWholeText(in DOMString content) raises(DOMException);
>+};

Comment 15

[Jeff Walden [:Waldo]]

Fixt!