Bug 168101 – Web Inspector: Add DOM breakpoints UI for node/subtree modification events

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

[Matt Baker]

Summary:
Add DOM breakpoints UI for node/subtree modification events. The backend agent InspectorDOMDebuggerAgent has existed for some time; this issue covers UI and layout tests.

This issue addresses breakpoints for node/subtree modifications only. The addition of UI for setting breakpoints on DOM events (click, focus, blur, etc) is covered here:
https://bugs.webkit.org/show_bug.cgi?id=127998

[Matt Baker]

Created attachment 301123 [details]
[Image] Debugger tab - WIP

New: DOM Breakpoints section inserted after script breakpoints.

[Matt Baker]

Created attachment 301124 [details]
[Image] Elements tab UI - wip

New: DOM breakpoints gutter. Includes context menu similar to that shown in Debugger tab (not shown)

[Joseph Pecoraro]

This looks great!

[Matt Baker]

Created attachment 302453 [details]
Patch

[Matt Baker]

Created attachment 302455 [details]
Patch

[Build Bot]

Comment on attachment 302455 [details]
Patch

Attachment 302455 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3175497

New failing tests:
inspector/dom-debugger/dom-breakpoints.html

[Build Bot]

Created attachment 302466 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Matt Baker]

Created attachment 302471 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 302471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302471&action=review

Not completely reviewed just yet.

> Source/WebInspectorUI/ChangeLog:35
> +        "resolved" once its location is pushed to the frontend. Unlike script
> +        breakpoints, there is no meaningful way to display an unresolved DOM
> +        breakpoint in the UI. As a result, only resolved breakpoints are made
> +        visible by the manager to the rest of the UI.

This is interesting. That means if the user has a ton of DOM Breakpoints saved that will never be encountered again they are wasting space in the WebInspector.Setting, never to be cleared. We have a similar issue with Breakpoints in general, but that is slightly worse in that those are all sent individually to the backend when the inspector opens. We may want to think about aging out breakpoints.

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:-112
> -void InspectorDOMDebuggerAgent::discardBindings()
> -{
> -    m_domBreakpoints.clear();
> -}

It is not clear to me why we are removing this instead of actually calling it, such as when DOMAgent has its own discardBindings().

When the page navigates all of the nodes in the m_domBreakpoints map will be invalidated and the map should be cleared. In fact, ideally it wouldn't ever contain stale Node pointer.

For example DOMDebuggerAgent would be potentially broken in this situation:

    1. Page is created with <div id="x">
        Node div#x is at <ADDR1>
    2. Inspector adds DOM Breakpoints on this node
        m_domBreakpoints.set(<ADDR1>, mask)
    3. Page navigates (I don't think InspectorDOMDebuggerAgent::didRemoveDOMNode gets called)
        => Nothing tells DOMDebugger to update
        => m_domBreakpoint has an entry with a stale Node*
    4. New node is created on the new page, <div id="y"> allocated at the same <ADDR1>
    5. Some kind of modification happens to this new node
        => Inspector triggers unexpected breakpoint

Reusing the same Pointer Address for similarly sized C++ classes (e.g. WebCore::Node) is not impossible (I've seen it recently with WebCore::CachedResources).

----

These are the following things that I think could be done: (this list was longer at somepoint!)

    • DOMDebugger should remove entries from the map when they are invalid
       - It already clears nodes via didRemoveDOMNode
       - It must discard / clear all entries when the page navigates (e.g. DOMAgent's discardBindings)

----

Side note: Please remove these unimplemented methods from InspectorDOMAgent:

    bool hasBreakpoint(Node*, int type);
    void updateSubtreeBreakpoints(Node* root, uint32_t rootMask, bool value);

There is some cleanup you could do to all this masking. The enum things you did with AsyncCallStacks was great, maybe you could clean this up a bit. I haven't read it completely yet.

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:249
>  void InspectorDOMDebuggerAgent::willInsertDOMNode(Node& parent)

Ryosuke says this is called at a bad location from WebCore.

InspectorInstrumentation::willInsertDOMNode in ContainerNode::replaceAllChildren (assuming we pause inside the JS call that modifies the attribute). Might be easy to move this out of the NoEventDisaptchAssertion blocks (the potential for arbitrary JS while WebCore has partially modified state then that is very bad).

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:278
>  void InspectorDOMDebuggerAgent::willModifyDOMAttr(Element& element)

Ryosuke says this is called at a bad location from WebCore.

InspectorInstrumentation::willModifyDOMAttr in Element::willModifyAttribute (assuming we pause inside the JS call that modifies the attribute).

Ask him for details for all of these to know when it is safe to do so.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:224
> +localizedStrings["DOM Breakpoints"] = "DOM Breakpoints";

I'm not against this name. But what do you of "Node Breakpoints" or "Element Breakpoints"?

Other things in the future could be "Event Breakpoints" (Events, Timers, etc), "Network Breakpoints" (XHR/Fetch), "Object Watchpoints".

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:88
> +    closed()

Nice job cleaning up here. I wonder if we should rename this from closed() to disconnect() because the Controller isn't really closed (the DebuggerSidebarPanel owning it is though), and closed carries that special weight given how it is used elsewhere.

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:113
> +        domNodeTreeElement.appendChild(breakpointTreeElement);
> +        if (shouldExpandTreeElement)

Style: Newline between these.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:54
> +        return WebInspector.debuggableType !== WebInspector.DebuggableType.JavaScript && !!window.DOMDebuggerAgent;

I do not think the first part is necessary. window.DOMDebuggerAgent is not available in JavaScript debuggables and if we did eventually expose it to Augmented JSContexts we would want the Agent feature check to work.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:71
> +    domBreakpointsForMainResource()

What about subframes? It seems the caller of this would ignore subframes entirely.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:121
> +        breakpoints.remove(breakpoint, true);
> +        if (!breakpoints.length)

Style: Newline between.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:170
> +        let url = breakpoints[0].url;

It is implicit here that all breakpoints share the same URL. We might want to assert that (in the loop below would be fine).

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:177
> +            DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);

We should not need to call removeDOMBreakpoint if this breakpoint was disabled. If it was disabled we would have already called DOMDebuggerAgent.removeDOMBreakpoint in _updateDOMBreakpoint earlier.

Scenario:

    1. Add a DOM Breakpoint
    2. Disable the DOM Breakpoint
    3. Delete all breakpoints on this Node
      => I would expect only only removeDOMBreakpoint call.

This raises another issue. Maybe we should check the results of these backend calls to ensure there are no errors.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:199
> +        let mainFrame = WebInspector.frameResourceManager.mainFrame;

What about subframes? This seems like it will be a common thread in this patch. I'm not sure what the best thing to do is, but we should think about it and have a policy in mind.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:201
> +            this._restoreWhenMainFrameAvailable = true;

This member variable should be initialized in the constructor as well for clarity.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:246
> +    _mainFrameDidChange(event)

What is the difference between mainFrameDidChange and mainResourceDidChange && event.target.isMainFrame?

I think MainResourceDidChange could be fired for subframes that navigate. It sounds like that event is not necessary if you only care about the MainFrame.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:284
> +            if (breakpoint.path === node.path)
> +                breakpoint.resolve();

This seems to do unnecessary extra work. We have a breakpoint that matches this node. We should be able to resolve it _with_ this node but currently resolve does a DOMAgent.pushNodeByPathToFrontend which can a relatively long round trip to get us this very same node.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:799
> +        case DebuggerAgent.PausedReason.DOM:
> +            return WebInspector.DebuggerManager.PauseReason.DOM;

Awesome! Nice job here. I think ideally we'd extend the inspector/debugger/pause-reason.html test to cover this, but your existing test covers it so I think I'm fine.

> Source/WebInspectorUI/UserInterface/Images/DOMBreakpoint.svg:1
> +<?xml version="1.0" encoding="utf-8"?>

What is our solution for Images/gtk?

> Source/WebInspectorUI/UserInterface/Main.html:727
> +    <script src="Controllers/DOMBreakpointTreeController.js"></script>

I think this can be sorted up above.

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:1
> +/*

-- JoePeck: This is how far I got in the review so far --

[Matt Baker]

Comment on attachment 302471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302471&action=review

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:224
>> +localizedStrings["DOM Breakpoints"] = "DOM Breakpoints";
> 
> I'm not against this name. But what do you of "Node Breakpoints" or "Element Breakpoints"?
> 
> Other things in the future could be "Event Breakpoints" (Events, Timers, etc), "Network Breakpoints" (XHR/Fetch), "Object Watchpoints".

Of the two alternatives I like "Element Breakpoints", since it is consistent with the tab name. DOM Breakpoints already has name recognition however, so I don't know that we'd want to change it.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:88
>> +    closed()
> 
> Nice job cleaning up here. I wonder if we should rename this from closed() to disconnect() because the Controller isn't really closed (the DebuggerSidebarPanel owning it is though), and closed carries that special weight given how it is used elsewhere.

I struggled to find a better name at the time. "disconnect" is perfect.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:71
>> +    domBreakpointsForMainResource()
> 
> What about subframes? It seems the caller of this would ignore subframes entirely.

Good catch. Will investigate.

>> Source/WebInspectorUI/UserInterface/Images/DOMBreakpoint.svg:1
>> +<?xml version="1.0" encoding="utf-8"?>
> 
> What is our solution for Images/gtk?

Will file a follow-up.

[Devin Rousso]

Comment on attachment 302471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302471&action=review

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:104
> +            console.assert(domNode, "Missing DOMNode for identifier", nodeIdentifier);

I think this may warrant WebInspector.reportInternalError.  Not really sure that we want to silently fail if the user attempts to add a breakpoint for a nonexistent node.

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:129
> +        console.assert(domNodeTreeElement);

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:134
> +        if (domNodeTreeElement.children.length)

NIT: I think you could just use `domNodeTreeElement.hasChildren`, but this is fine too.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:284
>> +                breakpoint.resolve();
> 
> This seems to do unnecessary extra work. We have a breakpoint that matches this node. We should be able to resolve it _with_ this node but currently resolve does a DOMAgent.pushNodeByPathToFrontend which can a relatively long round trip to get us this very same node.

I agree.  Since each breakpoint already has the node path, I think we should store the breakpoints in some way where they are uniquely identifiable by that path.  Possibly make the values of `_domBreakpointURLMap` be maps themselves?

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:55
> +    get resolved() { return !!this._domNodeIdentifier; }

Style: since this getter actually performs some logic, I think it would be better to be on it's own line.

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:57
> +    get disabled()

Style: this getter can be collapsed on a single line.

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:98
> +    saveIdentityToCookie(cookie)

Does the disabled value also need to be saved to this cookie?  Also, is there a reason to use the getters instead of the "private" member variables (e.g. `this._url` instead of `this.url`)?

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:-110
> -        if (representedObject instanceof WebInspector.DOMSearchMatchObject) {

I'm a little confused as to why you're removing this.  Could you explain?

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:111
> +            if (representedObject.frame) {

Style: I think you should combine these if statements.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:119
> +            if (representedObject.domNode)

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:207
> +            if (representedObject.domNode)

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:212
> +            if (representedObject.frame)

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.css:26
> +.item.dom-breakpoint .icon {

Since we can now preview RTL layouts, I think it might be worth it to add support while we do new features (or just add a FIXME).

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:38
> +        super(["dom-breakpoint", className], title, null, breakpoint, false);

NIT: I've been getting into the habit of creating const variables above super calls like this so that future changes don't require the developer to look at what WebInspector.GeneralTreeElement (and WebInspector.TreeElement) to see what each argument represents.

const subtitle = null;
const representedObject = breakpoint; // This one I'm on the fence about, because `representedObject` is so common.
const hasChildren = false;
super(["dom-breakpoint", className], title, subtitle, representedObject, hasChildren);

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:41
> +        this._statusImageElement.classList.add("status-image");

Calling the `status` setter will automatically wrap the <img> inside a <div> with the class "status", so I think you can drop the "status-image" class and just use the selector `.status > img`.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:54
> +            case WebInspector.DOMBreakpoint.Type.SubtreeModified:

NIT: deindent the cases.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:68
> +    get breakpoint() { return this.representedObject; }

Personally, I think that referring to the `representedObject` of a WebInspector.DOMBreakpointTreeElement implies that it will be a WebInspector.DOMBreakpoint, so I would rather get rid of this and just use `representedObject` instead.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:91
> +        this._boundStatusImageElementClicked = null;

Is it necessary to clear the bound functions?  Would keeping the bound functions prevent garbage collection in some way (honestly, still not entirely clear how it works)?

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:127
> +        contextMenu.appendItem(WebInspector.UIString("Delete Breakpoint"), function() {

Style: I think there should be a newline before this.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeTreeElement.js:37
> +    get domNode()

See my comment about `representedObject` in DOMBreakpointTreeElement.js.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:31
> +    padding-left: 13px;

See my comment about RTL support in DOMBreakpointTreeElement.css.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
> +    get breakpointGutterEnabled()

Style: this getter can be collapsed on a single line.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:68
> +    get breakpointStatus()

Style: this getter can be collapsed on a single line.

[Matt Baker]

Comment on attachment 302471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302471&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:104
>> +            console.assert(domNode, "Missing DOMNode for identifier", nodeIdentifier);
> 
> I think this may warrant WebInspector.reportInternalError.  Not really sure that we want to silently fail if the user attempts to add a breakpoint for a nonexistent node.

AFAIK, we don't usually use reportInternalError when doing sanity checks. The only way this assert would ever fire would be due to DOMDebuggerManager dispatching an event while in an inconsistent state. Such a regression should be caught by a regression test.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:134
>> +        if (domNodeTreeElement.children.length)
> 
> NIT: I think you could just use `domNodeTreeElement.hasChildren`, but this is fine too.

Not only is `hasChildren` more concise, it is probably the correct approach here. TreeElement.prototype.hasChildren is *not* equivalent to TreeElement.prototype.children.length > 0.

>> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:57
>> +    get disabled()
> 
> Style: this getter can be collapsed on a single line.

We've been formatting getter/setter pairs as a unit. Either both are on a single line or neither are.

>> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:98
>> +    saveIdentityToCookie(cookie)
> 
> Does the disabled value also need to be saved to this cookie?  Also, is there a reason to use the getters instead of the "private" member variables (e.g. `this._url` instead of `this.url`)?

I don't think a disabled cookie key is necessary. From what I recall this is used to restore the represented object selection in the UI, and only needs to uniquely identify the object. I will confirm this.

As far as this.url vs. this._url: I prefer to always use public facing getters internally, although I know this is not generally our practice. There is no measurable performance benefit to using the private member, and since getters are not always backed by a member variable it absolves the caller from having to check the implementation.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:38
>> +        super(["dom-breakpoint", className], title, null, breakpoint, false);
> 
> NIT: I've been getting into the habit of creating const variables above super calls like this so that future changes don't require the developer to look at what WebInspector.GeneralTreeElement (and WebInspector.TreeElement) to see what each argument represents.
> 
> const subtitle = null;
> const representedObject = breakpoint; // This one I'm on the fence about, because `representedObject` is so common.
> const hasChildren = false;
> super(["dom-breakpoint", className], title, subtitle, representedObject, hasChildren);

I think it makes sense to name boolean literals, but I'm not sure the other cases add much.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:68
>> +    get breakpoint() { return this.representedObject; }
> 
> Personally, I think that referring to the `representedObject` of a WebInspector.DOMBreakpointTreeElement implies that it will be a WebInspector.DOMBreakpoint, so I would rather get rid of this and just use `representedObject` instead.

I agree, the context is clear.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:91
>> +        this._boundStatusImageElementClicked = null;
> 
> Is it necessary to clear the bound functions?  Would keeping the bound functions prevent garbage collection in some way (honestly, still not entirely clear how it works)?

Not sure. In general I don't think it's common for tree elements to outlive the backing DOM element.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeTreeElement.js:37
>> +    get domNode()
> 
> See my comment about `representedObject` in DOMBreakpointTreeElement.js.

Yep!

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
>> +    get breakpointGutterEnabled()
> 
> Style: this getter can be collapsed on a single line.

See my earlier comment.

[Joseph Pecoraro]

Comment on attachment 302471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302471&action=review

Overall this looks great. r- because of some of the concerns raised (backend implementation questions), also I know you were planning on putting up an updated patch.

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:95
> +        WebInspector.domTreeManager.pushNodeByPathToFrontend(this._path, didPushNodeByPathToFrontend.bind(this));

Style: I think an inline arrow function would read nicer. It also gets rid of the bind.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:54
>> +        switch (type) {
>> +            case WebInspector.DOMBreakpoint.Type.SubtreeModified:
> 
> NIT: deindent the cases.

Style: Dedent the cases.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:59
> +                return WebInspector.UIString("Node Removed");

Ryosuke was suggesting maybe just "Removal", "Removed". I have no strong opinions here.

>>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:91
>>> +        this._boundStatusImageElementClicked = null;
>> 
>> Is it necessary to clear the bound functions?  Would keeping the bound functions prevent garbage collection in some way (honestly, still not entirely clear how it works)?
> 
> Not sure. In general I don't think it's common for tree elements to outlive the backing DOM element.

I'd suggest just creating these once in attach and keeping them around:

    if (!this._bound...) {
        // create all 3 for the first time
    }

I don't think it is common for us to have TreeElements alive and detached, and if we did I don't think its common to detach/reattach these.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:154
> +        if (!this._statusImageElement)
> +            return;

How is this case possible?

> Source/WebInspectorUI/UserInterface/Views/DOMNodeTreeElement.js:26
> +WebInspector.DOMNodeTreeElement = class DOMNodeTreeElement extends WebInspector.GeneralTreeElement

This is super generic DOMNodeTreeElement. I know we are thinking of potentially using something like this in an accessibility context, it seems weird in this generic class to assume delete / context menu have DOM Breakpoint specific behavior. For now its fine until there are multiple users and we need to abstract it. But something to keep in mind.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:71
> +        if (!WebInspector.domDebuggerManager.supported)

I'm normally a huge proponent of early returns but not in constructors. The next time someone else comes along and adds stuff to here they might inadvertently miss the early return. Lets nest the stuff below. In fact, the member variables should probably always be around.

>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
>>> +    get breakpointGutterEnabled()
>> 
>> Style: this getter can be collapsed on a single line.
> 
> See my earlier comment.

When we have both a getter and a setter, I prefer them both be multi-line and next to each other. I think it makes it clear that there is something special about the multiline: (1) it is either computed and not a simple get or (2) it may have a setter and that may have concerns.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:936
> +            if (pauseData && pauseData.nodeId) {

We only make use of pauseData.nodeId and pauseData.type. If we don't use pauseData.targetNode and pauseData.insertion we should eliminate these from the backend (InspectorDOMDebuggerAgent::descriptionForDOMEvent) and simplify it!

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:945
> +                }

We might need a bail if the domBreakpoint no longer exists. Technically I think this is possible if the frontend deleted a breakpoint and the backend hit it before processing the breakpoint removal.

> LayoutTests/inspector/dom-debugger/dom-breakpoints-expected.txt:8
> +PASS: Should not be disabled.
> +PASS: Should resolve immediately.

These would read better to me prefixed "Breakpoint should ...."

> LayoutTests/inspector/dom-debugger/dom-breakpoints-expected.txt:70
> +

Tests I think we are missing. Not all are required but some would certainly be worth adding:

    • DOMDebugger.setDOMBreakpoint with invalid nodeId
      => Should get an Error (backend looks broken here, it should be setting ErrorString)
    • DOMDebugger.setDOMBreakpoint with invalid type string
      => Should get an Error (backend looks broken here, it should be setting ErrorString)

    • DOMDebugger.removeDOMBreakpoint with invalid nodeId
      => Should get an Error (backend looks broken here, it should be setting ErrorString)
    • DOMDebugger.removeDOMBreakpoint with invalid type string
      => Should get an Error (backend looks broken here, it should be setting ErrorString)

    • Pause on a DOM "node-removed", then move the Node being removed while paused, then continue
    • Pause on a DOM "subtree-modified", then modify the subtree again while paused, then continue
    • Pause on a DOM "attribute-modified" for "style" attribute (this appears to have a different code path that is untested (InspectorDOMDebuggerAgent::didInvalidateStyleAttr))
    • Should Pause on a DOM "subtree-modified" when removing a Node from the subtree
    • Should Pause once if a parent has a "subtree-modified" breakpoint, a child has a "node-removed" breakpoint, and the child is removed (the node-removed breakpoint should be the expected pause reason)
    • A Node with all possible breakpoint types ("node-removed", "subtree-modified", "attribute-modified")
    • Pause on a regular breakpoint, and continue through some code that would trigger a DOM breakpoint
    • DOM Breakpoints on Nodes in subframes (eventually)

Given the backend issues we may want to remove DOMDebugger from previous backends so that we don't enable this on legacy backends that would have had issues. This entails removing DOMDebugger domain from:

    Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json
    Source/WebInspectorUI/Versions/Inspector-iOS-8.0.json
    Source/WebInspectorUI/Versions/Inspector-iOS-9.0.json
    Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json
    Source/WebInspectorUI/Versions/Inspector-iOS-10.0.json
    Source/WebInspectorUI/Versions/Inspector-iOS-10.3.json

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:5
> +<script src="../debugger/resources/log-active-stack-trace.js"></script>

It is awesome that we can reuse this because it looks great in test output!

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:15
> +setTimeout(() => {

Could this instead just go below "node-removed-test" as its own <script>, what is the significance of the timeout?

Maybe this was supposed to turn into a nodeInsertedTest?

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:55
> +        return new Promise((resolve, reject) => {
> +            InspectorTest.log("Wait for evaluate in page to return.");
> +            InspectorTest.evaluateInPage(expression, (error) => {
> +                if (error)
> +                    reject(error);
> +
> +                resolve();
> +            });
> +        });

This looks equivalent to:

    InspectorTest.log("Wait for evaluate in page to return.");
    return InspectorTest.evaluateInPage(expression);

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:93
> +                        InspectorTest.log("DOM breakpoint added.");

This could be an InspectorTest.pass to get slightly nicer output.

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:112
> +                    InspectorTest.expectEqual(breakpoint.type, type, "Should have expected type.");

Yeah, these are the cases where I think a little extra text in the message goes a long way. Here it is obvious that the type being checked is "breakpoint.type" but in the output its not obvious. So when it fails and we get "FAIL: Should have expected type" it won't be as clear.

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:126
> +                    let promise = WebInspector.debuggerManager.awaitEvent(WebInspector.DebuggerManager.Event.Resumed);
> +                    WebInspector.debuggerManager.resume();
> +                    return promise;

This should be equivalent to just:

    return WebInspector.debuggerManager.resume();

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:203
> +            .then(() => Promise.race([awaitEvaluateInPage("modifySubtreeTest()"), rejectOnPause()]))

Neat!

[Matt Baker]

Created attachment 303727 [details]
Patch

[Matt Baker]

UI testing
----------

Elements tab:
1. Add breakpoint using element tag name context menu ("Break on...")
2. DOM breakpoint indicator context menu:
  a) Uncheck breakpoint type
  b) Enable/disable breakpoint
  c) Delete breakpoint
3. Breakpoint should persist across page reload (may require expanding to reveal)
4. Breakpoint should persist after switching tabs
5. Breakpoint gutter should appear as needed, as disappear when the last breakpoint is removed

Debugger tab:
1. DOM Breakpoints section should initially be empty (showing "No Breakpoints")
2. Tree elements (node and breakpoint) added after adding a DOM breakpoint
3. Additional breakpoints added to same DOM node should have same parent
4. DOM node tree element context menu:
  a) Enable/disable breakpoints
  b) Remove breakpoints
5. DOM breakpoint tree element context menu:
  a) Enable/disable breakpoint
  b) Remove breakpoint

[Matt Baker]

UI testing (child frames)
-------------------------

Elements tab:
1. Breakpoint in child frame subtree should persist across page reload
2. Breakpoint in child frame subtree should persist across frame resource reload
3. Breakpoint in dynamically added child frame should appear once frame is added
4. Breakpoints in separate child frames with the same document URL should be independent

Debugger tab:
1. Breakpoint in child frame subtree is removed when frame removed
2. Breakpoints in separate child frames with the same document URL should have different parent tree elements

[Matt Baker]

Additional test coverage needed and filed here for follow-up:

Web Inspector: addition test coverage for DOM breakpoints
https://bugs.webkit.org/show_bug.cgi?id=169315

[Joseph Pecoraro]

Comment on attachment 303727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303727&action=review

r=me, but I really think we should address and test the backend issues with invalid parameters now instead of deferring. It should be trivial.

> Source/WebCore/ChangeLog:26
> +        DOMDebugger agent needs to discard bindings when the main frame loads.

When the main frame `navigates` is better than `loads`.

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:259
>  void InspectorDOMDebuggerAgent::willInsertDOMNode(Node& parent)

Do you have bugs to address Ryosuke's concerns about where these get called from WebCore? I want to make sure those do not get forgotten about because those are serious concerns.

> Source/WebCore/inspector/InspectorDOMDebuggerAgent.h:83
> +    void mainFrameDOMContentLoaded();

Put this up above in the InspectorInstrumentation section as that is most appropriate.

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:66
> +        if (!allowEditing)
> +            return;

Style: This early return is prone to future errors. I think the things below should be within an `if (allowEditing)` block.

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:88
> +        WebInspector.Frame.removeEventListener(null, null, this);
> +        WebInspector.domDebuggerManager.removeEventListener(null, null, this);

Missing WebInspector.DOMBreakpoint.removeEventListener

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:54
> +        this._restoringBreakpoints = true;
> +
> +        for (let cookie of this._domBreakpointsSetting.value) {
> +            let breakpoint = new WebInspector.DOMBreakpoint(cookie, cookie.type, cookie.disabled);
> +            this.addDOMBreakpoint(breakpoint);
> +        }
> +
> +        this._restoringBreakpoints = false;
> +        this._speculativelyResolveBreakpoints();

A bunch of this work doesn't need to happen if inspecting a backend that doesn't support DOMDebuggerAgent. We could reduce work in those cases, but it seems harmless to leave it in.

Have you ensured we don't run into any exceptions debugging a legacy backend?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:286
> +    _mainFrameDidChange()
> +    {
> +        this._speculativelyResolveBreakpoints();
> +    }

I think this is redundant given the _mainResourceDidChange event below. Is there ever a case where this will fire and that hasn't?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:293
> +                breakpoints.forEach((breakpoint) => breakpoint.domNodeIdentifier = null);

Style: Braces for the arrow function body since the return value is unimportant.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:298
> +        this._detachBreakpointsForFrame(frame);

Nit: This could be an else of the previous block because it is redundant in the case that frame is the main frame.

> Source/WebInspectorUI/UserInterface/Images/DOMBreakpoint.svg:1
> +<?xml version="1.0" encoding="utf-8"?>

What is the solution for Images/gtk?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:148
> +        WebInspector.domDebuggerManager.removeEventListener(null, null, this);
> +        WebInspector.DOMBreakpoint.removeEventListener(null, null, this);

This is missing WebInspector.debuggerManager which is used for Event.BreakpointsEnabledDidChange.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:642
> +        this.breakpointGutterEnabled = true;

Style: Newline after this. This is a complex operation not to be overlooked!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:653
> +        let updatedBreakpointNodes = new Set;

Nit: I'd suggest just "updatedNodes".

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1685
> +            this._statusImageElement.addEventListener("mousedown", (event) => event.stopPropagation());

Style: I'd suggest braces around the arrow function's body since the return value is ignored.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:936
> +            if (pauseData && pauseData.nodeId) {

Same comment as before. We don't use pauseData.targetNode and pauseData.insertion so we should eliminate these from the backend (InspectorDOMDebuggerAgent::descriptionForDOMEvent). Unless you think we will eventually use them, in which case a FIXME + bug mentioning we should use them would be appropriate.

> LayoutTests/inspector/dom-debugger/dom-breakpoints-expected.txt:57
> +

I still think there is a plethora of tests this could include that it does not. A good number from my previous comments are still relevant.

Some of which I still think will fail, namely invalid nodeId or invalid type string. Addressing these and testing them should be trivial. Please address at least these before landing.

[Matt Baker]

Created attachment 303785 [details]
Patch

[Matt Baker]

Comment on attachment 303727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303727&action=review

>> Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp:259
>>  void InspectorDOMDebuggerAgent::willInsertDOMNode(Node& parent)
> 
> Do you have bugs to address Ryosuke's concerns about where these get called from WebCore? I want to make sure those do not get forgotten about because those are serious concerns.

Filed follow-up:

Web Inspector: Relocate DOMDebuggerAgent instrumentation hooks to catch invalid node operations
https://bugs.webkit.org/show_bug.cgi?id=169343.

>> Source/WebInspectorUI/UserInterface/Images/DOMBreakpoint.svg:1
>> +<?xml version="1.0" encoding="utf-8"?>
> 
> What is the solution for Images/gtk?

Filed follow-up:

[GTK] Web Inspector: Add DOM breakpoint image for GTK+
https://bugs.webkit.org/show_bug.cgi?id=169326

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:936
>> +            if (pauseData && pauseData.nodeId) {
> 
> Same comment as before. We don't use pauseData.targetNode and pauseData.insertion so we should eliminate these from the backend (InspectorDOMDebuggerAgent::descriptionForDOMEvent). Unless you think we will eventually use them, in which case a FIXME + bug mentioning we should use them would be appropriate.

We may use them eventually, so tabling for now and will file a follow-up.

>> LayoutTests/inspector/dom-debugger/dom-breakpoints-expected.txt:57
>> +
> 
> I still think there is a plethora of tests this could include that it does not. A good number from my previous comments are still relevant.
> 
> Some of which I still think will fail, namely invalid nodeId or invalid type string. Addressing these and testing them should be trivial. Please address at least these before landing.

cq- for now, until I address this last item. In order to test these cases, the breakpoint's resolved state would need to be a separate property, set when the protocol methods return. This will likely impact the other tests.

[Matt Baker]

Created attachment 303860 [details]
Patch

[Matt Baker]

Patch for landing includes 4 new tests:

-- Running test case: SetBreakpointOnNonexistantWithInvalidNodeId
Attempting to set breakpoint.
Protocol result: Could not find node with given id
PASS: Protocol should return an error.
-- Running test teardown.

-- Running test case: SetBreakpointWithInvalidType
Attempting to set breakpoint.
Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type
PASS: Protocol should return an error.
-- Running test teardown.

-- Running test case: RemoveBreakpointWithInvalidNodeId
Attempting to remove breakpoint.
Protocol result: Could not find node with given id
PASS: Protocol should return an error.
-- Running test teardown.

-- Running test case: RemoveBreakpointWithInvalidType
Attempting to remove breakpoint.
Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type
PASS: Protocol should return an error.
-- Running test teardown.

[Matt Baker]

Created attachment 303861 [details]
Patch for landing

[Matt Baker]

(In reply to comment #23)
> Created attachment 303861 [details]
> Patch for landing

Fixed test name typo:

SetBreakpointOnNonexistantWithInvalidNodeId -> SetBreakpointWithInvalidNodeId

[Build Bot]

Comment on attachment 303861 [details]
Patch for landing

Attachment 303861 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3272493

New failing tests:
inspector/dom-debugger/dom-breakpoints.html

[Build Bot]

Created attachment 303873 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303861 [details]
Patch for landing

Attachment 303861 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3272666

New failing tests:
inspector/dom-debugger/dom-breakpoints.html

[Build Bot]

Created attachment 303874 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Matt Baker]

Created attachment 303876 [details]
Patch for landing

[Joseph Pecoraro]

Comment on attachment 303876 [details]
Patch for landing

New tests look good.

[WebKit Commit Bot]

Comment on attachment 303876 [details]
Patch for landing

Clearing flags on attachment: 303876

Committed r213626: <http://trac.webkit.org/changeset/213626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.