[Webkit-unassigned] [Bug 17125] Acid3 requires Text.replaceWholeText method from DOM Level 3

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=17125


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18955|review?                     |review-
               Flag|                            |




------- Comment #5 from darin at apple.com  2008-02-06 06:40 PDT -------
(From update of attachment 18955)
+        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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.