Web Inspector: CSS Regions: Removing a content node of a ContentFlow …

…from the DOM will send a 0 nodeId https://bugs.webkit.org/show_bug.cgi?id=123577 Source/WebCore: Reviewed by Timothy Hatcher. Test: inspector-protocol/model/content-flow-content-removal.html Do not send unregister events for the content nodes of a flow when the element is not part of the DOM anymore. We already send an unbind event, so the inspector is already notified that the node was removed. * inspector/InspectorCSSAgent.cpp: (WebCore::InspectorCSSAgent::didUnregisterNamedFlowContentElement): Source/WebInspectorUI: Reviewed by Timothy Hatcher. Fixed the content node removal from the content flow. * UserInterface/ContentFlowTreeContentView.js: * UserInterface/DOMTreeManager.js: (WebInspector.DOMTreeManager): (WebInspector.DOMTreeManager.prototype._createContentFlowFromPayload): Registered all the content nodes in the _contentNodesToFlowsMap. (WebInspector.DOMTreeManager.prototype._unbind): Added call to _removeContentNodeFromFlowIfNeeded. (WebInspector.DOMTreeManager.prototype._removeContentNodeFromFlowIfNeeded): Called from _unbind to check and remove a node from it's parent content flow if needed. (WebInspector.DOMTreeManager.prototype.unregisteredNamedFlowContentElement): LayoutTests: Reviewed by Timothy Hatcher. Added test to check that the notification that an element was removed from the ContentFlow is handled correctly in the WebInspector even if the element is not part of the DOM anymore. * inspector-protocol/model/content-flow-content-removal-expected.txt: Added. * inspector-protocol/model/content-flow-content-removal.html: Added. Canonical link: https://commits.webkit.org/142166@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@158854 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Nov 7, 2013 · 7e85897 · 7e85897
1 parent 37b28a2
commit 7e85897
Show file tree

Hide file tree

Showing 8 changed files with 156 additions and 5 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2013-11-07  Alexandru Chiculita  <achicu@adobe.com>
+
+        Web Inspector: CSS Regions: Removing a content node of a ContentFlow from the DOM will send a 0 nodeId
+        https://bugs.webkit.org/show_bug.cgi?id=123577
+
+        Reviewed by Timothy Hatcher.
+
+        Added test to check that the notification that an element was removed from the ContentFlow is handled
+        correctly in the WebInspector even if the element is not part of the DOM anymore.
+
+        * inspector-protocol/model/content-flow-content-removal-expected.txt: Added.
+        * inspector-protocol/model/content-flow-content-removal.html: Added.
+
 2013-10-24  Jer Noble  <jer.noble@apple.com>
 
         [MSE] Add mock MediaSource classes for testing.

diff --git a/LayoutTests/inspector-protocol/model/content-flow-content-removal-expected.txt b/LayoutTests/inspector-protocol/model/content-flow-content-removal-expected.txt
@@ -0,0 +1,9 @@
+Testing that the ContentFlows events are correctly dispatched when content nodes are detached from the DOM.
+
+PASS: ContentFlow was added.
+PASS: ContentFlow.contentNodes has a length of 2.
+PASS: ContentFlow.contentNodes[0].id is "#contentStatic".
+PASS: ContentFlow.contentNodes[1].id is "#contentRemoved".
+PASS: "#contentRemoved" was removed.
+PASS: "#contentRemoved" cannot be found in the contentNodes list.
+
diff --git a/LayoutTests/inspector-protocol/model/content-flow-content-removal.html b/LayoutTests/inspector-protocol/model/content-flow-content-removal.html
@@ -0,0 +1,64 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.content
+{
+    -webkit-flow-into: flow;
+}
+</style>
+<script type="text/javascript" src="../../http/tests/inspector-protocol/resources/protocol-test.js"></script>
+<script>
+function changeFlowContent()
+{
+    document.getElementById("contentRemoved").remove();
+}
+
+function test()
+{
+    InspectorTest.importInspectorScripts();
+
+    var contentFlow;
+
+    WebInspector.frameResourceManager.addEventListener(WebInspector.FrameResourceManager.Event.MainFrameDidChange, function(event) {
+        var domTree = WebInspector.frameResourceManager.mainFrame.domTree;
+        domTree.addEventListener(WebInspector.DOMTree.Event.RootDOMNodeInvalidated, onRootDOMNodeInvalidated, null);
+        domTree.addEventListener(WebInspector.DOMTree.Event.ContentFlowWasAdded, onContentFlowWasAdded, null);
+        domTree.requestContentFlowList();
+    });
+
+    function onRootDOMNodeInvalidated()
+    {
+        WebInspector.frameResourceManager.mainFrame.domTree.requestContentFlowList();
+    }
+
+    function onContentFlowWasAdded(event)
+    {
+        contentFlow = event.data.flow;
+        InspectorTest.assert(contentFlow.name === "flow", "ContentFlow was added.");
+        InspectorTest.assert(contentFlow.contentNodes.length === 2, "ContentFlow.contentNodes has a length of 2.");
+        InspectorTest.assert(contentFlow.contentNodes[0].getAttribute("id") === "contentStatic", "ContentFlow.contentNodes[0].id is \"#contentStatic\".");
+        InspectorTest.assert(contentFlow.contentNodes[1].getAttribute("id") === "contentRemoved", "ContentFlow.contentNodes[1].id is \"#contentRemoved\".");
+
+        contentFlow.addEventListener(WebInspector.ContentFlow.Event.ContentNodeWasRemoved, onContentNodeWasRemoved, null);
+
+        InspectorTest.sendCommand("Runtime.evaluate", {expression: "changeFlowContent()"});
+    }
+
+    function onContentNodeWasRemoved(event)
+    {
+        InspectorTest.assert(event.data.node.getAttribute("id") === "contentRemoved", "\"#contentRemoved\" was removed.");
+        InspectorTest.assert(contentFlow.contentNodes.indexOf(event.data.node) === -1, "\"#contentRemoved\" cannot be found in the contentNodes list.");
+        InspectorTest.completeTest();
+    }
+}
+</script>
+</head>
+<body onload="runTest()">
+    <p>Testing that the ContentFlows events are correctly dispatched when content nodes are detached from the DOM.</p>
+
+    <div id="contentStatic" class="content"></div>
+    <div id="contentRemoved" class="content"></div>
+
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,18 @@
+2013-11-07  Alexandru Chiculita  <achicu@adobe.com>
+
+        Web Inspector: CSS Regions: Removing a content node of a ContentFlow from the DOM will send a 0 nodeId
+        https://bugs.webkit.org/show_bug.cgi?id=123577
+
+        Reviewed by Timothy Hatcher.
+
+        Test: inspector-protocol/model/content-flow-content-removal.html
+
+        Do not send unregister events for the content nodes of a flow when the element is not part of the DOM
+        anymore. We already send an unbind event, so the inspector is already notified that the node was removed.
+
+        * inspector/InspectorCSSAgent.cpp:
+        (WebCore::InspectorCSSAgent::didUnregisterNamedFlowContentElement):
+
 2013-10-30  Jer Noble  <jer.noble@apple.com>
 
         [MSE] Add mock MediaSource classes for testing.

diff --git a/Source/WebCore/inspector/InspectorCSSAgent.cpp b/Source/WebCore/inspector/InspectorCSSAgent.cpp
@@ -775,6 +775,10 @@ void InspectorCSSAgent::didUnregisterNamedFlowContentElement(Document* document,
 
     ErrorString errorString;
     int contentElementNodeId = m_domAgent->pushNodeToFrontend(&errorString, documentNodeId, contentElement);
+    if (!contentElementNodeId) {
+        // We've already notified that the DOM node was removed from the DOM, so there's no need to send another event.
+        return;
+    }
     m_frontend->unregisteredNamedFlowContentElement(documentNodeId, namedFlow->name().string(), contentElementNodeId);
 }
 

diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
@@ -1,3 +1,22 @@
+2013-11-07  Alexandru Chiculita  <achicu@adobe.com>
+
+        Web Inspector: CSS Regions: Removing a content node of a ContentFlow from the DOM will send a 0 nodeId
+        https://bugs.webkit.org/show_bug.cgi?id=123577
+
+        Reviewed by  Timothy Hatcher.
+
+        Fixed the content node removal from the content flow.
+
+        * UserInterface/ContentFlowTreeContentView.js:
+        * UserInterface/DOMTreeManager.js:
+        (WebInspector.DOMTreeManager):
+        (WebInspector.DOMTreeManager.prototype._createContentFlowFromPayload): Registered all the content nodes
+        in the _contentNodesToFlowsMap.
+        (WebInspector.DOMTreeManager.prototype._unbind): Added call to _removeContentNodeFromFlowIfNeeded.
+        (WebInspector.DOMTreeManager.prototype._removeContentNodeFromFlowIfNeeded): Called from _unbind to check
+        and remove a node from it's parent content flow if needed.
+        (WebInspector.DOMTreeManager.prototype.unregisteredNamedFlowContentElement):
+
 2013-11-06  Timothy Hatcher  <timothy@apple.com>
 
         Properly null check positionToReveal in ResourceSidebarPanel.prototype.showSourceCode.

diff --git a/Source/WebInspectorUI/UserInterface/ContentFlowTreeContentView.js b/Source/WebInspectorUI/UserInterface/ContentFlowTreeContentView.js
@@ -183,6 +183,6 @@ WebInspector.ContentFlowTreeContentView.prototype = {
         contentNodeOutline.close();
         contentNodeOutline.element.remove();
 
-        this._nodesMap.remove(event.data.node.id);
+        this._nodesMap.delete(event.data.node.id);
     }
 };
diff --git a/Source/WebInspectorUI/UserInterface/DOMTreeManager.js b/Source/WebInspectorUI/UserInterface/DOMTreeManager.js
@@ -40,6 +40,7 @@ WebInspector.DOMTreeManager = function() {
     this._document = null;
     this._attributeLoadNodeIds = {};
     this._flows = {};
+    this._contentNodesToFlowsMap = new Map;
 }
 
 WebInspector.Object.addConstructorFunctions(WebInspector.DOMTreeManager);
@@ -337,6 +338,8 @@ WebInspector.DOMTreeManager.prototype = {
      */
     _unbind: function(node)
     {
+        this._removeContentNodeFromFlowIfNeeded(node);
+
         delete this._idToDOMNode[node.id];
         for (var i = 0; node.children && i < node.children.length; ++i)
             this._unbind(node.children[i]);
@@ -536,7 +539,14 @@ WebInspector.DOMTreeManager.prototype = {
     _createContentFlowFromPayload: function(flowPayload)
     {
         // FIXME: Collect the regions from the payload.
-        return new WebInspector.ContentFlow(flowPayload.documentNodeId, flowPayload.name, flowPayload.overset, flowPayload.content.map(this.nodeForId.bind(this)));
+        var flow = new WebInspector.ContentFlow(flowPayload.documentNodeId, flowPayload.name, flowPayload.overset, flowPayload.content.map(this.nodeForId.bind(this)));
+
+        for (var contentNode of flow.contentNodes) {
+            console.assert(!this._contentNodesToFlowsMap.has(contentNode.id));
+            this._contentNodesToFlowsMap.set(contentNode.id, flow);
+        }
+
+        return flow;
     },
 
     _updateContentFlowFromPayload: function(contentFlow, flowPayload)
@@ -557,6 +567,7 @@ WebInspector.DOMTreeManager.prototype = {
                 console.error("Error while getting the named flows for document " + documentNodeIdentifier + ": " + error);
                 return;
             }
+            this._contentNodesToFlowsMap.clear();
             var contentFlows = [];
             for (var i = 0; i < flows.length; ++i) {
                 var flowPayload = flows[i];
@@ -614,21 +625,37 @@ WebInspector.DOMTreeManager.prototype = {
     {
         var flowKey = WebInspector.DOMTreeManager._flowPayloadHashKey({documentNodeId: documentNodeIdentifier, name: flowName});
         console.assert(this._flows.hasOwnProperty(flowKey));
+        console.assert(!this._contentNodesToFlowsMap.has(contentNodeId));
 
         var flow = this._flows[flowKey];
         var contentNode = this.nodeForId(contentNodeId);
 
+        this._contentNodesToFlowsMap.set(contentNode.id, flow);
+
         if (nextContentElementNodeId)
             flow.insertContentNodeBefore(contentNode, this.nodeForId(nextContentElementNodeId));
         else
             flow.appendContentNode(contentNode);
     },
 
+    _removeContentNodeFromFlowIfNeeded: function(node)
+    {
+        if (!this._contentNodesToFlowsMap.has(node.id))
+            return;
+        var flow = this._contentNodesToFlowsMap.get(node.id);
+        this._contentNodesToFlowsMap.delete(node.id);
+        flow.removeContentNode(node);
+    },
+
     unregisteredNamedFlowContentElement: function(documentNodeIdentifier, flowName, contentNodeId)
     {
-        var flowKey = WebInspector.DOMTreeManager._flowPayloadHashKey({documentNodeId: documentNodeIdentifier, name: flowName});
-        console.assert(this._flows.hasOwnProperty(flowKey));
-        this._flows[flowKey].removeContentNode(this.nodeForId(contentNodeId));
+        console.assert(this._contentNodesToFlowsMap.has(contentNodeId));
+
+        var flow = this._contentNodesToFlowsMap.get(contentNodeId);
+        console.assert(flow.id === WebInspector.DOMTreeManager._flowPayloadHashKey({documentNodeId: documentNodeIdentifier, name: flowName}));
+
+        this._contentNodesToFlowsMap.delete(contentNodeId);
+        flow.removeContentNode(this.nodeForId(contentNodeId));
     }
 }