Bug 87140 - Web Inspector: Add clear cookies context menu item to Resources > Cookies
Summary: Web Inspector: Add clear cookies context menu item to Resources > Cookies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
: 109140 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-05-22 09:56 PDT by Paul Irish
Modified: 2013-04-01 02:41 PDT (History)
13 users (show)

See Also:


Attachments
Plumb cookie clearing throughout the Resources Cookie tree and views (11.52 KB, patch)
2013-03-25 02:08 PDT, PhistucK
no flags Details | Formatted Diff | Diff
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1 (11.43 KB, patch)
2013-03-25 02:55 PDT, PhistucK
no flags Details | Formatted Diff | Diff
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 2 (12.08 KB, patch)
2013-03-26 08:12 PDT, PhistucK
no flags Details | Formatted Diff | Diff
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 3 (12.05 KB, patch)
2013-03-26 10:52 PDT, PhistucK
no flags Details | Formatted Diff | Diff
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4 (12.00 KB, patch)
2013-03-27 07:44 PDT, PhistucK
no flags Details | Formatted Diff | Diff
A one-liner JS Compiler warning fix (1.06 KB, patch)
2013-03-28 02:01 PDT, PhistucK
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Irish 2012-05-22 09:56:11 PDT
In network tab, you can right click, and "Clear browser cookies"

1) Is this for all cookies in the browser or only the particular domain you're viewing? Wording would indicate the former, though that's much less desirable behavior than the latter.

2) If it is the latter, can we add "Clear domain cookies" to the context menu of Resources > Cookies?

Thanks
Comment 1 PhistucK 2013-03-25 02:07:54 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.
Comment 2 PhistucK 2013-03-25 02:08:44 PDT
Created attachment 194804 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views
Comment 3 Eugene Klyuchnikov 2013-03-25 02:42:16 PDT
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 4 PhistucK 2013-03-25 02:52:40 PDT
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.
Comment 5 PhistucK 2013-03-25 02:55:37 PDT
Created attachment 194814 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 1
Comment 6 Eugene Klyuchnikov 2013-03-25 03:10:24 PDT
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 7 Andrey Kosyakov 2013-03-26 04:56:41 PDT
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 8 PhistucK 2013-03-26 07:16:38 PDT
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.
Comment 9 PhistucK 2013-03-26 08:12:24 PDT
Created attachment 195091 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 2
Comment 10 Andrey Kosyakov 2013-03-26 10:26:13 PDT
(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 :)
Comment 11 PhistucK 2013-03-26 10:52:22 PDT
Created attachment 195118 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 3
Comment 12 Andrey Kosyakov 2013-03-27 07:18:37 PDT
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 13 PhistucK 2013-03-27 07:37:23 PDT
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!
Comment 14 PhistucK 2013-03-27 07:44:15 PDT
Created attachment 195322 [details]
Plumb cookie clearing throughout the Resources Cookie tree and views - post review 4
Comment 15 Vsevolod Vlasov 2013-03-27 07:51:55 PDT
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 16 WebKit Review Bot 2013-03-27 17:14:21 PDT
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>
Comment 17 WebKit Review Bot 2013-03-27 17:14:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Eugene Klyuchnikov 2013-03-27 23:33:10 PDT
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);
                   ^
Comment 19 PhistucK 2013-03-28 01:45:38 PDT
(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?
Comment 20 PhistucK 2013-03-28 02:01:08 PDT
Created attachment 195502 [details]
A one-liner JS Compiler warning fix
Comment 21 Vsevolod Vlasov 2013-03-28 05:12:35 PDT
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
Comment 22 PhistucK 2013-03-28 05:15:07 PDT
(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. :)
Comment 23 Eugene Klyuchnikov 2013-04-01 02:41:26 PDT
*** Bug 109140 has been marked as a duplicate of this bug. ***