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.
<rdar://problem/14671060>
Created attachment 208248 [details] Patch
Probably fixes https://bugs.webkit.org/show_bug.cgi?id=116241 Although not sure if the case is being flaky!
Created attachment 208261 [details] Patch
(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.
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.
Created attachment 208311 [details] Patch
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.
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.
Created attachment 208313 [details] Patch
Comment on attachment 208313 [details] Patch Clearing flags on attachment: 208313 Committed r153817: <http://trac.webkit.org/changeset/153817>
All reviewed patches have been landed. Closing bug.