1056214 - Use the addon-sdk repo's JSM modules when -o is used

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

In Bug 1037235 and Bug 1040432 there was a `modules` directory created in the addon-sdk repo which contains JSM modules.

At the moment it is only possible to test updates to the module by making the modifications to the m-c repo and building Firefox.  When we use the `-o` argument however I think we should be able to use the JSM modules that are in the addon-sdk so that modifying these modules is easier.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

I see four implementation options:

1) let prePath = getPref("extensions.sdk.jsm.prepath") || "resource://gre/modules/sdk/"; // export this from some module and use it whenever we import an sdk jsm module
2) modify require to import these modules somehow, like `require("sdk/jsm/...")` (this is pretty magic though)
3) Create a new resource uri, using "resource://sdk/modules/.." and is it instead of "resource://gre/modules/sdk/.." and overwriting this "sdk" resource uri when -o is used.
4) Continue using "resource://gre/modules/sdk/..", but create a second resource uri which points to the same place as "resource://gre/modules/sdk/.." if `-o` is not used, and points to the addon-sdk repo when `-o` is used.

What is your preference?

Comment 2

[Tomislav Jovanovic :zombie]

note: i used the standard SDK require() to load JSMs using a relative path from sdk/framescript/ but the solution in this bug will probably be even better, especially since i plan to move loading of those things to the platform once they stabilize..

Comment 3

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)
> I see four implementation options:
> 
> 1) let prePath = getPref("extensions.sdk.jsm.prepath") ||
> "resource://gre/modules/sdk/"; // export this from some module and use it
> whenever we import an sdk jsm module
> 2) modify require to import these modules somehow, like
> `require("sdk/jsm/...")` (this is pretty magic though)

I like this and I don't think we'd need to modify require. Require can already import jsms so all we need to do is add a new mapping to the paths in bootstrap.js to make say "modules/" map to "resource://gre/modules/sdk/". When -o is used you'd just need to set extensions.modules.<id>.path.modules to the new URI to use, bootstrap.js already has code to make that override the default.

> 3) Create a new resource uri, using "resource://sdk/modules/.." and is it
> instead of "resource://gre/modules/sdk/.." and overwriting this "sdk"
> resource uri when -o is used.

I think this would be problematic with multiple SDK add-ons in the same application.

> 4) Continue using "resource://gre/modules/sdk/..", but create a second
> resource uri which points to the same place as
> "resource://gre/modules/sdk/.." if `-o` is not used, and points to the
> addon-sdk repo when `-o` is used.

Same as 3.

> What is your preference?

I think 3 and 4 are non-starters. I'd go for 2 in preference.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think `require("toolkit/loader.jsm")` should load loader as a JSM. I do remember Jordan was tackling this, but not sure that has landed. I think this is option 2 but less magicy.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8477856 [details] [review]
Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3

Thanks Erik for the patch.

I like the changes but have few concerns that I'd rather have resolved before landing this (pointed out them in Github and also below):

1. I don't like `require("modules/system/Startup.jsm")` as I find `modules` prefix here confusing from the SDK user's perspective. I would like to settle on a better mapping like `jsm/Startup.jsm` or `shared/Startup.jsm` or `legacy/Statup.jsm` to make it clear that this is a directory containing `jsm` style modules or shared (across all addons) modules or legacy module system modules. I'd be also down with just putting those jsm in `https://github.com/mozilla/addon-sdk/tree/master/lib/sdk/system`
2. I think that code that lives under resource://gre/modules/commonjs/ should live under add-on sdk repo although I could be convinced other vise.

Let's figure out these and I'm happy to land this.

Thanks

Comment 7

[Tomislav Jovanovic :zombie]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> I think `require("toolkit/loader.jsm")` should load loader as a JSM. I do

this works already, though the module must have the explicit ".jsm" extension, as opposed to just ".js" that `modules/system/Startup.js` and `XulApp.js` now have.


> 1. I don't like `require("modules/system/Startup.jsm")` as I find `modules`
> [...]

without too much bikeshedding, i think `jsm/Startup.jsm` is better, as it's more specific, explicit,  possibly already familiar to (some) Fx addon devs, and eminently more googleable for everyone else than `modules`, `legacy` or `shared`, which can all mean a dozen different things.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> Comment on attachment 8477856 [details] [review]
> Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3
> 1. I don't like `require("modules/system/Startup.jsm")` as I find `modules`
> prefix here confusing from the SDK user's perspective. I would like to
> settle on a better mapping like `jsm/Startup.jsm` or `shared/Startup.jsm` or
> `legacy/Statup.jsm` to make it clear that this is a directory containing
> `jsm` style modules or shared (across all addons) modules or legacy module
> system modules. I'd be also down with just putting those jsm in
> `https://github.com/mozilla/addon-sdk/tree/master/lib/sdk/system`

using the `jsm` prefix seems fine to me.

> 2. I think that code that lives under resource://gre/modules/commonjs/
> should live under add-on sdk repo although I could be convinced other vise.

I don't understand this point, can you rephrase this?

> Let's figure out these and I'm happy to land this.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8477856 [details] [review]
Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3

I'm using a `jsm` prefix instead of a `module` prefix now.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #6)
> > Comment on attachment 8477856 [details] [review]
> > Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3
> > 1. I don't like `require("modules/system/Startup.jsm")` as I find `modules`
> > prefix here confusing from the SDK user's perspective. I would like to
> > settle on a better mapping like `jsm/Startup.jsm` or `shared/Startup.jsm` or
> > `legacy/Statup.jsm` to make it clear that this is a directory containing
> > `jsm` style modules or shared (across all addons) modules or legacy module
> > system modules. I'd be also down with just putting those jsm in
> > `https://github.com/mozilla/addon-sdk/tree/master/lib/sdk/system`
> 
> using the `jsm` prefix seems fine to me.
> 
> > 2. I think that code that lives under resource://gre/modules/commonjs/
> > should live under add-on sdk repo although I could be convinced other vise.
> 
> I don't understand this point, can you rephrase this?
> 
> > Let's figure out these and I'm happy to land this.

What I'm trying to say is that files:

lib/jsm/system/Startup.jsm
lib/jsm/system/XulApp.jsm

Are added to mozilla central, but not to an add-on sdk repo. I think they should be part of add-on sdk repo instead & be merged to mozilla central with a the rest of the sdk.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8477856 [details] [review]
Link to Github pull-request: https://github.com/erikvold/gecko-dev/pull/3

I'm accepting review, as my previous comment could be resolved afterwards.
I also don't quite see why longer paths like `jsm/system/Startup.jsm` are preferred over `jsm/Startup.jsm` but I also don't care enough to block this :)

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

I pushed this to try https://tbpl.mozilla.org/?tree=Try&rev=101c6d2ba1d3 and it passed, but now bug 1037235 was reverted and I don't want to rebase again so I'm going to wait for bug 1037235 to land.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Still waiting on bug 1037235

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

I'm going to need this in order to test changes for jpm and native jetpacks efficiently.  Otherwise I will have to land to m-c and wait a day or do custom builds.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 16

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562