1455750 - Provide method for server to skip pausing

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[David Walsh :davidwalsh]

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.

Comment 1

[David Walsh :davidwalsh]

Attachment: test.patch

Adding patch for server and WIP test.

Comment 2

[Jason Laster [:jlast]]

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

Comment 3

[David Walsh :davidwalsh]

Attachment: 1455750.patch

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.

Comment 4

[Jason Laster [:jlast]]

Attachment: Bug 1455750 - Provide method for server to skip pausing.

Review commit: https://reviewboard.mozilla.org/r/243870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243870/

Comment 5

[Jason Laster [:jlast]]

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 6

[Jason Laster [:jlast]]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243870/diff/1-2/

Comment 7

[Jim Blandy :jimb]

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.

Comment 8

[Jason Laster [:jlast]]

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.

Comment 9

[Jim Blandy :jimb]

Yes, that sounds great.

Comment 10

[Jim Blandy :jimb]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

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

Comment 11

[Jason Laster [:jlast]]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243870/diff/2-3/

Comment 12

[Jim Blandy :jimb]

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

Comment 13

[Jason Laster [:jlast]]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243870/diff/3-4/

Comment 14

[Jason Laster [:jlast]]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243870/diff/4-5/

Comment 15

[Jim Blandy :jimb]

Comment on attachment 8975613 [details]
Bug 1455750 - Provide method for server to skip pausing.

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

Looks great!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/135af0c54dd7
Provide method for server to skip pausing. r=jimb

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/135af0c54dd7