Bug 89735 - Web Inspector: Provide context menu 'Delete all watch expressions.'
Summary: Web Inspector: Provide context menu 'Delete all watch expressions.'
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-06-21 21:13 PDT by Rahul Tiwari
Modified: 2012-06-28 08:44 PDT (History)
12 users (show)

See Also:


Attachments
Patch (4.23 KB, patch)
2012-06-21 21:46 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2012-06-24 23:50 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2012-06-25 00:47 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2012-06-25 21:39 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.76 KB, patch)
2012-06-26 04:52 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2012-06-28 02:55 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff
Patch (4.50 KB, patch)
2012-06-28 04:12 PDT, Rahul Tiwari
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rahul Tiwari 2012-06-21 21:13:02 PDT
In the Watch Expressions Pane, currently we only have the the choice of deleting expressions one by one. It would be good to have context menu 'delete' and 'delete all expressions' options.
Comment 1 Rahul Tiwari 2012-06-21 21:13:30 PDT
Patch to follow.
Comment 2 Rahul Tiwari 2012-06-21 21:46:17 PDT
Created attachment 148959 [details]
Patch
Comment 3 Pavel Feldman 2012-06-21 21:53:09 PDT
Comment on attachment 148959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148959&action=review

Rahul Tiwari, I would kindly ask that you do not r+ inspector-related changes. Reviewership means that you understand the code you are reviewing.

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:258
> +    deleteAllExpressions: function()

This should be declared private.

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:261
> +            delete this.watchExpressions[i];

What are you trying to do here? Clearing the array?

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:274
> +    expressionCount: function()

should be private
Comment 4 Pavel Feldman 2012-06-21 21:54:48 PDT
> Rahul Tiwari, I would kindly ask that you do not r+ inspector-related changes. Reviewership means that you understand the code you are reviewing.

Oh, sorry, I did not realize that you r+-ed the change for yourself. You should set it to r? so that the reviewer either converted it to r- or r+.
Comment 5 Rahul Tiwari 2012-06-21 22:18:03 PDT
(In reply to comment #4)
> > Rahul Tiwari, I would kindly ask that you do not r+ inspector-related changes. Reviewership means that you understand the code you are reviewing.
> 
> Oh, sorry, I did not realize that you r+-ed the change for yourself. You should set it to r? so that the reviewer either converted it to r- or r+.

Thanks for the review Pavel. I will remember the review thing next time. Also, I will make the necessary changes. As for the following change:


> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:261
> +            delete this.watchExpressions[i];

> What are you trying to do here? Clearing the array?

Actually "delete this.watchExpressions[i];" only sets the 'i'th array entry to undefined, and does not physically delete it. Splicing the array is a better option. But since the existing functionality used this technique, therefore I used the same.

Please suggest me if I should let it stay the same or use splicing.
Comment 6 Rahul Tiwari 2012-06-24 23:36:53 PDT
(In reply to comment #4)
> > Rahul Tiwari, I would kindly ask that you do not r+ inspector-related changes. Reviewership means that you understand the code you are reviewing.
> 
> Oh, sorry, I did not realize that you r+-ed the change for yourself. You should set it to r? so that the reviewer either converted it to r- or r+.

I have made the required changes and spliced the array whenever an expression is deleted from Watch Expressions Panel.
Comment 7 Rahul Tiwari 2012-06-24 23:50:23 PDT
Created attachment 149250 [details]
Patch
Comment 8 WebKit Review Bot 2012-06-24 23:54:13 PDT
Attachment 149250 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Rahul Tiwari 2012-06-25 00:47:34 PDT
Created attachment 149261 [details]
Patch
Comment 10 Andrey Adaikin 2012-06-25 10:25:48 PDT
Comment on attachment 149261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149261&action=review

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:263
> +        this.watchExpressions.splice(0, expressionlength);

this.watchExpressions = []; ?

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:278
> +        var count= 0;

spaces around "="

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:279
> +        for (var i = 0; i < this.watchExpressions.length; ++i){

spaces between "){"

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:379
> +       var expressioncount= this.treeOutline.section._expressionCount();

spaces; expressioncount => expressionCount

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:381
> +            contextMenu.appendItem(WebInspector.UIString("Delete all watch expressions"), this._deleteAllExpressions.bind(this));

wrong indent
Comment 11 Rahul Tiwari 2012-06-25 20:43:41 PDT
(In reply to comment #10)
> (From update of attachment 149261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149261&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:263
> > +        this.watchExpressions.splice(0, expressionlength);
> 
> this.watchExpressions = []; ?
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:278
> > +        var count= 0;
> 
> spaces around "="
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:279
> > +        for (var i = 0; i < this.watchExpressions.length; ++i){
> 
> spaces between "){"
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:379
> > +       var expressioncount= this.treeOutline.section._expressionCount();
> 
> spaces; expressioncount => expressionCount
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:381
> > +            contextMenu.appendItem(WebInspector.UIString("Delete all watch expressions"), this._deleteAllExpressions.bind(this));
> 
> wrong indent

Thanks Andrey for the review. I will make the changes and post the patch.
Comment 12 Rahul Tiwari 2012-06-25 21:39:48 PDT
Created attachment 149447 [details]
Patch
Comment 13 Yury Semikhatsky 2012-06-26 03:34:05 PDT
Comment on attachment 149447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149447&action=review

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:278
> +        for (var i = 0; i < this.watchExpressions.length; ++i)

style nit: {} around multiline body
Comment 14 Rahul Tiwari 2012-06-26 04:34:58 PDT
(In reply to comment #13)
> (From update of attachment 149447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149447&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:278
> > +        for (var i = 0; i < this.watchExpressions.length; ++i)
> 
> style nit: {} around multiline body

Thanks Yury for the review. I was intending to use the same styling as you proposed, but noticed that the function just above it "findAddedTreeElement" also omitted the braces. So to keep the code consistent, I omitted the braces as well.
So should I change it?
Comment 15 Alexander Pavlov (apavlov) 2012-06-26 04:42:51 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 149447 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149447&action=review
> > 
> > > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:278
> > > +        for (var i = 0; i < this.watchExpressions.length; ++i)
> > 
> > style nit: {} around multiline body
> 
> Thanks Yury for the review. I was intending to use the same styling as you proposed, but noticed that the function just above it "findAddedTreeElement" also omitted the braces. So to keep the code consistent, I omitted the braces as well.
> So should I change it?

While Yury is away, please let me take liberty and suggest that you fix findAddedTreeElement() and saveExpressions(), too. Both of them exhibit this bad style and must be just a long-standing legacy code. In all reasonable cases we stick to the coding guidelines found at http://www.webkit.org/coding/coding-style.html (and this particular case is covered by http://www.webkit.org/coding/coding-style.html#braces-blocks).
Comment 16 Rahul Tiwari 2012-06-26 04:52:13 PDT
Created attachment 149507 [details]
Patch
Comment 17 Andrey Adaikin 2012-06-26 09:39:05 PDT
Comment on attachment 149507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149507&action=review

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:262
> +        this.watchExpressions = [];

BTW, I may have mislead you with this, if watchExpression happens to be a "reference" to an array stored in settings that we also want to update...
Comment 18 Rahul Tiwari 2012-06-28 02:55:17 PDT
Created attachment 149909 [details]
Patch
Comment 19 Rahul Tiwari 2012-06-28 02:58:41 PDT
(In reply to comment #17)
> (From update of attachment 149507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149507&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:262
> > +        this.watchExpressions = [];
> 
> BTW, I may have mislead you with this, if watchExpression happens to be a "reference" to an array stored in settings that we also want to update...

Thanks Yury for the Comment. I understand your point. Posted the patch with correction.
Comment 20 Rahul Tiwari 2012-06-28 03:00:06 PDT
(In reply to comment #17)
> (From update of attachment 149507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149507&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:262
> > +        this.watchExpressions = [];
> 
> BTW, I may have mislead you with this, if watchExpression happens to be a "reference" to an array stored in settings that we also want to update...

Thanks Andrey for the Comment. I understand your point. Posted the patch with correction.
Comment 21 Yury Semikhatsky 2012-06-28 03:11:41 PDT
(In reply to comment #17)
> (From update of attachment 149507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149507&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:262
> > +        this.watchExpressions = [];
> 
> BTW, I may have mislead you with this, if watchExpression happens to be a "reference" to an array stored in settings that we also want to update...

We shouldn't return a reference to an array from the setting and rely on the storage to be updated when the array is modified. All settings modifications must be done with Setting.set method which among other things will update the internal reference.


(In reply to comment #20)
> (In reply to comment #17)
> > (From update of attachment 149507 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149507&action=review
> > 
> > > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:262
> > > +        this.watchExpressions = [];
> > 
> > BTW, I may have mislead you with this, if watchExpression happens to be a "reference" to an array stored in settings that we also want to update...
> 
> Thanks Andrey for the Comment. I understand your point. Posted the patch with correction.

The new code is confusing. If we want to protect Setting's internal structure we should return a copy of the setting structure from Setting.get() Anyways in this particular place this.watchExpressions = []; works just fine as is followed by a call to Setting.set().
Comment 22 Yury Semikhatsky 2012-06-28 03:15:18 PDT
Comment on attachment 149909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149909&action=review

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:263
> +        this.watchExpressions.splice(0, expressionCount);

Please this.watchExpressions = []; as in the previous patch and file a bug that WebInspector.Setting.get() should return a copy of the setting value.
Comment 23 Rahul Tiwari 2012-06-28 03:54:11 PDT
(In reply to comment #22)
> (From update of attachment 149909 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149909&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:263
> > +        this.watchExpressions.splice(0, expressionCount);
> 
> Please this.watchExpressions = []; as in the previous patch and file a bug that WebInspector.Setting.get() should return a copy of the setting value.

Thanks Yury. I will raise a bug for that. Also I will make the required changes in the patch.
Comment 24 Rahul Tiwari 2012-06-28 04:12:34 PDT
Created attachment 149922 [details]
Patch
Comment 25 Rahul Tiwari 2012-06-28 05:38:53 PDT
Comment on attachment 149922 [details]
Patch

Thanks Yury for the review. Can you please give a cq+
Comment 26 WebKit Review Bot 2012-06-28 08:43:51 PDT
Comment on attachment 149922 [details]
Patch

Clearing flags on attachment: 149922

Committed r121433: <http://trac.webkit.org/changeset/121433>
Comment 27 WebKit Review Bot 2012-06-28 08:44:09 PDT
All reviewed patches have been landed.  Closing bug.