Bug 743382 - Make loader independent of other modules.

[Gozala]

This patch decouples loader from all the dependencies (except from self which I'll cover in next iteration). In more details it:

1. Loader option uriPrefix was renamed to prefixURI to be more consistent with other options.
2. Module exports are frozen now using Object.freeze of the parent compartment to workaround Bug 674195. This required switches from incompatible __defineGetter__
Object.defineProperty.
3. Most of the bootstrap code from loader and bootstrap.js was moved to helper addon/runner module that bootstrap.js uses to prepare SDK environment for add-on startup.
4. Made loader independent from unload module. Now message using observer-service is send to propagate unload.
5. In order to avoid circular dependency between observer-service and unload modules, lower lever observer-service APIs have being extracted to system/events module implementing familiar event emitter APIs, which is used
   by unload module to track loader unloads & by observer-service under the hood.
6. Globals are no longer initialized by loader, instead set of globals my be passed in while constructing loader. Also, globals like console are added later in the process via addon/runner. All modules that need to use console and such earlier will have to require explicitly.
7. Finally we implement require.main as specified by CommonJS. It's require.main === module for the main moudle
   now.
8. Loader module API is now simpler and more flexible, that was necessary for addon/runner. Also API exposed by loader now is a super-set of api-utils/sandbox which we will be able to remove in a future (let's do it in next iteration).

[Gozala]

I still plan on writing more documentation for loader module. And will have another iteration to remove dependency on self module. With this patch all example add-on works and all tests pass except one: test-tab.testGetTabForWindow (timed out)
which does not fails if I run only tab tests. I'll try to figure out what's an issues there, but I think we can go ahead with a review meanwhile.

[ochameau]

What about using something like this:

let sandboxOptions = {
  sandboxPrototype: options.prototype || {},
  wantXrays: options.wantXrays ||false,
  sandboxName: options.name
}
if (options.sandbox)
  sandboxOptions.sameGroupAs = sandbox;

Your current code is unecessary complex. The mapping between input options and the sandbox options, and what will be the default value is all but simple to catch. Using same variable name for both is a bad practice.

[Gozala]

What about using something like this:

let sandboxOptions = {
sandboxPrototype: options.prototype || {},
wantXrays: options.wantXrays ||false,
sandboxName: options.name
}
if (options.sandbox)
sandboxOptions.sameGroupAs = sandbox;

I wish I could! you will get warnings if options don't have those properties.

[Gozala]

If that would help I could do following instead:

let defualts = {
  principal: systemPrincipal,
  prototype:  {},
  wantXrays: false,
}

options = override(defualts, options)

if (options.sandbox)
  sandboxOptions.sameGroupAs = sandbox;

...

[ochameau]

Yes, it would.

[ochameau]

Don't you think unload should be factored out of cuddlefish?
For me, it should. Either addon/runner.js or unload.js should handle this.
It would avoid using observer-service and this jetpack unload event.
bootstrap.js can directly call one of those two modules on shutdown call.

Otherwise, it looks good. There is some comments to be added, may be some stuff to make clearer.
I'm waiting for tests fixes before giving final review.

[Gozala]

Don't you think unload should be factored out of cuddlefish?
For me, it should.

In that case firefox features implemented with sdk won't be able to use unload feature, also unit test will have hard time simulating unloads. So I thought it was better to do it via observer-service. Also I could be convinced otherwise.

[ochameau]

In any case, if they want to support unload feature, they will have to use unload module.
So that they will have to depend on it. So that it doesn't sounds wrong to require using unload module to fire unload.
Instead of loader.unload(), they will have to do loader.require("unload").send(...)

Am I missing something?

[Gozala]

In any case, if they want to support unload feature, they will have to use unload module.
So that they will have to depend on it. So that it doesn't sounds wrong to require using unload module to fire unload.
Instead of loader.unload(), they will have to do loader.require("unload").send(...)

Am I missing something?

Well yes they could use unload module to handle unloads, but they also may choose not to use unload module. This way standard observer-service can be used to listen for unloads as it's used to listen for other similar events like application unload etc... Which is arguably is a better idea, since observer service keeps weak references so listeners won't be kept if they can be garbage collected.

Also, this way loader does supports unload, everything else is just a sugar on top. All pieces become independent of each other:

• bootsrap.js just creates a loader and loads special add-on/runner into it, on shutdown it just unloads loader.
• addon-runner is just a glue that loads main module into given loader and has now knowledge of how unloads work.
• cuddlefish does not knows anything about anyone it just publishes event when unloaded.

I think it's best to reduce knowledge of independent components from each other, that way changes later will be easier.

BTW: What do you have against, I mean what's your objections to a way it's now ?

[ochameau]

We are talking about simplifying loader and removing features/dependencies.
It's quite simple. I don't see the value of this observer service event where we can call unload module directly.
But I didn't thought about offering a way to use another unload module, that's a feature that requires such thing.
It is not a big deal for me, I just thought there was room for more simplification.

[Gozala]

It is not a big deal for me, I just thought there was room for more simplification.

In that case let's leave it for now, specially since we'll have another iteration to get rid of self magic anyway.

[Gozala]

Ok so I believe I have addressed all the review comments. Also, I have trucked down a bug that was causing test failures, was a stupid one loader test were unloading an actual loader causing event listeners to be unregistered and there for causing timeouts. So I'll be waiting for a final review!

@ochameau I think you may also want to try your html-l10n branch on top of this one to see if it works.

[ochameau]

I tried to find any usage of second parameter prototype without success.
And I don't think we can overload a function prototype.

function foo(){}
foo.prototype = {bar:true};
foo.prototype.bar is undefined.

Looks like we should just do:

function iced(f) {
  Object.freeze(f.prototype);
  return Object.freeze(f);
}

[Gozala]

I'm sure you did some mistake you definitely can set function prototype (not redefine though) unless it's frozen. This works

function foo() {}
foo.prototype = undefined

typeof foo.prototype // undefined

As of prototype argument it's undefined doing exactly what I want ;) But yeah I can remove it, it's just at first I thought I'd non undefined prototype somewhere.

[Gozala]

P.S.: Also your example there returns true.

[ochameau]

I think there is something wrong with lambda/iced and I still hope Sandbox can be easier to read.
Otherwise it works fine with html localization branch, I just have to move html module initialization to addon/runner.js:startup()