WebCore:
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Mar 2008 01:07:58 +0000 (01:07 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Mar 2008 01:07:58 +0000 (01:07 +0000)
2008-03-25  Darin Adler  <darin@apple.com>

        Reviewed by Anders.

        - fix http://bugs.webkit.org/show_bug.cgi?id=17252
          Acid3 test removing Nodes during NodeIterator walk fails (affects Acid3 test 2)

        Test: traversal/acid3-test-2.html

        * bindings/js/JSNodeIteratorCustom.cpp:
        (WebCore::JSNodeIterator::nextNode): Update since result is PassRefPtr.
        (WebCore::JSNodeIterator::previousNode): Ditto.
        * dom/NodeIterator.cpp:
        (WebCore::NodeIterator::nextNode): Changed result to PassRefPtr. Added code to
        track both the current candidate (which needs to move along to the next node
        if current node is deleted) and the current provisional result (passed to acceptNode,
        and needs to be returned even if it's deleted).
        (WebCore::NodeIterator::previousNode): Ditto.
        (WebCore::NodeIterator::nodeWillBeRemoved): Call updateForNodeRemoval for
        m_candidateNode as well as m_referenceNode.
        * dom/NodeIterator.h: Use PassRefPtr for return values.

LayoutTests:

2008-03-25  Darin Adler  <darin@apple.com>

        Reviewed by Anders.

        - test for http://bugs.webkit.org/show_bug.cgi?id=17252
          Acid3 test removing Nodes during NodeIterator walk fails (affects Acid3 test 2)

        * traversal/acid3-test-2-expected.txt: Added.
        * traversal/acid3-test-2.html: Added.
        * traversal/resources/acid3-test-2.js: Added.
        * traversal/resources/exception-forwarding.js: Removed bogus extra line of code.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@31303 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/traversal/acid3-test-2-expected.txt [new file with mode: 0644]
LayoutTests/traversal/acid3-test-2.html [new file with mode: 0644]
LayoutTests/traversal/resources/acid3-test-2.js [new file with mode: 0644]
LayoutTests/traversal/resources/exception-forwarding.js
WebCore/ChangeLog
WebCore/bindings/js/JSNodeIteratorCustom.cpp
WebCore/dom/NodeIterator.cpp
WebCore/dom/NodeIterator.h

index d6984ce..aad495e 100644 (file)
@@ -1,3 +1,15 @@
+2008-03-25  Darin Adler  <darin@apple.com>
+
+        Reviewed by Anders.
+
+        - test for http://bugs.webkit.org/show_bug.cgi?id=17252
+          Acid3 test removing Nodes during NodeIterator walk fails (affects Acid3 test 2)
+
+        * traversal/acid3-test-2-expected.txt: Added.
+        * traversal/acid3-test-2.html: Added.
+        * traversal/resources/acid3-test-2.js: Added.
+        * traversal/resources/exception-forwarding.js: Removed bogus extra line of code.
+
 2008-03-24  Oliver Hunt  <oliver@apple.com>
 
         Reviewed by Mark Rowe.
diff --git a/LayoutTests/traversal/acid3-test-2-expected.txt b/LayoutTests/traversal/acid3-test-2-expected.txt
new file mode 100644 (file)
index 0000000..79aba27
--- /dev/null
@@ -0,0 +1,43 @@
+Test of behavior NodeIterator when nodes are removed, from Acid3.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS count is 1
+PASS nodea is nodeb
+PASS count is 2
+PASS nodea is nodeb
+PASS count is 3
+PASS nodea is nodeb
+PASS count is 4
+PASS nodea is nodeb
+PASS count is 5
+PASS nodea is nodeb
+PASS count is 6
+PASS nodea is nodeb
+PASS count is 7
+PASS nodea is nodeb
+PASS count is 8
+PASS nodea is nodeb
+PASS count is 9
+PASS nodea is nodeb
+PASS count is 10
+PASS nodea is nodeb
+PASS count is 11
+PASS nodea is nodeb
+PASS count is 12
+PASS nodea is nodeb
+PASS count is 13
+PASS nodea is nodeb
+PASS count is 14
+PASS nodea is nodeb
+PASS count is 15
+PASS nodea is nodeb
+PASS count is 16
+PASS nodea is nodeb
+PASS count is 17
+PASS nodea is nodeb
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/traversal/acid3-test-2.html b/LayoutTests/traversal/acid3-test-2.html
new file mode 100644 (file)
index 0000000..8221aa9
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../fast/js/resources/js-test-style.css">
+<script src="../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/acid3-test-2.js"></script>
+<script src="../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/traversal/resources/acid3-test-2.js b/LayoutTests/traversal/resources/acid3-test-2.js
new file mode 100644 (file)
index 0000000..bacf425
--- /dev/null
@@ -0,0 +1,73 @@
+description("Test of behavior NodeIterator when nodes are removed, from Acid3.");
+
+var iframe = document.createElement("iframe");
+iframe.setAttribute("src", "about:blank");
+document.body.appendChild(iframe);
+var doc = iframe.contentDocument;
+for (var i = doc.documentElement.childNodes.length-1; i >= 0; i -= 1)
+    doc.documentElement.removeChild(doc.documentElement.childNodes[i]);
+doc.documentElement.appendChild(doc.createElement('head'));
+doc.documentElement.firstChild.appendChild(doc.createElement('title'));
+doc.documentElement.appendChild(doc.createElement('body'));
+
+// test 2: Removing nodes during iteration
+var count = 0;
+var t1 = doc.body.appendChild(doc.createElement('t1'));
+var t2 = doc.body.appendChild(doc.createElement('t2'));
+var t3 = doc.body.appendChild(doc.createElement('t3'));
+var t4 = doc.body.appendChild(doc.createElement('t4'));
+var expect = function(n, node1, node2) {
+    count += 1;
+    shouldBe("count", "" + n);
+    nodea = node1;
+    nodeb = node2;
+    shouldBe("nodea", "nodeb");
+  };
+var callCount = 0;
+var filterFunctions = [
+    function (node) { expect(1, node, doc.body); return true; }, // filter 0
+    function (node) { expect(3, node, t1); return true; }, // filter 1
+    function (node) { expect(5, node, t2); return true; }, // filter 2
+    function (node) { expect(7, node, t3); doc.body.removeChild(t4); return true; }, // filter 3
+    function (node) { expect(9, node, t4); return true; }, // filter 4
+    function (node) { expect(11, node, t4); doc.body.removeChild(t4); return 2 /* REJECT */; }, // filter 5
+    function (node) { expect(12, node, t3); return true; }, // filter 6
+    function (node) { expect(14, node, t2); doc.body.removeChild(t2); return true; }, // filter 7
+    function (node) { expect(16, node, t1); return true; }, // filter 8
+];
+var i = doc.createNodeIterator(doc.documentElement.lastChild, 0xFFFFFFFF, function (node) { return filterFunctions[callCount++](node); }, true);
+    // * B 1 2 3 4
+expect(2, i.nextNode(), doc.body); // filter 0
+    // [B] * 1 2 3 4     
+expect(4, i.nextNode(), t1); // filter 1
+    // B [1] * 2 3 4
+expect(6, i.nextNode(), t2); // filter 2
+    // B 1 [2] * 3 4
+expect(8, i.nextNode(), t3); // filter 3
+    // B 1 2 [3] *
+doc.body.appendChild(t4);
+    // B 1 2 [3] * 4
+expect(10, i.nextNode(), t4); // filter 4
+    // B 1 2 3 [4] *
+expect(13, i.previousNode(), t3); // filters 5, 6
+    // B 1 2 3 * (4) // filter 5
+    // B 1 2 [3] *   // between 5 and 6
+    // B 1 2 * (3)   // filter 6
+    // B 1 2 * [3]
+expect(15, i.previousNode(), t2); // filter 7
+    // B 1 * (2) [3]
+    // -- spec says "For instance, if a NodeFilter removes a node
+    //    from a document, it can still accept the node, which
+    //    means that the node may be returned by the NodeIterator
+    //    or TreeWalker even though it is no longer in the subtree
+    //    being traversed."
+    // -- but it also says "If changes to the iterated list do not
+    //    remove the reference node, they do not affect the state
+    //    of the NodeIterator."
+    // B 1 * [3]
+expect(17, i.previousNode(), t1); // filter 8
+    // B [1] * 3
+
+document.body.removeChild(iframe);
+
+var successfullyParsed = true;
index 570769d..0875919 100644 (file)
@@ -27,6 +27,4 @@ shouldBe("w.nextSibling()", "document.body"); // 10
 shouldBe("w.previousSibling()", "document.body.previousSibling"); // 11
 shouldBe("iteration", "11");
 
-var succesfullyParsed = true;
-
 var successfullyParsed = true;
index ca45fbe..2ff27fc 100644 (file)
@@ -1,3 +1,25 @@
+2008-03-25  Darin Adler  <darin@apple.com>
+
+        Reviewed by Anders.
+
+        - fix http://bugs.webkit.org/show_bug.cgi?id=17252
+          Acid3 test removing Nodes during NodeIterator walk fails (affects Acid3 test 2)
+
+        Test: traversal/acid3-test-2.html
+
+        * bindings/js/JSNodeIteratorCustom.cpp:
+        (WebCore::JSNodeIterator::nextNode): Update since result is PassRefPtr.
+        (WebCore::JSNodeIterator::previousNode): Ditto.
+        * dom/NodeIterator.cpp:
+        (WebCore::NodeIterator::nextNode): Changed result to PassRefPtr. Added code to
+        track both the current candidate (which needs to move along to the next node
+        if current node is deleted) and the current provisional result (passed to acceptNode,
+        and needs to be returned even if it's deleted).
+        (WebCore::NodeIterator::previousNode): Ditto.
+        (WebCore::NodeIterator::nodeWillBeRemoved): Call updateForNodeRemoval for
+        m_candidateNode as well as m_referenceNode.
+        * dom/NodeIterator.h: Use PassRefPtr for return values.
+
 2008-03-25  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Anders
index 937efa0..11f8dc7 100644 (file)
@@ -41,7 +41,7 @@ JSValue* JSNodeIterator::nextNode(ExecState* exec, const List& args)
 {
     ExceptionCode ec = 0;
     JSValue* exception = 0;
-    Node* node = impl()->nextNode(ec, exception);
+    RefPtr<Node> node = impl()->nextNode(ec, exception);
     if (ec) {
         setDOMException(exec, ec);
         return jsUndefined();
@@ -50,14 +50,14 @@ JSValue* JSNodeIterator::nextNode(ExecState* exec, const List& args)
         exec->setException(exception);
         return jsUndefined();
     }
-    return toJS(exec, node);
+    return toJS(exec, node.get());
 }
 
 JSValue* JSNodeIterator::previousNode(ExecState* exec, const List& args)
 {
     ExceptionCode ec = 0;
     JSValue* exception = 0;
-    Node* node = impl()->previousNode(ec, exception);
+    RefPtr<Node> node = impl()->previousNode(ec, exception);
     if (ec) {
         setDOMException(exec, ec);
         return jsUndefined();
@@ -66,7 +66,7 @@ JSValue* JSNodeIterator::previousNode(ExecState* exec, const List& args)
         exec->setException(exception);
         return jsUndefined();
     }
-    return toJS(exec, node);
+    return toJS(exec, node.get());
 }
 
 }
index e623762..e22e8e6 100644 (file)
@@ -85,14 +85,14 @@ NodeIterator::~NodeIterator()
     root()->document()->detachNodeIterator(this);
 }
 
-Node* NodeIterator::nextNode(ExceptionCode& ec, JSValue*& exception)
+PassRefPtr<Node> NodeIterator::nextNode(ExceptionCode& ec, JSValue*& exception)
 {
     if (m_detached) {
         ec = INVALID_STATE_ERR;
         return 0;
     }
 
-    Node* result = 0;
+    RefPtr<Node> result;
 
     m_candidateNode = m_referenceNode;
     while (m_candidateNode.moveToNext(root())) {
@@ -100,28 +100,29 @@ Node* NodeIterator::nextNode(ExceptionCode& ec, JSValue*& exception)
         // In other words, FILTER_REJECT does not pass over descendants
         // of the rejected node. Hence, FILTER_REJECT is the same as FILTER_SKIP.
         exception = 0;
-        bool nodeWasAccepted = acceptNode(m_candidateNode.node.get(), exception) == NodeFilter::FILTER_ACCEPT;
+        RefPtr<Node> provisionalResult = m_candidateNode.node;
+        bool nodeWasAccepted = acceptNode(provisionalResult.get(), exception) == NodeFilter::FILTER_ACCEPT;
         if (exception)
             break;
         if (nodeWasAccepted) {
             m_referenceNode = m_candidateNode;
-            result = m_referenceNode.node.get();
+            result = provisionalResult.release();
             break;
         }
     }
 
     m_candidateNode.clear();
-    return result;
+    return result.release();
 }
 
-Node* NodeIterator::previousNode(ExceptionCode& ec, JSValue*& exception)
+PassRefPtr<Node> NodeIterator::previousNode(ExceptionCode& ec, JSValue*& exception)
 {
     if (m_detached) {
         ec = INVALID_STATE_ERR;
         return 0;
     }
 
-    Node* result = 0;
+    RefPtr<Node> result;
 
     m_candidateNode = m_referenceNode;
     while (m_candidateNode.moveToPrevious(root())) {
@@ -129,18 +130,19 @@ Node* NodeIterator::previousNode(ExceptionCode& ec, JSValue*& exception)
         // In other words, FILTER_REJECT does not pass over descendants
         // of the rejected node. Hence, FILTER_REJECT is the same as FILTER_SKIP.
         exception = 0;
-        bool nodeWasAccepted = acceptNode(m_candidateNode.node.get(), exception) == NodeFilter::FILTER_ACCEPT;
+        RefPtr<Node> provisionalResult = m_candidateNode.node;
+        bool nodeWasAccepted = acceptNode(provisionalResult.get(), exception) == NodeFilter::FILTER_ACCEPT;
         if (exception)
             break;
         if (nodeWasAccepted) {
             m_referenceNode = m_candidateNode;
-            result = m_referenceNode.node.get();
+            result = provisionalResult.release();
             break;
         }
     }
 
     m_candidateNode.clear();
-    return result;
+    return result.release();
 }
 
 void NodeIterator::detach()
@@ -152,6 +154,7 @@ void NodeIterator::detach()
 
 void NodeIterator::nodeWillBeRemoved(Node* removedNode)
 {
+    updateForNodeRemoval(removedNode, m_candidateNode);
     updateForNodeRemoval(removedNode, m_referenceNode);
 }
 
index a6648bf..35ba5a3 100644 (file)
@@ -37,8 +37,8 @@ namespace WebCore {
         NodeIterator(PassRefPtr<Node>, unsigned whatToShow, PassRefPtr<NodeFilter>, bool expandEntityReferences);
         virtual ~NodeIterator();
 
-        Node* nextNode(ExceptionCode&, KJS::JSValue*& exception);
-        Node* previousNode(ExceptionCode&, KJS::JSValue*& exception);
+        PassRefPtr<Node> nextNode(ExceptionCode&, KJS::JSValue*& exception);
+        PassRefPtr<Node> previousNode(ExceptionCode&, KJS::JSValue*& exception);
         void detach();
 
         Node* referenceNode() const { return m_referenceNode.node.get(); }
@@ -48,8 +48,8 @@ namespace WebCore {
         void nodeWillBeRemoved(Node*);
 
         // For non-JS bindings. Silently ignores the JavaScript exception if any.
-        Node* nextNode(ExceptionCode& ec) { KJS::JSValue* exception; return nextNode(ec, exception); }
-        Node* previousNode(ExceptionCode& ec) { KJS::JSValue* exception; return previousNode(ec, exception); }
+        PassRefPtr<Node> nextNode(ExceptionCode& ec) { KJS::JSValue* exception; return nextNode(ec, exception); }
+        PassRefPtr<Node> previousNode(ExceptionCode& ec) { KJS::JSValue* exception; return previousNode(ec, exception); }
 
     private:
         struct NodePointer {