devtools/server/actors/storage.js * Add getEditableFields() to the cookie actor. The presence of this method indicates that we are making some fields editable. The method simply returns an array of editable fields. * We were previously showing expired cookies, which are not erased until their host and path are hit e.g. by loading a page that uses the expired cookie. We no longer display these expired cookies as they are not valid as far as the system is concerned. * cellEditInParent takes the data in the format listed in the comments and does the actual editing of the cookie. That is it... very simple. The frontend is already handled so I guess I can close all of the storage inspector bugs marked [FRONTEND]. Review commit: https://reviewboard.mozilla.org/r/35253/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35253/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4abbeca1d291

Comment on attachment 8720276 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35253/diff/1-2/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e135a82afc67

At the moment the following fields are visible: - name - path - host - expires - created - lastAccessed - value - isDomain - isSecure - isHttpOnly Services.cookies.add() only allows the following fields to be changed: - expires - value - isDomain - isSecure - isHttpOnly Which means we have all these read-only fields: - name - path - host - created - lastAccessed Services.cookies.add() does not support editing the name, path or host, or at least not without some pretty serious bugs (these fields are used to uniquely identify the cookie). Removing the cookie before adding a new one works well and allows more fields to be edited but results in rows appearing and disappearing in the storage inspector. This is especially true when editing a value on the last row as the row is removed when the cookie is deleted and added when the cookie is re-added resulting in moving one line up. For the sake of usability though I am looking into making the table editor handle this situation better. This should leave the following fields editable: - name - path - host - expires - value - isDomain - isSecure - isHttpOnly

Review commit: https://reviewboard.mozilla.org/r/36029/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36029/

Test still to come,

Comment on attachment 8722435 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r?pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36029/diff/1-2/

Comment on attachment 8722435 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r?pbrosset https://reviewboard.mozilla.org/r/36029/#review32907 I've applied this patch (and the cookie backend one) locally and played with the storage inspector. Here are some things I found out which we may want to fix now before landing or later: - When I enter the edit mode in a cell, then the alternate background in the same column changes. See https://dl.dropboxusercontent.com/u/714210/storage-edit/alternate-background.gif - If the "Expires on" column contains a shorter string (like "session"), then switching this cell to edit mode adds an offset to the table and there's a gap to the right of the input that prevents it from being as long as the column itself. See https://dl.dropboxusercontent.com/u/714210/storage-edit/column-offset.gif . Also, I tried in the "storage" table, and saw this, which looks related: https://dl.dropboxusercontent.com/u/714210/storage-edit/columns-move-in-storage.gif - If I do edit a value, and tab out of the cell, it takes noticeably more time for the tab to be handled than when I don't change the value. When I don't change the value, tabbing through cells is instant. If I do change a value, tabbing works but there seems to be a refresh of the table, or something, that feels slow. Not sure how much can be done here to avoid this, but I do think that's an issue we'll want to address at some stage. Perhaps the table shouldn't get refreshed fully, which is what seems to be happening. - When I edit a value and tab out of the field, the whole column flashes orange, which is fine, but then, as I tab through next cells (without changing anything), then those cells flash too. See https://dl.dropboxusercontent.com/u/714210/storage-edit/cell-flash-after-edit.gif - I feel like "selected" should be a thing, like clicking (just once) on a cell should select it, and the selection should be visual (an outline should be drawn, see bug 1242851 btw and you might want to check with :yzen), and I feel like pressing enter on a selected cell should also trigger the edit mode. This could be a follow-up. - When entering the edit mode, I think the value should be selected by default. This is what we do in other parts of devtools and it's a useful pattern that people have muscle memory for I think. - There's a bug where the inplace-editor doesn't update its position: double-click on a cell, then click on a headers to sort the table differently: when the row that contained the edited cell moves to another position, the editor doesn't follow it. See https://dl.dropboxusercontent.com/u/714210/storage-edit/editor-should-follow-row.gif Please let me know how you'd want to proceed with this? I think some of these need to be solved now before landing, while others can be done in follow-up bugs. I think the cell flashing and editor position bugs should be fixed now. I've had a quick look at the code changes, they mostly look good. Please re-request review for your next commit, I'll spend a bit more time on the changes in TableWidget.js (the rest looks pretty ok to me). ::: devtools/client/storage/test/browser_storage_edit_cookies.js:83 (Diff revision 2) > + // TODO: Test editing cookies Is this coming in a separate patch?

I have / will fix most of those issues although some should be follow up bugs. Alternating row background... I suspect there may be a simple CSS fix for this :not([hidden]). I didn't notice the textbox not filling the column. The delay when changing values is because we need to wait for the row to update. There is no easy fix but maybe we could create some kind of expectation system that ignores remote changes if certain criteria are matched. We would need to type columns and perform validation to make that work properly though. We can hopefully make it feel faster though. I like the idea of single-click adding a focus ring to a field and the enter key editing. We shouldn't flash when no changes were made but we need to flash when fields are changed just in case the row has moved due to sorting. I have already made the change to select text by default when entering edit mode. The tests will be in the same patch but the changes are only in the test files so that shouldn't cause a problem.

Please bare in mind that 100% of these changes are to the backend code so they will be fixed in the patch in bug 1235350. I will my comments in that bug.

Review commit: https://reviewboard.mozilla.org/r/37501/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37501/

Looking at try results from the parent changeset: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=91eedd43f7cf There seems to be an issue with Windows pushes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=862bebe37327 I am going to have to build a Windows box to look into this.

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset https://reviewboard.mozilla.org/r/37501/#review34391 ::: devtools/client/storage/test/browser_storage_edit_cookies.js:67 (Diff revision 1) > + yield testCookieNames(); > + yield testEditCookies(); > + yield testEditCookiesWithKeyboard(); > + yield testTab(); Looks like a very easy test to split into 4 tests! Multiple short tests are better: easier to read, and less chances of timing out for running too long. ::: devtools/client/storage/test/browser_storage_edit_cookies.js:75 (Diff revision 1) > +function* testCookieNames() { Does this test (which is about editing cookies) really need to check that the table contains the right data? It seems like this should be done in other tests. ::: devtools/client/storage/test/head.js:612 (Diff revision 1) > + let cells = getColCells(id); > + let values = []; > + > + for (let {value} of cells) { > + values.push(value); > + } > + > + return values; Or simply: return getColCells(id).map(({value}) => value); ::: devtools/client/storage/test/head.js:668 (Diff revision 1) > + * Edit a cell value. Maybe also say that the cell is assumed to be in edit mode already (see startCellEdit) ::: devtools/client/storage/test/head.js:801 (Diff revision 1) > + ok(textbox, "Captain, we have the textbox"); Not a fan of adding an assertion in here, here's why: - adding it in the test that calls `getCurrentEditorValue` shows the intent of the test better, - the message is very generic, so not really helping when reading through logs when tests fail, - at some point in the future we might want to call `getCurrentEditorValue` knowing that a textbox doesn't exist yet/anymore. ::: devtools/server/actors/storage.js:684 (Diff revision 1) > + this.cellEditInParent = cookieHelpers.cellEditInParent; Hmm, why the word `cell` here? We don't have the concept of a table at this level. The table/column/row/cell idea is just something we use on the client-side to represent the data, but on the server, we deal with cookies, and storage, etc... So I don't think we should call this `cellEdit`. If the idea was to make something generic that would apply to all storage types, then at least something like `editItem` would be better. But seeing that we already have `getCookies`, `addCookie`, `removeCookie`, it would be more consistent to also have `editCookie`. ::: devtools/server/actors/storage.js:756 (Diff revision 1) > + * column: "value", > + * keyColumn: "name", > + * oldValue: "%7BHello%7D", > + * newValue: "%7BHelloo%7D", > + * row: { Same comment about the words `column` and `row` used here and below. I don't think we should use the table vocabulary here at actor level.

https://reviewboard.mozilla.org/r/37501/#review34391 > Looks like a very easy test to split into 4 tests! > Multiple short tests are better: easier to read, and less chances of timing out for running too long. More than happy to do that... split into 3 because we don't need to check that the table is properly populated as we do that in another test. > Does this test (which is about editing cookies) really need to check that the table contains the right data? It seems like this should be done in other tests. You are right... this is done in devtools/client/shared/test/browser_tableWidget_basic.js > Same comment about the words `column` and `row` used here and below. I don't think we should use the table vocabulary here at actor level. We now use field, key, items etc.

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37501/diff/1-2/

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37501/diff/2-3/

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37501/diff/3-4/

The test failures were due to the tests not properly cleaning up after themselves. We never noticed before this point due to dumb luck. I have made the tests better but in the future they still need a good tidy.

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37501/diff/4-5/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83113494ebec&group_state=expanded

Comment on attachment 8725475 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37501/diff/5-6/

Review commit: https://reviewboard.mozilla.org/r/41735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41735/

Comment on attachment 8733347 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41735/diff/1-2/

Comment on attachment 8733347 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41735/diff/2-3/

Comment on attachment 8733347 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro Already reviewed

https://reviewboard.mozilla.org/r/41735/#review38125 pbro has already reviewed this

Comment on attachment 8733347 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41735/diff/3-4/

https://hg.mozilla.org/integration/fx-team/rev/1f40e55de92a0e9398a4cde225b1bc27be51ef85 Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro

Comment on attachment 8733347 [details] MozReview Request: Bug 1231154 - Make cookies table fields editable via double-click in storage inspector r+pbro R+'d this already a while back, and this has landed. Clearing the R? flag.