1126193 - setBreakpoint should not take a full location.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

setBreakpoint doesn't actually need to take a full location object as argument. Since setBreakpoint is a member of SourceActor, the sourceActor is already implicit. Being able to pass a different sourceActor via the location object is confusing, and a potential source of bugs.

In addition, I would like to remove column breakpoints from the protocol for the time being, since they aren't actually supported on the protocol level. We need them for setting breakpoints on source mapped sources, but that's an implementation detail. Even though we expose column breakpoints to the protocol, their semantics aren't clear at the moment.

Moreover, column breakpoints aren't actually supported by the UI right now, so the only places they are actually used is inside tests. Let's remove column breakpoints from the protocol for now, and reintroduce them after we've cleaned up the rest of the breakpoint code.

Comment 1

[Eddy Bruel [:ejpbruel]]

So as it turns out, we cannot get rid of column breakpoints, *because* they are broken at the moment.

Some of our source mapping tests use column breakpoints to set a breakpoint on a line. I wondered why that was, since there is nothing in those tests that distinguishes these breakpoints from normal line breakpoints. We should therefore be able to replace those column breakpoints with line breakpoints, I thought.

Well, turns out there are several things wrong with line breakpoints that prevent this from working. First of all, line breakpoints on source mapped sources are converted to column breakpoints. We're currently doing that wrong in that a single line breakpoint gets converted to a single column breakpoint (it could be multiple), but that wouldn't be a problem for these tests as long as we set a breakpoint on the first column on the line that has executable code.

With that in mind, I expected to be able to simply replace setBreakpoint({ line: 20, column: 7 }) with setBreakpoint({ line: 20, column: 0}), with the latter being equivalent to setBreakpoint({ line: 20 }). Turns out that that's not the case if the line is indented. If the line is indented, the source map library finds the closest mapping that is *smaller* than the one requested, so we end up on line 19.

That's obviously the wrong thing to do, so I've opened bug 1126190 to address that problem. In the meantime, we'll have to stick with column breakpoints for a bit longer.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: patch

This patch replaces the location object passed to setBreakpoint with a line and column. I eventually want to get rid of the column argument as well (see the comment above for an explanation of why this is not yet possible at the moment).

Note that this patch is based on top of the patch in bug 1123693.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8555141 [details] [diff] [review]
patch

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

::: toolkit/devtools/server/actors/script.js
@@ +2726,5 @@
>  
>    _createBreakpoint: function(loc, originalLoc, condition) {
> +    return this.setBreakpoint(originalLoc.line, originalLoc.column, condition)
> +               .then(response =>
> +    {

Nit: { on same line as =>

@@ +2912,4 @@
>      });
>    },
>  
> +  setBreakpointForActor: function (actor) {

Need doc comments -- what does this do?

Comment 4

[Eddy Bruel [:ejpbruel]]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=816c4f3d0740

Comment 5

[Eddy Bruel [:ejpbruel]]

Several oranges on try, probably due to errors introduced during rebasing.

New try push with errors fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a09b9adf6bf1

Comment 6

[Eddy Bruel [:ejpbruel]]

Try looks green, landing on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/88f46f6098e8

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88f46f6098e8