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)
DevTools
Storage Inspector
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Reporter | ||
Comment 2•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → jsnajdr
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8730407 -
Attachment is obsolete: true
Reporter | ||
Comment 4•8 years ago
|
||
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.
Reporter | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8733894 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53eb2195e30
Reporter | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
(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 :(
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a900c10a5b44
Reporter | ||
Updated•8 years ago
|
Attachment #8735862 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
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
Comment 14•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/fx-team/rev/66f61a656571 https://hg.mozilla.org/integration/fx-team/rev/27104628616c
Keywords: checkin-needed
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/4f09dd6aa299 https://hg.mozilla.org/integration/fx-team/rev/b5300f1eae93
Assignee | ||
Comment 17•8 years ago
|
||
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)
Reporter | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
(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)
Assignee | ||
Comment 21•8 years ago
|
||
Fixed a merge conflict in browser.ini.
Assignee | ||
Updated•8 years ago
|
Attachment #8735764 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/64f6fc285131 https://hg.mozilla.org/integration/fx-team/rev/64550b75f51b
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jsnajdr)
Comment 23•8 years ago
|
||
backed out again for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8361303&repo=fx-team
Comment 24•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/68be59c75e1e https://hg.mozilla.org/integration/fx-team/rev/f82243c637f0
Assignee | ||
Comment 25•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71d7641a6018
Assignee | ||
Comment 28•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8735862 -
Attachment is obsolete: true
Attachment #8737161 -
Attachment is obsolete: true
Attachment #8737218 -
Attachment is obsolete: true
Attachment #8737218 -
Flags: feedback?(mratcliffe)
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/43360777775c
Keywords: checkin-needed
Comment 31•8 years ago
|
||
backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=8408141&repo=fx-team
Flags: needinfo?(jsnajdr)
Comment 32•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/f03e513337dc
Assignee | ||
Comment 33•8 years ago
|
||
Fixed test failure - use Unicode double quotes instead of the ASCII ones.
Assignee | ||
Updated•8 years ago
|
Attachment #8737729 -
Attachment is obsolete: true
Assignee | ||
Comment 34•8 years ago
|
||
Fixed the double-quotes, the failing test now passes locally, requesting checkin (for the last time hopefully)
Flags: needinfo?(jsnajdr)
Keywords: checkin-needed
Comment 35•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/62525d618060
Keywords: checkin-needed
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/62525d618060
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 42•8 years ago
|
||
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)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•