[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 bugzilla-daemon at webkit.org
Thu Feb 21 15:18:58 PST 2019

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

Attachment 362223: Patch


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

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


>> +	_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!

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

> +    _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!

> +	   for (var i = this._breakpointsTreeOutline.children.length - 1; i >=
0; --i) {
> +	       var treeElement = this._breakpointsTreeOutline.children[i];

Style: Final `var`s to remove!

More information about the webkit-reviews mailing list