- Revision
- 158708
- Author
- ach...@adobe.com
- Date
- 2013-11-05 18:15:50 -0800 (Tue, 05 Nov 2013)
Log Message
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".
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (158707 => 158708)
--- trunk/LayoutTests/ChangeLog 2013-11-06 02:09:45 UTC (rev 158707)
+++ trunk/LayoutTests/ChangeLog 2013-11-06 02:15:50 UTC (rev 158708)
@@ -1,3 +1,22 @@
+2013-11-05 Alexandru Chiculita <ach...@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 <mmaxfi...@apple.com>
text-decoration-skip: ink isn't tested with underlines that don't intersect the underlined text
Added: trunk/LayoutTests/inspector-protocol/dom/dom-remove-events-expected.txt (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom/dom-remove-events-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom/dom-remove-events-expected.txt 2013-11-06 02:15:50 UTC (rev 158708)
@@ -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
+
Added: trunk/LayoutTests/inspector-protocol/dom/dom-remove-events.html (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom/dom-remove-events.html (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom/dom-remove-events.html 2013-11-06 02:15:50 UTC (rev 158708)
@@ -0,0 +1,88 @@
+<html>
+<head>
+<script type="text/_javascript_" src=""
+<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>
Added: trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes-expected.txt (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes-expected.txt 2013-11-06 02:15:50 UTC (rev 158708)
@@ -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
+
Added: trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes.html (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes.html (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom/remove-multiple-nodes.html 2013-11-06 02:15:50 UTC (rev 158708)
@@ -0,0 +1,114 @@
+<html>
+<head>
+<script type="text/_javascript_" src=""
+<script>
+function moveNode()
+{
+ var targetElement = document.getElementById("target_element");
+ targetElement.addEventListener("DOMNodeRemoved", function() {
+ targetElement.removeEventListener("DOMNodeRemoved", arguments.callee);
+ document.getElementById("final_container").appendChild(this);
+ });
+ // Remove all the children inside parent container, including "#target_element".
+ var parentContainer = document.getElementById("parent_container");
+ parentContainer.textContent = "Parent container is now empty";
+}
+
+function test()
+{
+ var nodesById = {};
+
+ InspectorTest.eventHandler["DOM.setChildNodes"] = onSetChildNodes;
+ InspectorTest.eventHandler["DOM.childNodeRemoved"] = onChildNodeRemoved;
+ InspectorTest.eventHandler["DOM.childNodeInserted"] = onChildNodeInserted;
+
+ 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.log("Removing node " + getNodeIdentifier(nodeId));
+ delete nodesById[nodeId];
+ }
+
+ function onChildNodeInserted(response)
+ {
+ collectNode(response.params.node);
+ InspectorTest.log("Inserting node " + getNodeIdentifier(response.params.node.nodeId) + " into " + getNodeIdentifier(response.params.parentNodeId));
+ }
+
+ InspectorTest.sendCommand("DOM.getDocument", {}, onGotDocument);
+
+ function onGotDocument(msg)
+ {
+ InspectorTest.checkForError(msg);
+ InspectorTest.sendCommand("DOM.querySelectorAll", {"nodeId": msg.result.root.nodeId, "selector": "#parent_container,#final_container"}, onQuerySelectorAll);
+ }
+
+ function onQuerySelectorAll(response)
+ {
+ // Make sure we receive the children of the "#final_container" as they are added.
+ for (var i = 0; i < response.result.nodeIds.length; ++i)
+ InspectorTest.sendCommand("DOM.requestChildNodes", {nodeId: response.result.nodeIds[i]});
+
+ InspectorTest.sendCommand("Runtime.evaluate", {"_expression_": "moveNode()"}, function() {
+ InspectorTest.completeTest();
+ });
+ }
+}
+</script>
+</head>
+<body _onload_="runTest()">
+
+<p>Testing that DOM.childNodeRemoved is correctly triggered when all the parent children of a node are removed at once.</p>
+
+<div id="parent_container">
+ <div id="before">FAIL: Element "#before" should be removed</div>
+ <div id="target_element">Target element</div>
+ <div id="after">FAIL: Element "#after" should be removed</div>
+</div>
+
+<p id="final_container">Target element should have been moved after this line:</p>
+
+</body>
+</html>
Added: trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed-expected.txt (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed-expected.txt 2013-11-06 02:15:50 UTC (rev 158708)
@@ -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
+
Added: trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed.html (0 => 158708)
--- trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed.html (rev 0)
+++ trunk/LayoutTests/inspector-protocol/dom-debugger/node-removed.html 2013-11-06 02:15:50 UTC (rev 158708)
@@ -0,0 +1,146 @@
+<html>
+<head>
+<script type="text/_javascript_" src=""
+<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>
Modified: trunk/Source/WebCore/ChangeLog (158707 => 158708)
--- trunk/Source/WebCore/ChangeLog 2013-11-06 02:09:45 UTC (rev 158707)
+++ trunk/Source/WebCore/ChangeLog 2013-11-06 02:15:50 UTC (rev 158708)
@@ -1,3 +1,35 @@
+2013-11-05 Alexandru Chiculita <ach...@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.
+
+ 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):
+
2013-11-05 Ryuan Choi <ryuan.c...@samsung.com>
Unreviewed build fix on CMake based ports with GLES.
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (158707 => 158708)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2013-11-06 02:09:45 UTC (rev 158707)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2013-11-06 02:15:50 UTC (rev 158708)
@@ -585,6 +585,8 @@
void ContainerNode::removeBetween(Node* previousChild, Node* nextChild, Node& oldChild)
{
+ InspectorInstrumentation::didRemoveDOMNode(&oldChild.document(), &oldChild);
+
NoEventDispatchAssertion assertNoEventDispatch;
ASSERT(oldChild.parentNode() == this);
Modified: trunk/Source/WebCore/inspector/InspectorInstrumentation.h (158707 => 158708)
--- trunk/Source/WebCore/inspector/InspectorInstrumentation.h 2013-11-06 02:09:45 UTC (rev 158707)
+++ trunk/Source/WebCore/inspector/InspectorInstrumentation.h 2013-11-06 02:15:50 UTC (rev 158708)
@@ -119,6 +119,7 @@
static void willInsertDOMNode(Document*, Node* parent);
static void didInsertDOMNode(Document*, Node*);
static void willRemoveDOMNode(Document*, Node*);
+ static void didRemoveDOMNode(Document*, Node*);
static void willModifyDOMAttr(Document*, Element*, const AtomicString& oldValue, const AtomicString& newValue);
static void didModifyDOMAttr(Document*, Element*, const AtomicString& name, const AtomicString& value);
static void didRemoveDOMAttr(Document*, Element*, const AtomicString& name);
@@ -565,10 +566,20 @@
{
#if ENABLE(INSPECTOR)
FAST_RETURN_IF_NO_FRONTENDS(void());
- if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForDocument(document)) {
+ if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForDocument(document))
willRemoveDOMNodeImpl(instrumentingAgents, node);
+#else
+ UNUSED_PARAM(document);
+ UNUSED_PARAM(node);
+#endif
+}
+
+inline void InspectorInstrumentation::didRemoveDOMNode(Document* document, Node* node)
+{
+#if ENABLE(INSPECTOR)
+ FAST_RETURN_IF_NO_FRONTENDS(void());
+ if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForDocument(document))
didRemoveDOMNodeImpl(instrumentingAgents, node);
- }
#else
UNUSED_PARAM(document);
UNUSED_PARAM(node);
