Bug 23866 – Storage panel should be editable

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Anthony Ricaud]

The new listing is not editable. It would be a nice feature.

[Yael]

Created attachment 27855 [details]
Make DOMStorage view editable.

[Timothy Hatcher]

Comment on attachment 27855 [details]
Make DOMStorage view editable.


> +    get statusBarItems()
> +    {
> +        return [this.deleteButton];
> +    },

The delete button should be on the view side of the status bar, like we do in the ProfilesPanel/ProfilesView. A delete button here would imply deleting a local/session storage object not a row. r- for this

> +            this.visibleView._dataGrid._deleteRow();

Functions that are "public" like _deleteRow() should not have an underscore. Also deleteSelectedRow() would be best, since the row index isn't passed.

> +        var element = event.target.enclosingNodeOrSelfWithNodeName("td");

Why do you need to look for a element? Can't you get everything from dataGridNodeFromEvent?

> +            }
> +            else {

This should be: } else { on the same line.


> +        if (node && domStorage) {
> +            domStorage.removeItem(node.data[0]);
> +        }

No need for braces here.

[Yael]

(In reply to comment #2)
> (From update of attachment 27855 [details] [review])
> 
> > +    get statusBarItems()
> > +    {
> > +        return [this.deleteButton];
> > +    },
> 
> The delete button should be on the view side of the status bar, like we do in
> the ProfilesPanel/ProfilesView. A delete button here would imply deleting a
> local/session storage object not a row. r- for this
> 
> > +            this.visibleView._dataGrid._deleteRow();
> 
> Functions that are "public" like _deleteRow() should not have an underscore.
> Also deleteSelectedRow() would be best, since the row index isn't passed.
> 
> > +        var element = event.target.enclosingNodeOrSelfWithNodeName("td");
> 
> Why do you need to look for a element? Can't you get everything from
> dataGridNodeFromEvent?
> 
> > +            }
> > +            else {
> 
> This should be: } else { on the same line.
> 
> 
> > +        if (node && domStorage) {
> > +            domStorage.removeItem(node.data[0]);
> > +        }
> 
> No need for braces here.
> 

Thank you for the review. I will address your comments in my next patch. Regarding
> +        var element = event.target.enclosingNodeOrSelfWithNodeName("td");
I did that because I need to know not only the node, but also which of the columns was double clicked (is it key or is it value).
Could you suggest a better way to find which column was clicked? thanks!

[Yael]

Created attachment 28017 [details]
Addressing comment #2.

This patch is addressing comment #2, with the exception that I am still using
 var element = event.target.enclosingNodeOrSelfWithNodeName("td");
I am not sure how else to find out if a key was edited or a value was edited.

[Timothy Hatcher]

Comment on attachment 28017 [details]
Addressing comment #2.

Using enclosingNodeOrSelfWithNodeName("td") is fine, now that I understand it.

Nice work!

[Yael]

Thank you for the review. Now I need help committing this patch :-)

[Timothy Hatcher]

Landed in r41288.