Bug 119541 – Web Inspector: Use granular DOMStorage modification events to avoid complete DataGrid update.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Vivek Galatage]

InspectorDOMStorageAgent sends more granular events about the storage modifications. Using these would avoid the complete rebuilding of the DataGrid.

Fixes <rdar://problem/13223981>

Patch follows.

[Radar WebKit Bug Importer]

<rdar://problem/14671060>

[Vivek Galatage]

Created attachment 208248 [details]
Patch

[Vivek Galatage]

Probably fixes https://bugs.webkit.org/show_bug.cgi?id=116241 

Although not sure if the case is being flaky!

[Vivek Galatage]

Created attachment 208261 [details]
Patch

[Vivek Galatage]

(In reply to comment #3)
> Probably fixes https://bugs.webkit.org/show_bug.cgi?id=116241 
> 
> Although not sure if the case is being flaky!

Still the test case is flaky on WK2. Reverting back the TestExpectation.

[Joseph Pecoraro]

Comment on attachment 208261 [details]
Patch

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.contentViewForRepresentedObject(domStorage, true);
> +        if (!domStorageView)
> +            return;

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

[Vivek Galatage]

Created attachment 208311 [details]
Patch

[Vivek Galatage]

Comment on attachment 208261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208261&action=review

Thank you for the review.

>> 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.

Done.

>> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:78
>> +        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?

Done.

>> 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.

Removed.

>> 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.

Done.

>> 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};

Modified.

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

Modified.

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

Modified.

>> Source/WebInspectorUI/UserInterface/StorageManager.js:52
>> +    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.

Modified to have separate functions.

>> Source/WebInspectorUI/UserInterface/StorageManager.js:107
>> +            return;
> 
> Style: Place newlines after early returns. See domStorageWasUpdated above.

Done.

[Joseph Pecoraro]

Comment on attachment 208311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208311&action=review

Much nicer! This look great. You may want to remove that one extra line before landing.

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:80
> +        this._dataGrid.insertChild(childNode, this._dataGrid.children.length - 1);
> +        if (this._dataGrid.sortOrder)
> +            this._sortDataGrid();

In the future this could insert at sorted index, and be a binary search + insertion O(log n) instead of a sort O(n * log n). But this is probably good enough for now. I don't think there are too many sites with many localStorage values.

> Source/WebInspectorUI/UserInterface/DOMStorageContentView.js:168
> +        this._dataGridSorted = true;

This new property seems unnecessary. Nothing checks it and it didn't exist before. Looks like you can remove this line.

[Vivek Galatage]

Created attachment 208313 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 208313 [details]
Patch

Clearing flags on attachment: 208313

Committed r153817: <http://trac.webkit.org/changeset/153817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.