Diff
Modified: trunk/LayoutTests/ChangeLog (292753 => 292754)
--- trunk/LayoutTests/ChangeLog 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/LayoutTests/ChangeLog 2022-04-12 01:34:15 UTC (rev 292754)
@@ -1,3 +1,23 @@
+2022-04-11 Patrick Angle <pan...@apple.com>
+
+ Web Inspector: preserve DOM.NodeId if a node is removed and re-added
+ https://bugs.webkit.org/show_bug.cgi?id=189687
+
+ Reviewed by Devin Rousso.
+
+ Update test to reflect that DOM breakpoints are now able to be reattached to their original node upon insertion
+ following removal.
+
+ * inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html:
+ * inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt:
+ * inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt:
+
+ * inspector/dom-debugger/resources/dom-breakpoint-utilities.js:
+ (TestPage.registerInitializer.InspectorTest.DOMBreakpoint.createBreakpoint):
+ - Add an event handler to log the setting of the domNode for breakpoints. In order to prevent spurious output
+ when removing breakpoints, instrument the removal of breakpoints to remove the testing event handler before the
+ node is changed.
+
2022-04-11 Matteo Flores <matteo_flo...@apple.com>
[ Mac wk2 Debug ] accessibility/ancestor-computation.html is a constant timeout
Modified: trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt (292753 => 292754)
--- trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt 2022-04-12 01:34:15 UTC (rev 292754)
@@ -16,6 +16,8 @@
CALL STACK:
0: [F] testNodeRemovedAncestor
1: [P] Global Code
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
-- Running test teardown.
-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.BreakpointDisabled
@@ -22,6 +24,8 @@
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Disabling breakpoint...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause for disabled breakpoint.
-- Running test teardown.
@@ -29,6 +33,8 @@
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Disabling all breakpoints...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
Enabling all breakpoints...
PASS: Should not pause when all breakpoints disabled.
-- Running test teardown.
@@ -46,17 +52,25 @@
Setting condition to 'false'...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.
Setting condition to 'true'...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.
-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.Options.Condition.ConsoleCommandLineAPI
@@ -66,9 +80,13 @@
Setting condition to saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.
Adding saved console value 'true'...
@@ -75,9 +93,13 @@
Setting condition to saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.
-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.Options.Action.Log
@@ -86,6 +108,8 @@
Adding log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -92,6 +116,8 @@
Editing log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -99,6 +125,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -105,6 +133,8 @@
Editing log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -114,6 +144,8 @@
Adding evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -120,6 +152,8 @@
Editing evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -127,6 +161,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -133,6 +169,8 @@
Editing evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -143,6 +181,8 @@
Adding evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -150,6 +190,8 @@
Editing evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -158,6 +200,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -165,6 +209,8 @@
Editing evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
Modified: trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html (292753 => 292754)
--- trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html 2022-04-12 01:34:15 UTC (rev 292754)
@@ -46,8 +46,6 @@
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);
- InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");
-
InspectorTest.assert(paused, "Should pause.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},
@@ -72,13 +70,9 @@
InspectorTest.log("Disabling breakpoint...");
breakpoint.disabled = true;
- InspectorTest.assert(breakpoint.domNode === node, "Should have domNode.");
-
InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);
- InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");
-
InspectorTest.expectFalse(paused, "Should not pause for disabled breakpoint.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},
@@ -106,8 +100,6 @@
InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);
- InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");
-
InspectorTest.log("Enabling all breakpoints...");
await DebuggerAgent.setBreakpointsActive(true);
@@ -135,13 +127,9 @@
InspectorTest.log("Removing breakpoint...");
WI.domDebuggerManager.removeDOMBreakpoint(breakpoint);
- InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");
-
InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);
- InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");
-
InspectorTest.expectFalse(paused, "Should not pause for removed breakpoint.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},
Modified: trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt (292753 => 292754)
--- trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt 2022-04-12 01:34:15 UTC (rev 292754)
@@ -15,6 +15,8 @@
CALL STACK:
0: [F] testNodeRemovedDirect
1: [P] Global Code
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
-- Running test teardown.
-- Running test case: DOMBreakpoint.NodeRemoved.Direct.BreakpointDisabled
@@ -21,6 +23,8 @@
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV" DOM Breakpoint...
Disabling breakpoint...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should not pause for disabled breakpoint.
-- Running test teardown.
@@ -28,6 +32,8 @@
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV" DOM Breakpoint...
Disabling all breakpoints...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
Enabling all breakpoints...
PASS: Should not pause when all breakpoints disabled.
-- Running test teardown.
@@ -45,17 +51,25 @@
Setting condition to 'false'...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should not pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should not pause.
Setting condition to 'true'...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should pause.
-- Running test case: DOMBreakpoint.NodeRemoved.Direct.Options.Condition.ConsoleCommandLineAPI
@@ -65,9 +79,13 @@
Setting condition to saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should not pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should not pause.
Adding saved console value 'true'...
@@ -74,9 +92,13 @@
Setting condition to saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should pause.
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should pause.
-- Running test case: DOMBreakpoint.NodeRemoved.Direct.Options.Action.Log
@@ -85,6 +107,8 @@
Adding log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -91,6 +115,8 @@
Editing log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -98,6 +124,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -104,6 +132,8 @@
Editing log action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -113,6 +143,8 @@
Adding evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -119,6 +151,8 @@
Editing evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -126,6 +160,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -132,6 +168,8 @@
Editing evaluate action...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -142,6 +180,8 @@
Adding evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -149,6 +189,8 @@
Editing evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should pause.
@@ -157,6 +199,8 @@
Enabling auto-continue...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
@@ -164,6 +208,8 @@
Editing evaluate action using saved console value...
Triggering breakpoint...
+Breakpoint "domNode" set to "null".
+Breakpoint "domNode" set to "div#node-removed-direct-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.
Modified: trunk/LayoutTests/inspector/dom-debugger/resources/dom-breakpoint-utilities.js (292753 => 292754)
--- trunk/LayoutTests/inspector/dom-debugger/resources/dom-breakpoint-utilities.js 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/LayoutTests/inspector/dom-debugger/resources/dom-breakpoint-utilities.js 2022-04-12 01:34:15 UTC (rev 292754)
@@ -1,4 +1,12 @@
TestPage.registerInitializer(() => {
+ function handleDOMNodeDidChange(event) {
+ let breakpoint = event.target;
+ if (breakpoint.domNode)
+ InspectorTest.log(`Breakpoint "domNode" set to "${breakpoint.domNode.displayName}".`);
+ else
+ InspectorTest.log(`Breakpoint "domNode" set to "null".`);
+ };
+
InspectorTest.DOMBreakpoint = {};
InspectorTest.DOMBreakpoint.teardown = function(resolve, reject) {
@@ -9,7 +17,10 @@
};
InspectorTest.DOMBreakpoint.createBreakpoint = function(domNode, type) {
- return InspectorTest.DOMBreakpoint.addBreakpoint(new WI.DOMBreakpoint(domNode, type));
+ let breakpoint = new WI.DOMBreakpoint(domNode, type);
+ breakpoint.addEventListener(WI.DOMBreakpoint.Event.DOMNodeDidChange, handleDOMNodeDidChange);
+
+ return InspectorTest.DOMBreakpoint.addBreakpoint(breakpoint);
};
InspectorTest.DOMBreakpoint.addBreakpoint = function(breakpoint) {
@@ -55,4 +66,10 @@
});
});
};
+
+ // To prevent spurious `Breakpoint "domNode" set to "null".` logging upon removing breakpoints, we should remove the
+ // event listener before the breakpoint has removed its DOM node.
+ WI.domDebuggerManager.addEventListener(WI.DOMDebuggerManager.Event.DOMBreakpointRemoved, (event) => {
+ event.data.breakpoint.removeEventListener(WI.DOMBreakpoint.Event.DOMNodeDidChange, handleDOMNodeDidChange);
+ });
});
Modified: trunk/Source/WebCore/ChangeLog (292753 => 292754)
--- trunk/Source/WebCore/ChangeLog 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebCore/ChangeLog 2022-04-12 01:34:15 UTC (rev 292754)
@@ -1,3 +1,43 @@
+2022-04-11 Patrick Angle <pan...@apple.com>
+
+ Web Inspector: preserve DOM.NodeId if a node is removed and re-added
+ https://bugs.webkit.org/show_bug.cgi?id=189687
+
+ Reviewed by Devin Rousso.
+
+ Instead of unbinding and rebinding nodes upon removal/reinsertion to the DOM tree, we should only perform the
+ unbinding actions on a `Node` that is being destroyed.
+
+ This resolves an issue where console messages with DOM nodes would behave unexpectedly because the node for a
+ visible console message could have its children removed in `DOMManager.prototype._unbind`, even though the node
+ would still have its children when it is removed from the DOM tree (unless it is being destroyed, which is
+ handled by `DOMManager.prototype.willDestroyDOMNode`).
+
+ While not a cause of an issue here, it is worth noting (due to confusion it caused me while investigating) that
+ even after we stopped keeping `RefPtr<Node>` for nodes in r278785 the nodes themselves continued to be kept
+ alive as part of the `RemoteObject` mechanism because the inspector injected script holds on to nodes until the
+ `RemoteObject` is released.
+
+ * inspector/agents/InspectorCSSAgent.cpp:
+ (WebCore::InspectorCSSAgent::nodeLayoutContextTypeChanged):
+
+ * inspector/agents/InspectorDOMAgent.cpp:
+ (WebCore::InspectorDOMAgent::reset):
+ (WebCore::InspectorDOMAgent::bind):
+ (WebCore::InspectorDOMAgent::didCommitLoad):
+ (WebCore::InspectorDOMAgent::didInsertDOMNode):
+ (WebCore::InspectorDOMAgent::didRemoveDOMNode):
+ (WebCore::InspectorDOMAgent::pseudoElementDestroyed):
+ (WebCore::InspectorDOMAgent::unbind): Deleted.
+ - We should no longer be unbinding and rebinding nodes on insertion/removal, since nodes should keep the same ID
+ for their lifetime, and we will clean up the binding upon the Node's destruction.
+
+ (WebCore::InspectorDOMAgent::willDestroyDOMNode):
+ (WebCore::InspectorDOMAgent::destroyedNodesTimerFired):
+ - A Node in middle of being destroyed will no longer have a parent.
+
+ * inspector/agents/InspectorDOMAgent.h:
+
2022-04-11 Alan Bujtas <za...@apple.com>
[IFC][Integration] LayoutIntegration::LineLayout should not expose InlineContent
Modified: trunk/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp (292753 => 292754)
--- trunk/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp 2022-04-12 01:34:15 UTC (rev 292754)
@@ -980,10 +980,9 @@
return;
auto nodeId = domAgent->boundNodeId(&node);
- if (!nodeId && m_layoutContextTypeChangedMode == Protocol::CSS::LayoutContextTypeChangedMode::All) {
- // FIXME: <https://webkit.org/b/189687> Preserve DOM.NodeId if a node is removed and re-added
+ if (!nodeId && m_layoutContextTypeChangedMode == Protocol::CSS::LayoutContextTypeChangedMode::All)
nodeId = domAgent->identifierForNode(node);
- }
+
if (!nodeId)
return;
Modified: trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.cpp (292753 => 292754)
--- trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.cpp 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.cpp 2022-04-12 01:34:15 UTC (rev 292754)
@@ -362,8 +362,7 @@
m_revalidateStyleAttrTask->reset();
m_document = nullptr;
- m_destroyedDetachedNodeIdentifiers.clear();
- m_destroyedAttachedNodeIdentifiers.clear();
+ m_destroyedNodeIdentifiers.clear();
if (m_destroyedNodesTimer.isActive())
m_destroyedNodesTimer.stop();
}
@@ -400,6 +399,7 @@
Protocol::DOM::NodeId InspectorDOMAgent::bind(Node& node)
{
+ // Node binding is later balanced in `willDestroyDOMNode` which will remove the binding upon destruction.
return m_nodeToId.ensure(node, [&] {
auto id = m_lastNodeId++;
m_idToNode.set(id, node);
@@ -407,40 +407,6 @@
}).iterator->value;
}
-void InspectorDOMAgent::unbind(Node& node)
-{
- auto id = m_nodeToId.take(node);
- if (!id)
- return;
-
- m_idToNode.remove(id);
-
- if (node.isFrameOwnerElement()) {
- const HTMLFrameOwnerElement* frameOwner = static_cast<const HTMLFrameOwnerElement*>(&node);
- if (Document* contentDocument = frameOwner->contentDocument())
- unbind(*contentDocument);
- }
-
- if (is<Element>(node)) {
- Element& element = downcast<Element>(node);
- if (ShadowRoot* root = element.shadowRoot())
- unbind(*root);
- if (PseudoElement* beforeElement = element.beforePseudoElement())
- unbind(*beforeElement);
- if (PseudoElement* afterElement = element.afterPseudoElement())
- unbind(*afterElement);
- }
-
- if (auto* cssAgent = m_instrumentingAgents.enabledCSSAgent())
- cssAgent->didRemoveDOMNode(node, id);
-
- if (m_childrenRequested.remove(id)) {
- // FIXME: Would be better to do this iteratively rather than recursively.
- for (Node* child = innerFirstChild(&node); child; child = innerNextSibling(child))
- unbind(*child);
- }
-}
-
Node* InspectorDOMAgent::assertNode(Protocol::ErrorString& errorString, Protocol::DOM::NodeId nodeId)
{
Node* node = nodeForId(nodeId);
@@ -2452,7 +2418,6 @@
// Re-add frame owner element together with its new children.
auto parentId = boundNodeId(innerParentNode(frameOwner.get()));
m_frontendDispatcher->childNodeRemoved(parentId, frameOwnerId);
- unbind(*frameOwner);
auto value = buildObjectForNode(frameOwner.get(), 0);
Node* previousSibling = innerPreviousSibling(frameOwner.get());
@@ -2510,9 +2475,6 @@
if (containsOnlyHTMLWhitespace(&node))
return;
- // We could be attaching existing subtree. Forget the bindings.
- unbind(node);
-
ContainerNode* parent = node.parentNode();
auto parentId = boundNodeId(parent);
@@ -2544,7 +2506,6 @@
if (!parentId)
return;
- // FIXME: <webkit.org/b/189687> Preserve DOM.NodeId if a node is removed and re-added
if (!m_childrenRequested.contains(parentId)) {
// No children are mapped yet -> only notify on changes of hasChildren.
if (innerChildNodeCount(parent) == 1)
@@ -2551,7 +2512,6 @@
m_frontendDispatcher->childNodeCountUpdated(parentId, 0);
} else
m_frontendDispatcher->childNodeRemoved(parentId, boundNodeId(&node));
- unbind(node);
}
void InspectorDOMAgent::willDestroyDOMNode(Node& node)
@@ -2572,13 +2532,8 @@
// This can be called in response to GC. Due to the single-process model used in WebKit1, the
// event must be dispatched from a timer to prevent the frontend from making JS allocations
// while the GC is still active.
+ m_destroyedNodeIdentifiers.append(nodeId);
- // FIXME: <webkit.org/b/189687> Unify m_destroyedAttachedNodeIdentifiers and m_destroyedDetachedNodeIdentifiers.
- if (auto parentId = boundNodeId(node.parentNode()))
- m_destroyedAttachedNodeIdentifiers.append({ parentId, nodeId });
- else
- m_destroyedDetachedNodeIdentifiers.append(nodeId);
-
if (!m_destroyedNodesTimer.isActive())
m_destroyedNodesTimer.startOneShot(0_s);
}
@@ -2585,16 +2540,7 @@
void InspectorDOMAgent::destroyedNodesTimerFired()
{
- for (auto& [parentId, nodeId] : std::exchange(m_destroyedAttachedNodeIdentifiers, { })) {
- if (!m_childrenRequested.contains(parentId)) {
- auto* parent = nodeForId(parentId);
- if (parent && innerChildNodeCount(parent) == 1)
- m_frontendDispatcher->childNodeCountUpdated(parentId, 0);
- } else
- m_frontendDispatcher->childNodeRemoved(parentId, nodeId);
- }
-
- for (auto nodeId : std::exchange(m_destroyedDetachedNodeIdentifiers, { }))
+ for (auto nodeId : std::exchange(m_destroyedNodeIdentifiers, { }))
m_frontendDispatcher->willDestroyDOMNode(nodeId);
}
@@ -2734,7 +2680,6 @@
auto parentId = boundNodeId(parent);
ASSERT(parentId);
- unbind(pseudoElement);
m_frontendDispatcher->pseudoElementRemoved(parentId, pseudoElementId);
}
Modified: trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.h (292753 => 292754)
--- trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.h 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.h 2022-04-12 01:34:15 UTC (rev 292754)
@@ -230,7 +230,6 @@
// Node-related methods.
Inspector::Protocol::DOM::NodeId bind(Node&);
- void unbind(Node&);
Node* assertEditableNode(Inspector::Protocol::ErrorString&, Inspector::Protocol::DOM::NodeId);
Element* assertEditableElement(Inspector::Protocol::ErrorString&, Inspector::Protocol::DOM::NodeId);
@@ -277,8 +276,7 @@
std::unique_ptr<DOMEditor> m_domEditor;
WeakHashMap<RenderObject, Vector<size_t>> m_flexibleBoxRendererCachedItemsAtStartOfLine;
- Vector<Inspector::Protocol::DOM::NodeId> m_destroyedDetachedNodeIdentifiers;
- Vector<std::pair<Inspector::Protocol::DOM::NodeId, Inspector::Protocol::DOM::NodeId>> m_destroyedAttachedNodeIdentifiers;
+ Vector<Inspector::Protocol::DOM::NodeId> m_destroyedNodeIdentifiers;
Timer m_destroyedNodesTimer;
#if ENABLE(VIDEO)
Modified: trunk/Source/WebInspectorUI/ChangeLog (292753 => 292754)
--- trunk/Source/WebInspectorUI/ChangeLog 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebInspectorUI/ChangeLog 2022-04-12 01:34:15 UTC (rev 292754)
@@ -1,3 +1,30 @@
+2022-04-11 Patrick Angle <pan...@apple.com>
+
+ Web Inspector: preserve DOM.NodeId if a node is removed and re-added
+ https://bugs.webkit.org/show_bug.cgi?id=189687
+
+ Reviewed by Devin Rousso.
+
+ Instead of unbinding and rebinding nodes upon removal/reinsertion to the DOM tree, we should only perform the
+ unbinding actions on a `Node` that is being destroyed.
+
+ * UserInterface/Controllers/DOMDebuggerManager.js:
+
+ * UserInterface/Controllers/DOMManager.js:
+ (WI.DOMManager.prototype._pseudoElementAdded):
+
+ (WI.DOMManager.prototype.willDestroyDOMNode):
+ (WI.DOMManager.prototype._unbind):
+ - To match the backend, we should only unbind nodes when they are destroyed, not removed.
+
+ * UserInterface/Models/DOMNode.js:
+ (WI.DOMNode.prototype.newOrExistingFromPayload):
+ - Reuse an existing orphaned DOMNode if it already exists. In the future, we want to stop sending entire node
+ payloads to the frontend in those situations.
+
+ (WI.DOMNode.prototype._insertChild):
+ (WI.DOMNode.prototype._setChildrenPayload):
+
2022-04-11 Nikita Vasilyev <nvasil...@apple.com>
Web Inspector: REGRESSION(r290770): Styles: creating a new property scrolls the input to the top of the panel
Modified: trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js (292753 => 292754)
--- trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js 2022-04-12 01:34:15 UTC (rev 292754)
@@ -63,6 +63,8 @@
WI.URLBreakpoint.addEventListener(WI.Breakpoint.Event.ActionsDidChange, this._handleURLBreakpointActionsChanged, this);
WI.domManager.addEventListener(WI.DOMManager.Event.NodeRemoved, this._nodeRemoved, this);
+ WI.domManager.addEventListener(WI.DOMManager.Event.NodeDestroyed, this._nodeRemoved, this);
+
WI.domManager.addEventListener(WI.DOMManager.Event.NodeInserted, this._nodeInserted, this);
WI.networkManager.addEventListener(WI.NetworkManager.Event.MainFrameDidChange, this._mainFrameDidChange, this);
Modified: trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js (292753 => 292754)
--- trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js 2022-04-12 01:34:15 UTC (rev 292754)
@@ -188,10 +188,12 @@
willDestroyDOMNode(nodeId)
{
let node = this._idToDOMNode[nodeId];
+ console.assert(!node.parentNode, "Node should have been removed from its parent before `willDestroyDOMNode` is invoked.", node);
+
node.markDestroyed();
delete this._idToDOMNode[nodeId];
- this.dispatchEventToListeners(WI.DOMManager.Event.NodeRemoved, {node});
+ this.dispatchEventToListeners(WI.DOMManager.Event.NodeDestroyed, {node});
}
didAddEventListener(nodeId)
@@ -483,7 +485,7 @@
if (!parent)
return;
- var node = new WI.DOMNode(this, parent.ownerDocument, false, pseudoElement);
+ let node = WI.DOMNode.newOrExistingFromPayload(parent.ownerDocument, pseudoElement);
node.parentNode = parent;
this._idToDOMNode[node.id] = node;
console.assert(!parent.pseudoElements().get(node.pseudoType()));
@@ -510,6 +512,13 @@
_unbind(node)
{
+ // COMPATIBILITY (iOS 15.4): After iOS 15.4, the backend no longer unbinds nodes from their IDs until the node
+ // is destroyed. Because there are no protocol changes associated with this change, check for flexbox overlay
+ // support, which was also new after iOS 15.4.
+ // FIXME: <https://webkit.org/b/148680> Use explicit version checking.
+ if (WI.assumingMainTarget().hasCommand("DOM.showFlexOverlay"))
+ return;
+
node.markDestroyed();
delete this._idToDOMNode[node.id];
@@ -876,4 +885,5 @@
DOMNodeWasInspected: "dom-manager-dom-node-was-inspected",
InspectModeStateChanged: "dom-manager-inspect-mode-state-changed",
InspectedNodeChanged: "dom-manager-inspected-node-changed",
+ NodeDestroyed: "dom-manager-node-destroyed",
};
Modified: trunk/Source/WebInspectorUI/UserInterface/Models/DOMNode.js (292753 => 292754)
--- trunk/Source/WebInspectorUI/UserInterface/Models/DOMNode.js 2022-04-12 01:32:43 UTC (rev 292753)
+++ trunk/Source/WebInspectorUI/UserInterface/Models/DOMNode.js 2022-04-12 01:34:15 UTC (rev 292754)
@@ -163,6 +163,12 @@
// Static
+ static newOrExistingFromPayload(document, payload, {isInShadowTree} = {})
+ {
+ // FIXME: <webkit.org/b/238947> Don't send node payloads to the frontend for already-bound nodes.
+ return WI.domManager.nodeForId(payload.nodeId) || new WI.DOMNode(WI.domManager, document, !!isInShadowTree, payload);
+ }
+
static resetDefaultLayoutOverlayConfiguration()
{
let configuration = WI.DOMNode._defaultLayoutOverlayConfiguration;
@@ -1068,7 +1074,7 @@
_insertChild(prev, payload)
{
- var node = new WI.DOMNode(this._domManager, this.ownerDocument, this._isInShadowTree, payload);
+ let node = WI.DOMNode.newOrExistingFromPayload(this.ownerDocument, payload, {isInShadowTree: this._isInShadowTree});
if (!prev) {
if (!this._children) {
// First node
@@ -1101,10 +1107,9 @@
return;
this._children = this._shadowRoots.slice();
- for (var i = 0; i < payloads.length; ++i) {
- var node = new WI.DOMNode(this._domManager, this.ownerDocument, this._isInShadowTree, payloads[i]);
- this._children.push(node);
- }
+ for (let payload of payloads)
+ this._children.push(WI.DOMNode.newOrExistingFromPayload(this.ownerDocument, payload, {isInShadowTree: this._isInShadowTree}));
+
this._renumber();
}
