[webkit-reviews] review denied: [Bug 216897] Web Inspector: Collapse blackboxed call frames in Sources : [Attachment 413867] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 216897: Web Inspector: Collapse blackboxed call frames in Sources
https://bugs.webkit.org/show_bug.cgi?id=216897

Attachment 413867: Patch

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




--- Comment #21 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 413867
  --> https://bugs.webkit.org/attachment.cgi?id=413867
Patch

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

r-.  Both @Brian Burg and I have expressed concerns about a design that adds
any concept of "hierarchy" to the call stack.

I don't think having a tree item that replaces itself with the blackboxed
frames is a crazy concept, as we do something similar already when viewing >100
items in an Array in the Console.  Also, the developer has already expressed a
desire to hide these call frames, so I think it's fine to make it slightly
harder/different to access them, as they're likely not going to even want to.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1643
> +	   for (let i = 0; i < this.length; ++i) {

NIT: Do we care about arrays with holes?

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
> +	   return
!!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceC
ode);

While this may work for this situation, I don't think this is a good idea
because the underlying `Map` that's queried in
`WI.debuggerManager.blackboxDataForSourceCode` can change in between the
`WI.CallFrame` being created and `WI.CallFrame.prototype.get blackboxed` is
called (e.g. the developer un-blackboxes a source code after pausing so that
the next pause won't be blackboxed).  I'd move this logic so that the
blackboxed state is known by the time the object is constructed (or derived
when it's constructed) so that if the user changes the blackbox then it won't
affect this value.

Also, now that this exists on `WI.CallFrame` (and given what I wrote above), we
should update the existing callsites in call stack/frame code that calls
`WI.debuggerManager.blackboxDataForSourceCode` to instead use this.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:44
> +	       let callFrameTreeElement = new
WI.CallFrameTreeElement(blackboxedCallFrame);

NIT: you could inline this

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:59
> +	   let renderCallFrames = (callFrames, shouldSelectActiveFrame = false)
=> {

Why the `= false`?

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:61
> +		   const minGroupSize = 4;

This number seems arbitrary and likely to cause confusion.  If we're going to
hide blackboxed call frames, we should either always do it (regardless of how
many sequential call frames there are) or never do it.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:67
> +		   if (!Array.isArray(callFrame)) {

Can we rename this `callFrameOrBlackboxedGroup` so that this line of code isn't
as confusing?  Reading it as-is, it's obvious that a call frame is not an array
:/

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:73
> +		   }

Style: I'd add a newline after this

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:74
> +		   let blackboxedTreeElement = new
WI.BlackboxedGroupTreeElement(callFrame);

NIT: you could inline this