[webkit-reviews] review denied: [Bug 179807] Web Inspector: Clean up backtrace in Canvas details sidebar : [Attachment 327281] 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 327281: Patch

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




--- Comment #7 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 327281
  --> https://bugs.webkit.org/attachment.cgi?id=327281
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.E
lementClicked, {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.