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

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

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

Removed some stray dump statements

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.

Sorry about that. This patch should apply cleanly.

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

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

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

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

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