Bug 215795 – Web Inspector: allow DOM breakpoints to be configured

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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

[Radar WebKit Bug Importer]

<rdar://problem/68119248>

[Devin Rousso]

Created attachment 407948 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

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

[Blaze Burg]

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.

[Devin Rousso]

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`

[Devin Rousso]

Created attachment 408053 [details]
Patch

[EWS]

Committed r266669: <https://trac.webkit.org/changeset/266669>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408053 [details].