Land module loader to firefox

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: irakli, Assigned: irakli)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
First thing we need to land an SDK loader to firefox is to implement support for a system modules described here: https://github.com/mozilla/addon-sdk/wiki/JEP-packageless

Goal of this task would be to make changes to a loader that would allow:

1. loading it as a JSM.
2. Instantiation of loader.
3. Use of loader instance to load system modules from `resouruces:///modules/` as described in the packageless proposal.
Assignee: nobody → rFobic
This would allow us to land in firefox and do the linker changes in parallel or independently.
At the moment requirements of loader look as follows:


@packaging
api-utils/globals!
api-utils/process
api-utils/self!
api-utils/system
api-utils/unload

I think we should minimize that before attempts to land to firefox.

- @packaging is pseudo-module so it's fine

- api-utils/globals! This is layer of shims & workarounds for lack of system capabilities to write to stdout properly. I will try to separate it from loader.

- `api-utils/process` is there for E10S and in fact has lots of dependencies of it's
own so it would be good to get rid of it. @Mossop is there any news about E10S ? Maybe we should get rid our E10S abstraction layer it uses hidden window and bunch of other unnecessary stuff. 

- self! only loaded if require('self') is used maybe we could ignore it for the time being or update self so that it works the way it would in packageless.

- `api-utils/system` is there for a two reasons to provide backwards compatibility to
`exports.main` with `staticArgs` and `exit`.  

- `api-utils/unload` is there for a two reasons to provide a backwards compatibility for `exports.main` and to notify system about unload. The later one can be done using `nsIObserverService`.

I think we could make a module `deprecate/main` that would be loaded only if main.js exports `main` to provide these shims and recommend usage of `require('system')` instead.
Depends on: 743382
Depends on: 743384
Depends on: 743386
It looks like we should move Loader.main out of cuddlefish and move it to bootstrap.js or a module that will use cuddlefish in order to run an addon.
If we get rid of E10S stuff (I agree we should remove it), here is what I would do:
https://github.com/ochameau/addon-sdk/compare/factors-out-addon-stuff-from-cuddlefish-without-e10s
This patch introduce a new module "chrome-entry-point", that factors out Loader.main and some stuff from bootstrap.js:shutdown that requires some module.
I think it would be helpfull to have such common js module to handle *Addon* loading/unloading. So that we can use other modules easily like l10n, unload or system for these actions.
What I like in this approach is that it allows to not change much stuff for jetpack, while removing dependencies on Cuddlefish!

We may even keep E10S support with similar pattern:
https://github.com/ochameau/addon-sdk/compare/master...factors-out-addon-stuff-from-cuddlefish
A similar patch that introduce 2 new modules instead of just 1.
- chrome-entry-point: creates the browser node and inject a loader in it, load the second module,
- remote-entry-point: load and run main addon module

These patches are somekind of draft. Names may be really bad. We may go futher and move more code in these modules. (Like setDefaultPrefs, startup listener,...)
I would be totally fine if you think it is a bad approach and would go in a completely different way too.
Priority: -- → P1
Depends on: 749638

Comment 6

6 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1249d4fcdec82b4229a9563e3a6a1cf1d28e9421
Merge pull request #428 from Gozala/bug/loader-resolve@743359

Bug 743359 - Extract parts of loader that will land to firefox into separate module r=@ochameau

Comment 7

6 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4b6b60d818bc6c453d54c90f4a0c2721183634b7
Merge pull request #440 from Gozala/hotfix/loader-overlay@743359

Bug 743359 - Move module overlay logic from bootstrap to cuddlefish. r=@ochameau
Summary: Change loader so that it can load only system modules off manifest → Land module loader to firefox
After some discussing more details with @Mossop today we outlined couple of tasks that someone will need to implement:

1. I'm submitting patch against mozilla-central that @mossop is going to review.
2. We will need a makefile that will take care of copying modules from toolkit/addon-sdk/ to an appropriate place so that they appear under
resource://gre/modules/toolkit/. Same or similar makefile will copy files
from toolkit/addon-sdk/ to anappropriate place so that they appear under resource:///modules/ 
3. We will need a hook that will be invoked on commit to SDK's hg repo
and will copy SDK modules from to `toolkit/addon-sdk/`,  `browser/addon-sdk/` ..
depending on the module metadata.

Dietrich could someone from your team pick up 2-3 while ?
Gavin, Dietrich told I should talk with you since have taken over his role now (Congrats BTW)

Comment 10

6 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/74513c83a89438b4bf18770530448fca5d3dbce3
Merge pull request #443 from Gozala/hotfix/loadReason@743359

Bug 743359 - Fixing issues introduced by #428 r=@ochameau

Updated

6 years ago
Depends on: 756214

Updated

6 years ago
No longer depends on: 756214
Depends on: 756491
See Also: → bug 725409
Created attachment 625181 [details]
Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/2

Pointer to Github pull-request
Attachment #625181 - Flags: review?(dtownsend+bugmail)
I have updated document describing how synchronization of SDK repo
with mozilla-central should work with https://github.com/mozilla/addon-sdk/wiki/JEP-SDK-in-Firefox

Mossop: Could you please take a look to make sure I have not missed or miss-interpreted something.

Gavin: This should capture details for 2. 3. Let me know if you need
any more information to get someone started with these ?
Created 2. and 3. tasks as blocking bug 756542 and bug 756545 for this one.
Depends on: 756548
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #8)
> 2. We will need a makefile that will take care of copying modules from
> toolkit/addon-sdk/ to an appropriate place so that they appear under
> resource://gre/modules/toolkit/. Same or similar makefile will copy files
> from toolkit/addon-sdk/ to anappropriate place so that they appear under
> resource:///modules/ 

I assume that second "toolkit/addon-sdk/" is meant to be "browser/addon-sdk"?

An initial patch that adds the current modules to those directories would be useful for testing, but the build goop to do this is relatively simple. We can discuss further in bug 756542.

> 3. We will need a hook that will be invoked on commit to SDK's hg repo
> and will copy SDK modules from to `toolkit/addon-sdk/`, 
> `browser/addon-sdk/` ..
> depending on the module metadata.

I don't know who the best person is to write an hg hook for the SDK repo. But is that really blocking this work? How often are new modules added? Can we not just sync them manually periodically?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> I assume that second "toolkit/addon-sdk/" is meant to be "browser/addon-sdk"?

There is ongoing work to add support for Add-ons SDK to other applications like Thunderbird. So 'browser' doesn't seem to be the right place.
There are going to be two module locations, one in browser/ and one in toolkit/. His comment just mistakenly referred to both as being "toolkit/addon-sdk" - that's why I said "that second".
Created attachment 626207 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

Pointer to Github pull-request
Comment on attachment 626207 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

Ochameau or Mossop please pick up this review.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> There are going to be two module locations, one in browser/ and one in
> toolkit/. His comment just mistakenly referred to both as being
> "toolkit/addon-sdk" - that's why I said "that second".

Yeah you're right, sorry for that.
Comment on attachment 626207 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

I forgot I have created a Bug 756548 for this one, moving pathch there.
Attachment #626207 - Attachment is obsolete: true
Created attachment 629251 [details]
Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/3

Pointer to Github pull-request
Attachment #629251 - Flags: review?(dtownsend+bugmail)
Attachment #625181 - Attachment is obsolete: true
Attachment #625181 - Flags: review?(dtownsend+bugmail)

Comment 23

6 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/302a5533e975f73a987450ee73cfd948b9694aff
Fix regression from bug 743359. logFile and resultFile are always set but may be empty.

https://github.com/mozilla/addon-sdk/commit/490f59c8a568d765eb55022eb391a405d7a8c40c
Merge pull request #460 from ochameau/fix/loader-regression

Fix regression from bug 743359. r=@gozala
Depends on: 763006
Comment on attachment 629251 [details]
Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/3

I think we've iterated over all this enough elsewhere, this looks good to me.
Attachment #629251 - Flags: review?(dtownsend+bugmail) → review+
(In reply to Dave Townsend (:Mossop) from comment #24)
> Comment on attachment 629251 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/mozilla-central/pull/3
> 
> I think we've iterated over all this enough elsewhere, this looks good to me.

In that case would you be that kind to land this change ?

Thanks!
https://hg.mozilla.org/mozilla-central/rev/efa8bb276e3d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.