Created attachment 144314 [details] option for selecting/deselecting breakpoints. The option to select and de-select all breakpoint in Breakpoints pane can make use more intuitive. Mozilla has it and its quite handy in cases. Also attached is the screen shot for illustrating the problem statement. Shall upload the patch.
Created attachment 144728 [details] Patch
Comment on attachment 144728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144728&action=review Coul > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > + // select all breakpoints which are not enabled(un-checked). Please remove this comment it doesn't add anything to the code. > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > + // deselect all breakpoints which are enabled(checked). Please remove this comment it doesn't add anything to the code. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:178 > + // Get the count for checked breakpoints. Please remove the comment. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:192 > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selecAllBreakpoints.bind(this._breakpointManager)); You might want to hide this item if there is only one breakpoint. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:196 > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selecAllBreakpoints.bind(this._breakpointManager)); Could you change "Remove All JavaScript Breakpoints" title above to "Remove All Breakpoints" so that they are all stick to the same style?
Comment on attachment 144728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144728&action=review Please fix the style nits before landing the patch. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:183 > + for (var it = 0; it < breakpoints.length ; ++it) { odd space before the last ';'. We also don't use "it", since it is not an actual iterator. Use "i" as you would normally do while iterating an array. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:191 > + if(!selectBreakpointCount) missing space between "if" and opening parenthesis. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:193 > + else if (selectBreakpointCount == this._items.values().length) Please use "===" whenever possible.
Created attachment 144810 [details] Patch
Created attachment 144811 [details] Patch
Thanks Yury for review. (In reply to comment #2) > (From update of attachment 144728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144728&action=review > > Coul > > > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > > + // select all breakpoints which are not enabled(un-checked). > > Please remove this comment it doesn't add anything to the code. > removed. > > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > > + // deselect all breakpoints which are enabled(checked). > > Please remove this comment it doesn't add anything to the code. > removed. > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:178 > > + // Get the count for checked breakpoints. > > Please remove the comment. > removed. > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:192 > > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selecAllBreakpoints.bind(this._breakpointManager)); > > You might want to hide this item if there is only one breakpoint. > Ya true, done. Thanks. > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:196 > > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selecAllBreakpoints.bind(this._breakpointManager)); > > Could you change "Remove All JavaScript Breakpoints" title above to "Remove All Breakpoints" so that they are all stick to the same style? ok, done.
Thanks for review Alexander. (In reply to comment #3) > (From update of attachment 144728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144728&action=review > > Please fix the style nits before landing the patch. > > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:183 > > + for (var it = 0; it < breakpoints.length ; ++it) { > > odd space before the last ';'. We also don't use "it", since it is not an actual iterator. Use "i" as you would normally do while iterating an array. > oops!, thank you. > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:191 > > + if(!selectBreakpointCount) > > missing space between "if" and opening parenthesis. > done > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:193 > > + else if (selectBreakpointCount == this._items.values().length) > > Please use "===" whenever possible. done
Comment on attachment 144811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144811&action=review > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > + selecAllBreakpoints: function() Just noticed a typo: "selec" should be "select" > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > + var breakpoints = this._breakpoints.slice(); All indents should be exactly 4 spaces (you've got 2 or 3 here and below). > Source/WebCore/inspector/front-end/BreakpointManager.js:148 > + stray blank line > Source/WebCore/inspector/front-end/BreakpointManager.js:151 > + deselecAllBreakpoints: function() Same typo as above > Source/WebCore/inspector/front-end/BreakpointManager.js:153 > + var breakpoints = this._breakpoints.slice(); Please fix the indentation > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:180 > + var breakpoints = breakpointMap.values(); Please fix the indentation > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:190 > + stray blank line
Created attachment 144813 [details] Patch
(In reply to comment #8) > (From update of attachment 144811 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144811&action=review > > > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > > + selecAllBreakpoints: function() > > Just noticed a typo: "selec" should be "select" > :( , corrected , thank you. > > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > > + var breakpoints = this._breakpoints.slice(); > > All indents should be exactly 4 spaces (you've got 2 or 3 here and below). > done > > Source/WebCore/inspector/front-end/BreakpointManager.js:148 > > + > > stray blank line > done > > Source/WebCore/inspector/front-end/BreakpointManager.js:151 > > + deselecAllBreakpoints: function() > > Same typo as above > :( , corrected > > Source/WebCore/inspector/front-end/BreakpointManager.js:153 > > + var breakpoints = this._breakpoints.slice(); > > Please fix the indentation > done > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:180 > > + var breakpoints = breakpointMap.values(); > > Please fix the indentation > done > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:190 > > + > > stray blank line ok, removed.
Comment on attachment 144813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144813&action=review > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:189 > + if (this._items.values().length > 1) { optional: since you have this._items.values() retrieved at this point, you could assign it to a var and pass one into selectedBreakpointsCount() in place of this._items. Otherwise the patch looks good to me.
Created attachment 144835 [details] Patch
(In reply to comment #11) > (From update of attachment 144813 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144813&action=review > > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:189 > > + if (this._items.values().length > 1) { > > optional: since you have this._items.values() retrieved at this point, you could assign it to a var and pass one into selectedBreakpointsCount() in place of this._items. Otherwise the patch looks good to me. ya true, changed. thanks!
Comment on attachment 144835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144835&action=review > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > + var breakpoints = this._breakpoints.slice(); Why did you introduce slicing here? > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > + var breakpoints = this._breakpoints.slice(); Ditto > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:193 > + else if (selectBreakpointCount === this._items.values().length) You missed this line during the refactoring (this._items.values()), other than that you did exactly what I was talking about.
Comment on attachment 144835 [details] Patch r- per Alexander's comments.
Created attachment 145516 [details] Patch
(In reply to comment #14) > (From update of attachment 144835 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144835&action=review > > > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > > + var breakpoints = this._breakpoints.slice(); > > Why did you introduce slicing here? didn't include it intentionally. took the ref from removeAll function. But have changed it now. Thanks. > > > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > > + var breakpoints = this._breakpoints.slice(); > > Ditto > > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:193 > > + else if (selectBreakpointCount === this._items.values().length) > > You missed this line during the refactoring (this._items.values()), other than that you did exactly what I was talking about. errr...have changed it, thanks!
Comment on attachment 145516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145516&action=review localizedStrings is no longer binary. please edit it as a text file with UTF-8 encoding, this bug should show a nice diff in the file. > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > + selectAllBreakpoints: function() This name is misleading: should be enableAllBreakpoints. > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > + for (var i = 0; i < this._breakpoints.length; ++i) { ditto (disable) > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:178 > + function selectedBreakpointsCount(breakpoints) enabledBreakpointCount > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:192 > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selectAllBreakpoints.bind(this._breakpointManager)); Enable / disable. also, you should capitalize breakpoints depending on the useLowerCaseMenuTitles.
Created attachment 145710 [details] Patch
Thanks for review Pavel (In reply to comment #18) > (From update of attachment 145516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145516&action=review > > localizedStrings is no longer binary. please edit it as a text file with UTF-8 encoding, this bug should show a nice diff in the file. > I have edited the file as text in utf-8 but still not seeing nice diff. Has it something to do with svn:mime-type!, i tried deleting that also. Or I guess I am missing something. > > Source/WebCore/inspector/front-end/BreakpointManager.js:141 > > + selectAllBreakpoints: function() > > This name is misleading: should be enableAllBreakpoints. > ok, changed > > Source/WebCore/inspector/front-end/BreakpointManager.js:152 > > + for (var i = 0; i < this._breakpoints.length; ++i) { > > ditto (disable) > ok, changed > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:178 > > + function selectedBreakpointsCount(breakpoints) > > enabledBreakpointCount > ok, changed > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:192 > > + contextMenu.appendItem(WebInspector.UIString("Select All Breakpoints"), this._breakpointManager.selectAllBreakpoints.bind(this._breakpointManager)); > > Enable / disable. also, you should capitalize breakpoints depending on the useLowerCaseMenuTitles. changed.
Comment on attachment 145710 [details] Patch Attachment 145710 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12890636
Comment on attachment 145710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145710&action=review > Source/WebCore/inspector/front-end/BreakpointManager.js:142 > + { I don't quite like that you count and toggle breakpoints using different collections. You could do it all in sidebar and toggle exactly what you count. > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:195 > + if (!enableBreakpointCount) I don't think this logic is intuitive. You should add both actions at all times but disable 'enable all' when all are already enabled, you should disable 'disable all' when everything is already disabled
Created attachment 145757 [details] Patch
Thanks Pavel, (In reply to comment #22) > (From update of attachment 145710 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145710&action=review > > > Source/WebCore/inspector/front-end/BreakpointManager.js:142 > > + { > > I don't quite like that you count and toggle breakpoints using different collections. You could do it all in sidebar and toggle exactly what you count. > i could not get it fully, but have now only one function to toggle breakpoints. > > Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:195 > > + if (!enableBreakpointCount) > > I don't think this logic is intuitive. You should add both actions at all times but disable 'enable all' when all are already enabled, you should disable 'disable all' when everything is already disabled have done some change, is it ok?
Comment on attachment 145757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145757&action=review > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > + */ As I mentioned earlier, this should be in the BreakpointsSidebarPane. That way you would use single breakpoints collection. Otherwise looks good.
Comment on attachment 145757 [details] Patch Attachment 145757 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12903471
(In reply to comment #25) > (From update of attachment 145757 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145757&action=review > > > Source/WebCore/inspector/front-end/BreakpointManager.js:143 > > + */ > > As I mentioned earlier, this should be in the BreakpointsSidebarPane. That way you would use single breakpoints collection. Otherwise looks good. Thanks Pavel for feedback, but am not sure how to get the Breakpoint in BreakpointsSidebarPane. The collection to get enableCount in BreakpointsSidebarPane is this._items.values() and counted on checked property. And in BreakpointManager, I get collection of WebInspector.BreakpointManager.Breakpoint to toggle breakpoints. Since this._breakpoints of BreakpointManager is marked as private(_), I thought of delegating toggle function in BreakpointManager. I guess i am missing out on something here. Can you please let me know.
Comment on attachment 145757 [details] Patch Oh. OK. I thought sidebar had a copy of breakpoint objects collection.
Committed r119583: <http://trac.webkit.org/changeset/119583>