[webkit-reviews] review granted: [Bug 48206] Web Inspector: add support for errors, warnings and search to the storage panel. : [Attachment 71683] [PATCH] Proposed change (with proper changelog).

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 48206: Web Inspector: add support for errors, warnings and search to the
storage panel.
https://bugs.webkit.org/show_bug.cgi?id=48206

Attachment 71683: [PATCH] Proposed change (with proper changelog).
https://bugs.webkit.org/attachment.cgi?id=71683&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71683&action=review

> WebCore/inspector/front-end/Resource.js:552
> +    set searchMatches(x)

I think it is odd to have this on Resource. What search? Resource does not deal
with searching, panels do.

> WebCore/inspector/front-end/ResourceManager.js:351
> +		   view.clearMessages();

Do we need this now that there is an "errors-warnings-updated" event?

> WebCore/inspector/front-end/ResourceManager.js:424
> +	   WebInspector.panels.storage.addOrUpdateFrame(parentFrameId, frameId,
displayName);

Why is this still using the storage panel? Shouldn't this be self-contained and
storage/resources panel be using the ResourceManager?

> WebCore/inspector/front-end/ResourceManager.js:507
> +	       resourceForURL.push(resource);

When can there be multiple resources for a URL? Different frames?

> WebCore/inspector/front-end/ResourceManager.js:530
> +	   return this._resourcesByURL[url];

This can return an array, so is it really "resourceForURL"? Should this take
another param (frameId?) to guarentee just one resource is returned?

> WebCore/inspector/front-end/StoragePanel.js:260
> +	   this.showResource(WebInspector.resourceManager.resourceForURL(url),
line);

Can't this fail if the resource is an array?

> WebCore/inspector/front-end/StoragePanel.js:273
> +	   this._forAllResourceTreeElements(callback);

TreeOutline has findTreeElement you could use, which takes a representedObject
to find a TreeElement with the same representedObject. That would be faster
than a function call for all resources in the tree.

> WebCore/inspector/front-end/StoragePanel.js:276
> +	       var view =
WebInspector.ResourceManager.resourceViewForResource(resource);

I didn't expect ResourceManager to know about views. But I guess it is needed
for Resources and Scripts to share logic? Maybe that can change when we remove
Scripts.