Bug 86844 - Web Inspector: "Deactivate All Breapoints" should visually get reflected in Breakpoints pane.
Summary: Web Inspector: "Deactivate All Breapoints" should visually get reflected in B...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-18 05:03 PDT by sam
Modified: 2012-05-21 23:52 PDT (History)
12 users (show)

See Also:


Attachments
Attached is the screenm (94.13 KB, image/png)
2012-05-18 05:05 PDT, sam
no flags Details
Patch (2.39 KB, patch)
2012-05-18 05:16 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (118.42 KB, patch)
2012-05-21 02:38 PDT, sam
no flags Details | Formatted Diff | Diff
Screenshot for patch showing option style properties for deactive breakpoints and an option for updating state. (136.83 KB, image/png)
2012-05-21 02:40 PDT, sam
no flags Details
Patch (117.12 KB, patch)
2012-05-21 20:51 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (116.92 KB, patch)
2012-05-21 23:26 PDT, sam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sam 2012-05-18 05:03:53 PDT
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.
Comment 1 sam 2012-05-18 05:05:05 PDT
Created attachment 142689 [details]
Attached is the screenm
Comment 2 sam 2012-05-18 05:06:12 PDT
(In reply to comment #1)
> Created an attachment (id=142689) [details]
> Attached is the screenm
* Attached is the screenshot with proposed change.
Comment 3 sam 2012-05-18 05:16:52 PDT
Created attachment 142691 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2012-05-18 05:34:40 PDT
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
Comment 5 sam 2012-05-18 05:52:51 PDT
(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?
Comment 6 Pavel Feldman 2012-05-18 06:31:51 PDT
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.
Comment 7 sam 2012-05-18 08:57:14 PDT
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.
Comment 8 sam 2012-05-21 02:38:19 PDT
Created attachment 142976 [details]
Patch
Comment 9 sam 2012-05-21 02:40:48 PDT
Created attachment 142977 [details]
Screenshot for patch showing option style properties for deactive breakpoints and an option for updating state.
Comment 10 Pavel Feldman 2012-05-21 09:32:43 PDT
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.
Comment 11 sam 2012-05-21 20:51:00 PDT
Created attachment 143185 [details]
Patch
Comment 12 Pavel Feldman 2012-05-21 22:54:16 PDT
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));
Comment 13 Yury Semikhatsky 2012-05-21 23:03:08 PDT
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.
Comment 14 sam 2012-05-21 23:26:31 PDT
Created attachment 143199 [details]
Patch
Comment 15 WebKit Review Bot 2012-05-21 23:52:05 PDT
Comment on attachment 143199 [details]
Patch

Clearing flags on attachment: 143199

Committed r117917: <http://trac.webkit.org/changeset/117917>
Comment 16 WebKit Review Bot 2012-05-21 23:52:11 PDT
All reviewed patches have been landed.  Closing bug.