Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the node...
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2019 20:20:43 +0000 (20:20 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2019 20:20:43 +0000 (20:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203349

Reviewed by Matt Baker.

Source/WebCore:

Test: inspector/dom-debugger/dom-breakpoints.html

Replace `targetNode` (which was a `Runtime.RemoteObject`) with a `targetNodeId` (which is a
`DOM.NodeId`) when dispatching `DOMDebugger` pauses. Additionally, include the ancestor's
`DOM.NodeId` as the `targetNodeId` whenever an ancestor is removed that has a descendant
with a node removed DOM breakpoint.

* inspector/agents/page/PageDOMDebuggerAgent.h:
* inspector/agents/page/PageDOMDebuggerAgent.cpp:
(WebCore::PageDOMDebuggerAgent::willRemoveDOMNode):
(WebCore::PageDOMDebuggerAgent::descriptionForDOMEvent):

* inspector/agents/InspectorDOMAgent.h:
* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::pushNodeToFrontend): Added.

Source/WebInspectorUI:

Replace `targetNode` (which was a `Runtime.RemoteObject`) with a `targetNodeId` (which is a
`DOM.NodeId`) when dispatching `DOMDebugger` pauses. Additionally, include the ancestor's
`DOM.NodeId` as the `targetNodeId` whenever an ancestor is removed that has a descendant
with a node removed DOM breakpoint.

* UserInterface/Views/SourcesNavigationSidebarPanel.js:
(WI.SourcesNavigationSidebarPanel.prototype._updatePauseReasonSection):

* Localizations/en.lproj/localizedStrings.js:

LayoutTests:

* inspector/dom-debugger/dom-breakpoints.html:
* inspector/dom-debugger/dom-breakpoints-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/inspector/dom-debugger/dom-breakpoints-expected.txt
LayoutTests/inspector/dom-debugger/dom-breakpoints.html
Source/WebCore/ChangeLog
Source/WebCore/inspector/agents/InspectorDOMAgent.cpp
Source/WebCore/inspector/agents/InspectorDOMAgent.h
Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp
Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.h
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js

index 7a3364a..ee42594 100644 (file)
@@ -1,3 +1,13 @@
+2019-10-31  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the node is removed from the main DOM tree, not just when it's removed from it's parent
+        https://bugs.webkit.org/show_bug.cgi?id=203349
+
+        Reviewed by Matt Baker.
+
+        * inspector/dom-debugger/dom-breakpoints.html:
+        * inspector/dom-debugger/dom-breakpoints-expected.txt:
+
 2019-10-31  Ryosuke Niwa  <rniwa@webkit.org>
 
         Integrate resize event with HTML5 event loop
index 0e1342f..2a76720 100644 (file)
@@ -15,6 +15,10 @@ PASS: Breakpoint should have expected type.
 Call DOM operation.
 PAUSED:
 PASS: Pause reason should be DOM.
+PASS: Pause type should be 'subtree-modified'.
+PASS: Pause nodeId should be expected value.
+PASS: Pause insertion should be 'true'.
+PASS: Pause targetNodeId should match nodeId.
 CALL STACK:
 0: [F] subtreeModifiedTest
 1: [P] Global Code
@@ -45,6 +49,8 @@ PASS: Breakpoint should have expected type.
 Call DOM operation.
 PAUSED:
 PASS: Pause reason should be DOM.
+PASS: Pause type should be 'attribute-modified'.
+PASS: Pause nodeId should be expected value.
 CALL STACK:
 0: [F] attributeModifiedTest
 1: [P] Global Code
@@ -69,30 +75,65 @@ Wait for evaluate in page to return.
 PASS: Should not pause for removed breakpoint.
 -- Running test teardown.
 
--- Running test case: DOMBreakpoints.NodeRemoved.BreakpointEnabled
+-- Running test case: DOMBreakpoints.NodeRemovedSelf.BreakpointEnabled
 PASS: Added 'node-removed' breakpoint.
 PASS: Breakpoint should have expected type.
 Call DOM operation.
 PAUSED:
 PASS: Pause reason should be DOM.
+PASS: Pause type should be 'node-removed'.
+PASS: Pause nodeId should be expected value.
 CALL STACK:
-0: [F] nodeRemovedTest
+0: [F] nodeRemovedDirectTest
 1: [P] Global Code
 -- Running test teardown.
 
--- Running test case: DOMBreakpoints.NodeRemoved.BreakpointDisabled
+-- Running test case: DOMBreakpoints.NodeRemovedSelf.BreakpointDisabled
 PASS: Added 'node-removed' breakpoint.
 Wait for evaluate in page to return.
 PASS: Should not pause for disabled breakpoint.
 -- Running test teardown.
 
--- Running test case: DOMBreakpoints.NodeRemoved.DebuggerDisabled
+-- Running test case: DOMBreakpoints.NodeRemovedSelf.DebuggerDisabled
 PASS: Added 'node-removed' breakpoint.
 Wait for evaluate in page to return.
 PASS: Should not pause for disabled breakpoint.
 -- Running test teardown.
 
--- Running test case: DOMBreakpoints.NodeRemoved.RemoveBreakpoint
+-- Running test case: DOMBreakpoints.NodeRemovedSelf.RemoveBreakpoint
+PASS: Added 'node-removed' breakpoint.
+Remove breakpoint.
+Wait for evaluate in page to return.
+PASS: Should not pause for removed breakpoint.
+-- Running test teardown.
+
+-- Running test case: DOMBreakpoints.NodeRemovedAncestor.BreakpointEnabled
+PASS: Added 'node-removed' breakpoint.
+PASS: Breakpoint should have expected type.
+Call DOM operation.
+PAUSED:
+PASS: Pause reason should be DOM.
+PASS: Pause type should be 'node-removed'.
+PASS: Pause nodeId should be expected value.
+PASS: Pause targetNodeId should not match nodeId.
+CALL STACK:
+0: [F] nodeRemovedAncestorTest
+1: [P] Global Code
+-- Running test teardown.
+
+-- Running test case: DOMBreakpoints.NodeRemovedAncestor.BreakpointDisabled
+PASS: Added 'node-removed' breakpoint.
+Wait for evaluate in page to return.
+PASS: Should not pause for disabled breakpoint.
+-- Running test teardown.
+
+-- Running test case: DOMBreakpoints.NodeRemovedAncestor.DebuggerDisabled
+PASS: Added 'node-removed' breakpoint.
+Wait for evaluate in page to return.
+PASS: Should not pause for disabled breakpoint.
+-- Running test teardown.
+
+-- Running test case: DOMBreakpoints.NodeRemovedAncestor.RemoveBreakpoint
 PASS: Added 'node-removed' breakpoint.
 Remove breakpoint.
 Wait for evaluate in page to return.
index 047d69c..8f715b5 100644 (file)
@@ -12,18 +12,27 @@ function attributeModifiedTest() {
     document.getElementById("attribute-modified-test").setAttribute("display", "none");
 }
 
-function nodeRemovedTest() {
-    let node = document.getElementById("node-removed-test");
+function nodeRemovedDirectTest() {
+    let node = document.getElementById("node-removed-direct-test");
     let parent = node.parentNode;
     node.remove();
     parent.append(node);
 }
 
+function nodeRemovedAncestorTest() {
+    let node = document.getElementById("node-removed-ancestor-test");
+    let parent = node.parentNode;
+    let grandparent = parent.parentNode;
+    parent.remove(node);
+    grandparent.append(parent);
+}
+
 function test()
 {
     const subtreeModifiedTestId = "subtree-modified-test";
     const attributeModifiedTestId = "attribute-modified-test";
-    const nodeRemovedTestId = "node-removed-test";
+    const nodeRemovedDirectTestId = "node-removed-direct-test";
+    const nodeRemovedAncestorTestId = "node-removed-ancestor-test";
     const multipleBreakpointsTestId = "multiple-breakpoints-test";
 
     let debuggerPausedListener = null;
@@ -109,7 +118,7 @@ function test()
         });
     }
 
-    function addTestsForBreakpointType({name, elementIdentifier, type, expression}) {
+    function addTestsForBreakpointType({name, elementIdentifier, type, expression, insertion, shouldMatch}) {
         suite.addTestCase({
             name: `DOMBreakpoints.${name}.BreakpointEnabled`,
             description: "Check that debugger pauses for breakpoint type.",
@@ -128,8 +137,22 @@ function test()
                     let targetData = WI.debuggerManager.dataForTarget(WI.debuggerManager.activeCallFrame.target);
                     InspectorTest.log("PAUSED:");
                     InspectorTest.expectEqual(targetData.pauseReason, WI.DebuggerManager.PauseReason.DOM, "Pause reason should be DOM.");
-                    logActiveStackTrace();
 
+                    let pauseData = targetData.pauseData;
+                    InspectorTest.expectEqual(pauseData.type, type, `Pause type should be '${type}'.`);
+                    InspectorTest.expectEqual(pauseData.nodeId, domNodeIdentifier, `Pause nodeId should be expected value.`);
+
+                    if (insertion !== undefined)
+                        InspectorTest.expectEqual(insertion, target.pauseData.insertion, `Pause insertion should be '${insertion}'.`);
+
+                    if (pauseData.targetNodeId) {
+                        if (shouldMatch)
+                            InspectorTest.expectEqual(pauseData.targetNodeId, pauseData.nodeId, "Pause targetNodeId should match nodeId.");
+                        else
+                            InspectorTest.expectNotEqual(pauseData.targetNodeId, pauseData.nodeId, "Pause targetNodeId should not match nodeId.");
+                    }
+
+                    logActiveStackTrace();
                     return WI.debuggerManager.resume();
                 })
                 .then(resolve, reject);
@@ -217,6 +240,8 @@ function test()
         elementIdentifier: subtreeModifiedTestId,
         type: WI.DOMBreakpoint.Type.SubtreeModified,
         expression: "subtreeModifiedTest()",
+        insertion: true,
+        shouldMatch: true,
     });
 
     addTestsForBreakpointType({
@@ -227,10 +252,18 @@ function test()
     });
 
     addTestsForBreakpointType({
-        name: "NodeRemoved",
-        elementIdentifier: nodeRemovedTestId,
+        name: "NodeRemovedSelf",
+        elementIdentifier: nodeRemovedDirectTestId,
+        type: WI.DOMBreakpoint.Type.NodeRemoved,
+        expression: "nodeRemovedDirectTest()",
+    });
+
+    addTestsForBreakpointType({
+        name: "NodeRemovedAncestor",
+        elementIdentifier: nodeRemovedAncestorTestId,
         type: WI.DOMBreakpoint.Type.NodeRemoved,
-        expression: "nodeRemovedTest()",
+        expression: "nodeRemovedAncestorTest()",
+        shouldMatch: true,
     });
 
     suite.addTestCase({
@@ -353,7 +386,8 @@ function test()
 <div id="test-container" style="display: none">
     <div id="subtree-modified-test"></div>
     <div id="attribute-modified-test"></div>
-    <div id="node-removed-test"></div>
+    <div id="node-removed-direct-test"></div>
+    <div><div id="node-removed-ancestor-test"></div></div>
     <div id="multiple-breakpoints-test"></div>
 </div>
 </body>
index 2fd8572..f574d65 100644 (file)
@@ -1,3 +1,26 @@
+2019-10-31  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the node is removed from the main DOM tree, not just when it's removed from it's parent
+        https://bugs.webkit.org/show_bug.cgi?id=203349
+
+        Reviewed by Matt Baker.
+
+        Test: inspector/dom-debugger/dom-breakpoints.html
+
+        Replace `targetNode` (which was a `Runtime.RemoteObject`) with a `targetNodeId` (which is a
+        `DOM.NodeId`) when dispatching `DOMDebugger` pauses. Additionally, include the ancestor's
+        `DOM.NodeId` as the `targetNodeId` whenever an ancestor is removed that has a descendant
+        with a node removed DOM breakpoint.
+
+        * inspector/agents/page/PageDOMDebuggerAgent.h:
+        * inspector/agents/page/PageDOMDebuggerAgent.cpp:
+        (WebCore::PageDOMDebuggerAgent::willRemoveDOMNode):
+        (WebCore::PageDOMDebuggerAgent::descriptionForDOMEvent):
+
+        * inspector/agents/InspectorDOMAgent.h:
+        * inspector/agents/InspectorDOMAgent.cpp:
+        (WebCore::InspectorDOMAgent::pushNodeToFrontend): Added.
+
 2019-10-31  Andres Gonzalez  <andresg_22@apple.com>
 
         Rename AccessibilityObject::matchedParent as Accessibility::findAncestor and re-implement in a generic way so that can be used with both AccessibilityObjects and AXIsolatedTreeNodes.
index 3ead135..19bd04b 100644 (file)
@@ -541,6 +541,15 @@ void InspectorDOMAgent::discardBindings()
     m_childrenRequested.clear();
 }
 
+int InspectorDOMAgent::pushNodeToFrontend(Node* nodeToPush)
+{
+    if (!nodeToPush)
+        return 0;
+
+    ErrorString ignored;
+    return pushNodeToFrontend(ignored, boundNodeId(&nodeToPush->document()), nodeToPush);
+}
+
 int InspectorDOMAgent::pushNodeToFrontend(ErrorString& errorString, int documentNodeId, Node* nodeToPush)
 {
     Document* document = assertDocument(errorString, documentNodeId);
index d917e50..51639ab 100644 (file)
@@ -178,6 +178,7 @@ public:
 
     void styleAttributeInvalidated(const Vector<Element*>& elements);
 
+    int pushNodeToFrontend(Node*);
     int pushNodeToFrontend(ErrorString&, int documentNodeId, Node*);
     Node* nodeForId(int nodeId);
     int boundNodeId(const Node*);
index 9aa5e90..403d5c5 100644 (file)
@@ -187,15 +187,33 @@ void PageDOMDebuggerAgent::willRemoveDOMNode(Node& node)
     if (!m_debuggerAgent->breakpointsActive())
         return;
 
-    Node* parentNode = InspectorDOMAgent::innerParentNode(&node);
     if (hasBreakpoint(&node, NodeRemoved)) {
-        Ref<JSON::Object> eventData = JSON::Object::create();
+        auto eventData = JSON::Object::create();
         descriptionForDOMEvent(node, NodeRemoved, false, eventData.get());
         m_debuggerAgent->breakProgram(Inspector::DebuggerFrontendDispatcher::Reason::DOM, WTFMove(eventData));
-    } else if (parentNode && hasBreakpoint(parentNode, SubtreeModified)) {
-        Ref<JSON::Object> eventData = JSON::Object::create();
+        return;
+    }
+
+    uint32_t rootBit = 1 << NodeRemoved;
+    uint32_t derivedBit = rootBit << domBreakpointDerivedTypeShift;
+    uint32_t matchBit = rootBit | derivedBit;
+    for (auto& [nodeWithBreakpoint, breakpointTypes] : m_domBreakpoints) {
+        if (node.contains(nodeWithBreakpoint) && (breakpointTypes & matchBit)) {
+            auto eventData = JSON::Object::create();
+            descriptionForDOMEvent(*nodeWithBreakpoint, NodeRemoved, false, eventData.get());
+            if (auto* domAgent = m_instrumentingAgents.inspectorDOMAgent())
+                eventData->setInteger("targetNodeId"_s, domAgent->pushNodeToFrontend(&node));
+            m_debuggerAgent->breakProgram(Inspector::DebuggerFrontendDispatcher::Reason::DOM, WTFMove(eventData));
+            return;
+        }
+    }
+
+    auto* parentNode = InspectorDOMAgent::innerParentNode(&node);
+    if (parentNode && hasBreakpoint(parentNode, SubtreeModified)) {
+        auto eventData = JSON::Object::create();
         descriptionForDOMEvent(node, SubtreeModified, false, eventData.get());
         m_debuggerAgent->breakProgram(Inspector::DebuggerFrontendDispatcher::Reason::DOM, WTFMove(eventData));
+        return;
     }
 }
 
@@ -273,8 +291,7 @@ void PageDOMDebuggerAgent::descriptionForDOMEvent(Node& target, int breakpointTy
         if (domAgent) {
             // For inheritable breakpoint types, target node isn't always the same as the node that owns a breakpoint.
             // Target node may be unknown to frontend, so we need to push it first.
-            RefPtr<Inspector::Protocol::Runtime::RemoteObject> targetNodeObject = domAgent->resolveNode(&target, InspectorDebuggerAgent::backtraceObjectGroup);
-            description.setValue("targetNode", targetNodeObject);
+            description.setInteger("targetNodeId"_s, domAgent->pushNodeToFrontend(&target));
         }
 
         // Find breakpoint owner node.
index 77b1bbd..7ddf453 100644 (file)
@@ -64,6 +64,7 @@ private:
 
     void descriptionForDOMEvent(Node&, int breakpointType, bool insertion, JSON::Object& description);
     void updateSubtreeBreakpoints(Node*, uint32_t rootMask, bool set);
+    bool checkSubtreeForNodeRemoved(Node*);
     bool hasBreakpoint(Node*, int type);
 
     HashMap<Node*, uint32_t> m_domBreakpoints;
index 84b90e8..ba95f17 100644 (file)
@@ -1,3 +1,20 @@
+2019-10-31  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the node is removed from the main DOM tree, not just when it's removed from it's parent
+        https://bugs.webkit.org/show_bug.cgi?id=203349
+
+        Reviewed by Matt Baker.
+
+        Replace `targetNode` (which was a `Runtime.RemoteObject`) with a `targetNodeId` (which is a
+        `DOM.NodeId`) when dispatching `DOMDebugger` pauses. Additionally, include the ancestor's
+        `DOM.NodeId` as the `targetNodeId` whenever an ancestor is removed that has a descendant
+        with a node removed DOM breakpoint.
+
+        * UserInterface/Views/SourcesNavigationSidebarPanel.js:
+        (WI.SourcesNavigationSidebarPanel.prototype._updatePauseReasonSection):
+
+        * Localizations/en.lproj/localizedStrings.js:
+
 2019-10-31  Yury Semikhatsky  <yurys@chromium.org>
 
         Web Inspector: CONSOLE ERROR Shown panel style-rules must be visible
index ab16f9c..c1da5bb 100644 (file)
@@ -891,6 +891,7 @@ localizedStrings["Remove Local Override"] = "Remove Local Override";
 localizedStrings["Remove Watch Expression"] = "Remove Watch Expression";
 localizedStrings["Remove probe"] = "Remove probe";
 localizedStrings["Remove this breakpoint action"] = "Remove this breakpoint action";
+localizedStrings["Removed ancestor "] = "Removed ancestor ";
 localizedStrings["Removed descendant "] = "Removed descendant ";
 localizedStrings["Render Pipeline %d"] = "Render Pipeline %d";
 localizedStrings["Rendering Frames"] = "Rendering Frames";
index 935b20d..6001a0a 100644 (file)
@@ -1465,13 +1465,7 @@ WI.SourcesNavigationSidebarPanel = class SourcesNavigationSidebarPanel extends W
             let ownerElementRow = new WI.DetailsSectionSimpleRow(WI.UIString("Element"), WI.linkifyNodeReference(domNode));
             this._pauseReasonGroup.rows = [domBreakpointRow, ownerElementRow];
 
-            if (domBreakpoint.type !== WI.DOMBreakpoint.Type.SubtreeModified)
-                return true;
-
-            console.assert(pauseData.targetNode);
-
-            let remoteObject = WI.RemoteObject.fromPayload(pauseData.targetNode, target);
-            remoteObject.pushNodeToFrontend((nodeId) => {
+            let updateTargetDescription = (nodeId) => {
                 if (!nodeId)
                     return;
 
@@ -1481,14 +1475,33 @@ WI.SourcesNavigationSidebarPanel = class SourcesNavigationSidebarPanel extends W
                     return;
 
                 let fragment = document.createDocumentFragment();
-                let description = pauseData.insertion ? WI.UIString("Child added to ") : WI.UIString("Removed descendant ");
+
+                let description = null;
+                switch (domBreakpoint.type) {
+                case WI.DOMBreakpoint.Type.SubtreeModified:
+                    description = pauseData.insertion ? WI.UIString("Child added to ") : WI.UIString("Removed descendant ");
+                    break;
+                case WI.DOMBreakpoint.Type.NodeRemoved:
+                    description = WI.UIString("Removed ancestor ");
+                    break;
+                }
+                console.assert(description);
                 fragment.append(description, WI.linkifyNodeReference(node));
 
                 let targetDescriptionRow = new WI.DetailsSectionSimpleRow(WI.UIString("Details"), fragment);
                 targetDescriptionRow.element.classList.add("target-description");
 
                 this._pauseReasonGroup.rows = this._pauseReasonGroup.rows.concat(targetDescriptionRow);
-            });
+            };
+
+            if (pauseData.targetNodeId) {
+                console.assert(domBreakpoint.type === WI.DOMBreakpoint.Type.SubtreeModified || domBreakpoint.type === WI.DOMBreakpoint.Type.NodeRemoved);
+                updateTargetDescription(pauseData.targetNodeId);
+            } else if (pauseData.targetNode) { // COMPATIBILITY (iOS 13): `targetNode` was renamed to `targetNodeId` and was changed from a `Runtime.RemoteObject` to a `DOM.NodeId`.
+                console.assert(domBreakpoint.type === WI.DOMBreakpoint.Type.SubtreeModified);
+                let remoteObject = WI.RemoteObject.fromPayload(pauseData.targetNode, target);
+                remoteObject.pushNodeToFrontend(updateTargetDescription);
+            }
 
             return true;
         }