Closed Bug 935366 Opened 8 years ago Closed 7 years ago

Unify SourceMap.jsm and source-map.js

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: fitzgen, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 6 obsolete files)

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.
Assignee: nobody → nfitzgerald
Priority: -- → P3
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.
Assignee: nfitzgerald → nobody
Summary: Combine SourceMap.jsm and source-map.js similar to DevToolsUtils.js(m) → Unify SourceMap.jsm and source-map.js
Blocks: 1182194
Blocks: 965856
Attached patch patch v1 (obsolete) — Splinter Review
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
Attached patch patch v2 (obsolete) — Splinter Review
Fixed obvious failure.
But I still have test timeout without any error on browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js.
Attachment #8640454 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
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)
Attachment #8640993 - Attachment is obsolete: true
Attached patch patch v5 (obsolete) — Splinter Review
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
Attachment #8337102 - Attachment is obsolete: true
Attachment #8641132 - Attachment is obsolete: true
Attachment #8642397 - Flags: review?(nfitzgerald)
Assignee: nobody → poirot.alex
Attachment #8640459 - Flags: review?(nfitzgerald)
I think this patch is going to fix bug 965856 as well.
Attachment #8640459 - Flags: review?(nfitzgerald) → review+
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
Attachment #8642397 - Flags: review?(nfitzgerald) → feedback+
Attached patch patch v6Splinter Review
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).
Attachment #8642397 - Attachment is obsolete: true
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.
Attachment #8643952 - Flags: review?(nfitzgerald)
(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.
Attachment #8643952 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8644435 - Flags: review?(nfitzgerald)
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
Attachment #8644435 - Flags: review?(nfitzgerald) → review+
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.