Bug 63055 – Web Inspector: Feature Request: Able to remove all breakpoints

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

crbug: http://code.google.com/p/chromium/issues/detail?id=72199

Sometimes I have many breakpoints. Would like to see ability to remove them all in a single step instead of deleting them one by one.

[Ilya Tikhonovsky]

Created attachment 97948 [details]
[patch] initial version

[Alexander Pavlov (apavlov)]

Comment on attachment 97948 [details]
[patch] initial version

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

> Source/WebCore/ChangeLog:7
> +        CRBUG72199

Seemingly, we don't normally specify crbugs on the ChangeLog, do we?

> Source/WebCore/inspector/Inspector.json:1466
> +                    { "name": "breakpointsIds", "type": "array", "items": { "type": "string" }, "description": "List of breakpoint Ids" }

"breakpointIds" here and below

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:144
> +        contextMenu.appendItem(WebInspector.UIString("Remove All Breakpoints"), this._model.removeAllBreakpoints.bind(this._model));

Shouldn't this action rather be on the button bar or elsewhere? It is a global action, akin to "Toggle all breakpoints" and doubtfully should be bound to a breakpoint.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:529
> +                // There can already be a newer breakpoint;

";" -> "." ?

[Pavel Podivilov]

Comment on attachment 97948 [details]
[patch] initial version

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

Why can't you just remove breakpoints one by one? It will greatly simplify the code while performance difference would be negligible.

>> Source/WebCore/inspector/Inspector.json:1466
>> +                    { "name": "breakpointsIds", "type": "array", "items": { "type": "string" }, "description": "List of breakpoint Ids" }
> 
> "breakpointIds" here and below

Nit: breakpoint ids

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:242
> +void InspectorDebuggerAgent::removeBreakpoints(ErrorString*, PassRefPtr<InspectorArray> breakpoints)

breakpointIds?

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:246
> +    RefPtr<InspectorArray> breakpointsIds = breakpoints;

Why do you need this?

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:256
> +            m_breakpointIdToDebugServerBreakpointIds.remove(debugServerBreakpointIdsIterator);

Could you please extract the common part with removeBreakpoint to a separate function?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:472
> +            breakpoints.push(this._breakpointsByDebuggerId[id]);

What about disabled breakpoints? _breakpointsByDebuggerId is used for enabled breakpoints that are currently set on backend (including provisional breakpoints for other pages). You should iterate over sourceFile.breakpoints to remove breakpoints that are currently visible in UI.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:528
> +            if (sourceFile.breakpoints[breakpoint.lineNumber] === breakpoint) {

You will get an exception here for breakpoints without source files.

[Ilya Tikhonovsky]

Created attachment 97981 [details]
[patch] second version

the idea was slightly changed.
we want to remove all the breakpoints, resolved and unresolved.

[Pavel Podivilov]

Comment on attachment 97981 [details]
[patch] second version

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

It is counter-intuitive to remove breakpoints for all pages while breakpoints sidebar pane shows only breakpoints for current page. User will expect that only visible breakpoints will be removed, but he will actually lose all his breakpoints.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:460
> +            for (var id in this._breakpointsByDebuggerId) {

Shouldn't you remove disabled breakpoints as well?

[Pavel Feldman]

Comment on attachment 97981 [details]
[patch] second version

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

r- per Pavel's comment. I also think that introducing a new method that simply wipes out all breakpoints from the VM would be more reliable (i.e. remove all from VM instead of iterating over the map in the agent).

> Source/WebCore/inspector/Inspector.json:1465
> +                "parameters": [

no need to declare parameters.

[Ilya Tikhonovsky]

Created attachment 98910 [details]
[patch] third version

1) Show Unresolved Breakpoints item was added to the context menu
2) Remove All/Unresolved Breakpoints removes only visible breakpoints

[WebKit Review Bot]

Comment on attachment 98910 [details]
[patch] third version

Attachment 98910 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8950735

New failing tests:
inspector/debugger/debugger-breakpoints-not-activated-on-reload.html

[WebKit Review Bot]

Created attachment 98914 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yury Semikhatsky]

Comment on attachment 98910 [details]
[patch] third version

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This functionality should be tested.

> Source/WebCore/inspector/Inspector.json:1474
> +                "description": "Removes All JavaScript breakpoints."

All -> all

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:98
> +    unresolveBreakpoint: function(breakpoint)

Let's process all breakpoints once on navigation instead.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:177
> +            if (!element._data.sourceFile && !!element.parentNode != this._showUnresolvedBreakpoints)

This if looks overcomplicated, could you break it down into several smaller blocks?

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:182
>      _addListElement: function(element, beforeElement)

beforeElement is not used any more, please remove it.

[Pavel Podivilov]

Comment on attachment 98910 [details]
[patch] third version

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

Overall, it looks like there are ways more changes in DebuggerPresentationModel than really needed. You may just add breakpoint-added event for unresolved breakpoints as well.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:123
> +    void removeBreakpointFromDebugServer(const String& breakpointId);

Remove this.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:56
> +        if (!breakpoint.sourceFile)

Please change to isResolved here and below.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:72
> +        var displayName = breakpoint.url ? WebInspector.displayNameForURL(breakpoint.url) : WebInspector.displayNameForURL(breakpoint.sourceFileId);

This should not be changed. breakpoint.sourceFileId is script.sourceId for anonymous scripts, we don't want to expose it.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:150
> +        contextMenu.appendItem(WebInspector.UIString("Remove Breakpoint"), removeHandler, !breakpoint);

Should we display this item when there is no breakpoint?

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:178
> +                this._showUnresolvedBreakpoints ? this._addListElement(element) : this._removeListElement(element);

Could we filter breakpoints using css instead? Most of the logic changes here and below would not be needed.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:36
> +    this._breakpoints = {};

Why this is needed?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:64
> +    BreakpointUnresolved: "breakpoint-unresolved",

It would be easier just to mark all breakpoints as unresolved on reset instead of introducing new event.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:71
> +WebInspector.DebuggerPresentationModel.createBreakpointId = function(sourceFileId, lineNumber)

Why this code moved?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:182
> +                var breakpoint = new WebInspector.PresentationBreakpoint(sourceFile.id, sourceFile, breakpointData.lineNumber, breakpointData.condition, breakpointData.enabled);

Why is it needed?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:363
> +        var self = this;

Please use bind instead.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:480
> +            for (var sourceFileId in this._sourceFiles) {

It is better to do this synchronously, since new breakpoints could also be purged in this callback. From the other hand, the order of calls in protocol is guaranteed so you may effectively treat WebInspector.debuggerModel.removeAllBreakpoints as synchronous call.

[Ilya Tikhonovsky]

Created attachment 99080 [details]
[patch] fourth version

[Pavel Podivilov]

Comment on attachment 99080 [details]
[patch] fourth version

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

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:68
> +        var displayName = breakpoint.url ? WebInspector.displayNameForURL(breakpoint.url) : breakpoint.sourceFileId;

You should check if breakpoint is resolved here. "(program)" is used for breakpoints on anonymous script, this should not be changed.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:86
> +        while (currentElement != stopElement) {

Please use "!==" for consistency.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:167
> +    _removeAllVisibleBreakpoints: function()

removeVisibleBreakpoints?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:-343
> -            return;

Could you just dispatch breakpoint-added event in _restoreBreakpointsFromSettings instead of modifying this method?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:441
> +            var unresolvedBreakpoints = this._breakpointsWithoutSourceFile[sourceFileId];

May be we should better merge all breakpoints into a single map?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:443
> +                delete unresolvedBreakpoints[lineNumber];

You may need to remove breakpoint from backend here.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:461
> +            for (var sourceFileId in this._sourceFiles) {

Please execute this code synchronously, not in callback.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:541
> +            this.setBreakpoint(sourceFileId, breakpointData.lineNumber, breakpointData.condition, breakpointData.enabled);

Could you just dispatch breakpoint-added here for breakpoints without source file instead of calling setBreakpoint?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:661
> +                this.dispatchEventToListeners(WebInspector.DebuggerPresentationModel.Events.BreakpointAdded, breakpoint);

It is strange that you dispatch BreakpointAdded event here.

> Source/WebCore/inspector/front-end/inspector.css:4252
> +.hide-unresolved-breakpoints .info:not(:first-child) {

May be prefix with .breakpoint-list for clarity?

[Pavel Podivilov]

Comment on attachment 99080 [details]
[patch] fourth version

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

>> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:461
>> +            for (var sourceFileId in this._sourceFiles) {
> 
> Please execute this code synchronously, not in callback.

Please ignore this comment.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:464
> +                    continue;

Why do you skip source files without url?

[Yury Semikhatsky]

Comment on attachment 99080 [details]
[patch] fourth version

r- for now until the issue with navigation back and forth is not resolved.

[Alexander Pavlov (apavlov)]

*** Bug 55449 has been marked as a duplicate of this bug. ***

[Ilya Tikhonovsky]

Created attachment 119836 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119836 [details]
Patch

Attachment 119836 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10934478

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Pavel Feldman]

Comment on attachment 119836 [details]
Patch

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

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:151
> +        contextMenu.appendItem(WebInspector.UIString("Remove All JavaScript Breakpoints"), this._model.removeAllBreakpoints.bind(this._model, null), breakpointsStats.all == 0);

Can we start with introducing "Remote All Breakpoints" only? Also note that you should have 2 strings:

WebInspector.useLowerCaseMenuTitles() ? "Remote all breakpoints" : "Remote All Breakpoints"

[Ilya Tikhonovsky]

Created attachment 119848 [details]
Patch

[Pavel Feldman]

Comment on attachment 119848 [details]
Patch

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

> Source/WebCore/inspector/front-end/BreakpointManager.js:125
> +            this._deleteBreakpointFromUI(breakpoint);

nit: extract method?

[Ilya Tikhonovsky]

Committed r103229: <http://trac.webkit.org/changeset/103229>