This would allow developers to do things like: - ignore the first N pauses - evaluate JavaScript whenever an attribute of a particular node is changed without pausing
<rdar://problem/68119248>
Created attachment 407948 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:284 > + Vector<Node*> stack({ child }); > + while (!stack.isEmpty()) { i think it would actually be better/faster to just iterate each `HashMap` and use `Node::contains` to determine whether to remove
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1026 > + let stack = [node]; > + while (stack.length) { ditto
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1058 > + let stack = [node]; > + while (stack.length) { ditto
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review > Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:73 > + eventName: json.eventName, oops
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review r=me, nice work! > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:-121 > - if (rootBit & inheritableDOMBreakpointTypesMask) { Wow this is gross > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:171 > +static int calculateDistance(Node& child, Node& ancestor) Ugh, -1 is back. Can we use Optional and nullopt to represent this? > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:-177 > - if (hasBreakpoint(&parent, SubtreeModified)) { Can we do an empty hashmap check here for the fast path? > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:196 > + for (auto [breakpointOwner, breakpoint] : m_domSubtreeModifiedBreakpoints) { Can you explain with a comment or Changelog comment about what the algorithm is? Roughly, "look for the closest DOM node that has subtreeModified breakpoint". > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:224 > + int closestDistance = unrelated; Can we do an empty hashmap check here for the fast path? > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:283 > + Vector<Node*> stack({ child }); Needs high-level description of the algorithm. > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:297 > + if (auto* nextSibling = InspectorDOMAgent::innerNextSibling(child)) Nice. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:302 > + // We should get the target associated with the nodeIdentifier of this breakpoint. Nit: "we should.." sort of implies that we don't. Do we? Should this be a FIXME? > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:543 > + this._ignoreDOMBreakpointDOMNodeWillChange = true; Nit: It would be better to name this after the state being entered/exited, such as 'this._clearingDOMBreakpoints'. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:798 > + // We should get the target associated with the nodeIdentifier of this breakpoint. Ditto above. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:821 > + this._restoringBreakpoints = true; I like this in preference to this._ignoreFooBar. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:822 > + // We should get the target associated with the nodeIdentifier of this breakpoint. Ditto above. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:994 > + this._ignoreDOMBreakpointDOMNodeWillChange = true; Ditto above comment. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1019 > + if (!breakpoint.domNodeIdentifier) This could be expressed using Array.prototype.filter > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1036 > + resolvableBreakpoints.splice(i, 1); 🚧 Mutating while iterating 🚧 Is there a way to avoid this? It just makes me super anxious to do this on purpose. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1063 > + this._ignoreDOMBreakpointDOMNodeWillChange = true; Ditto above comment. > Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:66 > + console.assert(); This should have a message to assist in grepping for error messages ;-) > LayoutTests/inspector/dom-debugger/attribute-modified-style.html:120 > + testCaseNamePrefix: "Style.", So nice.
Comment on attachment 407948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407948&action=review >> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:-177 >> - if (hasBreakpoint(&parent, SubtreeModified)) { > > Can we do an empty hashmap check here for the fast path? it should be pretty fast if the `HashMap` is empty, but i suppose it couldn't hurt to avoid unnecessary allocations >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:302 >> + // We should get the target associated with the nodeIdentifier of this breakpoint. > > Nit: "we should.." sort of implies that we don't. Do we? Should this be a FIXME? this is part of the larger "advanced multi-target support all the things" that needs to be done at some point in the future right now there is no `WI.Target` associated with a given `WI.DOMNode` because all `WI.DOMNode` exist in the main target (subframes are in-process) >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1019 >> + if (!breakpoint.domNodeIdentifier) > > This could be expressed using Array.prototype.filter actually no, because `breakpoints` is a `Set` and sadly does not (yet) have any of the manipulation methods like `Array` :( I could `Array.from(breakpoints).filter`, but that's two full iterations vs one (although maybe we don't care because it's likely never gonna be more than a handful of items) >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1036 >> + resolvableBreakpoints.splice(i, 1); > > 🚧 Mutating while iterating 🚧 > > Is there a way to avoid this? It just makes me super anxious to do this on purpose. it's fine because we're iterating backwards i could make `breakpoints` back into a `Set` (i don't think `Iterator` has problems if the item is removed during iteration), but that's adding yet another full iteration >> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:66 >> + console.assert(); > > This should have a message to assist in grepping for error messages ;-) FYI this is meant to be the equivalent of `ASSERT_NOT_REACHED`
Created attachment 408053 [details] Patch
Committed r266669: <https://trac.webkit.org/changeset/266669> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408053 [details].