[webkit-reviews] review granted: [Bug 183420] Web Inspector: [META] Merge Resources and Debugger into a single Sources tab : [Attachment 362223] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 183420: Web Inspector: [META] Merge Resources and Debugger into a single
Sources tab
https://bugs.webkit.org/show_bug.cgi?id=183420

Attachment 362223: Patch

https://bugs.webkit.org/attachment.cgi?id=362223&action=review




--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 362223
  --> https://bugs.webkit.org/attachment.cgi?id=362223
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362223&action=review

rs=me

>>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:622
>> +	_addScript(script)
> 
> (I made it to this point and then got hungry)

I recognize this code is mostly migrating from Resources/Debugger. I'd love a
fresh approach to building up the sidebar reliably from the top level Targets,
but this is good for now!

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:695
> +	   let targetTreeElement = new WI.WorkerTreeElement(target);
> +	   this._workerTargetTreeElementMap.set(target, targetTreeElement);

I think it is fine to name this `this._workerTargeTreeElementMap` now, but in
concept this should really be for all targets so might go back to being
`this._targetTreeElementMap` in the future. Better to name is specifically now
so we can catch when we change it easily.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1442
> +    _handleDebuggerScriptsCleared(event)

This seems like it is doing a lot of work, removing resources 1 at a time and
reordering everything... If debugger scripts cleared isn't it basically
removing all scripts / resources?!

Room for optimizations in the future!

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1448
> +	   for (var i = this._breakpointsTreeOutline.children.length - 1; i >=
0; --i) {
> +	       var treeElement = this._breakpointsTreeOutline.children[i];

Style: Final `var`s to remove!