Implement DOM 3 replaceWholeText and wholeText

RESOLVED FIXED in mozilla2.0

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 308300 [details] [diff] [review]
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.
Attachment #308300 - Flags: superreview?(jonas)
Attachment #308300 - Flags: review?(jonas)
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);
I'm not convinced we should do this for FF3
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.
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.
Flags: blocking1.9?
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.
(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.
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.
Flags: blocking1.9? → blocking1.9-
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.
Created attachment 321298 [details] [diff] [review]
Updated to latest mozilla-central
Attachment #308300 - Attachment is obsolete: true
Attachment #321298 - Flags: superreview?(jonas)
Attachment #321298 - Flags: review?(jonas)
Attachment #308300 - Flags: superreview?(jonas)
Attachment #308300 - Flags: review?(jonas)
(Assignee)

Updated

10 years ago
Attachment #321298 - Flags: superreview?(jst)
Attachment #321298 - Flags: superreview?(jonas)
Attachment #321298 - Flags: review?(jonas)
Attachment #321298 - Flags: review?(Olli.Pettay)
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-.
Attachment #321298 - Flags: review?(Olli.Pettay) → review-
Created attachment 323344 [details] [diff] [review]
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?
Attachment #321298 - Attachment is obsolete: true
Attachment #323344 - Flags: superreview?(jst)
Attachment #323344 - Flags: review?(Olli.Pettay)
Attachment #321298 - Flags: superreview?(jst)
mozAutoSubtreeModified batches DOMSubtreeModified events. Not other mutation events.
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);
>+};
Attachment #323344 - Flags: review?(Olli.Pettay) → review+

Updated

10 years ago
Attachment #323344 - Flags: superreview?(jst) → superreview+
Fixt!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0

Updated

10 years ago
Keywords: dev-doc-needed

Updated

10 years ago
Depends on: 444030
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.