Web Inspector: Moving an element while in the DOMNodeRemoved handler …

…will hide it in the inspector

https://bugs.webkit.org/show_bug.cgi?id=123516

Reviewed by Timothy Hatcher.

Source/WebCore:

InspectorInstrumentation::willRemoveDOMNode was actually calling both willRemoveDOMNodeImpl and
didRemoveDOMNodeImpl, making the DOMAgent unbind the element even if it was still part of the DOM.

Because of that the DOMAgent was sending two events:
1. When the element was about to be removed, just before JS "DOMNodeRemoved" was triggered.
2. When the element was actually removed.

Note that inspector's event #2 will not know about the node, as it just removed it from the
internal hashmap, so it will just use a nodeID == 0 for it.

This patch adds a separate call to InspectorInstrumentation::didRemoveDOMNode, just before the
element is about to be removed. The InspectorInstrumentation::willRemoveDOMNode call is now only used
by the DOMDebugger to trigger the DOM breakpoints in the Web Inspector. That feature is not exposed
in the new Inspector UI, but can be used/tested using the protocol directly.

Tests: inspector-protocol/dom-debugger/node-removed.html
       inspector-protocol/dom/dom-remove-events.html
       inspector-protocol/dom/remove-multiple-nodes.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeBetween):
* inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::willRemoveDOMNode):
(WebCore::InspectorInstrumentation::didRemoveDOMNode):

LayoutTests:

Added tests to check that the DOM.childNodeRemoved inspector-protocol message is dispatched
correctly when DOM nodes are moved while inside the "DOMNodeRemoved" event handler.

* inspector-protocol/dom-debugger/node-removed-expected.txt: Added.
* inspector-protocol/dom-debugger/node-removed.html: Added. Checking that the DOMDebugger agent
is still sending the node-removed events.
* inspector-protocol/dom/dom-remove-events-expected.txt: Added.
* inspector-protocol/dom/dom-remove-events.html: Added. Test with a single DOM remove event.
* inspector-protocol/dom/remove-multiple-nodes-expected.txt: Added.
* inspector-protocol/dom/remove-multiple-nodes.html: Added. Test case when multiple children
are removed at once with parentNode.textContent = "String".


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

LayoutTests/ChangeLog

@@ -1,3 +1,22 @@
+2013-11-05  Alexandru Chiculita  <achicu@adobe.com>
+
+        Web Inspector: Moving an element while in the DOMNodeRemoved handler will hide it in the inspector
+        https://bugs.webkit.org/show_bug.cgi?id=123516
+
+        Reviewed by Timothy Hatcher.
+
+        Added tests to check that the DOM.childNodeRemoved inspector-protocol message is dispatched
+        correctly when DOM nodes are moved while inside the "DOMNodeRemoved" event handler.
+
+        * inspector-protocol/dom-debugger/node-removed-expected.txt: Added.
+        * inspector-protocol/dom-debugger/node-removed.html: Added. Checking that the DOMDebugger agent 
+        is still sending the node-removed events.
+        * inspector-protocol/dom/dom-remove-events-expected.txt: Added.
+        * inspector-protocol/dom/dom-remove-events.html: Added. Test with a single DOM remove event.
+        * inspector-protocol/dom/remove-multiple-nodes-expected.txt: Added.
+        * inspector-protocol/dom/remove-multiple-nodes.html: Added. Test case when multiple children
+        are removed at once with parentNode.textContent = "String".
+
 2013-11-05  Myles C. Maxfield  <mmaxfield@apple.com>
 
         text-decoration-skip: ink isn't tested with underlines that don't intersect the underlined text

LayoutTests/inspector-protocol/dom-debugger/node-removed-expected.txt

@@ -0,0 +1,9 @@
+Testing that DOM.childNodeRemoved is correctly triggered even when the element is moved while paused in the debugger on a DOMDebugger "node-removed" breakpoint.
+
+Breakpoints Enabled
+Found <script>
+Stopped on DOM breakpoint
+PASS: onChildNodeRemoved called for #target_element
+PASS: onChildNodeInserted called for parent node #final_container
+PASS: onChildNodeInserted called for child node #target_element
+

LayoutTests/inspector-protocol/dom-debugger/node-removed.html

@@ -0,0 +1,146 @@
+<html>
+<head>
+<script type="text/javascript" src="../../http/tests/inspector-protocol/resources/protocol-test.js"></script>
+<script>
+window.onload = runTest;
+
+function removeNode()
+{
+    document.getElementById("target_element").remove();
+}
+
+function moveNode()
+{
+    var targetElement = document.getElementById("target_element");
+    document.getElementById("final_container").appendChild(targetElement);
+}
+
+function test()
+{
+    var nodesById = {};
+
+    InspectorTest.eventHandler["DOM.setChildNodes"] = onSetChildNodes;
+    InspectorTest.eventHandler["DOM.childNodeRemoved"] = onChildNodeRemoved;
+    InspectorTest.eventHandler["DOM.childNodeInserted"] = onChildNodeInserted;
+    InspectorTest.eventHandler["Debugger.paused"] = onDebuggerPaused;
+    InspectorTest.eventHandler["Debugger.scriptParsed"] = onScriptParsed;
+
+    function createNodeAttributesMap(attributes)
+    {
+        var attributesMap = {};
+        for (var i = 0; i < attributes.length; i += 2)
+            attributesMap[attributes[i]] = attributes[i + 1];
+        return attributesMap;
+    }
+
+    function collectNode(node)
+    {
+        if (node.nodeType === 1)
+            node.attributes = createNodeAttributesMap(node.attributes);
+        if (node.children)
+            node.children.forEach(collectNode);
+        nodesById[node.nodeId] = node;
+    }
+
+    function nodeToString(node)
+    {
+        switch (node.nodeType) {
+        case 1:
+            return node.localName + "#" + node.attributes.id;
+        case 3:
+            return "<text node " + JSON.stringify(node.nodeValue) + ">";
+        default:
+            return "<nodeType " + node.nodeType + ">";
+        }
+    }
+
+    function getNodeIdentifier(nodeId)
+    {
+        if (!nodeId)
+            return "<invalid node id>";
+        var node = nodesById[nodeId];
+        return node ? nodeToString(node) : "<unknown node>";
+    }
+
+    function onSetChildNodes(response)
+    {
+        response.params.nodes.forEach(collectNode);
+    }
+
+    function onChildNodeRemoved(response)
+    {
+        var nodeId = response.params.nodeId;
+        InspectorTest.assert(getNodeIdentifier(nodeId) === "div#target_element", "onChildNodeRemoved called for #target_element");
+        delete nodesById[nodeId];
+    }
+
+    function onChildNodeInserted(response)
+    {
+        collectNode(response.params.node);
+        InspectorTest.assert(getNodeIdentifier(response.params.parentNodeId) === "div#final_container", "onChildNodeInserted called for parent node #final_container");
+        InspectorTest.assert(getNodeIdentifier(response.params.node.nodeId) === "div#target_element", "onChildNodeInserted called for child node #target_element");
+    }
+
+    function onDebuggerPaused(response)
+    {   
+        InspectorTest.log("Stopped on DOM breakpoint");
+        InspectorTest.sendCommand("Runtime.evaluate", {"expression": "moveNode()"}, function() {
+            InspectorTest.sendCommand("Debugger.resume");
+            InspectorTest.completeTest();
+        });
+    }
+
+    function onScriptParsed(messageObject)
+    {
+        // FIXME: The DOM breakpoints are not working unless there's a JS brakpoint set. Setting a fake breakpoint to workaround that.
+        // https://bugs.webkit.org/show_bug.cgi?id=123770 
+        if (/node-removed\.html$/.test(messageObject.params.url) && messageObject.params.startLine > 20) {
+            InspectorTest.eventHandler["Debugger.scriptParsed"] = null;
+            InspectorTest.log("Found <script>");
+            var location = {scriptId: messageObject.params.scriptId, lineNumber: messageObject.params.startLine + 2, columnNumber: 0};
+            InspectorTest.sendCommand("Debugger.setBreakpoint", {location: location}, function() {
+                InspectorTest.sendCommand("DOM.getDocument", {}, onGotDocument);
+            });
+        }
+    }
+
+    function onGotDocument(response)
+    {
+        InspectorTest.checkForError(response);
+        InspectorTest.sendCommand("DOM.querySelectorAll", {"nodeId": response.result.root.nodeId, "selector": "#target_element,#final_container"}, onQuerySelectorAll);
+    }
+
+    function onQuerySelectorAll(response)
+    {
+        var targetElementId = response.result.nodeIds[0];
+        var finalContainerId = response.result.nodeIds[1];
+
+        InspectorTest.sendCommand("DOMDebugger.setDOMBreakpoint", {nodeId: targetElementId, type: "node-removed"});
+        InspectorTest.sendCommand("DOM.requestChildNodes", {nodeId: finalContainerId});
+
+        InspectorTest.sendCommand("Runtime.evaluate", {"expression": "removeNode()"});        
+    }
+
+    InspectorTest.sendCommand("Debugger.enable", {});
+    InspectorTest.sendCommand("Debugger.setBreakpointsActive", {active: true}, function() {
+        InspectorTest.log("Breakpoints Enabled");
+    });
+}
+</script>
+</head>
+<body>
+
+<p>Testing that DOM.childNodeRemoved is correctly triggered even when the element is moved while paused in the debugger on a DOMDebugger "node-removed" breakpoint.</p>
+
+<div id="target_element"></div>
+<div id="final_container"></div>
+
+<!-- Script tag required to workaround bug 123770. See onScriptParsed for details. -->
+<script>// Line 0
+function testFunction() { // Line 1
+    console.log("FAIL: Workaround JS code should not run.");
+}
+</script>
+
+</body>
+</html>

LayoutTests/inspector-protocol/dom/dom-remove-events-expected.txt

@@ -0,0 +1,6 @@
+Testing that DOM.childNodeRemoved is correctly triggered only when the element is actually going to be removed from the DOM.
+
+PASS: onChildNodeRemoved called for #target_element
+PASS: onChildNodeInserted called for parent node #final_container
+PASS: onChildNodeInserted called for child node #target_element
+

LayoutTests/inspector-protocol/dom/dom-remove-events.html

@@ -0,0 +1,88 @@
+<html>
+<head>
+<script type="text/javascript" src="../../http/tests/inspector-protocol/resources/protocol-test.js"></script>
+<script>
+function moveNode()
+{
+    var tergetElement = document.getElementById("target_element");
+    tergetElement.addEventListener("DOMNodeRemoved", function() {
+        tergetElement.removeEventListener("DOMNodeRemoved", arguments.callee);
+        document.getElementById("final_container").appendChild(this);
+    });
+    tergetElement.remove();
+}
+
+function test()
+{
+    var nodesById = {};
+
+    InspectorTest.eventHandler["DOM.setChildNodes"] = onSetChildNodes;
+    InspectorTest.eventHandler["DOM.childNodeRemoved"] = onChildNodeRemoved;
+    InspectorTest.eventHandler["DOM.childNodeInserted"] = onChildNodeInserted;
+
+    function createNodeAttributesMap(node)
+    {
+        var attributes = {};
+        for (var i = 0; i < node.attributes.length; i += 2)
+            attributes[node.attributes[i]] = node.attributes[i + 1];
+        return attributes;
+    }
+
+    function collectNode(node)
+    {
+        nodesById[node.nodeId] = createNodeAttributesMap(node);
+    }
+
+    function getNodeIdentifier(nodeId)
+    {
+        var node = nodesById[nodeId];
+        return node ? node.id : "<unknown node>";
+    }
+
+    function onSetChildNodes(response)
+    {
+        response.params.nodes.forEach(collectNode);
+    }
+
+    function onChildNodeRemoved(response)
+    {
+        var nodeId = response.params.nodeId;
+        InspectorTest.assert(getNodeIdentifier(nodeId) === "target_element", "onChildNodeRemoved called for #target_element");
+        delete nodesById[nodeId];
+    }
+
+    function onChildNodeInserted(response)
+    {
+        collectNode(response.params.node);
+        InspectorTest.assert(getNodeIdentifier(response.params.parentNodeId) === "final_container", "onChildNodeInserted called for parent node #final_container");
+        InspectorTest.assert(getNodeIdentifier(response.params.node.nodeId) === "target_element", "onChildNodeInserted called for child node #target_element");
+    }
+
+    InspectorTest.sendCommand("DOM.getDocument", {}, onGotDocument);
+
+    function onGotDocument(msg)
+    {
+        InspectorTest.checkForError(msg);
+        InspectorTest.sendCommand("DOM.querySelector", {"nodeId": msg.result.root.nodeId, "selector": "#final_container"}, onQuerySelector);
+    }
+
+    function onQuerySelector(response)
+    {
+        // Make sure we receive the children of the "#final_container" as they are added.
+        InspectorTest.sendCommand("DOM.requestChildNodes", {nodeId: response.result.nodeId});
+        InspectorTest.sendCommand("Runtime.evaluate", {"expression": "moveNode()"}, function() {
+            InspectorTest.completeTest();
+        });
+    }
+}
+</script>
+</head>
+<body onload="runTest()">
+
+<p>Testing that DOM.childNodeRemoved is correctly triggered only when the element is actually going to be removed from the DOM.</p>
+
+<div id="target_element"></div>
+<div id="final_container"></div>
+
+</body>
+</html>

LayoutTests/inspector-protocol/dom/remove-multiple-nodes-expected.txt

@@ -0,0 +1,12 @@
+Testing that DOM.childNodeRemoved is correctly triggered when all the parent children of a node are removed at once.
+
+Parent container is now empty
+Target element should have been moved after this line:
+Target element
+
+Removing node div#target_element
+Inserting node div#target_element into p#final_container
+Removing node div#before
+Removing node div#after
+Inserting node <text node "Parent container is now empty"> into div#parent_container
+