Web Inspector: Bind backspace to delete cookies and DOM Storage

[Jonas Due Vesterheden]

It should be possible to delete the selected cookie in the storage section by pressing backspace. (https://bugs.webkit.org/show_bug.cgi?id=31157#c3)

[Brian Weinstein]

This should also affect DOM storage. Updating title and assigning to myself.

[Brian Weinstein]

Created attachment 42591 [details]
Fix

[Timothy Hatcher]

Comment on attachment 42591 [details]
Fix

You don't need to add an event listener. The handleKeyEvent function should already be called on the view by the panel.

The datagrid should not care about the key events either, just make the View call deleteSelectedRow.

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 42591 [details])
> You don't need to add an event listener. The handleKeyEvent function should
> already be called on the view by the panel.
> 
> The datagrid should not care about the key events either, just make the View
> call deleteSelectedRow.

Without putting the listener in CookieItemsView, it didn't seem to be responding to key events, but I might have had something else wrong there, I can try again.

Are you saying the handleKeyEvent in DOMStorageDataGrid shouldn't call DataGrid.handleKeyEvent?

[Brian Weinstein]

Created attachment 42624 [details]
Fix w/ Refactoring

Would be surprised if this was perfect, but it works, and is a nice cleanup to get all this code into DataGrid so other components can get it for free.

[Joseph Pecoraro]

Hey. This turned out to be a rather long comment because I went into detail.

> var dataGrid = new WebInspector.DataGrid(columns, null, null);

I don't know the style guidelines on this but I seem to recall a number of places where we get away with not explicitly passing the extra parameters (they are then "undefined" which act the same as "null" in the simple if statements).  Personally, I like explicit parameters like this.


> _ondblclick: function(event)
> {
>     if (this._editing)
>         return;
>     if (this._editingNode)
>         return;
> 
>     this._startEditing(event);
> },

The ifs can be combined with an || (I see its leftover code, from me!).


> _startEditing: function(event)
> {
>     var element = event.target.enclosingNodeOrSelfWithNodeName("td");
>     if (!element)
>         return;
> 
>     this._editingNode = this.dataGridNodeFromEvent(event);
>     if (!this._editingNode) {
>         if (!this.creationNode)
>             return;
>         this._editingNode = this.creationNode;
>     }
> 
>     // Force editing the "Key" column when editing the creation node
>     if (this._editingNode.isCreationNode)
>         return this._startEditingColumnOfDataGridNode(this._editingNode, 0);
> 
>     this._editing = true;
>     WebInspector.startEditing(element, this._editingCommitted.bind(this), this._editingCancelled.bind(this), element.textContent);
>     window.getSelection().setBaseAndExtent(element, 0, element, 1);
> },

This is a big concern of mine. I think there should be a 3rd delegate. Along with editCallback, deleteCallback, there should be a createCallback. I think that separate data grid will have different concerns about creation.

Most of this code is specific to DOMStorage (as evident by the comment about the "Key" column. I only added creating to DOM Storage because it was possible to create a DOM Storage with just 1 value (a single edit) as long as it was the "Key" value. More then 1/2 of the code here is creation code.

A double click on "no node" forces it to be the creationNode. I would break it down as:

  - in the constructor if creation is allowed, run addCreationNode

I had it so this was done in DOMStorageDataGrid. However, if we make it generic this might as well be handled by the constructor.

  - in startEditing, if it is no node and creation is allowed:
    - set the creation node to be the selected node
    - ???

Most of this is done, but the last step is debatable. Should we just set a flag that its creation mode and start editing any column? Should we start editing the first field (DOM Storage approach)? Should we run the callback and let the callback tell us which field to start editing (is this extensibility needed)? I like just editing the first field.


>          // FIXME: We need more column identifiers here throughout this function.
>          // Not needed yet since only editable DataGrid is DOM Storage, which is Key - Value.
>          var columnIdentifier = (element.hasStyleClass("0-column") ? 0 : 1);

Indeed this part should be made generic =). A poor (but workable) approach would be:

  element.className.match(/\b(\d+)-column\b/)[1]; // parseInt if needed

A better approach would be determining the column directly, without peeking at the style class.


>  this._editingCallback(columnIdentifier, newText);

You're lucky this worked =). This should most definitely be the callback made in the constructor. Its using the actual this._editingCallback in the "subclass" DOMStorageDataGrid instead of the editCallback (which happens to be the exact same thing) that you passed into the Constructor. It should be:

   this._editCallback(...);


> } else if (event.keyCode == "8" || event.keyCode == "46") {

event.keyCode is a numeric value. You get lucky with the ==. In this case it coerces the String into a Number and the comparison succeeds. Take this for instance:

  js> 1 == "1E0"
  true

I'd stick with numeric values and a strict equality:

   } else if (event.keyCode === 8 || event.keyCode === 46) {


Moving things to DataGrid is certainly a step in the right direction. After seeing it, I'm a little hesitant about passing functions into the constructor. How about instead of passing these functions into the Constructor they were made events triggered by the DataGrid itself? It already has all the plumbing. Would there be a big performance difference (these events aren't fired often):

  WebInspector.DataGridNode.prototype.__proto__ = WebInspector.Object.prototype;

[Timothy Hatcher]

Comment on attachment 42624 [details]
Fix w/ Refactoring


> -    this.dataGrid = new WebInspector.DataGrid(columns);
> +    this.dataGrid = new WebInspector.DataGrid(columns, null, null);

You don't really need to pass nul, null here, they will be undefined if left off and that works for your if (editCallback) and if (deleteCallback) checks.

We can get rid of DOMStorageDataGrid.js and make DOMStorageItemsView handle the callbacks.

[Timothy Hatcher]

Joe makes some good points! (I don't have time to comment specificly now, but I like all of them.)

[Brian Weinstein]

(In reply to comment #8)
> Joe makes some good points! (I don't have time to comment specificly now, but I
> like all of them.)

Agreed, thanks Joe! I will take both of these comments into account and send out a new patch. After these changes I wouldn't feel comfortable just committing it without another review.

[Timothy Hatcher]

(In reply to comment #6)
> Moving things to DataGrid is certainly a step in the right direction. After
> seeing it, I'm a little hesitant about passing functions into the constructor.
> How about instead of passing these functions into the Constructor they were
> made events triggered by the DataGrid itself? It already has all the plumbing.
> Would there be a big performance difference (these events aren't fired often):
> 
>   WebInspector.DataGridNode.prototype.__proto__ =
> WebInspector.Object.prototype;

Actually this wont be great as events, sicne usually there is one handler. Events are for one or many listeners.

Plus you would still need to enable editing, but you might not have event listeners.

[Joseph Pecoraro]

Brian, I tested preventDefault when deleting using ⌦ (delete) instead of ⌫ (backspace) in the Elements Tree Hierarchy. It does stop the ugly beep from happening!  Since were discussing this before, could you include this one line fix in your patch. You will probably need to do the same for backspace/delete in the data grid:

> @@ -195,6 +195,7 @@ WebInspector.ElementsTreeOutline.prototype = {
>          // Delete or backspace pressed, delete the node.
>          if (event.keyCode === 8 || event.keyCode === 46) {
>              selectedElement.remove();
> +            event.preventDefault();
>              return;
>          }

[Brian Weinstein]

Created attachment 42673 [details]
Fix w/ Refactoring + Remove DOMStorageDataGrid

If you could re-read the refactored code as well, that would be great, I don't want this to break things or introduce regressions. It worked fine in all my tests, but another pair of eyes or two would be great.

[Joseph Pecoraro]

> If you could re-read the refactored code as well, that would be great, I don't
> want this to break things or introduce regressions. It worked fine in all my
> tests, but another pair of eyes or two would be great.

It looks good. There might be one regression.


> this._keys = keys

Doesn't look like this is used or needed. Probably leftover code?


> 
> 
> _startEditingColumnOfDataGridNode: function(node, column)

Should just be one blank line instead of two.

>   _deleteButtonClicked: function(event)
>   {
> -   if (this._dataGrid) {
> -       this._dataGrid.deleteSelectedRow();
> -       
> -       this.show();
> -   }
> +  this.deleteSelectedRow();
> +  this.show();
>   },
>  
>  ...
>
>  deleteSelectedRow: function()
>  {
>      var node = this.selectedNode;
>      this._deleteCallback(node);
>  },

This could be a regression. Does the delete button still work for DOM Storage?

"deleteSelectedRow" sounds like it should be in DataGrid.js, I'd suggest it be moved there. The this.show() will probably be handled by the deleteCallback and can likely be dropped.


> // Force editing the "Key" column when editing the creation node

"Key" should probably be changed to the 1st column to be datagrid agnostic.


> Lastly, added a preventDefault call in ElementsTreeOutline to prevent the 
> inspector from beeping at you when you delete an element.

You should probably add the preventDefault to the Delete/Backspace handler you just added:

  } else if (event.keyCode === 8 || event.keyCode === 46) {
      if (this._deleteCallback) {
          handled = true;
          this._deleteCallback(this.selectedNode);
      }
  }

[Brian Weinstein]

Comment on attachment 42673 [details]
Fix w/ Refactoring + Remove DOMStorageDataGrid

Joe found a regression in this, which I've fixed, but this patch is wrong. Sending out a new version addressing his comments.

[Brian Weinstein]

Created attachment 42676 [details]
Fix w/ Refactoring + Deleted File + Fixes Delete Button Regression

[Timothy Hatcher]

Comment on attachment 42676 [details]
Fix w/ Refactoring + Deleted File + Fixes Delete Button Regression

Nice work!

Thanks for the reviewing Joe.

[Brian Weinstein]

Landed in r50613.