[webkit-reviews] review requested: [Bug 63055] Web Inspector: Feature Request: Able to remove all breakpoints : [Attachment 98910] [patch] third version

[bugzilla-daemon at webkit.org]

Pavel Podivilov <podivilov at chromium.org> has asked  for review:
Bug 63055: Web Inspector: Feature Request: Able to remove all breakpoints
https://bugs.webkit.org/show_bug.cgi?id=63055

Attachment 98910: [patch] third version
https://bugs.webkit.org/attachment.cgi?id=98910&action=review

------- Additional Comments from Pavel Podivilov <podivilov at chromium.org>
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.