Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Description • 12 years ago

The rules for relative source map URL resolution are defined here: https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit?pli=1#heading=h.lmz475t4mvbx

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 1 • 12 years ago

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

Updating the source map library in bug 772119 fixed the issues I was having with this patch! Woo!

Verified that using coffeescript's defaults results in debuggable coffescript files without needing to play with urls or source roots or anything!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 2 • 12 years ago

Comment on attachment 730378 [details] [diff] [review]
v1

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

This doesn't seem to work as expected in this test case:

http://astithas.com/test/coffee/binary_search.html

With the patch from bug 849071 I get a 404 for the coffee script file, when I toggle between original and generated sources.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +506,5 @@
>      return locationPromise.then(function (aLocation) {
>        let line = aLocation.line;
> +      if (this.dbg.findScripts({ url: aLocation.url }).length == 0 ||
> +          line < 0 ||
> +          line === null) {

Why not line == null to catch undefined, too?

@@ +2431,5 @@
>            return {
>              url: source,
>              line: line
>            };
> +        }.bind(this));

This is not necessary, there is no reference to |this| in this function.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 3 • 12 years ago

(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 730378 [details] [diff] [review]
> v1
> 
> Review of attachment 730378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to work as expected in this test case:
> 
> http://astithas.com/test/coffee/binary_search.html
> 
> With the patch from bug 849071 I get a 404 for the coffee script file, when
> I toggle between original and generated sources.

What is happening is that we are hitting this issue: https://github.com/mozilla/source-map/issues/43

Will fix.

> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +506,5 @@
> >      return locationPromise.then(function (aLocation) {
> >        let line = aLocation.line;
> > +      if (this.dbg.findScripts({ url: aLocation.url }).length == 0 ||
> > +          line < 0 ||
> > +          line === null) {
> 
> Why not line == null to catch undefined, too?

The source map lib will always return null, so I thought we could do strict equality, but I now realize that if there isn't a source map, we return what we were given so it could indeed be undefined. Will change.

> 
> @@ +2431,5 @@
> >            return {
> >              url: source,
> >              line: line
> >            };
> > +        }.bind(this));
> 
> This is not necessary, there is no reference to |this| in this function.

Good catch.

Will have a new patch up soon.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 4 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=8a5a2640e991

* Updated to the latest mozilla/source-map

  * This fixes the issues with joining relative paths to their root that you were experiencing.

* Added two more xpcshell tests

  * One for making sure absolute source map urls work

  * One for making sure that relative source map urls work

* Added changes suggested in last review.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 5 • 12 years ago

Comment on attachment 732194 [details] [diff] [review]
v2

Clearing review until the oranges are dealt with.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 6 • 12 years ago

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

Rebased on the new patch for bug 772119

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 7 • 12 years ago

Comment on attachment 734104 [details] [diff] [review]
V3

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +2364,5 @@
>      }
>  
>      return this.sourceMap(aScript)
> +      .then((aSourceMap) => {
> +        return [this.source(s) for (s of aSourceMap.sources)];

You could probably get rid of the braces and the |return| here if you care.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 8 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=3ddd5d28881d

* Removed the brackets+return that Panos mentioned

* Disabled source map tests that rely on file:// urls in b2g

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 9 • 12 years ago

> * Disabled source map tests that rely on file:// urls in b2g

Just want to note that this is what Panos and I decided upon in an irc conversation, for posterity.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 10 • 12 years ago

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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 11 • 12 years ago

Backed out on suspicion of mochitest-chrome leaks:
https://hg.mozilla.org/integration/fx-team/rev/4ca11b942d5a

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 12 • 12 years ago

Relanded:
https://hg.mozilla.org/integration/fx-team/rev/4572a726100e

	Tim Taubert [:ttaubert] (inactive)
	Comment 13 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4572a726100e