1525685 - Remove breakpoint sliding code

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: server/client patch

Breakpoint sliding is not needed anymore now that the client knows where it is able to set breakpoints. It helps to get rid of this in preparation for removing breakpoint actors, as it is tricky to support breakpoint sliding when there is no actor for the breakpoint.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: server changes

Changes to the server and SourceClient. I'll make a PR for the debugger.html changes separately.

Comment 2

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9041892 [details] [diff] [review]
server changes

Review of attachment 9041892 [details] [diff] [review]:
-----------------------------------------------------------------

My only hesitation on this is wondering if we should wait for column breakpoints to be on by default. That said, it's probably fine. There are definitely cases when column breakpoints are disabled and this branch still runs if I remember right.

Are all the tests passing on this? I'm surprised none needed changes.

::: devtools/server/actors/source.js
@@ -748,5 @@
> -   *
> -   * @returns A Boolean that is true if the BreakpointActor was set as a
> -   *          breakpoint handler on at least one script, and false otherwise.
> -   */
> -  _setBreakpointAtGeneratedLocation: function(actor, generatedLocation) {

This is used in the thread actor as well.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #2)

Comment on attachment 9041892 [details] [diff] [review]
server changes

Review of attachment 9041892 [details] [diff] [review]:

My only hesitation on this is wondering if we should wait for column
breakpoints to be on by default. That said, it's probably fine. There are
definitely cases when column breakpoints are disabled and this branch still
runs if I remember right.

This change blocks bug 1524374, which is necessary for correct breakpoint behavior with workers. If we keep the sliding code around then I'll have to find some way of implementing it with the changes in bug 1524374, which isn't a good use of time if we don't actually need sliding anywhere.

Are all the tests passing on this? I'm surprised none needed changes.

I haven't run mochitests but I did have to update some unit tests in the client (PR #7895). I'll post another patch if any test changes are needed.

::: devtools/server/actors/source.js
@@ -748,5 @@

• • @returns A Boolean that is true if the BreakpointActor was set as a

• •      breakpoint handler on at least one script, and false otherwise.
    

• */
• _setBreakpointAtGeneratedLocation: function(actor, generatedLocation) {

This is used in the thread actor as well.

Oops, I'll update the patch. I have a WIP for bug 1524374 which removes the call in the thread actor, which can be done without using private methods of the source actor I think.

Comment 4

[Jason Laster [:jlast]]

_setBreakpointAtGeneratedLocation

This might need to be kept, but this looks good otherwise as long as tests are passing.

Comment 5

[Logan Smyth [:loganfsmyth]]

(In reply to Brian Hackett (:bhackett) from comment #3)

This change blocks bug 1524374, which is necessary for correct breakpoint behavior with workers. If we keep the sliding code around then I'll have to find some way of implementing it with the changes in bug 1524374, which isn't a good use of time if we don't actually need sliding anywhere.

Fair enough. Go for it, we can always reevaluate if it causes issues after landing.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95bfbc0f513d
Remove server side breakpoint sliding code, r=jlast.

Comment 7

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 95bfbc0f513d (Bug 1525685) for xpcshell failures at test_setBreakpoint-on-line-with-no-offsets.js.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/afa72a00cd2231e5c7293b3a2566cafc68bc0c99

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=227237750&revision=95bfbc0f513dc12c23c87529fc3f446045dde557

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227237750&repo=mozilla-inbound&lineNumber=2115

Comment 8

[Cristian Brindusan [:cbrindusan]]

There are also failures on test_breakpoint-03.js
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227240888&repo=mozilla-inbound&lineNumber=1986

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb8d820d2c1
Remove server side breakpoint sliding code, r=jlast.

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/dcb8d820d2c1