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.
Created attachment 142689 [details] Attached is the screenm
(In reply to comment #1) > Created an attachment (id=142689) [details] > Attached is the screenm * Attached is the screenshot with proposed change.
Created attachment 142691 [details] Patch
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
(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 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.
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.
Created attachment 142976 [details] Patch
Created attachment 142977 [details] Screenshot for patch showing option style properties for deactive breakpoints and an option for updating state.
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.
Created attachment 143185 [details] Patch
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 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.
Created attachment 143199 [details] Patch
Comment on attachment 143199 [details] Patch Clearing flags on attachment: 143199 Committed r117917: <http://trac.webkit.org/changeset/117917>
All reviewed patches have been landed. Closing bug.