1055144 - Update Messaging.jsm API to use promises

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Promises offer a number of advantages over callbacks, such as safer error handling and cleaner code. One of the most common async operations for Android is sendMessageToJava, which uses callbacks for the response. Let's update this API to use promises.

Comment 1

[Wesley Johnston (:wesj)]

TBH, I intentionally wrote it this way originally because its super easy to wrap in promises if you want:

return new Promise(function(resolve, reject) {
  sendMessageToJava(msg, resolve, reject);
});

but doesn't require them if you don't.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Implement sendRequestToJava

Renames sendMessageToJava(message, callback) -> sendRequestToJava(message). I know you'd probably prefer using the same function name for both like we do now, but we can't do that using promises since there's no way to tell whether the message is a request or not given a single message argument. By splitting this into two separate functions, we also have more parity with our Java-side API.

I removed the "cancel" callback since a request should be an all-or-nothing scenario; either you get what you asked for or you don't. "cancel" essentially just eats the request, which could leak resources if caller is expecting a response.

Note that I added sendRequestToJava as an exported function in this patch, but it's pretty terrible adding more global exports in that module. I'm fixing this in bug 1043633; ideally, these will land together.

Comment 3

[Brian Nicholson (:bnicholson)]

Try push for this bug + bug 1043633: https://tbpl.mozilla.org/?tree=Try&rev=6d6529d514fb

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #1)
> TBH, I intentionally wrote it this way originally because its super easy to
> wrap in promises if you want:
> 
> return new Promise(function(resolve, reject) {
>   sendMessageToJava(msg, resolve, reject);
> });
> 
> but doesn't require them if you don't.

I don't think it's quite that easy -- note that sendMessageToJava takes a single callback function, not two of them. For an example of how we currently have to wrap the API using promises, see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?force=1#236.

But why wouldn't you want them? Promises provide better built-in support for errors, so avoiding promises loses that advantage. In general, Mozilla APIs are slowly replacing the older callback-based methods with promise-based ones.

Having promise-compatible APIs enable very elegant code. Consider:

let fn = Task.async(function* () {
  let response = yield sendRequestToJava({ foo: bar });
  console.log("got response: " + response);
});

Currently, that would have to be written like this:

let fn = Task.async(function* () {
  let response = yield new Promise(function (resolve, reject) {
    sendMessageToJava({ foo: bar }, function (res, err) {
      (err) ? reject(err) : resolve(res);
    });
  });
  console.log("got response: " + response);
});

Comment 5

[Wesley Johnston (:wesj)]

I've just forgotten how my api looked :) Its not hard enough to convert either way I guess:

var send = function(msg, callback) {
  sendMessageToJava(msg).then(callback)
                        .catch(callback.bind(this, null));
}

We talked about this a bit. I don't have a strong preference against, and I don't think anyone on our team will fight it anymore either(?) so lets give it a whirl. If a callers really really really needs callbacks, they can write their own nsIAndroidBridge methods?

Comment 6

[:Margaret Leibovic]

Drive-by request: Is Messaging.jsm something that we expect add-ons to use? If so, we should make an MDN article for Messaging.jsm, so that we can update it to talk about this change :)

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8474672 [details] [diff] [review]
Implement sendRequestToJava

Review of attachment 8474672 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/Messaging.jsm
@@ +5,5 @@
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

Do we need this? I thought promises were available in jsms now...

@@ +23,5 @@
> +    sendRequestToJava(aMessage)
> +      .then(response => aCallback(response, null))
> +      .catch(response => aCallback(null, response));
> +  } else {
> +    Services.androidBridge.handleGeckoMessage(aMessage);

So this becomes the API if you don't want a response, and the other becomes the API if you do. That scares me a bit. It seems easy to get wrong. I wonder if we can pass a boolean or something to the other method... (I hate that idea too).

@@ +30,5 @@
> +
> +/**
> + * Sends a message to Java, returning a promise that resolves to the response.
> + */
> +function sendRequestToJava(aMessage) {

I'd probably put this in your current RequestService

@@ +54,5 @@
>      aMessage.__guid__ = id;
>      Services.obs.addObserver(obs, aMessage.type + ":Response", false);
> +  });
> +
> +  Services.androidBridge.handleGeckoMessage(aMessage);

I'd put this inside the promise, after the response observer was added. This seems like it could be racy here.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8474672 [details] [diff] [review]
Implement sendRequestToJava

Review of attachment 8474672 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Wesley Johnston (:wesj) from comment #7)
> So this becomes the API if you don't want a response, and the other becomes
> the API if you do. That scares me a bit. It seems easy to get wrong. I
> wonder if we can pass a boolean or something to the other method... (I hate
> that idea too).
I suggested sendRequestForResponse in the other bug, since it kinda maps to Android's startActivityForResponse().

> I'd probably put this in your current RequestService
I know you're using the other bug for this. I'm fine either way.

I DO think the handleGeckoMessage call needs to be in the promise, so thats the r-. (But once its fixed I don't mind if you flip to r+)

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #8)
> I DO think the handleGeckoMessage call needs to be in the promise

It doesn't; the Promise spec says that the function given to Promise is executed immediately. But it works either way, so I'll move it there if you think it's clearer.

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Implement sendRequestForResult

Updated for comments and rebased onto bug 1043633.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Change callback-based sendMessageToJava calls to Messaging.sendRequestForResult

Updates remaining sendMessageToJava calls to use new API.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e382e31a4600

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8479382 [details] [diff] [review]
Implement sendRequestForResult

Review of attachment 8479382 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/Messaging.jsm
@@ +9,2 @@
>  Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Random move?

@@ +20,5 @@
>  
> +  if (aCallback) {
> +    Messaging.sendRequestForResult(aMessage)
> +      .then(result => aCallback(result, null),
> +            error => aCallback(null, error));

So pretty :)

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8479384 [details] [diff] [review]
Change callback-based sendMessageToJava calls to Messaging.sendRequestForResult

Review of attachment 8479384 [details] [diff] [review]:
-----------------------------------------------------------------

Nice :) Try run?

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Nice :) Try run?

Done (comment 11).

Comment 15

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/b5646b91eff5
https://hg.mozilla.org/integration/fx-team/rev/29d6f1d24751

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5646b91eff5
https://hg.mozilla.org/mozilla-central/rev/29d6f1d24751