Bug 179807 – Web Inspector: Clean up backtrace in Canvas details sidebar

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Created attachment 327133 [details]
[Image] Hovered call frame tree element

Summary:
Clean up backtrace in Canvas details sidebar. The call frame items should use a tree outline to match call frames in the Debugger tab.

Call frames can be clicked to go to the source location, but not selected, so add a "selectable" property to TreeOutline. When selectable is false, tree items are highlighted on hover and dispatch click events, but cannot be selected.

[Radar WebKit Bug Importer]

<rdar://problem/35604378>

[Matt Baker]

Created attachment 327135 [details]
Patch

[Devin Rousso]

Comment on attachment 327135 [details]
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.ElementClicked, {element: element.treeElement});

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

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 327135 [details]
> 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 😃)

I'm adding CallFrameTreeOutline and TreeOutlineDetailsSection for this.

> > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:283
> > +            element.treeElement.treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementClicked, {element: element.treeElement});
> 
> NIT: I'd name the object key "treeElement" instead of just "element".

I agree these need to be renamed, but I don't want to do it now since other TreeOutline clients will need to be changed.

[Matt Baker]

Going to create CallFrameTreeController instead. A new TreeOutline subclass doesn't make sense, since we're specializing behavior here, and don't need to be so tightly coupled to TreeOutline.

This is a similar to DOMBreakpointTreeController and XHRBreakpointTreeController.

[Matt Baker]

Created attachment 327281 [details]
Patch

[Devin Rousso]

Comment on attachment 327281 [details]
Patch

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

r-.  I think that, while adding CallFrameTreeController is great, we don't want to mix View and Controller logic/code.  I'm fine with keeping CallFrameTreeController, but I think that we should either replace `get treeOutline` with specific getters for only the things that are needed (basically just `get element`) or require that a TreeOutline be passed in at construction (like Breakpoints and XHR).

> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:26
> +WI.CallFrameTreeController = class CallFrameTreeController extends WI.Object

NIT: this doesn't need to extend from WI.Object.

> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:32
> +        this._treeOutline = new WI.TreeOutline;

It seems very weird to create a UI object inside of a controller class.  Both Breakpoints and XHR pass in the TreeOutline to the controller, thereby removing the need for the `treeOutline` getter.  The idea that we have this controller that "owns" a UI element is very weird.  I'd rather the TreeOutline be created elsewhere and then passed into this controller.

> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:45
> +    get callFrames()
> +    {
> +        return this._callFrames;
> +    }

Style: make this a single line.

> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:53
> +        this._callFrames = callFrames;

Add `this._callFrames = null;` to the constructor please.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:139
> +        this._backtraceTreeController.treeOutline.selectable = false;

Both callers of `set selectable` provide a value of false.  Are we planning on moving existing backtrace TreeOutlines to using this controller, or is this supposed to be for the cases where we don't want a selectable tree outline?  Since this value is unlikely to flip-flop, I'd make it a constructor argument of CallFrameTreeController, so you don't have to go through `get treeOutline`.

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

You commented on the previous patch that renaming the key "element" to "treeElement" would require updating other clients, but this event is entirely new, and this patch contains all listeners for it.  I don't understand what you mean by what you said.

[Matt Baker]

Comment on attachment 327281 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:32
>> +        this._treeOutline = new WI.TreeOutline;
> 
> It seems very weird to create a UI object inside of a controller class.  Both Breakpoints and XHR pass in the TreeOutline to the controller, thereby removing the need for the `treeOutline` getter.  The idea that we have this controller that "owns" a UI element is very weird.  I'd rather the TreeOutline be created elsewhere and then passed into this controller.

There is nothing fundamentally weird about this. View controllers in both AppKit and UIKit manage the lifecycle of their views. I can imagine an approach to our UI where some views are created manually, and others are entirely managed by a controller. The only reason I didn't change the DOM and XHR breakpoint tree controllers is that their tree outlines are created by NavigationSidebarPanel.

That said, I'm fine with changing this to be consistent with how the other tree controllers are used.

>> Source/WebInspectorUI/UserInterface/Controllers/CallFrameTreeController.js:45
>> +    }
> 
> Style: make this a single line.

I thought we settled on only making properties a single line if both the getter and setter are a single line.

>> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:139
>> +        this._backtraceTreeController.treeOutline.selectable = false;
> 
> Both callers of `set selectable` provide a value of false.  Are we planning on moving existing backtrace TreeOutlines to using this controller, or is this supposed to be for the cases where we don't want a selectable tree outline?  Since this value is unlikely to flip-flop, I'd make it a constructor argument of CallFrameTreeController, so you don't have to go through `get treeOutline`.

I can't think of a scenario where this property would change after being set. Maybe this should be a constructor argument instead of a property. Then CallFrameTreeController could check it and listen for SelectionDidChange or ElementClicked based on the value.

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:283
>> +            element.treeElement.treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementClicked, {element: element.treeElement});
> 
> You commented on the previous patch that renaming the key "element" to "treeElement" would require updating other clients, but this event is entirely new, and this patch contains all listeners for it.  I don't understand what you mean by what you said.

What I meant was that I'd need to update the other events to use `treeElement` instead of `element` in their event data, in order to be consistent.

[Matt Baker]

Created attachment 327715 [details]
Patch

[Matt Baker]

This makes creating non-selectable tree outlines ugly in the common case:

let treeOutline = new WI.TreeOutline(null, true);

I think this is acceptable for now. Later, it could be made tidier by refactoring places that pass a custom list element.

[Matt Baker]

(In reply to Matt Baker from comment #10)
> This makes creating non-selectable tree outlines ugly in the common case:
> 
> let treeOutline = new WI.TreeOutline(null, true);

let treeOutline = new WI.TreeOutline(null, false);

[Devin Rousso]

Comment on attachment 327715 [details]
Patch

r=me.  This looks so much better :)  Thanks for putting up with my NITs.

[WebKit Commit Bot]

Comment on attachment 327715 [details]
Patch

Clearing flags on attachment: 327715

Committed r225259: <https://trac.webkit.org/changeset/225259>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.