[webkit-reviews] review denied: [Bug 119541] Web Inspector: Use granular DOMStorage modification events to avoid complete DataGrid update. : [Attachment 208261] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Vivek Galatage
<vivekg at webkit.org>'s request for review:
Bug 119541: Web Inspector: Use granular DOMStorage modification events to avoid
complete DataGrid update.
https://bugs.webkit.org/show_bug.cgi?id=119541

Attachment 208261: Patch
https://bugs.webkit.org/attachment.cgi?id=208261&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208261&action=review


Looks good overall, just needs a bit of polish. I also have a bunch of minor
comments and questions I'd like to see answered.

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:67
> +	   for (var i = 0; i < this._dataGrid.children.length; ++i)

Style: This for loop should have braces about its body.

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:78
> +	   this._dataGrid.insertChild(childNode, this._dataGrid.children.length
- 1);
> +	   childNode.revealAndSelect();

What if the dataGrid is sorted, this just inserts the child at the end?

Maybe that makes sense because you're only adding items at the bottom. But
then, when does the sort eventually happen?

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:92
> +		   if (childNode.data[1] === oldValue) {

This check doesn't seem necessary. You're going to replace the data value
anyways, and remove any other nodes with the same key (which shouldn't happen
either). I think you should remove this if check, unless you have a reason it
needs to be here.

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:95
> +		       childNode.revealAndSelect();

I'm a little worried about these revealAndSelect calls.

If you're adding a localStorage value, and the page itself triggers an update
(localStorage.x = localStorage.x + 1), will this event cause you to lose your
place editing and reveal & select the updated value? If localStorage is
changing frequently that would make it hard to modify. Though, I think we
probably already have these issues, so they should be addressed in a follow-up.


> Source/WebInspectorUI/UserInterface/DOMStorageObserver.js:59
> +	   var eventData = { key: key };

Style: WebInspector style for object literals is: {key: value, key: value}. So
this should be:

    var eventData = {key: key};

> Source/WebInspectorUI/UserInterface/DOMStorageObserver.js:66
> +	   var eventData = { key: key, value: value };

Style: here too.

> Source/WebInspectorUI/UserInterface/DOMStorageObserver.js:73
> +	   var eventData = { key: key, oldValue: oldValue, value: value };

Style: here too.

> Source/WebInspectorUI/UserInterface/StorageManager.js:52
> +    DOMStorageItemsCleared: "storage-manager-dom-storage-items-cleared",
> +    DOMStorageItemRemoved: "storage-manager-dom-storage-item-removed",
> +    DOMStorageItemAdded: "storage-manager-dom-storage-item-added",
> +    DOMStorageItemUpdated: "storage-manager-dom-storage-item-updated",

Declaring Events like this and not dispatching an event is a little misleading.
Someone might come along, see these events listed, register for one, and be
surprised when their handler is not called.

You could make a separate enum, or just make 4 functions for the observer to
call, and share the logic to get the domStorageView for a storage id. I think 4
functions would be fine.

> Source/WebInspectorUI/UserInterface/StorageManager.js:107
> +	   var domStorage = this._domStorageForId(id);
> +	   if (!domStorage)
> +	       return;
> +	   var domStorageView =
WebInspector.contentBrowser.contentViewContainer.contentViewForRepresentedObjec
t(domStorage, true);
> +	   if (!domStorageView)
> +	       return;

Style: Place newlines after early returns. See domStorageWasUpdated above.