Bug 86844 – Web Inspector: "Deactivate All Breapoints" should visually get reflected in Breakpoints pane.

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

[sam]

Currently if we deactivate all breakpoints from scripts panel, it deactivates all breakpoints. But the same is not reflected in Breakpoints pane. It would be more intuitive if this gets reflected visually as well.

[sam]

Created attachment 142689 [details]
Attached is the screenm

[sam]

(In reply to comment #1)
> Created an attachment (id=142689) [details]
> Attached is the screenm
* Attached is the screenshot with proposed change.

[sam]

Created attachment 142691 [details]
Patch

[Alexander Pavlov (apavlov)]

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

> Source/WebCore/ChangeLog:9
> +        Breakpoints-pane as well

nit: we usually spell panes, like "the Breakpoints pane", without dashes. The entire description can also fit a single line (WebKit has no limitations on the line length, except the programmer's reason.)

> Source/WebCore/inspector/front-end/inspector.css:2142
> +.breakpoints-list-deactivated {

You could have used the selector of ".breakpoints-deactivated .breakpoint-list" and avoid having to modify ScriptsPanel.js

[sam]

(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=142691&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Breakpoints-pane as well
> 
> nit: we usually spell panes, like "the Breakpoints pane", without dashes. The entire description can also fit a single line (WebKit has no limitations on the line length, except the programmer's reason.)
oh ok, I will change it. Thank you.
> 
> > Source/WebCore/inspector/front-end/inspector.css:2142
> > +.breakpoints-list-deactivated {
> 
> You could have used the selector of ".breakpoints-deactivated .breakpoint-list" and avoid having to modify ScriptsPanel.js

Ya, I had earlier thought of the same thing but this would also affect DOM Breakpoint pane having same .breakpoint-list class. But I guess currently "Deactivate All Breakpoints" is only applicable on JS breakpoints.
So this would give a false impression of DOM breakpoints being deactivated, which is currently not the case. Your thoughts?

[Pavel Feldman]

Comment on attachment 142691 [details]
Patch

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

Although it looks Ok, the new UI does not suggest how to enable breakpoints. Could we add a "Enable breakpoints" button right into the section as well?

> Source/WebCore/inspector/front-end/inspector.css:2145
> +    font-style: italic;

We don't use italic for disabled items.

[sam]

Thanks for review Pavel.
(In reply to comment #6)
> (From update of attachment 142691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142691&action=review
> 
> Although it looks Ok, the new UI does not suggest how to enable breakpoints. Could we add a "Enable breakpoints" button right into the section as well?
Ya, true we should have Enable breakpoints into the section. Was just thinking can we have it in the context pane along with "Remove Breakpoints".etc on right click? Your thoughts?


> 
> > Source/WebCore/inspector/front-end/inspector.css:2145
> > +    font-style: italic;
> 
> We don't use italic for disabled items.
oh, I will correct it.

[sam]

Created attachment 142976 [details]
Patch

[sam]

Created attachment 142977 [details]
Screenshot for patch showing option style properties for deactive breakpoints and an option for updating state.

[Pavel Feldman]

Comment on attachment 142976 [details]
Patch

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

> Source/WebCore/inspector/front-end/BreakpointManager.js:141
> +    updateActiveStateForBreakpoints: function()

Breakpoint manager does not have to be involved, just inline it.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:48
> +    this.breakpointActiveState = true;

You should use WebInspector.debuggerModel.breakpointsActive() so that you don't need to keep them in sync.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:177
> +        contextMenu.appendItem(breakpointActivationTitle,this._breakpointManager.updateActiveStateForBreakpoints.bind(this._breakpointManager)); 

" " before "," please.

[sam]

Created attachment 143185 [details]
Patch

[Pavel Feldman]

Comment on attachment 143185 [details]
Patch

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

r- for a tiny error, then it is good to go.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:181
> +        contextMenu.appendItem(breakpointActivationTitle, toggleActiveStateForBreakpoints.bind()); 

You should either bind(this) or not bind at all. In this case, you don't need to bind since you don't access this in the toggle function. You could also do:

var breakpointActive = WebInspector.debuggerModel.breakpointsActive();
var title = WebInspector.UIString(breakpointActive ? "Deactivate All Breakpoints" : "Activate All Breakpoints");
contextMenu.appendItem(title, WebInspector.debuggerModel.setBreakpointsActive.bind(WebInspector.debuggerModel, !breakpointActive));

[Yury Semikhatsky]

Comment on attachment 143185 [details]
Patch

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

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:181
> +        contextMenu.appendItem(breakpointActivationTitle, toggleActiveStateForBreakpoints.bind()); 

toggleActiveStateForBreakpoints.bind() -> toggleActiveStateForBreakpoints.bind(this), we don't call bind without parameters.

[sam]

Created attachment 143199 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143199 [details]
Patch

Clearing flags on attachment: 143199

Committed r117917: <http://trac.webkit.org/changeset/117917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.