[webkit-reviews] review denied: [Bug 107937] Web Inspector: CPU pegged when inspecting LocalStorage that mutates. : [Attachment 186560] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vivek Galatage
<vivek.vg at samsung.com>'s request for review:
Bug 107937: Web Inspector: CPU pegged when inspecting LocalStorage that
mutates.
https://bugs.webkit.org/show_bug.cgi?id=107937

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186560&action=review


> Source/WebCore/inspector/front-end/DOMStorageItemsView.js:71
> +	   var rootNode = this._dataGrid.rootNode();

You should not be doing this all when the view is not visible.

> Source/WebCore/inspector/front-end/DOMStorageItemsView.js:80
> +		   if (childNode.data.key === key && childNode.data.value ===
oldValue) {

Why do you need to compare the old values? If key is getting removed, it does
not matter what value it had.

> Source/WebCore/inspector/front-end/DOMStorageItemsView.js:88
> +	       rootNode.appendChild(childNode);

This is still going to be slow if localstorage is being populated within a
loop. I wonder if it makes sense to send keys only and request values lazily.
Could be a part of a separate change though.

> Source/WebCore/inspector/front-end/DOMStorageItemsView.js:94
> +		   if (childNode.data.key === key && childNode.data.value ===
oldValue) {

ditto

> Source/WebCore/inspector/front-end/DOMStorageItemsView.js:104
>      update: function()

This mo longer needs to be public.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:120
> +   
WebInspector.domStorageModel.addEventListener(WebInspector.DOMStorageModel.Even
ts.DOMStorageChanged, this._domStorageChanged, this);

While you are here, it might make sense to move the listener registration from
the panel into the views.