Bug 234614 – Web Inspector: Sources: expanding a grouping of blackboxed call frames should be persistent

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

[Devin Rousso]

# STEPS TO REPRODUCE
1. inspect <http://bogojoker.com/shell/>
2. create a "click" global Event Breakpoint
3. blackbox the <http://bogojoker.com/shell/js/jquery.js> script
4. click on the down arrow in the page
-> should pause on <http://bogojoker.com/shell/js/easySlider.min.js:64> with two blackboxed call frames
5. click on the "Blackboxed - 2 call frames" to expand
-> should see two call frames in jquery.js
6. Step into

## EXPECTED
the two jquery call frames remain as they are (i.e. are not re-collapsed)

## ACTUAL
the two jquery call frames are re-collapsed


# DISCUSSION

If the developer has explicitly decided to show blackboxed call frames, we should respect that decision.  Requiring them to re-expand after _every_ debugger action is very hostile.  I think so long as the blackboxed call frames remain the same we should persist the expansion.  Only if the location of those blackboxed frames changes should we re-collapse.

[Radar WebKit Bug Importer]

<rdar://problem/86989232>

[Devin Rousso]

Created attachment 448324 [details]
Patch

[Patrick Angle]

Comment on attachment 448324 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:30
> +        (WI.ThreadTreeElement.prototype.refresh):

Can you add a line or two here describing the refactoring you did so that it is easier to understand at a quick glance what is happening now.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:556
> +        for (let blackboxedCallFrameGroupToAutoExpand of this._blackboxedCallFrameGroupsToAutoExpand) {
> +            if (blackboxedCallFrameGroupToAutoExpand.length !== blackboxedCallFrameGroup.length)
> +                continue;
> +
> +            for (let i = 0; i < blackboxedCallFrameGroupToAutoExpand.length; ++i) {
> +                if (!blackboxedCallFrameGroupToAutoExpand[i].isEqual(blackboxedCallFrameGroup[i]))
> +                    return false;
> +            }
> +
> +            return true;
> +        }

This doesn't look quite right...If we have expanded two sets of blackboxed frames, each with the same number of frames, we will do the right thing for the first set of frames, but for the second set of frames my understanding here is that we would pass the check on :547 when comparing the second set of frames with the remembered first set of frames, and then proceed to check that the frames match between the two sets, and when they don't we end up returning instead of checking the second set of remembered frames. I think :552 needs to continue the outer loop with some additional piece of state.

[Devin Rousso]

Comment on attachment 448324 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:556
>> +        }
> 
> This doesn't look quite right...If we have expanded two sets of blackboxed frames, each with the same number of frames, we will do the right thing for the first set of frames, but for the second set of frames my understanding here is that we would pass the check on :547 when comparing the second set of frames with the remembered first set of frames, and then proceed to check that the frames match between the two sets, and when they don't we end up returning instead of checking the second set of remembered frames. I think :552 needs to continue the outer loop with some additional piece of state.

Oh wow great catch.  Yeah I think :552 should be a `continue`.

[Devin Rousso]

Created attachment 448330 [details]
Patch

[Patrick Angle]

Comment on attachment 448330 [details]
Patch

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

rs=me

> Source/WebInspectorUI/ChangeLog:40
> +        Add a convenience function to compare two `WI.CallFrame`. Right now it just looks as the

Nit: s/as/at

[Devin Rousso]

Created attachment 448334 [details]
Patch

[EWS]

Committed r287590 (245719@main): <https://commits.webkit.org/245719@main>

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