Bug 87644 – Web Inspector: Option for selecting/deselecting all breakpoints in breakpoint pane

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

[sam]

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.

[sam]

Created attachment 144728 [details]
Patch

[Yury Semikhatsky]

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?

[Alexander Pavlov (apavlov)]

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.

[sam]

Created attachment 144810 [details]
Patch

[sam]

Created attachment 144811 [details]
Patch

[sam]

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.

[sam]

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

[Alexander Pavlov (apavlov)]

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

[sam]

Created attachment 144813 [details]
Patch

[sam]

(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.

[Alexander Pavlov (apavlov)]

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.

[sam]

Created attachment 144835 [details]
Patch

[sam]

(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!

[Alexander Pavlov (apavlov)]

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.

[Yury Semikhatsky]

Comment on attachment 144835 [details]
Patch

r- per Alexander's comments.

[sam]

Created attachment 145516 [details]
Patch

[sam]

(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!

[Pavel Feldman]

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.

[sam]

Created attachment 145710 [details]
Patch

[sam]

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.

[Build Bot]

Comment on attachment 145710 [details]
Patch

Attachment 145710 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12890636

[Pavel Feldman]

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

[sam]

Created attachment 145757 [details]
Patch

[sam]

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?

[Pavel Feldman]

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.

[Build Bot]

Comment on attachment 145757 [details]
Patch

Attachment 145757 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12903471

[sam]

(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.

[Pavel Feldman]

Comment on attachment 145757 [details]
Patch

Oh. OK. I thought sidebar had a copy of breakpoint objects collection.

[Ilya Tikhonovsky]

Committed r119583: <http://trac.webkit.org/changeset/119583>