[webkit-reviews] review requested: [Bug 59193] Web Inspector: Use different SourceFrame instances for ResourcesPanel and NetworkPanel : [Attachment 94781] Patch with fixes

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has asked  for review:
Bug 59193: Web Inspector: Use different SourceFrame instances for
ResourcesPanel and NetworkPanel
https://bugs.webkit.org/show_bug.cgi?id=59193

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
(In reply to comment #16)
> (From update of attachment 94610 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=94610&action=review
> 
> > Source/WebCore/inspector/front-end/ResourceView.js:95
> > +	 _contentLoaded: function (callback, text)
> 
> Why not make it a local function in requestContent?
Done.

> > Source/WebCore/inspector/front-end/ResourceView.js:-220
> > -	     function contentLoaded(text)
> 
> What's the reason for moving this out of the requestContent?
None :) Moved back in.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:407
> > +	     return treeElement.sourceView();
> 
> Can treeElement be undefined?
No, revision treeElement always already exists when showRevisionView is called.


> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1189
> > +	 this._resource.addEventListener("errors-warnings-cleared",
this._errorsWarningsCleared, this);
> 
> You can listen for ConsoleCleared for consistency, otherwise some
console-related events come from Resources while others from the Console
directly.
Now listening to both events from resource only to keep logic for resource
selection at one place.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:-1289
> > -	     if (this._storagePanel.currentQuery)
> 
> Should this check be preserved?
Done.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1344
> > +	     if (this._storagePanel.currentQuery)
> 
> Remove this.
Done.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1411
> > +	     delete oldView;
> 
> This doesn't make sense, please remove.
Done.