Bug 216897 – Web Inspector: Collapse blackboxed call frames in Sources

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Currently, blackboxed stack trace items are semi-transparent. We should collapse them by default to make the UI less cluttered.

<rdar://69325903>

[Nikita Vasilyev]

Created attachment 412160 [details]
Patch

[Nikita Vasilyev]

Created attachment 412161 [details]
[Image] Before

[Nikita Vasilyev]

Created attachment 412162 [details]
[Image] After

The page I used to take the screenshots: https://nv.github.io/webkit-inspector-bugs/react-and-blackboxing/text-field.html

[Patrick Angle]

Comment on attachment 412160 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:43
> +localizedStrings["%d call frames blackboxed"] = "%d call frames blackboxed";

It looks odd to me to have the number of call frames come before the reason they are collapsed. Looking at it in the context of the rest of the stack trace, I think I would expect something more akin to `"Blackboxed — %d call frames"`.

[Blaze Burg]

Comment on attachment 412160 [details]
Patch

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

There are some smaller coding fixes to address, which I commented about. There is the larger issue of the UI design, which does not address some revisions that we discussed yesterday.

We had consensus that the eyeball should belong on the trailing (right) side of the TreeElement, and upon the eyeball being clicked, it shows the frames that have been hidden. Patrick also had a good suggestion for formatting the label. Lastly, I still would prefer to not use a disclosure triangle at all, since it breaks the visual flow of the call frames and implies a hierarchical relationship between call frames, which is misleading. A clickable placeholder that disappears (or turns into an eyeball on the top grouped frame) would be my preference, as it requires one click and can use a button appearance to look more clickable.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:43
>> +localizedStrings["%d call frames blackboxed"] = "%d call frames blackboxed";
> 
> It looks odd to me to have the number of call frames come before the reason they are collapsed. Looking at it in the context of the rest of the stack trace, I think I would expect something more akin to `"Blackboxed — %d call frames"`.

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548
> +        function flush(blackboxFrameEndIndex) {

This is named as an index, but it can take the value callFrames.length, which is not a valid index. Please adjust the caller and the code here (and add assertions) so only valid indices can be passed.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:552
> +            let adjacentCount = blackboxFrameEndIndex - blackboxFrameStartIndex;

(:548) ...you'll need to add back 1 here to endIndex.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:558
> +        }

This function would be more naturally expressed using Array.prototype.splice() once the start/end index of the blackboxed call frame range has been found. Then we can get rid of groupedCallFrames altogether. It would be good to copy the `callFrames` parameter using callFrames.slice() to avoid mutating the original.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:567
> +                    sourceCodeToBlackboxData.set(callFrame.sourceCodeLocation.sourceCode, blackboxData);

It seems like this Map is never read, can we remove it?

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:401
> +        experimentalSettingsView.addSetting(WI.UIString("Debugging:"), WI.settings.experimentalCollapseBlackboxedCallFrames, WI.UIString("Collapse blackboxed call frames"));

Both of these labels need informative UIString keys/descriptions.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1958
> +        if (treeElement instanceof WI.GeneralTreeElement)

Why is this needed?

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:63
> +                    let blackboxedTreeElement = new WI.GeneralTreeElement("blackboxed-group", title);

It's going to be a lot easier to iterate on this if we have a TreeElement subclass for the blackboxed call frame group.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:69
> +                } else {

Please put this branch first and early continue at the end of it. This will help reduce the amount of unneeded indentation.

if (!Array.isArray(callFrame)) {
    // ...
    continue;
}

// Handle grouping case

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:79
> +            renderCallFrames(WI.debuggerManager.groupBlackboxedCallFrames(callFrames), true);

It makes no sense to be for DebuggerManager to be involved here, all it's doing in the function is restructuring the array.

It would be better to define this helper on Array.prototype in WI.Utilities (Utilities.js) so that we can make this work for any array & predicate, and write unit tests for it.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:111
> +            if (WI.settings.experimentalCollapseBlackboxedCallFrames.value)

To avoid duplication and unnecessary detail in this already large function, please put the settings check inside renderCallFrames()

[Devin Rousso]

Comment on attachment 412160 [details]
Patch

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

(In reply to Brian Burg from comment #5)
> We had consensus that the eyeball should belong on the trailing (right) side of the TreeElement, and upon the eyeball being clicked, it shows the frames that have been hidden. Patrick also had a good suggestion for formatting the label. Lastly, I still would prefer to not use a disclosure triangle at all, since it breaks the visual flow of the call frames and implies a hierarchical relationship between call frames, which is misleading. A clickable placeholder that disappears (or turns into an eyeball on the top grouped frame) would be my preference, as it requires one click and can use a button appearance to look more clickable.
I agree with this approach.  We should also try to make it such that once the placeholder is clicked, it is never shown again (i.e. `WI.ThreadTreeElement.prototype.refresh` should not create another `WI.BlackboxedCallFramesPlaceholderTreeElement` for that particular grouping).

Also, please make sure that this works correctly with existing functionality:
 - →/space/enter should probably act as though the eye was pressed
 - ⌘C should probably copy the blackboxed callframes (even when not shown) instead of the `WI.BlackboxedCallFramesPlaceholderTreeElement`
 - filtering should show the `WI.BlackboxedCallFramesPlaceholderTreeElement` if one of it's blackboxed call frames matches the query
I vaguely remember there being assumptions made in `WI.SourcesNavigationSidebarPanel` that the `_callStackTreeOutline` is ever only two children deep (first level is the `WI.ThreadTreeElement` and the second level is the `WI.CallFrameTreeElement`), so you might want to look around and check.

>> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:63
>> +                    let blackboxedTreeElement = new WI.GeneralTreeElement("blackboxed-group", title);
> 
> It's going to be a lot easier to iterate on this if we have a TreeElement subclass for the blackboxed call frame group.

+1 to this

Also, both `WI.CallFrameView` and `WI.CallFrameTreeElement` query for `WI.debuggerManager.blackboxDataForSourceCode(callFrame.sourceCodeLocation.sourceCode)`.  Perhaps we should just move that to a `this._blackboxed` in `WI.CallFrame` (exposed via `get blackboxed`) so that this same logic can be shared everywhere.  Furthermore, by doing this it should make the logic of grouping blackboxed call frames together much easier as all you'd need to do is save sequential `WI.CallFrame` into a separate array
```
    let blackboxedCallFrames = [];
    for (let callFrame of callFrames) {
        if (callFrame.blackboxed) {
            blackboxedCallFrames.push(callFrame);
            continue;
        }


        if (blackboxedCallFrames.length)
            this.appendChild(new WI.BlackboxedCallFramesPlaceholderTreeElement(blackboxedCallFrames.splice()));

        ...
    }
```

[Nikita Vasilyev]

(In reply to Brian Burg from comment #5)
I'm not sure how to reconcile these two suggestions.

> We had consensus that the eyeball should belong on the trailing (right) side
> of the TreeElement, and upon the eyeball being clicked, it shows the frames
> that have been hidden.

This sounds like the eyeball becomes, functionally, one-way disclosure triangle.

    onInput - myApp.js
    Blackboxed — 5 call frames       👁
    render - myApp.js

> Lastly, I still would prefer to not use a disclosure triangle at all,
> since it breaks the visual flow of the call frames and implies a
> hierarchical relationship between call frames, which is misleading. A
> clickable placeholder that disappears (or turns into an eyeball on the top
> grouped frame) would be my preference, as it requires one click and can use
> a button appearance to look more clickable.

So, now you're suggesting to use a clickable placeholder that isn't an eyeball, is that right?

    onInput - myApp.js
    Blackboxed — 5 call frames       [placeholder]
    render - myApp.js

What would that placeholder be?

After clicking [placeholder], you're suggesting to show this, is that right?

    onInput - myApp.js
    Blackboxed — 5 call frames         👁
    dispatchEvent — react.js
    unstable_runWithPriority — react.js
    runWithPriority$1 — react.js
    discreteUpdates$1 — react.js
    discreteUpdates — react.js
    render - myApp.js

[Nikita Vasilyev]

(In reply to Patrick Angle from comment #4)
> Comment on attachment 412160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412160&action=review
> 
> > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:43
> > +localizedStrings["%d call frames blackboxed"] = "%d call frames blackboxed";
> 
> It looks odd to me to have the number of call frames come before the reason
> they are collapsed. Looking at it in the context of the rest of the stack
> trace, I think I would expect something more akin to `"Blackboxed — %d call
> frames"`.

Yes, I had the exact string at one of the iterations (that I didn't share) :)
I agree with you.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #7)
> (In reply to Brian Burg from comment #5)
> I'm not sure how to reconcile these two suggestions.
> 
> > We had consensus that the eyeball should belong on the trailing (right) side of the TreeElement, and upon the eyeball being clicked, it shows the frames that have been hidden.
> 
> This sounds like the eyeball becomes, functionally, one-way disclosure triangle.
> 
>     onInput - myApp.js
>     Blackboxed — 5 call frames       👁
>     render - myApp.js
I think "one-way disclosure triangle" is a good way to describe it.

> > Lastly, I still would prefer to not use a disclosure triangle at all, since it breaks the visual flow of the call frames and implies a hierarchical relationship between call frames, which is misleading. A clickable placeholder that disappears (or turns into an eyeball on the top grouped frame) would be my preference, as it requires one click and can use a button appearance to look more clickable.
> 
> So, now you're suggesting to use a clickable placeholder that isn't an eyeball, is that right?
> 
>     onInput - myApp.js
>     Blackboxed — 5 call frames       [placeholder]
>     render - myApp.js
> 
> What would that placeholder be?
> 
> After clicking [placeholder], you're suggesting to show this, is that right?
> 
>     onInput - myApp.js
>     Blackboxed — 5 call frames         👁
>     dispatchEvent — react.js
>     unstable_runWithPriority — react.js
>     runWithPriority$1 — react.js
>     discreteUpdates$1 — react.js
>     discreteUpdates — react.js
>     render - myApp.js
I think he's suggesting that the "Blackboxed - 5 call frames    👁" would disappear entirely.  Basically, when the status icon is clicked (or any of the other "activate" actions, like space/enter/→/etc.) the `WI.BlackboxedCallFramesPlaceholderTreeElement` should be removed and all of the blackboxed `WI.CallFrame` it represented should be inserted in that spot.

[Federico Bucchi]

Comment on attachment 412160 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:544
> +        let groupedCallFrames = [];

Can be `const`

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:554
> +                let blackboxedFrames = callFrames.slice(blackboxFrameStartIndex, blackboxFrameEndIndex);

Can it be `const` ?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:561
> +            let callFrame = callFrames[i];

Is this reassigned?  Can it be `const` ?

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

Is this reassigned?  Can it be `const` ?

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

Is this reassigned?  Can it be `const` ?

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:70
> +                    let callFrameTreeElement = new WI.CallFrameTreeElement(callFrame);

Is this reassigned?  Can it be `const` ?

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:109
> +            let callFrames = startIndex ? currentStackTrace.callFrames.slice(startIndex) : currentStackTrace.callFrames;

Is this reassigned?  Can it be `const` ?

[Devin Rousso]

Comment on attachment 412160 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:544
>> +        let groupedCallFrames = [];
> 
> Can be `const`

Our style for using `const` is to only use it when the value is never changed across Web Inspector sessions (i.e. a true constant).

[Nikita Vasilyev]

Comment on attachment 412160 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:567
>> +                    sourceCodeToBlackboxData.set(callFrame.sourceCodeLocation.sourceCode, blackboxData);
> 
> It seems like this Map is never read, can we remove it?

It's read 4 lines above.

>>> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:63
>>> +                    let blackboxedTreeElement = new WI.GeneralTreeElement("blackboxed-group", title);
>> 
>> It's going to be a lot easier to iterate on this if we have a TreeElement subclass for the blackboxed call frame group.
> 
> +1 to this
> 
> Also, both `WI.CallFrameView` and `WI.CallFrameTreeElement` query for `WI.debuggerManager.blackboxDataForSourceCode(callFrame.sourceCodeLocation.sourceCode)`.  Perhaps we should just move that to a `this._blackboxed` in `WI.CallFrame` (exposed via `get blackboxed`) so that this same logic can be shared everywhere.  Furthermore, by doing this it should make the logic of grouping blackboxed call frames together much easier as all you'd need to do is save sequential `WI.CallFrame` into a separate array
> ```
>     let blackboxedCallFrames = [];
>     for (let callFrame of callFrames) {
>         if (callFrame.blackboxed) {
>             blackboxedCallFrames.push(callFrame);
>             continue;
>         }
> 
> 
>         if (blackboxedCallFrames.length)
>             this.appendChild(new WI.BlackboxedCallFramesPlaceholderTreeElement(blackboxedCallFrames.splice()));
> 
>         ...
>     }
> ```

"Perhaps we should just move that to a `this._blackboxed` in `WI.CallFrame` (exposed via `get blackboxed`) so that this same logic can be shared everywhere."

In my patch, if there are 30 blackboxed call frames coming from the same source-mapped resource (e.g. react-dom.development.js, which is a real-world example), WI.debuggerManager.blackboxDataForSourceCode runs only once. 
Adding blackboxed getter on WI.CallFrame would make WI.debuggerManager.blackboxDataForSourceCode run 30 times.

This might not be very performance sensitive code but I wanted explain my reasoning.

>> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:79
>> +            renderCallFrames(WI.debuggerManager.groupBlackboxedCallFrames(callFrames), true);
> 
> It makes no sense to be for DebuggerManager to be involved here, all it's doing in the function is restructuring the array.
> 
> It would be better to define this helper on Array.prototype in WI.Utilities (Utilities.js) so that we can make this work for any array & predicate, and write unit tests for it.

You mean a generic array function to to restructure an array (without any blackboxing logic), right? This might be a good idea.

[Nikita Vasilyev]

Created attachment 413717 [details]
[Image] Mockup

Brian & Devin: is that what you had in mind? What do you think.

1st row: global toggle
2nd row: previous patch

[Devin Rousso]

Comment on attachment 412160 [details]
Patch

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

(In reply to Nikita Vasilyev from comment #13)
> Created attachment 413717 [details]
> [Image] Mockup
> 
> Brian & Devin: is that what you had in mind? What do you think.
> 
> 1st row: global toggle
> 2nd row: previous patch
I feel like we should show _something_ to indicate that there are blackboxed call frames so that the developer isn't clueless as to "how did I get from the above call frame to here" (i.e. some indication that work happened between the two non-blackboxed call frames).

The idea I had in mind was sorta in-between these two ideas:

    [f] onInput - myApp.js
    👁 Blackboxed — 5 call frames <button>Reveal</button>
    [f] render - myApp.js

where clicking the <button>Reveal</button> will replace that `WI.TreeElement` with the blackboxed call frames

    [f] onInput - myApp.js
    [f] dispatchEvent — react.js
    [f] unstable_runWithPriority — react.js
    [f] runWithPriority$1 — react.js
    [f] discreteUpdates$1 — react.js
    [f] discreteUpdates — react.js
    [f] render - myApp.js

I'd imagine that these new blackboxed call frames would have the same UI as we show now (i.e. slightly transparent).

I'm not entirely sure if/how we'd wanna make that action persistent (e.g. should Web Inspector remember to show those blackboxed call frames so long as the call stack above those blackboxed call frames is the same, should Web Inspector reset it each time the developer steps, etc.), but that might be something to discover after living on this for a bit.

>>>> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:63
>>>> +                    let blackboxedTreeElement = new WI.GeneralTreeElement("blackboxed-group", title);
>>> 
>>> It's going to be a lot easier to iterate on this if we have a TreeElement subclass for the blackboxed call frame group.
>> 
>> +1 to this
>> 
>> Also, both `WI.CallFrameView` and `WI.CallFrameTreeElement` query for `WI.debuggerManager.blackboxDataForSourceCode(callFrame.sourceCodeLocation.sourceCode)`.  Perhaps we should just move that to a `this._blackboxed` in `WI.CallFrame` (exposed via `get blackboxed`) so that this same logic can be shared everywhere.  Furthermore, by doing this it should make the logic of grouping blackboxed call frames together much easier as all you'd need to do is save sequential `WI.CallFrame` into a separate array
>> ```
>>     let blackboxedCallFrames = [];
>>     for (let callFrame of callFrames) {
>>         if (callFrame.blackboxed) {
>>             blackboxedCallFrames.push(callFrame);
>>             continue;
>>         }
>> 
>> 
>>         if (blackboxedCallFrames.length)
>>             this.appendChild(new WI.BlackboxedCallFramesPlaceholderTreeElement(blackboxedCallFrames.splice()));
>> 
>>         ...
>>     }
>> ```
> 
> "Perhaps we should just move that to a `this._blackboxed` in `WI.CallFrame` (exposed via `get blackboxed`) so that this same logic can be shared everywhere."
> 
> In my patch, if there are 30 blackboxed call frames coming from the same source-mapped resource (e.g. react-dom.development.js, which is a real-world example), WI.debuggerManager.blackboxDataForSourceCode runs only once. 
> Adding blackboxed getter on WI.CallFrame would make WI.debuggerManager.blackboxDataForSourceCode run 30 times.
> 
> This might not be very performance sensitive code but I wanted explain my reasoning.

True, but each `WI.CallFrameTreeElement` already has to query for `WI.debuggerManager.blackboxDataForSourceCode` anyways, so you'd potentially be eliminating extra work.

If you're really worried about the performance aspect (IMO this is probably not an issue), you could make it into a parameter for `WI.CallFrame` and have this logic be inside `WI.DebuggerManager` when the `WI.CallFrame` is created instead (that way it can be tested too).

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #14)
> Comment on attachment 412160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412160&action=review
> 
> (In reply to Nikita Vasilyev from comment #13)
> > Created attachment 413717 [details]
> > [Image] Mockup
> > 
> > Brian & Devin: is that what you had in mind? What do you think.
> > 
> > 1st row: global toggle
> > 2nd row: previous patch
> I feel like we should show _something_ to indicate that there are blackboxed
> call frames so that the developer isn't clueless as to "how did I get from
> the above call frame to here" (i.e. some indication that work happened
> between the two non-blackboxed call frames).

I agree.

> 
> The idea I had in mind was sorta in-between these two ideas:
> 
>     [f] onInput - myApp.js
>     👁 Blackboxed — 5 call frames <button>Reveal</button>
>     [f] render - myApp.js
> 
> where clicking the <button>Reveal</button> will replace that
> `WI.TreeElement` with the blackboxed call frames
> 
>     [f] onInput - myApp.js
>     [f] dispatchEvent — react.js
>     [f] unstable_runWithPriority — react.js
>     [f] runWithPriority$1 — react.js
>     [f] discreteUpdates$1 — react.js
>     [f] discreteUpdates — react.js
>     [f] render - myApp.js
> 
> I'd imagine that these new blackboxed call frames would have the same UI as
> we show now (i.e. slightly transparent).

This sounds fine to me. I'm not sold on the reveal button though.

With the disclosure triangles we provide a familiar interface to show more items. It's already keyboard accessible — right arrow key expands the subtree. It looks to me that we're trying hard to avoid nesting, and the price we're paying is a new unfamiliar UI to expand items.

[Nikita Vasilyev]

Comment on attachment 412160 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:558
>> +        }
> 
> This function would be more naturally expressed using Array.prototype.splice() once the start/end index of the blackboxed call frame range has been found. Then we can get rid of groupedCallFrames altogether. It would be good to copy the `callFrames` parameter using callFrames.slice() to avoid mutating the original.

To me, the line below looks harder to read than the one above:

    callFrames.splice(blackboxFrameStartIndex, (blackboxFrameEndIndex - blackboxFrameStartIndex), callFrames.slice(blackboxFrameStartIndex, blackboxFrameEndIndex))

>> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:111
>> +            if (WI.settings.experimentalCollapseBlackboxedCallFrames.value)
> 
> To avoid duplication and unnecessary detail in this already large function, please put the settings check inside renderCallFrames()

This makes a lot of sense :)

[Nikita Vasilyev]

Created attachment 413861 [details]
Patch

[Nikita Vasilyev]

Created attachment 413863 [details]
Patch

[Nikita Vasilyev]

Created attachment 413865 [details]
[Image] After

[Nikita Vasilyev]

Created attachment 413867 [details]
Patch

[Devin Rousso]

Comment on attachment 413867 [details]
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.sourceCode);

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

[Nikita Vasilyev]

Comment on attachment 413867 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
>> +        return !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode);
> 
> 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.

I'm not sure what you're suggesting. Do you want to add `this._blackboxed = !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode)` into CallFrame constructor method?

[Nikita Vasilyev]

Comment on attachment 413867 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1643
>> +        for (let i = 0; i < this.length; ++i) {
> 
> NIT: Do we care about arrays with holes?

Array with holes, such as [1, , 3]? Don't they act the same as [1, undefined, 3]? I'm not sure what you're implying; I might not be aware of the arrays with holes issue.

>> 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.

It isn't useful to hide one blackboxed call frame because it doesn't save any space.

Initially I had this number set to 2. Greg suggested bumping it up. I've used Web Inspector for some time with `minGroupSize = 4` and I've grown to like it. It's fairly common to have blackboxed call frames mixed with native call frames (e.g. of bound functions) which sometimes creates a few small blackboxed groups. Both Greg and BJ think it's useful for `minGroupSize` to be >2.

[Nikita Vasilyev]

Created attachment 415612 [details]
Patch

[Nikita Vasilyev]

Created attachment 415613 [details]
[Video] With patch applied

Note that the eye icon is no longer present before the "Blackboxed" text. I removed it as per BJ's suggestion. From the following discussions with Greg and BJ, we intent to use a different icon, such as a box. The icon isn't ready so currently there's no icon there.

[Nikita Vasilyev]

Created attachment 415614 [details]
Patch

Rebaselined.

[Nikita Vasilyev]

Created attachment 415783 [details]
Patch

[Nikita Vasilyev]

Created attachment 415786 [details]
[Image] With patch applied

I styled "Blackboxed — N call frames" the same as the existing blackboxed call frame items - by adding `opacity: 0.5`.

I didn't make text italic because on macOS it's almost never used.

[Blaze Burg]

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:208
> +localizedStrings["Blackboxed @ Debugger Call Stack"] = "Blackboxed";

Nit: the @ part should explain where to find the string: '@ call stack when paused in Sources Tab'

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1627
> +Object.defineProperty(Array.prototype, "groupBy",

A plain English description of what this does would be useful as a comment.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1629
> +    value(groupFunction, minGroupSize)

minGroupSize seems like an optional parameter, thus it should have a default value.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:31
> +

This looks weird.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:53
> +        this.listItemElement.addEventListener("mousedown", (event) => {

Please move the listener to its own method since it doesn't need to capture anything in onattach().

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

I feel like this constant should be available on WI.BlackboxedGroupTreeElement. I wouldn't look here in ThreadTreeElement if I had to update the value.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:78
> +        renderCallFrames(callFrames, true);

Generally speaking, we try to avoid bool parameters that could just as easily be put into an {options} = {} parameter. Not as important here since it's a local function, but it did throw me off.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1104
> +WI.TreeOutline.DepthPadding = 10;

This name could be improved. Without finding all uses, I'm not sure what 'depth' refers to, nor what is being padded. Maybe WI.TreeOutline.LeadingIndentationOffset or something?

[Devin Rousso]

Comment on attachment 413867 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1643
>>> +        for (let i = 0; i < this.length; ++i) {
>> 
>> NIT: Do we care about arrays with holes?
> 
> Array with holes, such as [1, , 3]? Don't they act the same as [1, undefined, 3]? I'm not sure what you're implying; I might not be aware of the arrays with holes issue.

```
let x = [];
x[0] = 0;
x[42] = 42;
console.log(x.length);   // 43
x.forEach(console.log); // this only logs 0 and 42
```

Because you're traversing based on `length`, you'll end up iterating over values that may not actually exist.  To be technically correct, you should add a `this.hasOwnProperty(i)` check.

>>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
>>> +        return !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode);
>> 
>> 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.
> 
> I'm not sure what you're suggesting. Do you want to add `this._blackboxed = !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode)` into CallFrame constructor method?

Either in the constructor or as another argument after `isTailDeleted` (which frankly we should probably combine into an `options = {}` at this point).

>>> 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.
> 
> It isn't useful to hide one blackboxed call frame because it doesn't save any space.
> 
> Initially I had this number set to 2. Greg suggested bumping it up. I've used Web Inspector for some time with `minGroupSize = 4` and I've grown to like it. It's fairly common to have blackboxed call frames mixed with native call frames (e.g. of bound functions) which sometimes creates a few small blackboxed groups. Both Greg and BJ think it's useful for `minGroupSize` to be >2.

It's not about saving space.  It's about being consistent.  If the developer decides to blackbox something, we should _always_ show it as being blackboxed, not based on how many previous/subsequent blackboxed call frames there are.  It could cause confusion for the developer if they set up a blackbox and then only sometimes see the "Blackboxed - %d Call Frames" text.

Also again if the developer has set up a blackbox, that should be an indication for us that they do not want to see those call frames at all.  We should default to the behavior of hiding as much info about any blackboxed call frames as possible regardless of whether or not it saves space (which it will for >1 sequential call frame).  The only reason we provide the ability to show the blackboxed call frames is on the off-chance that the developer does want to delve into the code for some reason, which I imagine is going to be extremely rare.

[Devin Rousso]

Comment on attachment 415783 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:207
> +/* Blackboxed - N call frames */

this needs a better comment

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1559
> +/* Blackboxed - N call frames */

this needs a better comment

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:26
> +.blackboxed-group {

Please prefix this these `.tree-outline .item` so that there's less of a chance of these styles accidentally conflicting with anything in the future.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:30
> +.blackboxed-group:not(.selected) {

IMO it's odd that using ↑/↓ actually does select it.  I would expect ↑/↓ to basically skip over it (just like how we skip over async boundaries).  I suppose we want to be able to show blackboxed frames via keyboard navigation, but it still feels kinda weird because the main content area doesn't change when its selected 🤔

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:39
> +    content: "";

This looks really weird.  I feel like we either need some sort of icon (maybe a transparent version of the eye?) or need to style the text differently (maybe italics?) so that this looks different from the rest of the items.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:43
> +    -webkit-margin-start: var(--tree-outline-icon-margin-start);

`margin-inline-start`

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:30
> +        super("");

Literals should use a `const` variable when it's not clear what they do.
```
    const title = "";
```

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:32
> +        this._callFrames = callFrames;

We should add
```
    console.assert(Array.isArray(callFrames) && callFrames.length && callFrames.every((callFrame) => callFrame instanceof WI.CallFrame);
```
before `super(title);`.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:39
> +        // Don't allow collapsing.

`console.assert(false, "Should not be able to collapse a blackboxed group.");`

Also, is it actually even possible to reach this?

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:42
> +    // Overrides from TreeElement (Private)

This should just be `// Protected`.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:53
> +        this.listItemElement.addEventListener("mousedown", (event) => {

This shouldn't be inline since it doesn't need any capture.

Also, is this actually necessary?  I think you can use `toggleOnClick` instead.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:62
> +        iconElement.className = WI.GeneralTreeElement.IconElementStyleClassName;

Can we subclass `WI.GeneralTreeElement` instead of using its CSS classes and mimicking its DOM structure?

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:86
> +        this.selectable = false;

Why not do this in the constructor? (I think you may need `toggleOnClick`)

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:91
> +            let treeOutlineItemPadding = window.getComputedStyle(this.childrenListNode).getPropertyValue("--tree-outline-item-padding");
> +            let treeOutlineItemPaddingNewValue = parseInt(treeOutlineItemPadding) - WI.TreeOutline.DepthPadding;
> +            this.childrenListNode.style.setProperty("--tree-outline-item-padding", `${treeOutlineItemPaddingNewValue}px`);

This seems exceptionally hacky.  Instead of adding the `WI.CallFrameTreeElement` to this as children, we should lazily create them in `onexpand` and then do something like `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` and then `this.parent.removeChild(this)`.  This will let us avoid having to muck with CSS and keep the state of the `WI.TreeOutline` clean since the `WI.BlackboxedGroupTreeElement` will no longer be in the DOM (it will also let you avoid the `.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon` CSS rule because `WI.BlackboxedGroupTreeElement` will no longer be a `.parent`).

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:405
> +        experimentalSettingsView.addSetting(WI.UIString("Debugging:", "Debugging: @ Experimental Settings", "Category label for experimental settings related to debugging."), WI.settings.experimentalCollapseBlackboxedCallFrames, WI.UIString("Collapse blackboxed call frames", "Collapse blackboxed call frames @ Experimental Settings", "Setting to collapse blackboxed call frames in the debugger."));

We should also `listenForChange(WI.settings.experimentalCollapseBlackboxedCallFrames);` so that we're consistent with all the other experimental settings.

Also, we should have this behind `if (WI.DebuggerManager.supportsBlackboxingScripts())`.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:62
> +                let isBlackboxed = (callFrame) => callFrame.blackboxed;

NIT: This could be inlined.

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

NIT: I feel like this should be switched because the blackboxes-group path is less code (and less indentation is better IMO).

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:106
> +            let callFrames = startIndex ? currentStackTrace.callFrames.slice(startIndex) : currentStackTrace.callFrames;

NIT: This could be inlined.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:107
> +            renderCallFrames(callFrames, false);

Style: The `false` is unnecessary.

[Nikita Vasilyev]

Comment on attachment 415783 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1559
>> +/* Blackboxed - N call frames */
> 
> this needs a better comment

Note that the most important thing for localizers is verbatim surrounding text. Here, it's important to know that there's "Blackboxed —" before "%d call frames". What are you suggesting?

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:30
>> +.blackboxed-group:not(.selected) {
> 
> IMO it's odd that using ↑/↓ actually does select it.  I would expect ↑/↓ to basically skip over it (just like how we skip over async boundaries).  I suppose we want to be able to show blackboxed frames via keyboard navigation, but it still feels kinda weird because the main content area doesn't change when its selected 🤔

Yes, I do think we should provide a way to show blackboxed frames via keyboard. I'd prefer selecting the 1st blackboxed call frame on expansion rather getting rid of keyboard navigation altogether.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:39
>> +        // Don't allow collapsing.
> 
> `console.assert(false, "Should not be able to collapse a blackboxed group.");`
> 
> Also, is it actually even possible to reach this?

Yes, by pressing left arrow key. It's expected not to collapse so I don't think `console.assert` is needed.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:62
>> +        iconElement.className = WI.GeneralTreeElement.IconElementStyleClassName;
> 
> Can we subclass `WI.GeneralTreeElement` instead of using its CSS classes and mimicking its DOM structure?

Subclassing GeneralTreeElement was my 1st stub at it, actually, and it was more complex. GeneralTreeElement has `_disclosureButton`, and this one doesn't.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:86
>> +        this.selectable = false;
> 
> Why not do this in the constructor? (I think you may need `toggleOnClick`)

I want it to be selectable by up/down keys, not just by mouse clicking.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:91
>> +            this.childrenListNode.style.setProperty("--tree-outline-item-padding", `${treeOutlineItemPaddingNewValue}px`);
> 
> This seems exceptionally hacky.  Instead of adding the `WI.CallFrameTreeElement` to this as children, we should lazily create them in `onexpand` and then do something like `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` and then `this.parent.removeChild(this)`.  This will let us avoid having to muck with CSS and keep the state of the `WI.TreeOutline` clean since the `WI.BlackboxedGroupTreeElement` will no longer be in the DOM (it will also let you avoid the `.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon` CSS rule because `WI.BlackboxedGroupTreeElement` will no longer be a `.parent`).

`this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` — this seems to me just a different kind of hacky.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:405
>> +        experimentalSettingsView.addSetting(WI.UIString("Debugging:", "Debugging: @ Experimental Settings", "Category label for experimental settings related to debugging."), WI.settings.experimentalCollapseBlackboxedCallFrames, WI.UIString("Collapse blackboxed call frames", "Collapse blackboxed call frames @ Experimental Settings", "Setting to collapse blackboxed call frames in the debugger."));
> 
> We should also `listenForChange(WI.settings.experimentalCollapseBlackboxedCallFrames);` so that we're consistent with all the other experimental settings.
> 
> Also, we should have this behind `if (WI.DebuggerManager.supportsBlackboxingScripts())`.

Oh, thanks.

[Devin Rousso]

Comment on attachment 415783 [details]
Patch

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

>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1559
>>> +/* Blackboxed - N call frames */
>> 
>> this needs a better comment
> 
> Note that the most important thing for localizers is verbatim surrounding text. Here, it's important to know that there's "Blackboxed —" before "%d call frames". What are you suggesting?

We almost always include information about where this can be found in the UI and what "kind" of display it has (e.g. title, label, etc.).  I don't think this comment is clear that "Blackboxed - %d call frames" is the literal text being shown or whether "Blackboxed - " is some sort of "category" for a bunch of localized strings.  I think a better comment would be something like (wording could be bikeshedded):
```
    WI.UIString("%d call frames", "%d call frames @ Blackboxed Group in Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in the debugger call stack when paused instead of subsequent call frames that have been blackboxed.");
```

>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:62
>>> +        iconElement.className = WI.GeneralTreeElement.IconElementStyleClassName;
>> 
>> Can we subclass `WI.GeneralTreeElement` instead of using its CSS classes and mimicking its DOM structure?
> 
> Subclassing GeneralTreeElement was my 1st stub at it, actually, and it was more complex. GeneralTreeElement has `_disclosureButton`, and this one doesn't.

The `_disclosureButton` is `display: none;` when there are no children.  I don't understand how it would've been more complex when you're effectively copying the DOM structure and CSS classes here (which could make modifying that same structure in `WI.GeneralTreeElement` more difficult in the future as the engineer may not know that this exists).

Also, we've been trying to move away from having constant variables for CSS classes.  This makes that more difficult.

>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:91
>>> +            this.childrenListNode.style.setProperty("--tree-outline-item-padding", `${treeOutlineItemPaddingNewValue}px`);
>> 
>> This seems exceptionally hacky.  Instead of adding the `WI.CallFrameTreeElement` to this as children, we should lazily create them in `onexpand` and then do something like `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` and then `this.parent.removeChild(this)`.  This will let us avoid having to muck with CSS and keep the state of the `WI.TreeOutline` clean since the `WI.BlackboxedGroupTreeElement` will no longer be in the DOM (it will also let you avoid the `.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon` CSS rule because `WI.BlackboxedGroupTreeElement` will no longer be a `.parent`).
> 
> `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` — this seems to me just a different kind of hacky.

How is that hacky?  We're using existing API to effectively replace one tree element with multiple.  We do this kind of thing in a few different places already in Web Inspector (both with `WI.TreeOutline` and with regular DOM nodes).

[Blaze Burg]

Comment on attachment 415783 [details]
Patch

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

>>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1559
>>>> +/* Blackboxed - N call frames */
>>> 
>>> this needs a better comment
>> 
>> Note that the most important thing for localizers is verbatim surrounding text. Here, it's important to know that there's "Blackboxed —" before "%d call frames". What are you suggesting?
> 
> We almost always include information about where this can be found in the UI and what "kind" of display it has (e.g. title, label, etc.).  I don't think this comment is clear that "Blackboxed - %d call frames" is the literal text being shown or whether "Blackboxed - " is some sort of "category" for a bunch of localized strings.  I think a better comment would be something like (wording could be bikeshedded):
> ```
>     WI.UIString("%d call frames", "%d call frames @ Blackboxed Group in Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in the debugger call stack when paused instead of subsequent call frames that have been blackboxed.");
> ```

Devin's suggestion looks good to me.

>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:30
>>> +.blackboxed-group:not(.selected) {
>> 
>> IMO it's odd that using ↑/↓ actually does select it.  I would expect ↑/↓ to basically skip over it (just like how we skip over async boundaries).  I suppose we want to be able to show blackboxed frames via keyboard navigation, but it still feels kinda weird because the main content area doesn't change when its selected 🤔
> 
> Yes, I do think we should provide a way to show blackboxed frames via keyboard. I'd prefer selecting the 1st blackboxed call frame on expansion rather getting rid of keyboard navigation altogether.

Since we don't have agreement on expected behavior, let's address this in a followup. It's not enabled by default, so this can be fixed whenever.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:39
>> +    content: "";
> 
> This looks really weird.  I feel like we either need some sort of icon (maybe a transparent version of the eye?) or need to style the text differently (maybe italics?) so that this looks different from the rest of the items.

Work on a new blackboxing icon is still underway. Let's address this in a followup. Nikita, please file a bug for this if you haven't already.

>>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:62
>>>> +        iconElement.className = WI.GeneralTreeElement.IconElementStyleClassName;
>>> 
>>> Can we subclass `WI.GeneralTreeElement` instead of using its CSS classes and mimicking its DOM structure?
>> 
>> Subclassing GeneralTreeElement was my 1st stub at it, actually, and it was more complex. GeneralTreeElement has `_disclosureButton`, and this one doesn't.
> 
> The `_disclosureButton` is `display: none;` when there are no children.  I don't understand how it would've been more complex when you're effectively copying the DOM structure and CSS classes here (which could make modifying that same structure in `WI.GeneralTreeElement` more difficult in the future as the engineer may not know that this exists).
> 
> Also, we've been trying to move away from having constant variables for CSS classes.  This makes that more difficult.

I agree with Devin. Rolling a separate tree element will cause accessibility issues (no ARIA roles) and code maintenance issues (when the CSS changes).

>>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:91
>>>> +            this.childrenListNode.style.setProperty("--tree-outline-item-padding", `${treeOutlineItemPaddingNewValue}px`);
>>> 
>>> This seems exceptionally hacky.  Instead of adding the `WI.CallFrameTreeElement` to this as children, we should lazily create them in `onexpand` and then do something like `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` and then `this.parent.removeChild(this)`.  This will let us avoid having to muck with CSS and keep the state of the `WI.TreeOutline` clean since the `WI.BlackboxedGroupTreeElement` will no longer be in the DOM (it will also let you avoid the `.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon` CSS rule because `WI.BlackboxedGroupTreeElement` will no longer be a `.parent`).
>> 
>> `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame), this.parent.children.indexOf(this))` — this seems to me just a different kind of hacky.
> 
> How is that hacky?  We're using existing API to effectively replace one tree element with multiple.  We do this kind of thing in a few different places already in Web Inspector (both with `WI.TreeOutline` and with regular DOM nodes).

I agree with Devin's suggestion for how to implement this.

As written, this is an undesirable way to set the padding, because it forces a sync style recalc on expand.

[Nikita Vasilyev]

Created attachment 416048 [details]
Patch

Addressed >95% of the review feedback.

BlackboxedGroupTreeElement is no longer keyboard accessible. Hopefully, I'll convince y'all to implement that later.

[Nikita Vasilyev]

Created attachment 416049 [details]
[Video] With patch applied

[Nikita Vasilyev]

Created attachment 416050 [details]
Patch

Rebaselined.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #30)
> Comment on attachment 413867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413867&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
> >>> +        return !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode);
> >> 
> >> 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.
> > 
> > I'm not sure what you're suggesting. Do you want to add `this._blackboxed = !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode)` into CallFrame constructor method?
> 
> Either in the constructor or as another argument after `isTailDeleted`
> (which frankly we should probably combine into an `options = {}` at this
> point).

I'm concerned about unnecessary performance hit in the areas where `blackboxed` getter is currently unused, such as Timelines and Network. This was one thing that I didn't change in the latest patch.

[Devin Rousso]

Comment on attachment 416050 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:8
> +        Stack trace often has dozens blackboxed call frames when using blackboxing for JS-frameworks such as React.js.

"Stack traces often have dozens of ..."

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1642
> +                result.push(...group);

we have a `pushAll` utility for this

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1649
> +        for (let i = 0; i < this.length; ++i) {
> +            if (!this.hasOwnProperty(i))
> +                continue;
> +
> +            let item = this[i];

NIT: another option could be to use
```
    this.forEach((item, i) => {
```

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:65
> +    get blackboxed()

(In reply to Nikita Vasilyev from comment #38)
> (In reply to Devin Rousso from comment #30)
> > Comment on attachment 413867 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=413867&action=review
> > 
> > >>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
> > >>> +        return !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode);
> > >> 
> > >> 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.
> > > 
> > > I'm not sure what you're suggesting. Do you want to add `this._blackboxed = !!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.sourceCode)` into CallFrame constructor method?
> > 
> > Either in the constructor or as another argument after `isTailDeleted` (which frankly we should probably combine into an `options = {}` at this point).
> 
> I'm concerned about unnecessary performance hit in the areas where `blackboxed` getter is currently unused, such as Timelines and Network. This was one thing that I didn't change in the latest patch.
That was why I suggested that you make it a parameter that's provided to the `WI.CallFrame` so that it's only calculated in `WI.DebuggerManager.prototype.debuggerDidPause`.

Also, if you're worried about performance (which again I don't think is really that big of a concern as I doubt most stacks are going to get that deep), at the absolute minimum this value should be memoized so we don't have to recompute it each time `get blackboxed` is invoked.  I would strongly suggest the above though as that avoids any chance of `get blackboxed` being invoked "too late" (e.g. the user changes the blackbox after `WI.CallFrame` is instantiated but before `get blackboxed` is called).

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:31
> +    content: "";

Please include a `FIXME: <https://webkit.org/b/######> ...` comment here for a bug that will add the icon.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:36
> +.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon {
> +    margin-inline-start: var(--tree-outline-icon-margin-start);
> +}

Is this still needed?

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:33
> +        let title = WI.UIString("Blackboxed", "Blackboxed @ Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in the debugger call stack when paused instead of subsequent call frames that have been blackboxed.");

We need a singular version of this.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:34
> +        let classNames = ["blackboxed-group"];
> +        let title = WI.UIString("Blackboxed", "Blackboxed @ Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in the debugger call stack when paused instead of subsequent call frames that have been blackboxed.");
> +        let subtitle = WI.UIString("%d call frames", "call frames @ Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in the debugger call stack when paused instead of subsequent call frames that have been blackboxed.").format(callFrames.length);

Style: These should be `const`.

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:52
> +    toggleOnClick(event)

`toggleOnClick` is expected to be (and is used as) a property just like `selectable`.

> Source/WebInspectorUI/UserInterface/Views/CallFrameView.css:35
> +    opacity: var(--blackboxed-tree-item-opacity);

We don't need to do this for only one usage.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:327
> +        if (treeElement.treeOutline && !treeElement.treeOutline.selectable)

I'd include a comment in the ChangeLog explaining that this is necessary since `WI.BlackboxedGroupTreeElement` removes itself when expanded.

[Nikita Vasilyev]

Created attachment 416060 [details]
Patch

[Devin Rousso]

Comment on attachment 416060 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:218
>      static fromPayload(target, payload)

This is called by `WI.StackTrace.fromPayload`, which is called by `WI.DebuggerManager.prototype.debuggerDidPause`.  Do we need that path as well?

[Nikita Vasilyev]

Comment on attachment 416060 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:218
>>      static fromPayload(target, payload)
> 
> This is called by `WI.StackTrace.fromPayload`, which is called by `WI.DebuggerManager.prototype.debuggerDidPause`.  Do we need that path as well?

`let asyncStackTrace = WI.StackTrace.fromPayload(target, asyncStackTracePayload);`

This one, right? Hm, somehow blackboxing for async stack traces works already.

[Nikita Vasilyev]

Comment on attachment 416060 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:218
>>>      static fromPayload(target, payload)
>> 
>> This is called by `WI.StackTrace.fromPayload`, which is called by `WI.DebuggerManager.prototype.debuggerDidPause`.  Do we need that path as well?
> 
> `let asyncStackTrace = WI.StackTrace.fromPayload(target, asyncStackTracePayload);`
> 
> This one, right? Hm, somehow blackboxing for async stack traces works already.

My bad, it didn't work. Updating patch...

[Nikita Vasilyev]

Created attachment 416070 [details]
Patch

[Devin Rousso]

Comment on attachment 416070 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:64
> +    get blackboxed() { return this._blackboxed; }

Please create a followup bug for updating other callsites of `WI.debuggerManager.blackboxDataForSourceCode` to use this instead.  I believe there are three right now:
 - `WI.StackTrace.prototype.get firstNonNativeNonAnonymousNotBlackboxedCallFrame`
 - `WI.CallFrameTreeElement`
 - `WI.CallFrameView`

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:214
> +        let blackboxed = sourceCodeLocation ? !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode) : false;

`sourceCodeLocation && !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode)`

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:259
> +        let blackboxed = sourceCodeLocation ? !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode) : false;

ditto (:214)

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:33
> +    content: "";

We should include some sort of icon here in the meantime so that if the followup gets deferred or rolled out for some reason then the UI isn't left in a bad state.  Usually I use `TypeIcons.svg#Source-{light,dark}` for temporary things like this.

[Nikita Vasilyev]

Created attachment 419213 [details]
Patch

Addressed comments and rebaselined.

[Nikita Vasilyev]

Comment on attachment 416070 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:64
>> +    get blackboxed() { return this._blackboxed; }
> 
> Please create a followup bug for updating other callsites of `WI.debuggerManager.blackboxDataForSourceCode` to use this instead.  I believe there are three right now:
>  - `WI.StackTrace.prototype.get firstNonNativeNonAnonymousNotBlackboxedCallFrame`
>  - `WI.CallFrameTreeElement`
>  - `WI.CallFrameView`

Bug 221373 - Web Inspector: refactoring: CallFrame.prototype.blackboxed instead of WI.debuggerManager.blackboxDataForSourceCode

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:214
>> +        let blackboxed = sourceCodeLocation ? !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode) : false;
> 
> `sourceCodeLocation && !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode)`

Fixed.

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:259
>> +        let blackboxed = sourceCodeLocation ? !!WI.debuggerManager.blackboxDataForSourceCode(sourceCodeLocation.sourceCode) : false;
> 
> ditto (:214)

Fixed.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:33
>> +    content: "";
> 
> We should include some sort of icon here in the meantime so that if the followup gets deferred or rolled out for some reason then the UI isn't left in a bad state.  Usually I use `TypeIcons.svg#Source-{light,dark}` for temporary things like this.

I intent to use the question mark box icon. If we decide to use a different icon, we can do it as a follow up.

[Devin Rousso]

Comment on attachment 419213 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:32
> +    content: url(../Images/TypeIcons.svg#TypeUndefined-light);

did you mean `TypeIcons.svg#Blackboxed-light`?

> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:37
> +        content: url(../Images/TypeIcons.svg#TypeUndefined-dark);

did you mean `TypeIcons.svg#Blackboxed-dark`?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #48)
> Comment on attachment 419213 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419213&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:32
> > +    content: url(../Images/TypeIcons.svg#TypeUndefined-light);
> 
> did you mean `TypeIcons.svg#Blackboxed-light`?
> 
> > Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:37
> > +        content: url(../Images/TypeIcons.svg#TypeUndefined-dark);
> 
> did you mean `TypeIcons.svg#Blackboxed-dark`?

Yes 🤦‍♂️

[Nikita Vasilyev]

Created attachment 419247 [details]
Patch

[EWS]

Committed r272371: <https://trac.webkit.org/changeset/272371>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419247 [details].