Bug 235274 – Web Inspector: add a switch to control whether breakpoint evaluations (condition, ignore count, actions) are also affected by script blackboxing

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

[Devin Rousso]

being able to defer breakpoint evaluations until the next actual pause can sometimes be far more useful than doing the breakpoint evaluations at the breakpoint's original location

[Devin Rousso]

Created attachment 449273 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 449274 [details]
Patch

[Devin Rousso]

Created attachment 449275 [details]
Patch

[Devin Rousso]

Created attachment 449278 [details]
Patch

[Devin Rousso]

Comment on attachment 449278 [details]
Patch

oops wrong bug

[Devin Rousso]

Comment on attachment 449275 [details]
Patch

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

> Source/JavaScriptCore/debugger/Debugger.cpp:884
>      bool pauseNow = m_pauseAtNextOpportunity;

I think this might need to be updated to include `m_pauseOnStepNext` and `m_pauseOnStepOut` (and maybe `m_pauseOnCallFrame` for Step out) as possible signals to `pauseNow` as well.  We should also probably only consider `m_pauseAtNextOpportunity` if we're not `m_afterBlackboxedScript` either.  That's the only way for sure to know if we're stepping vs pausing because of a breakpoint.

We shouldn't have to worry about things like "Continue to location" because that breakpoint is not configurable, and therefore can't be marked as auto-continue.

[Devin Rousso]

Created attachment 449700 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/87930496>

[Patrick Angle]

Comment on attachment 449700 [details]
Patch

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

Frontend changes LGTM with a nit and a question. Backend changes also look good to me, but is outside of my comfort zone to "r+".

> Source/JavaScriptCore/debugger/Debugger.cpp:890
> +        if (!afterBlackboxedScript)
> +            didPauseForStep = true;

Nit: `didPauseForStep = !afterBlackboxedScript;`?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:1265
> +        // COMPATIBILITY (iOS 15): Debugger.setBlackboxBreakpointEvaluations did not exist yet.

Nit: "iOS 15.4"

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:113
> +        let blackboxBreakpointEvaluationsExplanationCheckbox = blackboxBreakpointEvaluationsExplanationLabel.appendChild(document.createElement("input"));

Not sure if there is a precedent for what I'm about to suggest, but we should consider not showing this option when inspecting a pre-iOS 15.4 target. It might be a bit odd for this setting to only sometimes be here, but if someone is using the setting they might be more confused if the setting is here but doesn't do anything. At least if the setting they previously toggled while inspecting a different target is not there in an instance targeting a pre-iOS 15.4 debuggable it might provide a better clue as to why breakpoints aren't working they way the user might have expected. Thoughts?

[Devin Rousso]

Comment on attachment 449700 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:113
>> +        let blackboxBreakpointEvaluationsExplanationCheckbox = blackboxBreakpointEvaluationsExplanationLabel.appendChild(document.createElement("input"));
> 
> Not sure if there is a precedent for what I'm about to suggest, but we should consider not showing this option when inspecting a pre-iOS 15.4 target. It might be a bit odd for this setting to only sometimes be here, but if someone is using the setting they might be more confused if the setting is here but doesn't do anything. At least if the setting they previously toggled while inspecting a different target is not there in an instance targeting a pre-iOS 15.4 debuggable it might provide a better clue as to why breakpoints aren't working they way the user might have expected. Thoughts?

Oh wow oops yeah I totally forgot to add a compatibility guard 🤦‍♂️  Good catch!

[Devin Rousso]

Created attachment 450950 [details]
Patch

[Patrick Angle]

Comment on attachment 450950 [details]
Patch

I've re-reviewed since you've been having trouble finding a reviewer. rs=me.

[EWS]

Committed r290720 (247967@main): <https://commits.webkit.org/247967@main>

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