Summary: | Web Inspector: Provide context menu 'Delete all watch expressions.' | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rahul Tiwari <rahultiwari.cse.iitr> | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Enhancement | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, vivekgalatage, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Rahul Tiwari
2012-06-21 21:13:02 PDT
Patch to follow. Created attachment 148959 [details]
Patch
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 > 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+.
(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. (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. Created attachment 149250 [details]
Patch
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.
Created attachment 149261 [details]
Patch
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 (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. Created attachment 149447 [details]
Patch
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 (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? (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). Created attachment 149507 [details]
Patch
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... Created attachment 149909 [details]
Patch
(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. (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. (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 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. (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. Created attachment 149922 [details]
Patch
Comment on attachment 149922 [details]
Patch
Thanks Yury for the review. Can you please give a cq+
Comment on attachment 149922 [details] Patch Clearing flags on attachment: 149922 Committed r121433: <http://trac.webkit.org/changeset/121433> All reviewed patches have been landed. Closing bug. |