Bug 89735 – Web Inspector: Provide context menu 'Delete all watch expressions.'

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

[Rahul Tiwari]

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.

[Rahul Tiwari]

Patch to follow.

[Rahul Tiwari]

Created attachment 148959 [details]
Patch

[Pavel Feldman]

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

[Pavel Feldman]

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

[Rahul Tiwari]

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

[Rahul Tiwari]

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

[Rahul Tiwari]

Created attachment 149250 [details]
Patch

[WebKit Review Bot]

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.

[Rahul Tiwari]

Created attachment 149261 [details]
Patch

[Andrey Adaikin]

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

[Rahul Tiwari]

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

[Rahul Tiwari]

Created attachment 149447 [details]
Patch

[Yury Semikhatsky]

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

[Rahul Tiwari]

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

[Alexander Pavlov (apavlov)]

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

[Rahul Tiwari]

Created attachment 149507 [details]
Patch

[Andrey Adaikin]

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

[Rahul Tiwari]

Created attachment 149909 [details]
Patch

[Rahul Tiwari]

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

[Rahul Tiwari]

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

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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.

[Rahul Tiwari]

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

[Rahul Tiwari]

Created attachment 149922 [details]
Patch

[Rahul Tiwari]

Comment on attachment 149922 [details]
Patch

Thanks Yury for the review. Can you please give a cq+

[WebKit Review Bot]

Comment on attachment 149922 [details]
Patch

Clearing flags on attachment: 149922

Committed r121433: <http://trac.webkit.org/changeset/121433>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.