895616 - get{Original/Generated}Location should take object/keyword arguments instead of positional arguments

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

There would be a lot less unpacking and destructuring and the code would be more readable.

Comment 1

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

In toolkit/devtools/server/actors/script.js, there are two methods on the ThreadSources class which could take objects instead of positional arguments and it would save us a lot of plumbing:

1. ThreadSources.prototype.originalPositionFor

2. ThreadSources.prototype.generatedPositionFor

So to summarize, instead of taking (url, line, column) they should just take a single argument that is an object of the form { url: ..., line: ..., column: ... }.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Hi Nick, I am looking to pick up the bug. It seems pretty straight forward. I will try to catch you on #devtools (nick: gluong).

Comment 3

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

Awesome!

Also note that the methods are actually

1. ThreadSources.prototype.getOriginalLocation

2. ThreadSources.prototype.getGeneratedLocation

I accidentally got them mixed up with methods of the other name they call, which are actually on the SourceMapConsumer object.

Also, MXR code search is really helpful for this kind of bug: http://mxr.mozilla.org/mozilla-central/search?string=getOriginalLocation

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 895616.patch

Refactored out the positional arguments in place of object/keyword arguments. Note the arrow function change in getGeneratedLocation in the then().

I didn't manager to debug script.js using the Browser Debugger. So, I am curious on how I would get that to work since the browser debugger didn't find script.js. I was basically playing around with the debugger to see if the script.js would be loaded. 

Are there any test cases that would need to be updated?

Thanks!

Comment 5

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

Comment on attachment 789354 [details] [diff] [review]
895616.patch

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

(In reply to Gabriel Luong (:gluong) from comment #4)
> Created attachment 789354 [details] [diff] [review]
> 895616.patch
> 
> Refactored out the positional arguments in place of object/keyword
> arguments. Note the arrow function change in getGeneratedLocation in the
> then().

Looks good, just see my comments below about using url/line/column instead of aSourceUrl/aLine/aColumn.

> I didn't manager to debug script.js using the Browser Debugger. So, I am
> curious on how I would get that to work since the browser debugger didn't
> find script.js. I was basically playing around with the debugger to see if
> the script.js would be loaded.

Unfortunately, we can't use the browser debugger on the debugger server, because it is interacting with the debugger server itself. One of the limitations of the debugger api is that we can't debug any js global which has frames on the stack. Effectively, this means we can't debug ourselves. We can debug the debugger front end, though.

> Are there any test cases that would need to be updated?

This is tested to varying degrees by a bunch of our existing tests. Every breakpoint test tests this code. Source map tests tests this code. Getting the current frame tests this code. If the existing tests are passing, I'm happy.

> 
> Thanks!

No, thank you!

Flag me for review when you upload the updated patch :)

::: toolkit/devtools/server/actors/script.js
@@ +540,5 @@
>        let { url, line, column } = packet.frame.where;
> +      this.sources.getOriginalLocation({
> +          aSourceUrl: url,
> +          aLine: line,
> +          aColumn: column }).then(aOrigPosition => {

Nit: we should avoid unpacking url/line/column from packet.frame.where and just call `this.sources.getOriginalLocation(packet.frame.where)` when we can (ie when url/line/column aren't being used directly in other places). If I'm not mistaken, we can avoid the unpacking here.

@@ +890,5 @@
>        let { url, line, column } = form.where;
> +      let promise = this.sources.getOriginalLocation({
> +          aSourceUrl: url,
> +          aLine: line,
> +          aColumn: column })

ditto

@@ +3301,2 @@
>     */
> +  getOriginalLocation: function TS_getOriginalLocation(aArgs) {

Please change the aArgs parameter to have line/column/url properties instead of aLine/aColumn/aSourceUrl properties. This is what allows us to skip a bunch of unpacking. I know we prepend the "a" for arguments, but we usually don't carry that into the properties as well.

Aside, I think this is a great opportunity for destructuring, since we expect an object with the same properties every time:

  function TS_getOriginalLocation({ url, line, column }) {

@@ +3339,2 @@
>     */
> +  getGeneratedLocation: function TS_getGeneratedLocation(aArgs) {

ditto

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Updated patch

Updated patch and finally understood what was meant by destructuring. Note I avoided some unnecessary unpacking with aRequest.location (line 932) and aResponse.actualLocation (Line 957). Thanks for the help Nick!

Comment 7

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

Comment on attachment 789983 [details] [diff] [review]
Updated patch

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

Great stuff! Assuming this try push comes back green, we will land it in fx-team :)

https://tbpl.mozilla.org/?tree=Try&rev=634f11aabdf3

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Seeing a lot of test-failure. I am wondering how I should go about interpreting the try results.

Comment 9

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

Gabriel, try results can be hard to parse, but given the amount and consistency of failures, I'm willing to bet that there are bugs in this patch (rather than existing intermittants cropping up). Digging through the logs, I found an error stack that looks pretty suspect:

00:20:55     INFO -  DBG-SERVER: ReferenceError: originalSource is not defined
00:20:55     INFO -  TA_onSetBreakpoint/</<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:972
00:20:55     INFO -  resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:122
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:47
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:157
00:20:55     INFO -  TA_onSetBreakpoint/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:955
00:20:55     INFO -  resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:122
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:47
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:157
00:20:55     INFO -  TA_onSetBreakpoint@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:933
00:20:55     INFO -  DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:933
00:20:55     INFO -  LDT_send/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/transport.js:239
00:20:55     INFO -  makeInfallible/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:61

and

00:20:55     INFO -  resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:839: error: ReferenceError: originalSource is not defined
00:20:55     INFO -  TA_onSetBreakpoint/</<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:972
00:20:55     INFO -  resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:122
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:47
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:157
00:20:55     INFO -  TA_onSetBreakpoint/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:955
00:20:55     INFO -  resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:122
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:47
00:20:55     INFO -  then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/commonjs/sdk/core/promise.js:157
00:20:55     INFO -  TA_onSetBreakpoint@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:933
00:20:55     INFO -  DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:933
00:20:55     INFO -  LDT_send/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/transport.js:239
00:20:55     INFO -  makeInfallible/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:61

Are the tests passing when you run them locally?

Comment 10

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

Gabriel, are you still working on this bug?

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Hey Nick, I was working on this last night, and have been inactive since I was busy finishing up my coop project. I am down to 6 errors from the 11 errors I was seeing. I believe the problem lies in name collisions. I will try to catch you on irc tonight, otherwise, I will post up the updated patch and error log.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 895616.patch

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

So, the root problem I encounter was name collision, and didn't refactor some of the arguments. I think the second problem arise after pulling the latest code since I never saw them on mxr. The patch for 908337 will likely need to be merged to make use of the object/keyword arguments.

Comment 14

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

Comment on attachment 794702 [details] [diff] [review]
895616.patch

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

Looks good!

Here's a try push: https://tbpl.mozilla.org/?tree=Try&rev=b2ead314726d

If it comes back green, I'll push it to fx-team :)

Comment 15

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

Landed in fx-team!

https://hg.mozilla.org/integration/fx-team/rev/b0e50fa72058

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Thanks Nick!

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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