We have two copies of the source map lib based on the environment it is needed in. We should eliminate one copy by using the trick that DevToolsUtils.js(m) use.

All debugger server tests pass, but the source map tests themselves depend on requiring sub-modules of the source map lib which is broken with this approach.

I'm unassigning myself because I won't be working on this anytime soon.

This patch removes SourceMap.jsm completely. Instead, we use the devtools module loader to load source-map.js. Here is the related PR for upstream changes: https://github.com/mozilla/source-map/pull/198 In Utils.jsm, I'm not using Require.jsm anymore, nor am I using devtools module loader, that, to allow source-map *and* tests to run in the same scope and share the same define/require definition! Also, I don't understand why I get various random changes in source-map.js as I used the 0.4.4 tag and it seems to be the last used update from bug 1182278. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6648ca6c0af5

Note that, it should ease implementing bug 888524, but my priority here is to just drop the JSM to ease hacking on devtools codebase.

SourceMap seems to be the last one to use Require.jsm, let's remove it! https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6c5b598475

Fixed obvious failure. But I still have test timeout without any error on browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js.

After various iterations, here is a patch that should have a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0f1866c8b2 (I'm expecting this one to fail on xpcshell, it was with a previous iteration) https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bdffa5a19eb (But I just pushed this patch just for xpcshell here)

Finally got a green try. I had to change source-map require path in order to support bug 1134628 initiative to have require path matching filesystem layout. https://treeherder.mozilla.org/#/jobs?repo=try&revision=81c89c3c53c6

I think this patch is going to fix bug 965856 as well.

Comment on attachment 8642397 [details] [diff] [review] patch v5 Review of attachment 8642397 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but we need to figure out how to avoid breaking pretty-fast and its tests on node. I think we should make sure that the loader still supports `require("source-map")`. The alternative would be adding var sourceMap = this.sourceMap || (function () { try { return require("source-map"); } catch (e) { } }()) || require("toolkit/devtools/sourcemap/source-map"); Which is a lot more gross IMO. ::: toolkit/devtools/pretty-fast/pretty-fast.js @@ +17,5 @@ > }(this, function () { > "use strict"; > > var acorn = this.acorn || require("acorn/acorn"); > + var sourceMap = this.sourceMap || require("devtools/toolkit/sourcemap/source-map"); This will break the node version of pretty-fast and the pretty-fast tests. This line needs to remain `require("source-map")` for those to work. ::: toolkit/devtools/pretty-fast/tests/unit/test.js @@ +524,5 @@ > } > > ]; > > +var sourceMap = this.sourceMap || require("devtools/toolkit/sourcemap/source-map"); Ditto ::: toolkit/devtools/sourcemap/UPGRADING.md @@ +12,1 @@ > $ cp dist/test/* /path/to/mozilla-central/toolkit/devtools/sourcemap/tests/unit/ While you're here, can you turn this comment into a shell script so we can do $ ./upgrade-source-map.sh

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88aab4eb9d2 Ok, so I kept require("source-map"), for now. We will most likely change that in bug 1134628 in order to make require path more explicit, by mapping filesystem layout (and make it easier to reload tools from source directory, without build system).

Comment on attachment 8643952 [details] [diff] [review] patch v6 (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #12) > Comment on attachment 8642397 [details] [diff] [review] > ::: toolkit/devtools/sourcemap/UPGRADING.md > @@ +12,1 @@ > > $ cp dist/test/* /path/to/mozilla-central/toolkit/devtools/sourcemap/tests/unit/ > > While you're here, can you turn this comment into a shell script so we can do > > $ ./upgrade-source-map.sh Writting such shell script would require many parameters: git revision, path to gecko, figure out if it is better to use npm run-script or nodejs. I think it is already handy to have these instructions and let you debug each line which can easily fail for various reasons. It would be more reasonable to offer such script if we get rid of the build/npm/node step.

(In reply to Alexandre Poirot [:ochameau] from comment #13) > Ok, so I kept require("source-map"), for now. > We will most likely change that in bug 1134628 > in order to make require path more explicit, by mapping filesystem layout > (and make it easier to reload tools from source directory, without build > system). I don't think we want this to change because third party libs and our own libs that also live outside of m-c expect to be able to `require("source-map")`. Maintaining upstream patches is not fun.

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

Comment on attachment 8644435 [details] [diff] [review] Fix b2g scope issue Nick, there is a failure on b2g, typical issue with scope on b2g where we have to bind exported symbols on `this`. Here is the failure: https://treeherder.mozilla.org/logviewer.html#?job_id=4108119&repo=fx-team As I wasn't sure if that was also failure for loadSubScript, I also added some fixes in source-map.js.

Comment on attachment 8644435 [details] [diff] [review] Fix b2g scope issue Review of attachment 8644435 [details] [diff] [review]: ----------------------------------------------------------------- r=me with corresponding PR to upstream mozilla/source-map

Related github PR: https://github.com/mozilla/source-map/pull/201

I have to headout, if this hotfix wasn't enough, please backout all patches of this bug as well as bug 1188401 that will revert-conflict on top of this bug. Here is the try run for the hotfix: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=13cfa6b095a0

https://hg.mozilla.org/mozilla-central/rev/a926fd3044b7 https://hg.mozilla.org/mozilla-central/rev/a0d4ce93ef07 https://hg.mozilla.org/mozilla-central/rev/13cfa6b095a0