Provide method for server to skip pausing

RESOLVED FIXED in Firefox 62

Status

P2
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: davidwalsh, Assigned: davidwalsh83)

Tracking

unspecified
Firefox 62

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 months ago
Relates to:

https://github.com/devtools-html/debugger.html/pull/5998

We need method to signal to the server that pausing should be disabled based on a UI component.
(Reporter)

Comment 1

11 months ago
Posted patch test.patch (obsolete) — Splinter Review
Adding patch for server and WIP test.
looking good!

I had a bit of downtime at the airport, so i pushed it a bit further:

https://gist.github.com/jasonLaster/c8ff9b043ee7ad35fbbdd6e5b1c7ffd2
https://gist.github.com/jasonLaster/3269714ec81cb0bbfd24af037a4d6142

^ not sure which is easier to pull down

I think the callback approach is totally valid too, just had this approach in my head so i ran with it.
My concern with using the pauses counter is that this code could be async and so pauses 0 might be true even if it was going to paused pretty soon. I'm not really sure, I also think the executeSoon is important, but i dont really know why. A lot of this is a mystery for me too.

This test passes for me locally, I was not able to do the 3rd part where I tested skipping a breakpoint...
(Reporter)

Comment 3

11 months ago
Posted patch 1455750.patchSplinter Review
I've updated the patch to use consistent naming (always "paus*ing*" instead of "paus*es*".  I've also removed a few debug statements.

Notable is my change to breakpoints-02.js; without the `onSkipPausing` addition, the test breaks.  Adding it and then doing a second execution does not break, thus skipPausing is doing what it's meant to.
Attachment #8969797 - Attachment is obsolete: true
Attachment #8970232 - Flags: review?(jlaster)

Updated

11 months ago
Assignee: nobody → davidwalsh83
Priority: -- → P2
Comment hidden (mozreview-request)
Jim, I just created https://reviewboard.mozilla.org/r/243870/ to get your thoughts on a skip pausing flag which would prevent the debugger from pausing for breakpoints and debugger statements. 


The use case is just that you want to do some setup work quickly without pausing, then when you're good, you re-enable pausing, and everything should just work.

I was on the fence about adding skipping for stepping, but i think it is good to be consistent.
Comment hidden (mozreview-request)

Comment 7

10 months ago
It seems to me that when the user presses the 'step in' or 'step over' buttons, they've indicated exactly how they want the debugger to behave. If they wanted it to continue, they could have pressed that button.

A way to temporarily disable all breakpoints makes plenty of sense, on the other hand. That would be a much simpler patch to review. So I guess I just want to confirm that affecting all these other sorts of pauses is really desirable before I review the patch.
Flags: needinfo?(jlaster)
i agree with that and would prefer a simpler patch. 

how does it sound to limit it to breakpoints, debugger statements, and exception unwinds, which would all be things the user might want to temporarily ignore.
Flags: needinfo?(jlaster)

Comment 9

10 months ago
Yes, that sounds great.

Comment 10

10 months ago
mozreview-review
Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

https://reviewboard.mozilla.org/r/243870/#review250486
Attachment #8975613 - Flags: review?(jimb)
Comment hidden (mozreview-request)

Comment 12

10 months ago
mozreview-review
Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

https://reviewboard.mozilla.org/r/243870/#review250498

::: devtools/server/actors/thread.js:1521
(Diff revision 3)
> -      : this._pauseAndRespond(frame, { type: "debuggerStatement" });
> +    }
> +
> +    return this._pauseAndRespond(frame, { type: "debuggerStatement" });
> +  },
> +
> +  onSkipPausing: function({ skip }) {

Since its role is reduced, 'skipPausing' doesn't seem like the right name for this any more. How about 'skipBreakpoints'? (debugger statements are really just a kind of manual breakpoint.)
Attachment #8975613 - Flags: review?(jimb)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

10 months ago
mozreview-review
Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

https://reviewboard.mozilla.org/r/243870/#review252714

Looks great!
Attachment #8975613 - Flags: review?(jimb) → review+

Comment 16

10 months ago
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/135af0c54dd7
Provide method for server to skip pausing. r=jimb

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/135af0c54dd7
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

9 months ago
Product: Firefox → DevTools
Attachment #8970232 - Flags: review?(jlaster)
You need to log in before you can comment on or make changes to this bug.