618080 - Refactor e10s adapter interface to be easier to understand and document

Status: RESOLVED FIXED

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

Description

[Atul Varma [:atul]]

// Currently an e10s-adapter looks like this:

if (this.sendMessage) {
  /* We're being evaluated in the Addon Process. Set up message-passing
   * infrastructure to communicate with the Firefox Process
   * and export an API. */
} else {
  /* We're being evaluated in the Firefox Process. */
  exports.register = function register(process) {
    /* Set-up Firefox Process message-passing infrastructure
     * to communicate with the given Addon Process. */
  };
}

// The starting conditional is confusing and the global
// scope is polluted w/ message send/receive handlers.

// Another weirdness here is that the module is loaded twice,
// once in each process, but it's loaded as 'foo-e10s-adapter'
// in one and 'foo' in the other. It also exports the
// interface for 'foo' in one process, but a single
// 'register' function in the other. This feels rather
// schizophrenic.

---------------------------------------------------------
ALTERNATIVE 1: e10s adapters only export two functions.
---------------------------------------------------------

exports.createProxyModule = function(chrome) {
  // This function is only called on the addon side.
  return {
    doThing: function(x) {
      return chrome.call("doThing", x);
      }
    }
  };
};

exports.registerAddon = function(addon) {
  // This function is only called on the chrome side.
  addon.on('dothing', function(event, x) {
    return x + 1;
  });
};

// Disadvantages:
//
// * It's a bit more verbose than the current sol'n.
//
// * Being more verbose, it might be harder to remember.
//
// * It will require a decent bit of work to implement.
//
// Advantages:
//
// * The module always exports the same interface,
//   regardless of which process it's loaded into.
//
// * The module can be loaded as the same name,
//   e.g. 'foo-e10s-adapter', in both processes.
//
// * It requires one less kind of global scope in the
//   addon process, b/c the 'chrome' capability is passed
//   into an exported function.
//
// * Because dependencies are passed into exports
//   rather than ambient, this could be easier to unit
//   test.
//
// * The 'createProxyModule' function is a bit reminiscent
//   of the define() function from the CommonJS async
//   require() proposal, so it might be easier to grok.

---------------------------------------------------------
ALTERNATIVE 2: 'chrome' global in addon process
---------------------------------------------------------

if (this.chrome) {
  /* We're being evaluated in the child (Addon) Process. Set up message-passing
   * infrastructure to communicate with the Firefox Process
   * and export an API. */
  chrome.on('foo', function() { /* do stuff */ });
  chrome.send('howdy');
  var result = chrome.call('blarg');
} else {
  /* We're being evaluated in the Firefox Process. */
  exports.register = function register(addon) {
    /* Set-up Firefox Process message-passing infrastructure
     * to communicate with the given child Addon Process. */
    addon.send('o hello there.');
    addon.on('foo', function() {});
  };
}

// Disadvantages:
//
// * Doesn't fix all the confusing things about the current
//   paradigm.
//
// * Readers might interpret the starting conditional
//   the wrong way, i.e. thinking that 'this.chrome'
//   being true means that the code is being executed
//   in the chrome process.
//
// Advantages:
//
// * The starting conditional is a bit easier to understand.
//
// * All globals in the addon process are combined into
//   the 'chrome' global.

Comment 1

[Atul Varma [:atul]]

Drew, Myk, and Warner, do you have any opinions on this?

Comment 2

[Atul Varma [:atul]]

Note also that both alternatives described above change the cross-process messaging interface from the nsIJetpack-style one [1] to an EventEmitter-style one, which is more consistent with the rest of the SDK.

Note lastly that this API won't just be used by jetpack core devs who are porting existing modules to e10s; it will also hopefully be used by advanced library authors who want to expose a subset of chrome functionality to other addon authors.

[1] https://developer.mozilla.org/en/nsIJetpack

Comment 3

[Atul Varma [:atul]]

I talked to Drew, Myk, and Warner about these options and they all vastly preferred "ALTERNATIVE 1: e10s adapters only export two functions". Will look into how hard this will be to implement in the current architecture.

Comment 4

[Atul Varma [:atul]]

Urg, it is actually fairly non-trivial, so I'm just implementing "ALTERNATIVE 2: 'chrome' global in addon process" now over here:

https://github.com/toolness/jetpack-sdk/tree/bug-618080

Comment 5

[Atul Varma [:atul]]

Attachment: Pointer to pull request

Comment 6

[Atul Varma [:atul]]

Comment on attachment 497415 [details]
Pointer to pull request

Here is what I would like to tour the rocketeers through at tomorrow's workshop.

Comment 7

[Atul Varma [:atul]]

For a really basic overview of the changes made to this API, if you're familiar w/ the existing one, see:

https://github.com/toolness/jetpack-sdk/blob/bug-618080/packages/api-utils/docs/e10s.md

Comment 8

[Atul Varma [:atul]]

I presented the new API implemented by this patch to the Jetpack core development team on Tuesday and it went pretty well--everyone seemed to understand the concepts quite well.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 497415 [details]
Pointer to pull request

diff --git a/packages/api-utils/docs/e10s.md b/packages/api-utils/docs/e10s.md

> +As explained in the [Out-of-Process Add-ons][] internals guide, an *e10s adapter* is the primary mechanism through which chrome functionality is exposed to add-on processes. It is a single piece of code evaluated *twice*—once in each process—and typically has the following form:

It's hard to review files with long lines that don't have line breaks, as both GitHub and `git diff` in a terminal window cut off lines instead of wrapping them (and `git diff` doesn't even give you the option to scroll them into view, while GitHub only gives you that option via a horizontal scrollbar that is offscreen for all but the last page of changes).

It's also hard for hackers to later change files like this and for code archeologists to see what changed when.  So these lines should all be hard-wrapped to 80 characters.


> +## Events ##
> +
> +Chrome and add-on processes can asynchronously send arbitrary events to each other. The <code>[EventEmitter][]</code> interface has been overloaded to make handling these events simple and intuitive. For instance, here's a trivial e10s adapter that uses events:

I like the use of the event metaphor to describe this asynchronous message passing, since it resembles the use of events in high-level APIs for similar purposes!


> +<api name="send">
> +@method
> +  Sends an event asynchronously to the chrome process. Any additional arguments after `type` are passed as arguments to event listeners in the chrome process.
> +@param type {string}
> +  The type of event to send.
> +</api>

Nit: the JavaScript reference specifies optional arbitrary arguments with the construct "[arg1[, arg2[, ... argN]]". It would be useful to do so in this doc as well.


diff --git a/static-files/md/dev-guide/e10s.md b/static-files/md/dev-guide/e10s.md

> +First, the Jetpack Platform Loader initializes the Jetpack Platform and loads `main.js` in a separate process. We call this the *add-on process*.

We don't use "Jetpack Platform" anywhere else except in this document and the accompanying diagram, and it isn't defined in the glossary.  We should call it something else, like simply "add-on", i.e. "First, the add-on loader initializes the add-on and loads `main.js` in a separate process."


Otherwise, this looks fine.  Feedback+, and I reviewed the changes in addition to providing feedback, but r- due to the issue with long lines.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

One more review comment: the changes also need to update the tests landed in the fix for bug 617499.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

My impression is that this is good to go, modulo review issues, so I checked in a version of this patch with the review issues addressed.


Merge Commit:

https://github.com/mozilla/addon-sdk/commit/cb9739d463a3cfcd31215612f2a6e059ce8c601f


Review Comment Commits:

https://github.com/mozilla/addon-sdk/commit/8d2b97baaabb47c2bff8780d2f703127c43d21fe

https://github.com/mozilla/addon-sdk/commit/a25e4c83ce87f3d97342fd245c61fc538c1a15e1

https://github.com/mozilla/addon-sdk/commit/8267e0315b8a388269d64bf597a1124147ee0410


Topic Branch Commits:

https://github.com/toolness/jetpack-sdk/commit/c6d1ce050276dd6199c2ed43ada01866b7db4a8a

https://github.com/toolness/jetpack-sdk/commit/45f7cb2f23d408a6b292d8bf0fabb1e9672ea7d0

https://github.com/toolness/jetpack-sdk/commit/aec3c5f1211629a42a417179451c0c80884f0a2a

https://github.com/toolness/jetpack-sdk/commit/8a696e5bcb4a98dbe4919049eea18425c83c368f

https://github.com/toolness/jetpack-sdk/commit/9a9bca6bcdb6869032cab989d81db217ed9a7966

https://github.com/toolness/jetpack-sdk/commit/d9eb768b2213a7476c8c86070a709c55198b3707

https://github.com/toolness/jetpack-sdk/commit/7d164a0e370d9895c5a46d1324ad8be845d6e576

https://github.com/toolness/jetpack-sdk/commit/7dd2873fcc9ef38512ecdb58e9d3b933c1177a8a

https://github.com/toolness/jetpack-sdk/commit/aff5aa481205ce6fb7c27ca9e876fba9353b987e

https://github.com/toolness/jetpack-sdk/commit/e806834f89e0bfbce2b5a7a4e0dd3f18a3761b59

https://github.com/toolness/jetpack-sdk/commit/e3a5df801dc91a74fadfe34da5e20e7031f8959b

https://github.com/toolness/jetpack-sdk/commit/d78ca9b373e6467eca4356ff7464d560b4a7b9be

https://github.com/toolness/jetpack-sdk/commit/91b30a811f85a27e817da1aad58624353fd5285b

https://github.com/toolness/jetpack-sdk/commit/51d0f3d5e23cb740c3377b2063e10570b749048d

https://github.com/toolness/jetpack-sdk/commit/50066371722b11d235a3e925884618bb659751cb

https://github.com/toolness/jetpack-sdk/commit/4bb69343d3a83d24df84937c6afa2743a3170d47


Merge With Local Topic Branch:

https://github.com/mozilla/addon-sdk/commit/9e40ad03ce2acac9cf0e0eeb1e3319ad00988272

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Or, as gitbot puts it:

https://github.com/mozilla/addon-sdk/compare/723e4be...cb9739d