1123693 - setBreakpoint should take an original location as argument

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

See the description for bug 1123686 for an explanation as to why this is needed.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Comment 2

[James Long (:jlongster)]

This patch is a little bigger, and my brain is fried. I will review it first thing in the morning!

Comment 3

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=0f805c106eb8

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Removed some stray dump statements

Comment 5

[James Long (:jlongster)]

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

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

::: toolkit/devtools/server/actors/script.js
@@ +2900,5 @@
>     * @param object aLocation
>     *        The location of the breakpoint (in the generated source, if source
>     *        mapping).
>     */
> +  setBreakpoint: function (originalLine, originalColumn, condition) {

Update the method doc with the new parameters, plz

@@ -2906,4 @@
>      return this.threadActor.sources.getGeneratedLocation({
>        sourceActor: this,
> -      line: originalLocation.line,
> -      column: originalLocation.column

I'm confused which patch this patch is based on. I thought it might be based on https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1123686&attachment=8551805, but I don't see the changes there to `setBreakpoint` here. Below you use `setBreakpointForActor` for example (in the old code, not looking at this patch's changed), where was the introduced?

Hard to parse this patch without the context of other work this was based on. Not sure if I'm just confused or what.

@@ -5463,5 @@
>      }
>  
>      let fetching = fetch(aAbsSourceMapURL, { loadFromCache: false })
>        .then(({ content }) => {
> -        dump("FAK U " + content + "\n");

Which rev/commit is this based on? I don't see this on master: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L5470

::: toolkit/devtools/server/tests/unit/test_sourcemaps-03.js
@@ -145,5 @@
>  
> -  code += "//# sourceMappingURL=data:text/json;base64," + btoa(map.toString());
> -  dump("CODE " + code + "\n");
> -  dump("MAP " + JSON.stringify(new SourceMapConsumer(JSON.stringify(map))._originalMappings) + "\n");
> -

Which rev/commit is this based on? Same comment here about logs. Also, you removed code that suffixes `code` with the source map.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Sorry about that. This patch should apply cleanly.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Ugh. Forgot to update the documentation for setBreakpoint as you requested.

Comment 8

[Eddy Bruel [:ejpbruel]]

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

Nick said he'd take a couple of these reviews since James is at ReactConf this week. Thanks Nick!

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Comment 10

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

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

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

Can you make sure to generate patcehs with 8 lines of context (-U 8)?

I don't feel like I have enough to tell what's going on here, and don't want to swap back and forth between editor and splinter.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Hi Nick. Do you think you would be able to review this patch today? It's blocking me from landing the other patches you reviewed.

Comment 12

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

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

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

::: toolkit/devtools/server/actors/script.js
@@ +2906,4 @@
>        sourceActor: this,
> +      line: originalLocation.line,
> +      column: originalLocation.column
> +    }).then((generatedLocation) => {

Nit: drop parens on 1-arity arrow functions

@@ +2973,5 @@
>  
> +    const { line: actualLine, entryPoints } = result;
> +
> +    const actualLocation = actualLine !== line
> +                         ? { sourceActor: sourceActor, line: actualLine }

Nit: indent 2 spaces, not lined up with = pls

Comment 13

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5747cd8619a

Comment 14

[Eddy Bruel [:ejpbruel]]

Try looks green, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/a938c9783b91

Comment 15

[Carsten Book [:Tomcat]]

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