1031192 - [meta] Edit/Delete/Add support for Storage Inspector

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, enhancement, P2)

Description

[Girish Sharma [:Optimizer]]

This requires:

- Protocol changes to the storage actor.
- Front end changes to allow editing.

Editing in the front end will be possible using the variables view where we allow editing the values, or plain text editing in a separate sidebar tab.

For adding a new entry, a blank variables view can be presented in case of cookies. or we can just give a simple text box to begin with.

Comment 1

[Girish Sharma [:Optimizer]]

mass component move . filter on #MassComponentMoveStorageInspector

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: WIP

This patch contains the UI part of the bug and functional delete action.

This also clubs into bug 1031194 as it adds the context menu. (I added context menu because without a toolbar, context menu is the only way to trigger edit actions)

Next up is delete all and then edit and add.

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: WIP 2

Updated patch.

 - Edit, Delete, Delete All works for Cookies. Still looking why Add does not
 - Add, Edit, Delete, Delete All  works for Local/Session storage
 - Things are still flaky wrt IDB.

In this patch, the edit/add form once visible, stays there until the user presses cancel or switches to another item in the tree. So if you have 5 local storage rows, and you open the edit form, then even if you select any other row, the variables view sidebar won't appear.

Is this UX behavior correct ? Any suggestions otherwise ?

Comment 4

[Girish Sharma [:Optimizer]]

Oh, builds at : https://tbpl.mozilla.org/?tree=Try&rev=673ee507a6bb

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: WIP 3

- Add for cookies is working now
 - Add/Edit for indexedDB items is working. Although, you will have to type in a JSON.parse-able stringified JSON value in the value text box.

trying to figure out why delete/delete all are not working for indexedDB, while the code seems to be correct.

New builds at : https://tbpl.mozilla.org/?tree=Try&rev=b701291c010f

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8481970 [details] [diff] [review]
WIP 3

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

The UI for this is nice, but I would increase the size of the headings (Edit, Add) a little bit (kinda like the Edit and Resend header in the Netmonitor).

::: browser/devtools/storage/storage.xul
@@ +48,5 @@
> +                   value="&storage.form.name;"/>
> +            <textbox flex="1"
> +                     add-only="true"
> +                     id="storage-form-name"
> +                     class="tabpanel-summary-input"/>

I think there's already a class which styles inputs (.devtools-textinput).

::: browser/devtools/storage/ui.js
@@ +528,5 @@
> +    }
> +    else if (action == "storage-edit") {
> +      this.showEditForm(type, host, this.table.selectedRow);
> +    }
> +    else if (action == "storage-add") {

Why not a switch statement instead of this bunch of else ifs ?

::: browser/themes/shared/devtools/storage.css
@@ +36,5 @@
> +  background: #F7F7F7; /* Background-Sidebar */
> +}
> +
> +.theme-dark #storage-form {
> +  background-color: #343c45; /* Toolbars */

Don't see why this isn't following the sidebar background color. It'd be nice if you could switch to it, to ease up Brian's work on bug 947242.

Comment 10

[Florent Fayolle]

Firebug.next report:
https://github.com/firebug/firebug.next/issues/373

Any news for this enhancement? It was really appreciated in Firebug 2.0 and it would be great to have it in Firebug 3.0.

Florent

Comment 11

[Florent Fayolle]

:Optimizer Are you planning to resume your work on this?

Florent

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Note that the using JSON to serialize indexedDB values is by definition lossy (as experienced in bug 1159545) since IndexedDB values have the full fidelity of the structured clone algorithm, so it might be advisable to remove IndexedDB support from this patch until:
- Structured clone is used for transit for IndexedDB values (bug 1159545 or whatever it gets duped to or ends up depending on)
- Blobs are dealt with (bug 1054028 touches on this).  This probably needs to be handled by creating a devtools managed handle system that holds onto the Blob reference from the object and gives it some unique identifier.  Then when the object is re-constituted in the actual process, the Blob reference can be reconstituted.

Comment 13

[Tim Nguyen :ntim]

Attachment: Rebased

Rebased the patch and made it work again.

Comment 17

[Tim Nguyen :ntim]

Attachment: Part 1 : Add support for row context menus in TableWidget

Split off the patch in smaller parts. This is all :Optimizer's work, which is why I kept his name.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@ntim: Can you let us know the status of this bug... I mean, does the patch work?

Comment 19

[Tim Nguyen :ntim]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> @ntim: Can you let us know the status of this bug... I mean, does the patch
> work?
I haven't finished splitting up the patch, and the only one I have is the one I've uploaded here, which I haven't tested (requires the rest of the patch).

The very old "Rebased Patch" is obsolete with the e10s work that has been done.

Comment 20

[Sebastian Zartner [:sebo]]

Tim, Mike, would it make sense splitting this bug into smaller ones, so each storage feature can be tackled individually?

Sebastian

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Sebastian Zartner [:sebo] from comment #20)
> Tim, Mike, would it make sense splitting this bug into smaller ones, so each
> storage feature can be tackled individually?
> 
> Sebastian

We can do that. We need a way to tell the table widget what is possible e.g. for indexedDB we only get a snapshot of the database and have no events for changes (they are coming) but obviously editing there is out of the question. For cookies though it should be easy.

I won't have time in the next week to work on this so if you want to make a start you are more than welcome.

Comment 22

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #20)
> Tim, Mike, would it make sense splitting this bug into smaller ones, so each
> storage feature can be tackled individually?
> 
> Sebastian

I'm not too fond of doing this, because we may end up shipping code with comments like "unimplemented", whereas here, we could have had written multiple patches then ship them when everything is done.

But since you've already filed bugs, that should be fine too. I've changed your bugs to backend bugs, simply because it's more convenient to add an UI when everything is done instead of handling uimplemented functionality in the UI. Also, I've merged edit/add with delete bugs since they share the same underlying work. IndexedDB is a different story so the separation should be fine.

Comment 23

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen [:ntim] from comment #22)
> (In reply to Sebastian Zartner [:sebo] from comment #20)
> > Tim, Mike, would it make sense splitting this bug into smaller ones, so each
> > storage feature can be tackled individually?
> > 
> > Sebastian
> 
> I'm not too fond of doing this, because we may end up shipping code with
> comments like "unimplemented", whereas here, we could have had written
> multiple patches then ship them when everything is done.

It allows to ship parts individually instead of having to wait until everything is done. I assume the code paths would be mostly independent, so there shouldn't be a high risk of "unimplemented" comments.

> But since you've already filed bugs, that should be fine too. I've changed
> your bugs to backend bugs, simply because it's more convenient to add an UI
> when everything is done instead of handling uimplemented functionality in
> the UI.

Fine for me. I guess this still requires some more UI or backend bugs to be filed and a few bug dependency changes. Though I need to get some sleep now.

> Also, I've merged edit/add with delete bugs since they share the
> same underlying work.

Might be the same underlying work, but different UI. But anyway, feel free to sort the bugs for your convenience.

Sebastian

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Tim Nguyen [:ntim] from comment #22)
> (In reply to Sebastian Zartner [:sebo] from comment #20)
> > Tim, Mike, would it make sense splitting this bug into smaller ones, so each
> > storage feature can be tackled individually?
> > 
> > Sebastian
> 
> I'm not too fond of doing this, because we may end up shipping code with
> comments like "unimplemented", whereas here, we could have had written
> multiple patches then ship them when everything is done.
> 
> But since you've already filed bugs, that should be fine too. I've changed
> your bugs to backend bugs, simply because it's more convenient to add an UI
> when everything is done instead of handling uimplemented functionality in
> the UI. Also, I've merged edit/add with delete bugs since they share the
> same underlying work. IndexedDB is a different story so the separation
> should be fine.

@ntim: Forget about us splitting this up... can you tell me the status of this bug e.g. what works and doesn't work. We are planning on landing this very soon so we need to get things moving and I am happy to base my changes on top of your code.

Comment 25

[Tim Nguyen :ntim]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> @ntim: Forget about us splitting this up... can you tell me the status of
> this bug e.g. what works and doesn't work. We are planning on landing this
> very soon so we need to get things moving and I am happy to base my changes
> on top of your code.

The only patch I had finished rebasing was the TableWidget patch (which I've posted). I had some incomplete rebases of the rest of the patch, but never got the time to finish them, and now they're all outdated with the file move.

Comment 29

[Erik Krause]

Will the ability to add data in Storage inspector be implemented?

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

All relevant dependencies are fixed, closing.