[webkit-reviews] review denied: [Bug 179807] Web Inspector: Clean up backtrace in Canvas details sidebar : [Attachment 327135] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 179807: Web Inspector: Clean up backtrace in Canvas details sidebar
https://bugs.webkit.org/show_bug.cgi?id=179807

Attachment 327135: Patch

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




--- Comment #3 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 327135
  --> https://bugs.webkit.org/attachment.cgi?id=327135
Patch

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

r-, as I'd like to see a version that doesn't involve recreating the
TreeOutline on each refresh.  Also, if we want to match the Debugger sidebar,
we should change the RecordingTraceSidebarPanel as well.  I think it will be
very similar to the logic in this SidebarPanel.  (If you wanted to make a
Utility version of this to avoid duplicated code that would also be great 😃)

> Source/WebInspectorUI/ChangeLog:10
> +	   No longer needed.

This comment is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:331
> +	  
backtraceTreeOutline.addEventListener(WI.TreeOutline.Event.ElementClicked,
(event) => {

I think this will keep references to each TreeOutline that is created, so
either we need to remove the event listener or just save the TreeOutline to a
variable.  I'd prefer the latter.

    let callFrames = this._canvas.backtrace;
    this._backtraceSection.element.hidden = !callFrames.length;

    if (!this._backtraceTreeOutline) {
	this._backtraceTreeOutline = new TreeOutline;
	this._backtraceTreeOutline.selectable = false;
       
this._backtraceTreeOutline.addEventListener(WI.TreeOutline.Event.ElementClicked
, this._handleBacktraceTreeOutlineElementClicked.bind(this));
	this._backtraceRow.element.appendChild(backtraceTreeOutline.element);
    }

    this._backtraceTreeOutline.removeChildren(true);
    for (let callFrame of callFrames)
	backtraceTreeOutline.appendChild(new
WI.CallFrameTreeElement(callFrame));

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:283
> +	      
element.treeElement.treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.E
lementClicked, {element: element.treeElement});

NIT: I'd name the object key "treeElement" instead of just "element".