[webkit-reviews] review denied: [Bug 17125] Acid3 requires Text.replaceWholeText method from DOM Level 3 : [Attachment 18997] Address Darin's concerns (delta from previous patch)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 17125: Acid3 requires Text.replaceWholeText method from DOM Level 3
http://bugs.webkit.org/show_bug.cgi?id=17125

Attachment 18997: Address Darin's concerns (delta from previous patch)
http://bugs.webkit.org/attachment.cgi?id=18997&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.