Closed Bug 1231437 Opened 8 years ago Closed 8 years ago

[FRONTEND] Add "Remove localstorage item" context menu entry to storage inspector

Categories

(DevTools :: Storage Inspector, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: miker, Assigned: jsnajdr)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 7 obsolete files)

      No description provided.
This is a quick and dirty proof-of-concept patch that adds a "Delete This Item" context menu to Storage Inspector. Works only for localStorage and sessionStorage. I'll work on it further to clean it up (especially, I want to add proper support for context menu in TableWidget) and make it work for all storage types.

Also, it'll need to be reconciled with work already done in bug 1231154 and others.

Mike, please tell me if I'm on the right track and also how to coordinate this with your work.
Attachment #8730407 - Flags: feedback?(mratcliffe)
Comment on attachment 8730407 [details] [diff] [review]
Storage Inspector: context menu to remove localStorage item

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

Looks great and will happily live with my editable fields patch with minimal tweaks.

It would be awesome if you could add a test.

::: devtools/client/shared/widgets/TableWidget.js
@@ +1031,5 @@
>  
> +  if (column.table.cellContextMenuId) {
> +    this.label.setAttribute("context", column.table.cellContextMenuId);
> +    this.label.addEventListener("contextmenu", (event) => {
> +      let target = event.originalTarget;

This should work:

this.label.addEventListener("contextmenu", (event) => {
  let target = event.originalTarget.closest("[data-id]");

  column.table.contextMenuRowId = target.getAttribute("data-id");
}, false);

::: devtools/client/storage/ui.js
@@ +680,5 @@
> +    let type = this.table.datatype;
> +    let actor = this.storageTypes[type];
> +    let rowId = this.table.contextMenuRowId;
> +
> +    console.log("Removing key in " + type + ":", rowId);

You probably want to remove the console.log.
Attachment #8730407 - Flags: feedback?(mratcliffe) → feedback+
Assignee: nobody → jsnajdr
This patch adds support for removing items from Cookie, Local Storage and Session Storage stores. On right click on any item, a context menu with "Delete" action appears. I didn't implement other storage types like IndexedDB or Cache yet, because these have a multi-level structure and will be slightly more complex. Better to proceed in small increment and leave them for a follow-up bug.

Changes I did:

In the TableWidget.js:
- the constructor has a new cellContextMenuId option, with an ID of popup menu to attach to every cell
- after a right-click on a row, set the table's contextMenuRowId property - this is the ID of the clicked row, similar to the selectedRow property. Displaying the context menu is independent from selecting a row: this avoids unwanted side-effects like opening the sidebar when one wants to just display the context menu.

In storage.xul/.dtd:
- added the popupmenu element, plus label for localization

In storage/ui.js:
- add the context menu option to the created TableWidget
- before showing the context menu, check if the storage actor has the removeItem method - if it doesn't, don't show the menu
- handle the command click by calling the actor's removeItem method.

In backend:
- added removeItem method to the cookie actor. It calls a new 'removeCookie' method in turn. Its implementation is inspired by existing cookie-deleting code in GCLI. By the way, the GCLI should start using the storage actor, because right now, it seems to not work in e10s - I'm getting a 'CPOW forbidden' error there.
- added removeItem method to the local/sessionStorage actor - straighforward, easier than cookies.

In tests:
- added new browser_storage_delete.js mochitest
- some refactoring: moved waitForContextMenu function from webconsole's head.js into shared-head.js, imported shared-head.js into storage/test/head.js

This patch should be applied to a tree that already has the cookie-edit patches. I'll verify it again once these changes land in mozilla-central and will resolve any conflicts if neccessary.
Attachment #8733894 - Flags: review?(mratcliffe)
Attachment #8730407 - Attachment is obsolete: true
Comment on attachment 8733894 [details] [diff] [review]
Storage Inspector: context menu to remove cookie/storage item

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

The patch is awesome... thanks for working on this.

Please add the following line to devtools/client/storage/test/head.js:8
```
/* import-globals-from ../../framework/test/shared-head.js */
```

This allows eslint to import the globals for validation.

One thing we need to fix before landing this... there is no visual indicator as to what we are deleting. Can you add the cells value and the rows key to the context menu when a cell is clicked... of course, these values should be trimmed to between 10 - 20 chars.
Comment on attachment 8733894 [details] [diff] [review]
Storage Inspector: context menu to remove cookie/storage item

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

The patch is awesome... thanks for working on this.

Please add the following line to devtools/client/storage/test/head.js:8
```
/* import-globals-from ../../framework/test/shared-head.js */
```

This allows eslint to import the globals for validation.

One thing we need to fix before landing this... there is no visual indicator as to what we are deleting. Can you add the cells value and the rows key to the context menu when a cell is clicked... of course, these values should be trimmed to between 10 - 20 chars.
Attachment #8733894 - Flags: review?(mratcliffe)
Fixed issues from review:
- the context menu action has label 'Delete "name"', displaying the (unique) name of the item. Trimmed with ellipsis when too long.
- added import-globals-from to the tests where necessary, to prevent ESLint errors.
Attachment #8735764 - Flags: review?(mratcliffe)
Attachment #8733894 - Attachment is obsolete: true
Comment on attachment 8735764 [details] [diff] [review]
Storage Inspector: context menu to remove cookie/storage item

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

Perfect, great work.
Attachment #8735764 - Flags: review?(mratcliffe) → review+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> Perfect, great work.

Thank you Mike! Unfortunately, the try run is failing in browser_storage_dynamic_updates.js. The gWindow variable is reported as undefined. It seems to be caused by including the shared-head.js. If I revert to the old head.js without the loadSubScript, the test starts working again. Mysterious :(
(In reply to Jarda Snajdr [:jsnajdr] from comment #9)
> Unfortunately, the try run is failing in browser_storage_dynamic_updates.js.

Oops, caused by conflicting definitions of the addTab function.
Fixed the test failures from try run in browser_storage_dynamic_updates.js. These were caused by a different definition of addTab in shared-head.js and storage/test/head.js. I removed the definition in head.js and adapted the code to the global definition (it has a different return value).

Also added import-globals-from head.js to all storage tests - makes them almost completely eslint-clean! Except references to gBrowser global variable.

Committed as a separate commit, to make review easier.
Attachment #8735862 - Flags: review?(mratcliffe)
Attachment #8735862 - Flags: review?(mratcliffe) → review+
Keywords: checkin-needed
Comment on attachment 8735862 [details] [diff] [review]
Storage Inspector: context menu to remove cookie/storage item (part 2: tests)

Renaming so it has a different name than the other patch (so I can check this in with more ease).
Attachment #8735862 - Attachment filename: Bug-1231437---Storage-Inspector-context-menu-to-re.patch → storage-test-fix.patch
sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=8335235&repo=fx-team
Flags: needinfo?(jsnajdr)
Mike, do you have any suggestion what might cause these failures? They seem to be related to the cookie-editing bug 1231154, not this one.

INFO -  132 INFO TEST-UNEXPECTED-FAIL |
  devtools/client/storage/test/browser_storage_cookies_edit.js |
  A promise chain failed to handle a rejection:
    - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1125
    - Error: Connection closed, pending request to server1.conn1.child1/cookies28, type getEditableFields failed

Looks like the connection to the debugger was closed in the middle of the getEditableFields request.
Flags: needinfo?(mratcliffe)
I think that the sheriff has just scanned the checkins for cookies and backed your patches out... I suspect that the issue is really caused by https://hg.mozilla.org/integration/fx-team/rev/4317688957ef27ba4a9c005127f08b1fc07b281a.
Flags: needinfo?(mratcliffe)
As explained in comment 17 and comment 18, the test failures are almost certainly caused by something else than this patch. Requesting checkin again.
Flags: needinfo?(jsnajdr)
Keywords: checkin-needed
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> I think that the sheriff has just scanned the checkins for cookies and
> backed your patches out... I suspect that the issue is really caused by
> https://hg.mozilla.org/integration/fx-team/rev/
> 4317688957ef27ba4a9c005127f08b1fc07b281a.

yeah did check when the test was checkedin and that was this bug. 

Jarda, i tried to re-check this patches and got
renamed 1231437 -> Bug-1231437---Storage-Inspector-context-menu-to-re.patch
applying Bug-1231437---Storage-Inspector-context-menu-to-re.patch
patching file devtools/client/storage/test/browser.ini
Hunk #1 FAILED at 11
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/storage/test/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1231437---Storage-Inspector-context-menu-to-re.patch
can you take a look ? thanks !
Flags: needinfo?(jsnajdr)
Fixed a merge conflict in browser.ini.
Attachment #8735764 - Attachment is obsolete: true
Flags: needinfo?(jsnajdr)
Just importing the shared-head.js breaks the storage tests. Verified on:
./mach mochitest --e10s devtools/client/storage/test/browser_storage_sessionstorage_edit.js
Attachment #8737218 - Flags: feedback?(mratcliffe)
Attachment #8737218 - Flags: feedback?(jryans)
Comment on attachment 8737218 [details] [diff] [review]
minimal change needed to break the storage tests

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

Thanks for the minimal patch!  After trying this locally, it appears the issue is that shared-head.js registers a cleanup function[1] that closes the toolbox, whereas the standalone storage head.js did not do so before.  Closing the toolbox is likely triggers the error to come out here.

I do think it's a good idea for tests to clean up and close the toolbox, so we do want that behavior enabled.  However, solving the test issues that result seem outside the scope of the feature here.

So, I would purpose not using shared-head.js for this bug, and instead file a new one for Storage to make use of shared-head.js, and the resulting errors that come out can be worked on over there.

[1]: https://dxr.mozilla.org/mozilla-central/rev/bccb11375f2af838cda714d42fd8cef78f5c7bf1/devtools/client/framework/test/shared-head.js#83
Attachment #8737218 - Flags: feedback?(jryans)
See Also: → 1261785
New version of the patch that removes the refactoring of tests to use shared-head.js, because that caused test failures and had to be backed out. Moved that to a separate bug 1261785. I submitted a try run and will request checkin after it succeeds.
Attachment #8735862 - Attachment is obsolete: true
Attachment #8737161 - Attachment is obsolete: true
Attachment #8737218 - Attachment is obsolete: true
Attachment #8737218 - Flags: feedback?(mratcliffe)
Try run looks good, requesting checkin.
Keywords: checkin-needed
backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=8408141&repo=fx-team
Flags: needinfo?(jsnajdr)
Fixed test failure - use Unicode double quotes instead of the ASCII ones.
Attachment #8737729 - Attachment is obsolete: true
Fixed the double-quotes, the failing test now passes locally, requesting checkin (for the last time hopefully)
Flags: needinfo?(jsnajdr)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62525d618060
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1268844
No longer depends on: 1268844
Depends on: 1268844
I've added a bit on this in the Storage Inspector page:
https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Editing_storage_items

Let me know if this covers it!
Flags: needinfo?(mratcliffe)
Yup, that covers it perfectly.
Flags: needinfo?(mratcliffe)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.