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
Created attachment 449273 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 449274 [details] Patch
Created attachment 449275 [details] Patch
Created attachment 449278 [details] Patch
Comment on attachment 449278 [details] Patch oops wrong bug
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.
Created attachment 449700 [details] Patch
<rdar://problem/87930496>
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?
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!
Created attachment 450950 [details] Patch
Comment on attachment 450950 [details] Patch I've re-reviewed since you've been having trouble finding a reviewer. rs=me.
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].