Bug 17125

It actually requires .replaceWholeText. I'd expect a patch to implement both methods.

Created attachment 18899 [details] First pass at wholeText/replaceWholeText support LayoutTests/ChangeLog | 21 +++++ .../core/textreplacewholetext01-expected.txt | 3 +- .../core/textreplacewholetext02-expected.txt | 3 +- .../core/textreplacewholetext03-expected.txt | 3 +- .../core/textreplacewholetext04-expected.txt | 3 +- .../core/textreplacewholetext05-expected.txt | 3 +- .../core/textreplacewholetext06-expected.txt | 3 +- .../core/textreplacewholetext07-expected.txt | 3 +- .../xhtml/level3/core/textwholetext01-expected.txt | 3 +- .../xhtml/level3/core/textwholetext02-expected.txt | 3 +- .../xhtml/level3/core/textwholetext03-expected.txt | 3 +- .../fast/dom/Window/window-properties-expected.txt | 1 + WebCore/ChangeLog | 17 ++++ WebCore/dom/Node.cpp | 2 +- WebCore/dom/Text.cpp | 80 ++++++++++++++++++++ WebCore/dom/Text.h | 4 + WebCore/dom/Text.idl | 4 + 17 files changed, 138 insertions(+), 21 deletions(-)

Comment on attachment 18899 [details] First pass at wholeText/replaceWholeText support Maciej has convinced me to improve handling of EntityReferences as part of this patch (and add some test cases to make sure we make them ReadOnly nodes). I'm gone until Wed. night, at which point I'll look at finishing this.

Created attachment 18955 [details] Updated after Maciej's comments LayoutTests/ChangeLog | 27 ++++++ .../core/textreplacewholetext01-expected.txt | 3 +- .../core/textreplacewholetext02-expected.txt | 3 +- .../core/textreplacewholetext03-expected.txt | 3 +- .../core/textreplacewholetext04-expected.txt | 3 +- .../core/textreplacewholetext05-expected.txt | 3 +- .../core/textreplacewholetext06-expected.txt | 3 +- .../core/textreplacewholetext07-expected.txt | 3 +- .../xhtml/level3/core/textwholetext01-expected.txt | 3 +- .../xhtml/level3/core/textwholetext02-expected.txt | 3 +- .../xhtml/level3/core/textwholetext03-expected.txt | 3 +- .../readonly-exceptions-expected.txt | 23 +++++ .../dom/EntityReference/readonly-exceptions.html | 13 +++ .../dom/EntityReference/resources/TEMPLATE.html | 13 +++ .../resources/readonly-exceptions.js | 33 +++++++ .../fast/dom/Window/window-properties-expected.txt | 1 + WebCore/ChangeLog | 23 +++++ WebCore/dom/Document.cpp | 7 ++- WebCore/dom/Node.cpp | 2 +- WebCore/dom/Text.cpp | 89 ++++++++++++++++++++ WebCore/dom/Text.h | 4 + WebCore/dom/Text.idl | 4 + 22 files changed, 247 insertions(+), 22 deletions(-)

Comment on attachment 18955 [details] Updated after Maciej's comments + if (n->nodeType() == Node::ENTITY_REFERENCE_NODE) { + // We would need to visit EntityReference child text nodes if they existed + ASSERT(!n->hasChildNodes()); + break; + } + if (!n->isTextNode()) + break; The code above is (slightly) unnecessarily inefficient. The code to handle EntityReference could simply be an assertion. And if not, and you want to retain the nodeType check, then it's better to not call both nodeType() and isTextNode(). If you have the node type, you can check if something's a text node without a second virtual function call. Because DOM mutation events, specifically the DOMNodeRemovedEvent, can do arbitrary operations, the replaceWholeText function needs some work to be robust in the face of event handler functions that do strange things. + for (Node* n = startText; n != this;) { This loop variable needs to be a RefPtr. The removeChild function could remove the last reference to the parent, so the node "n" could be deleted unless someone has ref'd it. Similarly, the current node ("this") needs to go into a RefPtr before calling functions that can in turn invoke mutation event handlers. + parentNode()->removeChild(nodeToRemove, ignored); + ASSERT(!ignored); This assertion is incorrect. For example, the mutation function could remove the node inside the function, which will result in a NOT_FOUND_ERR. So it may be OK to ignore the error, but it's not OK to assert there was no error. + Node* onePastEndText = endText->nextSibling(); + for (Node* n = nextSibling(); n != onePastEndText;) { Same thing here. Both onePastEndText and n need to be RefPtr. + parentNode()->removeChild(nodeToRemove, ignored); + ASSERT(!ignored); And this assertion is also incorrect. + setData(newText, ignored); + ASSERT(!ignored); To make this assertion correct, you'll need to reset "ignored" to 0 just before calling setData since the earlier functions could have changed ignored to a non-0 value. Or you could remove the assertion. If you removed all the assertions, then you could leave ignored uninitialized. + Text* replaceWholeText(const String&, ExceptionCode&); The return value of the function needs to be a PassRefPtr. It's possible that mutation event handlers will remove the text node from tree. If they do, it's important that we don't return a deleted object. The usual way we fix this elsewhere is to use PassRefPtr even though in the normal case the object will be in the DOM tree. This is why the return value from Node::appendChild, for example, is PassRefPtr and not a raw pointer. + String wholeText(); Could we make this a const member? +shouldBe("entityReference.textContent", "''"); // change to '<' when we support EntityReference nodes +shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added. +shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added. I'd prefer that the shouldBe results reflect correct behavior, rather than the current behavior of our engine. The expected test results will still regression-test our current behavior, because FAIL and the current values will be in the test output.

+ Node* onePastEndText = endText->nextSibling(); + for (Node* n = nextSibling(); n != onePastEndText;) { Same thing here. Both onePastEndText and n need to be RefPtr. If we're protecting ourselves from insane mutation event handlers... couldn't a mutation event handler end up removing onePastEndText from the sibling list? Thus if onePastEndText != 0, we'd crash, no?

Bah! This is insane. Mutation event handlers could do all sorts of crazy things, including transplanting the node we're next going to traverse to somewhere else in the document. Thus making it impossible to simply traverse the sibling list. We'd end up just removing random nodes until we walked off the end of whatever node-list we got transplanted into!

CCing darin so he can see my comments about the insanity that is trying to protect ourselves from rogue mutation event handlers.

I originally had wholeText() const, but thought all the const and const_cast crud was ugly. I added it back in my forthcoming patch however.

Created attachment 18996 [details] Address Darin's concerns (delta from previous patch) .../readonly-exceptions-expected.txt | 6 +- .../resources/readonly-exceptions.js | 6 +- WebCore/dom/Text.cpp | 70 +++++++++++--------- WebCore/dom/Text.h | 4 +- 4 files changed, 46 insertions(+), 40 deletions(-)

Comment on attachment 18996 [details] Address Darin's concerns (delta from previous patch) I can upload a combined patch if you need.

Created attachment 18997 [details] Address Darin's concerns (delta from previous patch) .../readonly-exceptions-expected.txt | 6 +- .../resources/readonly-exceptions.js | 6 +- WebCore/dom/Text.cpp | 70 +++++++++++--------- WebCore/dom/Text.h | 4 +- 4 files changed, 46 insertions(+), 40 deletions(-)

(In reply to comment #6) > If we're protecting ourselves from insane mutation event handlers... couldn't a > mutation event handler end up removing onePastEndText from the sibling list? > Thus if onePastEndText != 0, we'd crash, no? Our goal is to protect ourselves well enough from insane event handlers to prevent them from causing a crash or infinite loop. That may be an impossible challenge though. We should at least do the basics for now and think more deeply about how to handle the tougher issues.

Comment on attachment 18997 [details] Address Darin's concerns (delta from previous patch) I would have preferred to review a combined patch: + case Node::CDATA_SECTION_NODE: + t = static_cast<const Text*>(n); The above is incorrect. A CDATA section will be a CharacterData node and *not* a Text node. So it's an error to cast to Text*. Text is derived from CharacterData, so if we want to handle both we should be using the type CharacterData everywhere, and not Text. I feel bad for spreading the const disease into this code with my suggestion of marking wholeText() const. Sorry. I probably would have put the const_cast calls at the call sites inside Text::wholeText and left the helper functions themselves alone. + RefPtr<Node> nodeToRemove(n.release()); n = n->nextSibling(); This doesn't look like it will work. Calling release() will set n to 0 and you'll crash on the n->nextSibling() call. You should re-run the tests.

Yes, the RefPtr nodeToRemove(n.release()) was bogus. I'll fix. I think you're confused about CDATA_SECTION_NODE. That means its a CDataSection, which is a subclass of Text (which is a subclass of CharacterData). class CDATASection : public Text

(In reply to comment #15) > I think you're confused about CDATA_SECTION_NODE. That means its a > CDataSection, which is a subclass of Text (which is a subclass of > CharacterData). > > class CDATASection : public Text Got it. My bad.

Created attachment 18999 [details] Add wholeText/replaceWholeText support (final squashed patch, tests all pass) LayoutTests/ChangeLog | 27 ++++++ .../core/textreplacewholetext01-expected.txt | 3 +- .../core/textreplacewholetext02-expected.txt | 3 +- .../core/textreplacewholetext03-expected.txt | 3 +- .../core/textreplacewholetext04-expected.txt | 3 +- .../core/textreplacewholetext05-expected.txt | 3 +- .../core/textreplacewholetext06-expected.txt | 3 +- .../core/textreplacewholetext07-expected.txt | 3 +- .../xhtml/level3/core/textwholetext01-expected.txt | 3 +- .../xhtml/level3/core/textwholetext02-expected.txt | 3 +- .../xhtml/level3/core/textwholetext03-expected.txt | 3 +- .../readonly-exceptions-expected.txt | 23 +++++ .../dom/EntityReference/readonly-exceptions.html | 13 +++ .../dom/EntityReference/resources/TEMPLATE.html | 13 +++ .../resources/readonly-exceptions.js | 33 +++++++ .../fast/dom/Window/window-properties-expected.txt | 1 + WebCore/ChangeLog | 23 +++++ WebCore/dom/Document.cpp | 7 ++- WebCore/dom/Node.cpp | 2 +- WebCore/dom/Text.cpp | 95 ++++++++++++++++++++ WebCore/dom/Text.h | 4 + WebCore/dom/Text.idl | 4 + 22 files changed, 253 insertions(+), 22 deletions(-)

Created attachment 19000 [details] Add wholeText/replaceWholeText support (final squashed patch, tests all pass) LayoutTests/ChangeLog | 33 +++++++ .../core/textreplacewholetext01-expected.txt | 3 +- .../core/textreplacewholetext02-expected.txt | 3 +- .../core/textreplacewholetext03-expected.txt | 3 +- .../core/textreplacewholetext04-expected.txt | 3 +- .../core/textreplacewholetext05-expected.txt | 3 +- .../core/textreplacewholetext06-expected.txt | 3 +- .../core/textreplacewholetext07-expected.txt | 3 +- .../xhtml/level3/core/textwholetext01-expected.txt | 3 +- .../xhtml/level3/core/textwholetext02-expected.txt | 3 +- .../xhtml/level3/core/textwholetext03-expected.txt | 3 +- .../readonly-exceptions-expected.txt | 23 +++++ .../dom/EntityReference/readonly-exceptions.html | 13 +++ .../dom/EntityReference/resources/TEMPLATE.html | 13 +++ .../resources/readonly-exceptions.js | 33 +++++++ .../fast/dom/Text/replaceWholeText-expected.txt | 13 +++ LayoutTests/fast/dom/Text/replaceWholeText.html | 13 +++ LayoutTests/fast/dom/Text/resources/TEMPLATE.html | 13 +++ .../fast/dom/Text/resources/replaceWholeText.js | 16 ++++ .../fast/dom/Window/window-properties-expected.txt | 1 + WebCore/ChangeLog | 23 +++++ WebCore/dom/Document.cpp | 7 ++- WebCore/dom/Node.cpp | 2 +- WebCore/dom/Text.cpp | 91 ++++++++++++++++++++ WebCore/dom/Text.h | 4 + WebCore/dom/Text.idl | 4 + 26 files changed, 310 insertions(+), 22 deletions(-)

Comment on attachment 19000 [details] Add wholeText/replaceWholeText support (final squashed patch, tests all pass) r=me

Landed in r30088.

Mass moving XML DOM bugs to the "DOM" Component.