Closed Bug 1231434 Opened 4 years ago Closed 4 years ago

[BACKEND] Add "Remove all cookies" context menu entry to storage inspector

Categories

(DevTools :: Storage Inspector, defect)

defect
Not set

Tracking

(firefox48 verified)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: miker, Assigned: jsnajdr)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Assignee: nobody → jsnajdr
This patch adds a "Delete All" context menu item to storage inspector. Works for cookies, localStorage and sessionStorage.

Few changes that might need some explanation:
- I improved the TableWidget.remove method so that it doesn't throw a TypeError when the item removed doesn't exist in the table.
- I improved the selectTreeItem function in test/head.js so that it can be used to select a tree item that is already selected. Previously, it would wait forever for a "store-objects-updated" event that never comes. Tests had to be careful to not try to select the wrong thing.
- Fixed the StorageActor.update method to correctly handle deletes: batching several deletes together (in boundUpdate) didn't work previously - it sent only the last one. Also, if the "delete" action is triggered multiple times, it's now sent to the client only once. Sometimes, the nsCookieService sends the cookie-changed:deleted event twice, so this can easily happen.

There's one issue I'd like to discuss: what does it mean to "Delete All" in cookies? If there are domain cookies (e.g., ".example.com") they appear in both "a.example.com" and "b.example.com" lists. The "Delete All" action can then have two meanings:

1. Delete all cookies from the list: in case of "a.example.com", delete all from "a.example.com" and ".example.com" hosts. This will also change the "b.example.com" list. This is what my patch does. Also, Chrome does this when clicking on "Clear" action in the tree's context menu or on "Clear All" in the table.

2. The context menu shows "Delete from a.example.com" or "Delete from .example.com", depending on what is the host of the cookie I clicked on in the table. Then, only cookies with exactly that host will be deleted. This is what Chrome does on the "Delete from ..." action.

I'd also like to add a "Delete All" context menu to the tree widget before landing this.
Attachment #8738157 - Flags: feedback?(mratcliffe)
Attachment #8738157 - Flags: feedback?(hholmes)
(In reply to Jarda Snajdr [:jsnajdr] from comment #1)
> Created attachment 8738157 [details] [diff] [review]
> Add 'Delete All' context menu entry to storage inspector
> 
> This patch adds a "Delete All" context menu item to storage inspector. Works
> for cookies, localStorage and sessionStorage.
> 
> Few changes that might need some explanation:
> - I improved the TableWidget.remove method so that it doesn't throw a
> TypeError when the item removed doesn't exist in the table.
> - I improved the selectTreeItem function in test/head.js so that it can be
> used to select a tree item that is already selected. Previously, it would
> wait forever for a "store-objects-updated" event that never comes. Tests had
> to be careful to not try to select the wrong thing.
> - Fixed the StorageActor.update method to correctly handle deletes: batching
> several deletes together (in boundUpdate) didn't work previously - it sent
> only the last one. Also, if the "delete" action is triggered multiple times,
> it's now sent to the client only once. Sometimes, the nsCookieService sends
> the cookie-changed:deleted event twice, so this can easily happen.
> 
> There's one issue I'd like to discuss: what does it mean to "Delete All" in
> cookies? If there are domain cookies (e.g., ".example.com") they appear in
> both "a.example.com" and "b.example.com" lists. The "Delete All" action can
> then have two meanings:
> 
> 1. Delete all cookies from the list: in case of "a.example.com", delete all
> from "a.example.com" and ".example.com" hosts. This will also change the
> "b.example.com" list. This is what my patch does. Also, Chrome does this
> when clicking on "Clear" action in the tree's context menu or on "Clear All"
> in the table.
> 
> 2. The context menu shows "Delete from a.example.com" or "Delete from
> .example.com", depending on what is the host of the cookie I clicked on in
> the table. Then, only cookies with exactly that host will be deleted. This
> is what Chrome does on the "Delete from ..." action.
> 

Option 2: I think that some users would expect just a.example.com cookies to be deleted and some would expect a.example.com and .example.com to be deleted. There should be no chance of confusion so we should go for option 2.

> I'd also like to add a "Delete All" context menu to the tree widget before
> landing this.

That makes sense. In the cookie tree we may be best to offer the hostnames as in option 2 above.
Comment on attachment 8738157 [details] [diff] [review]
Add 'Delete All' context menu entry to storage inspector

Review of attachment 8738157 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I still think we should go with option 2 (see comment 3)
Attachment #8738157 - Flags: feedback?(mratcliffe) → feedback+
Comment on attachment 8738157 [details] [diff] [review]
Add 'Delete All' context menu entry to storage inspector

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> Comment on attachment 8738157 [details] [diff] [review]
> Add 'Delete All' context menu entry to storage inspector
> 
> Review of attachment 8738157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but I still think we should go with option 2 (see comment 3)

I'm going to defer to Mike's expertise on this. I'm not entirely sure which case is more expected with users.
Attachment #8738157 - Flags: feedback?(hholmes)
New version of the patch with this UI behavior:
- for cookies, include a new action 'Delete All From ".example.org"' that removes only cookies with exact host match
- table for cookies and (local|session)Storage has "Delete All" action that deletes everything in the table
- the tree now has a context menu with "Delete All" action that does the same thing as in table
- overall, this behavior is very similar to Chrome

To get here, I had to do quite a lot of related changes:

TreeWidget.js:
- added support for context menu
- the right-click that displays the context menu also selects the item...
- ...and that's why I changed the event that causes the selection from "click" to "mousedown"
- refactored selecting the item so that all logic is performed in the "set selectedItem" setter and the click and keypress handlers just call the setter. Previously, they also did a lot of the internal work (setting _selectedLabel, emitting the "select" event...). Very useful in tests, see below.
- as a very nice side-effect, this improves the behavior of how the items are expanded and collapsed. Now, it will never happen again that you click on an expanded item to select it and it collapses. Selecting an item will always get it expanded. Only the second click on already selected item will collapse it.

ui.js:
- because setting the tree.selectedItem now does everything it should, namely firing the "select" event, we don't need to call fetchStorageObjects explicitly in populateStorageTree - it's performed by the onHostSelect handler.
- I removed the shouldResetColumns flag, because it's synonymous with REASON.POPULATE

mochitests:
When I started using the "mousedown" event to select tree items, I then struggled with adapting the tests - we need to synthesize the "mousedown" event instead of "click" in selectTreeItem. But for some reason, I couldn't make it work. So I decided to abandon synthesizing clicks, but instead start calling the "set selectedItem" method of the tree. Much simpler, and also it makes the tests run much faster! That's because the click() function always waits 200ms before returning.
Attachment #8738909 - Flags: ui-review?(hholmes)
Attachment #8738909 - Flags: review?(mratcliffe)
Attachment #8738157 - Attachment is obsolete: true
The try run fails in the browser_treeWidget_mouse_interaction.js test (because of the click->mousedown change). I'll update the patch.
Fixed the failing browser_treeWidget_mouse_interaction.js test. Turned out to be caused by incorrect attachment param when emitting the TreeWidget's select event (array of IDs vs. its stringified version).
Attachment #8738960 - Flags: ui-review?(hholmes)
Attachment #8738960 - Flags: review?(mratcliffe)
Attachment #8738909 - Attachment is obsolete: true
Attachment #8738909 - Flags: ui-review?(hholmes)
Attachment #8738909 - Flags: review?(mratcliffe)
Comment on attachment 8738960 [details] [diff] [review]
Add 'Delete All' context menu entry to storage inspector

Review of attachment 8738960 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jarda Snajdr [:jsnajdr] from comment #9)
> Created attachment 8738960 [details] [diff] [review]
> Add 'Delete All' context menu entry to storage inspector
> 
> Fixed the failing browser_treeWidget_mouse_interaction.js test. Turned out
> to be caused by incorrect attachment param when emitting the TreeWidget's
> select event (array of IDs vs. its stringified version).

Yes... we need to make these params more consistent, rename vars to make things easier to understand and generally tidy up. I am going to be converting the TableWidget to HTML soon, maybe using React or maybe not, it depends on the state of our React tables but that will be a good time to convert things.

This patch looks spot on... fantastic work!

r+ assuming green try.
Attachment #8738960 - Flags: review?(mratcliffe) → review+
There are several failures in the try run. Always in non-e10s mode, timeout in browser_storage_delete_tree.js, and then some other failures that are probably caused by the first test not cleaning up correctly. Going to investigate and fix.
Fixed the failing tests with these two changes:
- when performing delete through a context-menu action, start waiting for the 'store-objects-updated' event before clicking in the context menu. Otherwise, when the right race condition occurs, the event can be already fired before the listener is attached.
- in waitForContextMenu, scroll the element into view before triggering the contextmenu event on it

Try run with these changes succeeded beautifully, except some unrelated already known intermittents:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30952d4994ba
Attachment #8739652 - Flags: review+
Attachment #8738960 - Attachment is obsolete: true
Attachment #8738960 - Flags: ui-review?(hholmes)
Keywords: checkin-needed
Backed out for eslint errors: https://hg.mozilla.org/integration/fx-team/rev/9332103718ec

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8562718&repo=fx-team

TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:41:18 | Missing space after *. (generator-star-spacing)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:44:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:50:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:54:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:61:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_cookies_delete_all.js:65:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_delete_all.js:11:18 | Missing space after *. (generator-star-spacing)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_delete_all.js:18:8 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/storage/test/browser_storage_delete_all.js:36:8 | Strings must use doublequote. (quotes)

Seems like eslint doesn't like static template strings. Fix would be easy, no idea if anybody is interested in allowing template strings without variables.
Flags: needinfo?(jsnajdr)
This is caused because ESLint changes in bug 1257246 landed Friday late afternoon and they report more errors now. I've never seen them when developing or running try runs.

The template strings make sense and will be easy to fix, but I don't understand how the generator-star-spacing works here: why is function*() wrong, and function* () and function *() are good? Is the rule configured correctly? .eslintrc says:

"generator-star-spacing": [2, "after"],

but the documentation at http://eslint.org/docs/rules/generator-star-spacing seems to describe a very different syntax.

ni? for Patrick, please forward if someone else knows better.
Flags: needinfo?(jsnajdr) → needinfo?(pbrosset)
Fixed eslint errors (template strings and generator function spacing)
Attachment #8739873 - Flags: review+
Attachment #8739652 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Jarda Snajdr [:jsnajdr] from comment #17)
> This is caused because ESLint changes in bug 1257246 landed Friday late
> afternoon and they report more errors now. I've never seen them when
> developing or running try runs.
Right, bad timing ... sorry this ended up in a back-out for you.
> The template strings make sense and will be easy to fix, but I don't
> understand how the generator-star-spacing works here: why is function*()
> wrong, and function* () and function *() are good? Is the rule configured
> correctly? .eslintrc says:
> 
> "generator-star-spacing": [2, "after"],
See bug 1263258 comment 2 for more discussions about this.
Right now, only function* () should be accepted by our eslint config. function*() and function *() should produce errors.
> but the documentation at http://eslint.org/docs/rules/generator-star-spacing
> seems to describe a very different syntax.
Are you sure? "after" is a valid keyword in this documentation. It's one of the shorthands you can use. Also 2 is an alias for "error". eslint used to only allow 0/1/2, but recently changed to allow "warning" and "error".
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbro] from comment #20)
> Are you sure? "after" is a valid keyword in this documentation. It's one of
> the shorthands you can use. Also 2 is an alias for "error". eslint used to
> only allow 0/1/2, but recently changed to allow "warning" and "error".

You're right, I didn't realize that 2 is an alias for "error" and that shorthands are possible in the config.
https://hg.mozilla.org/mozilla-central/rev/8ff3c4cb70c3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Duplicate of this bug: 1231438
Duplicate of this bug: 1190648
Duplicate of this bug: 1231435
Duplicate of this bug: 1231440
Duplicate of this bug: 1231444
The Bug was about to Add 'Delete All' context menu entry to storage inspector. I have seen the implementation on Latest Beta 48.0b7 on Windows 10, 64 Bit.

This Bug is now verified as fixed on Latest Firefox Beta 48.0b6 

Build ID: 20160711002726
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [bugday-20160713]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.