Bug 87644 - Web Inspector: Option for selecting/deselecting all breakpoints in breakpoint pane
Summary: Web Inspector: Option for selecting/deselecting all breakpoints in breakpoint...
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: sam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-28 02:57 PDT by sam
Modified: 2012-06-06 04:28 PDT (History)
13 users (show)

See Also:


Attachments
option for selecting/deselecting breakpoints. (209.65 KB, image/png)
2012-05-28 02:57 PDT, sam
no flags Details
Patch (61.26 KB, patch)
2012-05-29 23:49 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (61.75 KB, patch)
2012-05-30 07:32 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (61.77 KB, patch)
2012-05-30 07:36 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (61.76 KB, patch)
2012-05-30 07:53 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (61.75 KB, patch)
2012-05-30 09:11 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (61.72 KB, patch)
2012-06-03 22:22 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2012-06-05 00:22 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2012-06-05 04:48 PDT, sam
pfeldman: review+
buildbot: commit-queue-
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-28 02:57:17 PDT
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.
Comment 1 sam 2012-05-29 23:49:00 PDT
Created attachment 144728 [details]
Patch
Comment 2 Yury Semikhatsky 2012-05-30 07:16:37 PDT
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 3 Alexander Pavlov (apavlov) 2012-05-30 07:23:48 PDT
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.
Comment 4 sam 2012-05-30 07:32:18 PDT
Created attachment 144810 [details]
Patch
Comment 5 sam 2012-05-30 07:36:55 PDT
Created attachment 144811 [details]
Patch
Comment 6 sam 2012-05-30 07:40:42 PDT
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.
Comment 7 sam 2012-05-30 07:42:11 PDT
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 8 Alexander Pavlov (apavlov) 2012-05-30 07:44:50 PDT
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
Comment 9 sam 2012-05-30 07:53:07 PDT
Created attachment 144813 [details]
Patch
Comment 10 sam 2012-05-30 07:55:18 PDT
(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 11 Alexander Pavlov (apavlov) 2012-05-30 08:04:45 PDT
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.
Comment 12 sam 2012-05-30 09:11:49 PDT
Created attachment 144835 [details]
Patch
Comment 13 sam 2012-05-30 09:13:31 PDT
(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 14 Alexander Pavlov (apavlov) 2012-05-30 09:17:09 PDT
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 15 Yury Semikhatsky 2012-05-31 01:19:57 PDT
Comment on attachment 144835 [details]
Patch

r- per Alexander's comments.
Comment 16 sam 2012-06-03 22:22:36 PDT
Created attachment 145516 [details]
Patch
Comment 17 sam 2012-06-03 22:43:14 PDT
(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 18 Pavel Feldman 2012-06-04 05:22:41 PDT
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.
Comment 19 sam 2012-06-05 00:22:15 PDT
Created attachment 145710 [details]
Patch
Comment 20 sam 2012-06-05 00:27:13 PDT
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 21 Build Bot 2012-06-05 01:05:43 PDT
Comment on attachment 145710 [details]
Patch

Attachment 145710 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12890636
Comment 22 Pavel Feldman 2012-06-05 01:29:30 PDT
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
Comment 23 sam 2012-06-05 04:48:59 PDT
Created attachment 145757 [details]
Patch
Comment 24 sam 2012-06-05 04:56:55 PDT

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 25 Pavel Feldman 2012-06-05 05:26:36 PDT
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 26 Build Bot 2012-06-05 05:54:05 PDT
Comment on attachment 145757 [details]
Patch

Attachment 145757 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12903471
Comment 27 sam 2012-06-05 06:25:07 PDT
(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 28 Pavel Feldman 2012-06-05 07:15:59 PDT
Comment on attachment 145757 [details]
Patch

Oh. OK. I thought sidebar had a copy of breakpoint objects collection.
Comment 29 Ilya Tikhonovsky 2012-06-06 04:28:12 PDT
Committed r119583: <http://trac.webkit.org/changeset/119583>