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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 16 18:40:58 PST 2020

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

Attachment 413867: Patch


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

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

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

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

NIT: you could inline this

More information about the webkit-reviews mailing list