| Summary: | Web Inspector: Add clear cookies context menu item to Resources > Cookies | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Paul Irish <paulirish> |
| Component: | Web Inspector (Deprecated) | Assignee: | Vsevolod Vlasov <vsevik> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | apavlov, bweinstein, caseq, eustas, joepeck, keishi, loislo, pfeldman, phistuck, pmuellr, rik, webkit.review.bot, yurys |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
|
Description
Paul Irish
2012-05-22 09:56:11 PDT
Please, rename this bug to - Web Inspector: Plumb and expose cookie clearing options throughout the Resources Cookies tree and views I created a patch that adds the following - - A clear button when viewing a non read only CookieItemsView (I re-used an existing glyph that means clear in other contexts). - A "Clear all from (domain)" context menu option when right clicking on a cookie row that will only remove cookies that match the specific domain of the cookie row (.example.com, but not www.example.com). - A "Clear all" context menu option when right clicking within a CookieItemsView and when right clicking on a cookie row. - A "Clear" context menu option when right clicking on a cookie tree list item. Also - - The delete button now stays invisible until you select a cookie row. - Some drive by JSDoc fixes. The patch does not add - - "Clear session cookies" (and "Clear session cookies from (domain)"). - "Clear persistent cookies" (and "Clear persistent cookies from (domain)"). - "Clear all" (or other wording) when right clicking on the "Cookies" tree title. When I debug websites, sometimes cookies influence the issues I am seeing. Most of the time, only session cookies are interfering, or only persistent cookies are interfering. Removing only one of the kinds keeps me logged in (for example) while debugging, which helps me further debug the issue. I do not feel strongly about it, of course, but I think it may be useful. Let me know if this is reasonable and I will add it to the patch. The "Clear all" context menu on the "Cookies" tree title, if implemented, would clear all of the cookies these views show, not all of the browser cookies (though I can also add that there, if you want). Let me know if this is reasonable (and suggest better wording, I guess) and I will add it to the patch. Created attachment 194804 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views
Comment on attachment 194804 [details] Plumb cookie clearing throughout the Resources Cookie tree and views View in context: https://bugs.webkit.org/attachment.cgi?id=194804&action=review > Source/WebCore/inspector/front-end/CookieItemsView.js:109 > + if (this._cookiesTable.selectedCookie()) 1) Braces are placed in the same line for conditional/loop operator 2) If block contains only one line - do not place it to braces. > Source/WebCore/inspector/front-end/CookiesTable.js:58 > + { Ditto > Source/WebCore/inspector/front-end/CookiesTable.js:61 > + else Ditto. > Source/WebCore/inspector/front-end/CookiesTable.js:98 > + if (event.target.isSelfOrDescendant(this._dataGrid.headerTableBody)) { Remove braces. > Source/WebCore/inspector/front-end/CookiesTable.js:103 > + if (this._refreshCallback) { Remove braces. > Source/WebCore/inspector/front-end/CookiesTable.js:115 > + } else { Ditto. > Source/WebCore/inspector/front-end/CookiesTable.js:161 > + { Move brace in previous line > Source/WebCore/inspector/front-end/ResourcesPanel.js:518 > + this._cookieViews[cookieDomain].clear(); Fix spacing. Comment on attachment 194804 [details] Plumb cookie clearing throughout the Resources Cookie tree and views View in context: https://bugs.webkit.org/attachment.cgi?id=194804&action=review Thank you very much for the review. Embarrassing nits. I really thought I nailed it. I guess the first iteration was correct and I broke it when I added more code. :(/ >> Source/WebCore/inspector/front-end/CookieItemsView.js:109 >> + if (this._cookiesTable.selectedCookie()) > > 1) Braces are placed in the same line for conditional/loop operator > 2) If block contains only one line - do not place it to braces. Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:58 >> + { > > Ditto Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:61 >> + else > > Ditto. Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:98 >> + if (event.target.isSelfOrDescendant(this._dataGrid.headerTableBody)) { > > Remove braces. Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:103 >> + if (this._refreshCallback) { > > Remove braces. Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:115 >> + } else { > > Ditto. Looks weird (if with braces, else without). But, done. >> Source/WebCore/inspector/front-end/CookiesTable.js:161 >> + { > > Move brace in previous line Done. >> Source/WebCore/inspector/front-end/ResourcesPanel.js:518 >> + this._cookieViews[cookieDomain].clear(); > > Fix spacing. Done. Created attachment 194814 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1
Comment on attachment 194814 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1 View in context: https://bugs.webkit.org/attachment.cgi?id=194814&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Usually "no new tests" is removed from changelog entry. This label is used to attract developers attention before commiting patch. > Source/WebCore/ChangeLog:17 > + (WebInspector.CookieItemsView): IMHO member items should be either commented, or removed (with adding comment to file entry) Comment on attachment 194814 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1 View in context: https://bugs.webkit.org/attachment.cgi?id=194814&action=review > Source/WebCore/inspector/front-end/CookieItemsView.js:150 > + this._cookiesTable.clear(""); Just this._cookiesTable.clear()? > Source/WebCore/inspector/front-end/CookieItemsView.js:161 > + this._deleteButton.visible = true; I wonder if we ever hide the button? > Source/WebCore/inspector/front-end/CookiesTable.js:66 > + if (selectedCallback) { Please drop redundant '{' & '}' > Source/WebCore/inspector/front-end/CookiesTable.js:95 > + if (event.target.isSelfOrDescendant(this._dataGrid.headerTableBody)) This looks a bit fragile. Can we check for the result of dataGridNodeFromNode() instead? > Source/WebCore/inspector/front-end/CookiesTable.js:110 > + contextMenu.appendItem(String.sprintf(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all from \"%s\"" : "Clear All from \"%s\""), cookieDomain), this._clearAndRefresh.bind(this, cookieDomain)); String.sprintf is redundant here, WebInspector.UIString() would actually do formatting for you, i.e.: ...appendItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all from \"%s\"" : "Clear All from \"%s\"", cookieDomain)) > Source/WebCore/inspector/front-end/CookiesTable.js:114 > + contextMenu.appendItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all" : "Clear All"), this._clearAndRefresh.bind(this, "")); "" looks redundant here as well. > Source/WebCore/inspector/front-end/CookiesTable.js:150 > + var cookies, j, cookieCount; Please move the declarations to where the variables are initialized. > Source/WebCore/inspector/front-end/ResourcesPanel.js:516 > + clearCookies: function(treeElement, cookieDomain) Please annotate. > Source/WebCore/inspector/front-end/ResourcesPanel.js:1946 > + onattach: function() style: add one empty line above this. > Source/WebCore/inspector/front-end/ResourcesPanel.js:1952 > + _handleContextMenuEvent: function(event) Please annotate. > Source/WebCore/inspector/front-end/ResourcesPanel.js:1959 > + _clearCookies: function(domain) ditto. Comment on attachment 194814 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1 View in context: https://bugs.webkit.org/attachment.cgi?id=194814&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. > > Usually "no new tests" is removed from changelog entry. > > This label is used to attract developers attention before commiting patch. Done. >> Source/WebCore/ChangeLog:17 >> + (WebInspector.CookieItemsView): > > IMHO member items should be either commented, or removed (with adding comment to file entry) Done. >> Source/WebCore/inspector/front-end/CookieItemsView.js:150 >> + this._cookiesTable.clear(""); > > Just this._cookiesTable.clear()? Done. >> Source/WebCore/inspector/front-end/CookieItemsView.js:161 >> + this._deleteButton.visible = true; > > I wonder if we ever hide the button? We do now, as part of this patch. Before, it would have always been visible, even when nothing is selected, which occasionally had weird consequences when actually clicking on it and it was a wrong behavior anyway. >> Source/WebCore/inspector/front-end/CookiesTable.js:66 >> + if (selectedCallback) { > > Please drop redundant '{' & '}' Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:95 >> + if (event.target.isSelfOrDescendant(this._dataGrid.headerTableBody)) > > This looks a bit fragile. Can we check for the result of dataGridNodeFromNode() instead? Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:110 >> + contextMenu.appendItem(String.sprintf(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all from \"%s\"" : "Clear All from \"%s\""), cookieDomain), this._clearAndRefresh.bind(this, cookieDomain)); > > String.sprintf is redundant here, WebInspector.UIString() would actually do formatting for you, i.e.: > ...appendItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all from \"%s\"" : "Clear All from \"%s\"", cookieDomain)) I did not know UIString() can do that. Thank you! Done. >> Source/WebCore/inspector/front-end/CookiesTable.js:114 >> + contextMenu.appendItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all" : "Clear All"), this._clearAndRefresh.bind(this, "")); > > "" looks redundant here as well. Do you prefer null? Do you prefer a typeof domain === "string" check within _clearAndRefresh? I do not think it can be this._clearAndRefresh.bind(this), because then the first parameter would be the event or something context menu adds, right? >> Source/WebCore/inspector/front-end/CookiesTable.js:150 >> + var cookies, j, cookieCount; > > Please move the declarations to where the variables are initialized. This was done on purpose - declaring variables within loops and such gives a false sense that it works, while it really does not (declarations are evaluated once, at the beginning of a function/global scope). Do you insist? >> Source/WebCore/inspector/front-end/ResourcesPanel.js:516 >> + clearCookies: function(treeElement, cookieDomain) > > Please annotate. Done. And removed the seemingly redundant treeElement. >> Source/WebCore/inspector/front-end/ResourcesPanel.js:1946 >> + onattach: function() > > style: add one empty line above this. Done. >> Source/WebCore/inspector/front-end/ResourcesPanel.js:1952 >> + _handleContextMenuEvent: function(event) > > Please annotate. Done. I hope I got it right ({Event} event?). >> Source/WebCore/inspector/front-end/ResourcesPanel.js:1959 >> + _clearCookies: function(domain) > > ditto. Done. Created attachment 195091 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 2
(In reply to comment #8) > >> Source/WebCore/inspector/front-end/CookiesTable.js:114 > >> + contextMenu.appendItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Clear all" : "Clear All"), this._clearAndRefresh.bind(this, "")); > > > > "" looks redundant here as well. > > Do you prefer null? > Do you prefer a typeof domain === "string" check within _clearAndRefresh? > I do not think it can be this._clearAndRefresh.bind(this), because then the first parameter would be the event or something context menu adds, right? I think we do not pass any parameters to context menu callback, but you point is still good if we ever start passing additional parameters, so I guess just binding null to first argument makes sense. > >> Source/WebCore/inspector/front-end/CookiesTable.js:150 > >> + var cookies, j, cookieCount; > > > > Please move the declarations to where the variables are initialized. > > This was done on purpose - declaring variables within loops and such gives a false sense that it works, while it really does not (declarations are evaluated once, at the beginning of a function/global scope). We generally declare in the place of initialization through the rest of the code, and the consensus here is that whatever surprises could pop up due to JS promoting these variables to the function scope, would likely be caught by closure compiler. > Done. I hope I got it right ({Event} event?). Yep :) Created attachment 195118 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 3
Comment on attachment 195118 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 3 View in context: https://bugs.webkit.org/attachment.cgi?id=195118&action=review > Source/WebCore/inspector/front-end/CookieItemsView.js:110 > + if (this._cookiesTable.selectedCookie()) > + this._deleteButton.visible = true; nit: perhaps, this._deleteButton.visible = !!this._cookiesTable.selectedCookie() ? > Source/WebCore/inspector/front-end/CookieItemsView.js:154 > + _clearButtonClicked: function() mit: why not bind clear directly? > Source/WebCore/inspector/front-end/CookiesTable.js:151 > + { style: please keep block's opening { on previous line. Comment on attachment 195118 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 3 View in context: https://bugs.webkit.org/attachment.cgi?id=195118&action=review >> Source/WebCore/inspector/front-end/CookieItemsView.js:110 >> + this._deleteButton.visible = true; > > nit: perhaps, this._deleteButton.visible = !!this._cookiesTable.selectedCookie() ? I like it. Done! >> Source/WebCore/inspector/front-end/CookieItemsView.js:154 >> + _clearButtonClicked: function() > > mit: why not bind clear directly? For the same reason _refreshButtonClicked is bound instead of this.update. I guess for private button action and public API separation. I do not feel strongly about it, but we should be consistent. Should I change both of them or keep both of them as is? >> Source/WebCore/inspector/front-end/CookiesTable.js:151 >> + { > > style: please keep block's opening { on previous line. Damn that fixation of mine. Done! Created attachment 195322 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4
Comment on attachment 195322 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4 View in context: https://bugs.webkit.org/attachment.cgi?id=195322&action=review Thanks for fixing this! > Source/WebCore/inspector/front-end/ResourcesPanel.js:521 > + this._cookieViews[cookieDomain].clear(); I would prefer if we had some CookieModel and didn't need to rely on view to clear cookies, but it seems not that important. Comment on attachment 195322 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4 Clearing flags on attachment: 195322 Committed r147031: <http://trac.webkit.org/changeset/147031> All reviewed patches have been landed. Closing bug. Comment on attachment 195322 [details] Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4 View in context: https://bugs.webkit.org/attachment.cgi?id=195322&action=review > Source/WebCore/inspector/front-end/CookiesTable.js:146 > + * @param {string=} domain JS Compiler reports: Source/WebCore/inspector/front-end/CookiesTable.js:82: WARNING - actual parameter 1 of WebInspector.CookiesTable.prototype.clear does not match formal parameter found : (null|string) required: (string|undefined) this.clear(domain); ^ (In reply to comment #18) > (From update of attachment 195322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195322&action=review > > > Source/WebCore/inspector/front-end/CookiesTable.js:146 > > + * @param {string=} domain > > JS Compiler reports: > > Source/WebCore/inspector/front-end/CookiesTable.js:82: WARNING - actual parameter 1 of WebInspector.CookiesTable.prototype.clear does not match formal parameter > found : (null|string) > required: (string|undefined) > this.clear(domain); > ^ Oops. I will upload a one-liner patch later today. Is a new bug required for this? Created attachment 195502 [details]
A one-liner JS Compiler warning fix
Comment on attachment 195502 [details] A one-liner JS Compiler warning fix We have a one patch per bug policy in WebKit. This one does not require a bug however, I have landed this fix for you: http://trac.webkit.org/changeset/147096 (In reply to comment #21) > (From update of attachment 195502 [details]) > We have a one patch per bug policy in WebKit. This one does not require a bug however, I have landed this fix for you: http://trac.webkit.org/changeset/147096 Thank you. :) *** Bug 109140 has been marked as a duplicate of this bug. *** |