Bug 48206 – Web Inspector: add support for errors, warnings and search to the storage panel.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 71682 [details]
[PATCH] Proposed change.

Drive-by factor out of resource tree model in the resource manager.

[Pavel Feldman]

Created attachment 71683 [details]
[PATCH] Proposed change (with proper changelog).

[Timothy Hatcher]

Comment on attachment 71683 [details]
[PATCH] Proposed change (with proper changelog).

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.

[Pavel Feldman]

Thanks for the 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.
> 

I think I can remove this one. Was just doing it for speed: there is no way to look up tree item by view fast:
- search results come in terms of views
- views point to resources
- tree items point to resources

so it was non-trivial to go view -> tree item (there is no map from resources to tree items in storage panel, i didn't want to use treeitem's represented object). I'll use it now though.

> > WebCore/inspector/front-end/ResourceManager.js:351
> > +                view.clearMessages();
> 
> Do we need this now that there is an "errors-warnings-updated" event?

True, will use it.

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

ResourceManager has resource tree model that is maintaining the frame / resource hierarchy. Storage view is supposed to listen to that model. However, so far, storage view is the only listener, so I am getting away with direct calls and this unhealthy dependency. Change was already big, will do it later.
 
> > WebCore/inspector/front-end/ResourceManager.js:507
> > +            resourceForURL.push(resource);
> 
> When can there be multiple resources for a URL? Different frames?
> 

Yes.

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

Good catch. This is actually a bug. Talking about frameId, yes, we should pass it here. Unfortunately, the client of the call (console) does not have it yet, so we should push frameId along with the console message. I'll file a bug on it once I land this.

> > WebCore/inspector/front-end/StoragePanel.js:260
> > +        this.showResource(WebInspector.resourceManager.resourceForURL(url), line);
> 
> Can't this fail if the resource is an array?
> 

Yes, you caught a bug above.

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

As I mentioned above, I did not want to use it initially, but I will now.
 
> > 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.

Exactly.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/Resource.js
	M	WebCore/inspector/front-end/ResourceManager.js
	M	WebCore/inspector/front-end/Settings.js
	M	WebCore/inspector/front-end/StoragePanel.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.js
Committed r70445