967325 - Implement Java->Gecko request API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

While working on bug 946022, I've found that we could really use a simple way to make asynchronous requests to Gecko. For example, in requestAutocomplete, we will send "edit" messages to Gecko; Gecko then does some validation, then responds with success or an error. By having a real API for this, we can hide all of the temporary listeners and unique ID generation otherwise required.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Implement sendRequestToGecko for async Gecko requests

Inspired by Chrome's extension message passing API (http://developer.chrome.com/extensions/messaging.html). Gecko calls addRequestListener to listen for these requests, and Java uses sendBroadcastRequest to initiate the request. Sample code incoming next.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Sample code

Sample code using the API here. With this patch applied, whenever you click Request Desktop Site, the following appears in logcat:

E/GeckoConsole( 6160): BRN: got request from Java: hello
D/BRN     ( 6160): got response from Gecko: bar

Comment 3

[Richard Newman [:rnewman]]

Am I right in thinking that this is the mirror image of Bug 870371? If so, can we make these things overlap?

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Sample code 2

This API should allow us to get rid of the whole X:Get/X:Data convention we currently use when making requests to Gecko. Here's another (more realistic) example patch that drops the SearchEngines:Data message.

I think this API will also allow us to simplify things like PrefsHelper. Bug 753312 implemented a less general version of these ID-based callbacks to solve some of the same problems.

(In reply to Richard Newman [:rnewman] from comment #3)
> Am I right in thinking that this is the mirror image of Bug 870371? If so,
> can we make these things overlap?

I took a brief look at 870371, but I'm not sure -- I'll have to take a closer look tomorrow (or maybe Nick can say).

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8369790 [details] [diff] [review]
Implement sendRequestToGecko for async Gecko requests

I've gone through several more iterations of this as I've worked on bug 946022. I'll wait until that's further along before updating this again.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Implement sendBroadcastRequest for async Gecko requests, v2

Here's the current version I'm using that the patches in bug 946022 are based on. I haven't touched this in awhile, so there are things that need updating (such as moving this to the new Messaging.jsm).

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest API

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest tests

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Created attachment 8461100 [details] [diff] [review]
> Implement GeckoRequest API

In case you're wondering why aListener is an object with an onResponse callback rather than just passing the callback itself, I did that for consistency with Services.obs.addObserver():

let Thing = {
  foo: "foo",
  bar: "bar",

  init: function () {
    Services.obs.addObserver(this, "Thing:Observe", false);
    Services.obs.addObserver(this, "Thing:Observe2", false);
    RequestService.addListener(this, "Thing:Request");
    RequestService.addListener(this, "Thing:Request2");
  },

  observe: function (aSubject, aTopic, aData) {
    if (aTopic == "Thing:Observe") {
      // ...
    } else {
      // ...
    }
  },

  onRequest: function (aMessage, aData, aSendResponse) {
    if (aMessage == "Thing:Request") {
      aSendResponse({ response: this.foo });
    } else {
      aSendResponse({ response: this.bar });
    }
  }
};

We could instead pass the function directly, in which case the request listener part of the code might look like this:

let Thing = {
  ...

  init: function () {
    ...
    RequestService.addListener(this.onRequest.bind(this), "Thing:Request");
    RequestService.addListener(this.onRequest2.bind(this), "Thing:Request2");
  },

  onRequest: function (aData, aSendResponse) {
    aSendResponse({ response: this.foo });
  },

  onRequest2: function (aData, aSendResponse) {
    aSendResponse({ response: this.bar });
  }
};

I do like how the second approach separates each callback into its own function, and it also allows the aMessage parameter to be omitted. The downside is that it's a bit more error-prone to keep "this" intact in the callback bodies (requiring the bind). I'm fine with either API if you guys have any opinions.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8461100 [details] [diff] [review]
Implement GeckoRequest API

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

I don't hate this, but I had some ideas/questions. So the f+ means I could be convinced to take it as is :)

::: mobile/android/base/GeckoAppShell.java
@@ +410,5 @@
>          PENDING_EVENTS.add(e);
>      }
>  
> +    @RobocopTarget
> +    public static void sendRequestToGecko(final GeckoRequest request) {

Put this in EventDispatcher, not GeckoAppShell. To be honest, I want to move the other even firing code from GeckoAppShell into EventDispatcher as well. Which makes me wonder if this will work well... i.e. that API would look like:

EventDispatcher.sendToGecko(event, data);

and this would add:
EventDispatcher.sendToGecko(request);

I would expect something more like:
EventDispatcher.sendToGecko(event, data, callback);

so that the callback looked like an optional third parameter. Maybe that's too much magic.

::: mobile/android/modules/RequestService.jsm
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [ "RequestService" ];

I kinda just want to put this in Messaging.jsm. Is there a good reason to have a separate jsm for it? It might be nice if the callback parameter was option here actually, then we could move all our listeners to this, even if they don't expect a response. That doesn't gain us much unless we want to move off the nsIObserver service some day (Maybe would make it easier to append a window id to every message?).

@@ +17,5 @@
> +
> +  addListener: function (aListener, aMessage) {
> +    if (aMessage in this._listeners) {
> +      Cu.reportError("Error in addListener: A listener already exists for message " + aMessage);
> +      return;

throw so the caller knows this didn't work.

@@ +22,5 @@
> +    }
> +
> +    if (typeof aListener.onRequest !== "function") {
> +      Cu.reportError("Error in addListener: Listener must have an onRequest method for message " + aMessage);
> +      return;

Same. Throw here.

@@ +32,5 @@
> +
> +  removeListener: function (aMessage) {
> +    if (!(aMessage in this._listeners)) {
> +      Cu.reportError("Error in removeListener: There is no listener for message " + aMessage);
> +      return;

And throw here.

@@ +42,5 @@
> +
> +  observe: function (aSubject, aTopic, aData) {
> +    let wrapper = JSON.parse(aData);
> +    let listener = this._listeners[aTopic];
> +    listener.onRequest(aTopic, wrapper.data, function (response) {

It would be nice if this was more like:

var resp = listener.onRequest(aTopic, wrapper.data);
sendMessageToJava(resp);

but that doesn't allow async actions to work very well. We could do something like:

var resp = listener.onRequest(aTopic, wrapper.data);
if (resp instanceof Promise) {
  resp.then(sendMessageToJava)
      .catch(sendErrorToJava);
} else {
  sendMessageToJava();
}

That's actually a bit safer because if the listener forgets to return something, we'll still notify java. If they return a promise that never resolves... that's a bug :) In fact, you could wrap your call in a promise so that you didn't even really need the instanceof check. i.e.

new Promise((resolve, reject) => {
  resolve(listener.onRequest());
}).then(sendMesageToJava)
  .catch(sendErrorToJava);

A caller would use:

RequestService.addListener(function() {
  return 5;

  // or!

  return new Promise((resolve, reject) => {
    resolve(5+5);
  });
}, "something");

@@ +51,5 @@
> +    });
> +  },
> +};
> +
> +let RequestService = Object.freeze({

I'm not a big fan of freezing things in JS. It feels against the spirit of the language. I'll leave it up to you though.

@@ +60,5 @@
> +   * listener *must* call aSendResponse to prevent leaks.
> +   *
> +   * Example usage:
> +   *   RequestService.addListener({
> +   *     onRequest: function (aMessage, aData, aSendResponse) {

Why does this take an obj parameter and not just a function? Future proofing?

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #10)
> ::: mobile/android/base/GeckoAppShell.java
> @@ +410,5 @@
> >          PENDING_EVENTS.add(e);
> >      }
> >  
> > +    @RobocopTarget
> > +    public static void sendRequestToGecko(final GeckoRequest request) {
> 
> Put this in EventDispatcher, not GeckoAppShell.

Is EventDispatcher the right place? Now, it's used solely for incoming (Gecko->Java) message dispatching, so adding a sendRequestToGecko() method there might seem out of place. But I guess that won't be true if we move all sendXToGecko() methods there.

> To be honest, I want to move
> the other even firing code from GeckoAppShell into EventDispatcher as well.
> Which makes me wonder if this will work well... i.e. that API would look
> like:
> 
> EventDispatcher.sendToGecko(event, data);

I'd like these changes too, though this would end up touching a ton of files. I can file a follow-up refactoring bug.

> ::: mobile/android/modules/RequestService.jsm
> @@ +4,5 @@
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +this.EXPORTED_SYMBOLS = [ "RequestService" ];
> 
> I kinda just want to put this in Messaging.jsm. Is there a good reason to
> have a separate jsm for it? It might be nice if the callback parameter was
> option here actually, then we could move all our listeners to this, even if
> they don't expect a response. That doesn't gain us much unless we want to
> move off the nsIObserver service some day (Maybe would make it easier to
> append a window id to every message?).

The main reason I didn't put it in Messaging.jsm was namespacing; Messaging.jsm doesn't follow the normal jsm convention of exporting a namespace object and just exports sendMessageToJava globally. I didn't want to pollute the global namespace more by exporting addRequestListener/removeRequestListener globally. I guess I could add the RequestService object as-is and export that alongside sendMessageToJava, but then that seems like it doesn't belong.

Ideally, sendMessageToJava would be namespaced (and maybe rename the module to JavaBridge?) so that we could have them all together as JavaBridge.sendMessage, JavaBridge.addListener, and JavaBridge.removeListener. But again, that will touch a lot of code, so maybe another follow-up...

Regarding factoring out our Services.obs.addObserver() listeners to this module: I was thinking the same myself. It would be nice to have a central messaging API for Java instead of having these pieces strewn out everywhere. Of course, the damage is already done, but I guess a fix is better late than never.
 
> @@ +42,5 @@
> but that doesn't allow async actions to work very well. We could do
> something like:
<snip>

Yeah, I did consider using promises for the reasons you mentioned, but didn't see an obvious way to integrate them nicely into the API. I'll play around with the different approaches to see what works well.

> @@ +51,5 @@
> > +    });
> > +  },
> > +};
> > +
> > +let RequestService = Object.freeze({
> 
> I'm not a big fan of freezing things in JS. It feels against the spirit of
> the language. I'll leave it up to you though.

OK -- I guess it might make sense to leave it open in case add-ons want to inject some kind of benchmarking/debugging logic.

> @@ +60,5 @@
> > +   * listener *must* call aSendResponse to prevent leaks.
> > +   *
> > +   * Example usage:
> > +   *   RequestService.addListener({
> > +   *     onRequest: function (aMessage, aData, aSendResponse) {
> 
> Why does this take an obj parameter and not just a function? Future proofing?

Heh, beat you to it! See comment 9. I'm happy to change this to take a function if you guys prefer.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8461100 [details] [diff] [review]
Implement GeckoRequest API

Downgrading to f? until a new patch is up with the proposed changes.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest API, v2

I kept sendRequestToGecko in GeckoAppShell to keep it together with the sendEventToGecko methods, and didn't want to move it yet since that change will touch a lot of files. I filed bug 1043632 to address this.

I moved RequestService into Messaging.jsm, but I'm not really happy with the awkward exporting of sendMessageToJava + RequestService in that module. Ideally, we would namespace these functions so they'd be attached to a single exported object. Filed as bug 1043633.

All other comments have been addressed. I ended up using a generator function as the observer, which yields at the listener call. This allows the listener to be either a normal function or a generator function, the latter which can be used for async callbacks. I included a couple of examples in the JS documentation in Messaging.jsm.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest tests, v2

Updated tests for GeckoRequest. I'll wait for r+ on the first patch before flipping this to r?.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8462016 [details] [diff] [review]
Implement GeckoRequest tests, v2

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

::: mobile/android/base/GeckoRequest.java
@@ +9,5 @@
>  import org.mozilla.gecko.util.NativeJSObject;
>  
>  import android.util.Log;
>  
> +@RobocopTarget

I'm pretty sure this will prevent the whole class from being optimized, which you probably don't want. Maybe @RobocopTarget the constructor instead?

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8462015 [details] [diff] [review]
Implement GeckoRequest API, v2

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

Looks okay overall. I just have some questions.

::: mobile/android/base/GeckoAppShell.java
@@ +410,5 @@
>          PENDING_EVENTS.add(e);
>      }
>  
> +    @RobocopTarget
> +    public static void sendRequestToGecko(final GeckoRequest request) {

Can you add some javadoc comments?

I agree with moving this to EventDispatcher (maybe rename EventDispatcher to something else) in a followup bug.

@@ +417,5 @@
> +        EventDispatcher.getInstance().registerGeckoThreadListener(new NativeEventListener() {
> +            @Override
> +            public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +                EventDispatcher.getInstance().unregisterGeckoThreadListener(this, event);
> +                if (!message.has("response")) {

Note that has() returns false for both undefined and null properties, which is different from JSONObject.has(). Make sure that's what you want.

@@ +421,5 @@
> +                if (!message.has("response")) {
> +                    request.onError();
> +                    return;
> +                }
> +                request.onResponse(message.getObject("response"));

Do callers expect onError/onResponse to run on the Gecko thread? We should either document that or post to a more appropriate thread.

::: mobile/android/base/GeckoRequest.java
@@ +1,1 @@
> +package org.mozilla.gecko;

Consider putting it in org.mozilla.gecko.util

@@ +41,5 @@
> +    public abstract void onResponse(NativeJSObject nativeJSObject);
> +
> +    @RobocopTarget
> +    public void onError() {
> +        throw new GeckoRequestException("Unhandled error for GeckoRequest: " + eventName);

I would just use a built-in exception class. A custom class takes up space and seems unnecessary.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8462016 [details] [diff] [review]
Implement GeckoRequest tests, v2

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

::: mobile/android/base/tests/robocop.ini
@@ +127,5 @@
>  skip-if = android_version == "10"
>  [testJavascriptBridge]
>  [testNativeCrypto]
>  [testSessionHistory]
> +[testGeckoRequest]

Alphabetize within the UITest section

::: mobile/android/base/tests/testGeckoRequest.java
@@ +70,5 @@
> +                responseReceived = true;
> +            }
> +        });
> +
> +        getSolo().waitForCondition(new Condition() {

Use WaitHelper.waitFor

@@ +77,5 @@
> +                return responseReceived;
> +            }
> +        }, MAX_WAIT_MS);
> +
> +        getAsserter().ok(responseReceived, "Received response for registered listener", null);

I think you want to use methods in AssertionHelper?

::: mobile/android/base/tests/testGeckoRequest.js
@@ +1,3 @@
> +Components.utils.import("resource://gre/modules/Messaging.jsm");
> +
> +new JavaBridge(this);

You want to call disconnect() on your JavaBridge object at the end of the test.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 8462015 [details] [diff] [review]
Implement GeckoRequest API, v2

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

Nice. Some nits, and a question about the error handling in JS.

::: mobile/android/base/GeckoAppShell.java
@@ +417,5 @@
> +        EventDispatcher.getInstance().registerGeckoThreadListener(new NativeEventListener() {
> +            @Override
> +            public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +                EventDispatcher.getInstance().unregisterGeckoThreadListener(this, event);
> +                if (!message.has("response")) {

Make "response" a const (you use it in multiple places).

::: mobile/android/base/GeckoRequest.java
@@ +17,5 @@
> +    private final GeckoEvent event;
> +    private final String eventName;
> +
> +    public GeckoRequest(String eventName, Object data) {
> +        this.eventName = eventName;

I'd just use "name" or "type" instead of "eventName"

@@ +26,5 @@
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSON error", e);
> +        }
> +
> +        event = GeckoEvent.createBroadcastEvent(eventName, message.toString());

Is there some reason to build this here and hold on to it?

::: mobile/android/modules/Messaging.jsm
@@ +124,5 @@
> +
> +    try {
> +      response = yield listener(wrapper.data);
> +    } catch (e) {
> +      Cu.reportError(e);

Should this send an error back to Java? In fact, (once that's fixed) throwing is the only way the real listener could send back an error too. Might be worth documenting.

Comment 19

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest API, v3

Updated with all review comments addressed. Some answers below.

(In reply to Jim Chen [:jchen :nchen] from comment #16)
> @@ +417,5 @@
> > +        EventDispatcher.getInstance().registerGeckoThreadListener(new NativeEventListener() {
> > +            @Override
> > +            public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> > +                EventDispatcher.getInstance().unregisterGeckoThreadListener(this, event);
> > +                if (!message.has("response")) {
> 
> Note that has() returns false for both undefined and null properties, which
> is different from JSONObject.has(). Make sure that's what you want.

Yeah, a null response means there was an error. In fact, the callback response *must* be an object, so any non-object (a string, number, etc) returned by the callback function will also be considered an error.

The reason for this limitation is that NativeJSObject has no JSONObject#get() equivalent where an arbitrary type can be read (if we had that, I'd probably change the onResponse callback to have a generic Object instead of a NativeJSONObject).

> Do callers expect onError/onResponse to run on the Gecko thread? We should
> either document that or post to a more appropriate thread.

I would think so -- that's the same thread handleMessage callbacks are executed on. I don't know of a more appropriate default, and I think it's better to let the callbacks be responsible for posting to another thread, if necessary, rather than assuming the callback should happen on the background thread/UI thread/etc. and being wrong.

(In reply to Wesley Johnston (:wesj) from comment #18)
> ::: mobile/android/modules/Messaging.jsm
> @@ +124,5 @@
> > +
> > +    try {
> > +      response = yield listener(wrapper.data);
> > +    } catch (e) {
> > +      Cu.reportError(e);
> 
> Should this send an error back to Java? In fact, (once that's fixed)
> throwing is the only way the real listener could send back an error too.
> Might be worth documenting.

Yeah, it does send an error back to Java (see note about null above).

In general, though, this callback shouldn't be used to send back an error from Gecko. The reason is that the Java-side request handler would have no way to differentiate between "expected" requests (where the listener would intentionally throw, as you said) versus logic/coding errors that shouldn't be recoverable. If the listener wants to send back an error, it can just include an error flag/error data in the response object, and GeckoRequest#onResponse() handler can handle the error appropriately. I've updated the JavaDocs explaining this.

I'm still on the fence about whether the onError callback is worth having at all...the only reason it exists is so the Robocop test can hook into it.

Comment 20

[Brian Nicholson (:bnicholson)]

(In reply to Jim Chen [:jchen :nchen] from comment #17)
> ::: mobile/android/base/tests/testGeckoRequest.js
> @@ +1,3 @@
> > +Components.utils.import("resource://gre/modules/Messaging.jsm");
> > +
> > +new JavaBridge(this);
> 
> You want to call disconnect() on your JavaBridge object at the end of the
> test.

I thought I might need that too, so I tried adding a listener like we do at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testEventDispatcher.js#5. But when I added logging to the do_register_cleanup() callback, I never saw the callback get executed. I also pushed the test to try without the disconnect() call, and everything was green.

In general, trying to figure out some of the Robocop JS API was frustrating. Most of the tests use add_test()/run_next_test(), but I guess I shouldn't use that here since this isn't a JavaScriptTest? It's also unclear what do_test_pending() does, if anything (which is another line that shows up in testEventDispatcher.js linked above). There isn't much documentation about any of these.

Can you give a brief overview of which Robocop methods to call, and when? I didn't want to contribute to more cargo cult code if these calls have no effect.

Comment 21

[Brian Nicholson (:bnicholson)]

do_test_finished() is one more method I forgot to mention whose purpose is unclear: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testEventDispatcher.js#52

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8468906 [details] [diff] [review]
Implement GeckoRequest API, v3

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

LGTM.

::: mobile/android/base/GeckoAppShell.java
@@ +427,5 @@
> +
> +        EventDispatcher.getInstance().registerGeckoThreadListener(new NativeEventListener() {
> +            @Override
> +            public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +                EventDispatcher.getInstance().unregisterGeckoThreadListener(this, event);

I filed bug 1050444 to fix the memory leak that (I think) will result from this kind of usage. Essentially after we unregister here, EventDispatcher will still keep the event in its map, thus leaking memory because we're never using this event again.

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Brian Nicholson (:bnicholson) from comment #21)
> do_test_finished() is one more method I forgot to mention whose purpose is
> unclear:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> testEventDispatcher.js#52

So JavaBridge is just a messaging tool. The test itself is still (mostly) based on xpcshell tests, which are documented at [1] and because Java/JS messaging is async, we should follow the async usage at [2].

I think it's a known bug that the cleanup methods are not called at the end of the test, but in the meantime, tests should still follow the protocol and register for cleanup.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
[2] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Async_tests

Comment 24

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #21)
> do_test_finished() is one more method I forgot to mention whose purpose is
> unclear

do_test_pending and do_test_finished should pair up. It's how you tell the test runner that you're waiting for an async response.

add_test/run_next_test is the alternative async test approach.

You should always use run_next_test in new code.

Comment 25

[Brian Nicholson (:bnicholson)]

Attachment: Implement GeckoRequest tests, v3

Addressed feedback.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8469656 [details] [diff] [review]
Implement GeckoRequest tests, v3

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

lgtm w/ nits.

::: mobile/android/base/tests/robocop.ini
@@ +122,5 @@
>  [testAboutHomeVisibility]
>  # disabled on Android 2.3; bug 946656
>  skip-if = android_version == "10"
>  [testAppMenuPathways]
> +[testGeckoRequest]

nit: Alphabetize.

::: mobile/android/base/tests/testGeckoRequest.java
@@ +39,5 @@
> +        // Register a listener for this request.
> +        js.syncCall("add_request_listener", REQUEST_EVENT);
> +
> +        // Make sure we receive the expected response.
> +        checkFooRequest();

To be more thorough, you can check onError was not called for checkFooRequest and checkUnregisteredRequest.

@@ +59,5 @@
> +        js.syncCall("finish_test");
> +    }
> +
> +    private void checkFooRequest() {
> +        responseReceived = false;

`volatile` or `AtomicBoolean`.

Are you using a member var because with function-scoped vars, the var would need to be final to be passed into the anonymous functions? This can be fixed with the `AtomicBoolean`.

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 8468906 [details] [diff] [review]
Implement GeckoRequest API, v3

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

::: mobile/android/base/util/GeckoRequest.java
@@ +75,5 @@
> +     *
> +     * In general, this should not be overridden since there's no way to differentiate between
> +     * expected errors and logic errors in JS. If the Gecko-side request handler wants to send a
> +     * recoverable error to Java, it should include any error data in the response object that the
> +     * {@link #onResponse(NativeJSObject)} callback can handle as necessary.

:( I'm not in love with this. I really like how node basically forces you to at least acknowledge there might be errors. i.e. I think callers onError should be abstract here and I think we should just forward whatever JS error object we get up the tree here. I wish I'd done that in the JS api a little better. But I'm also not going to hold this up on it either.

Comment 28

[Brian Nicholson (:bnicholson)]

(In reply to Michael Comella (:mcomella) from comment #26)
> To be more thorough, you can check onError was not called for
> checkFooRequest and checkUnregisteredRequest.

Since onError throws by default and the test will fail anyway, I didn't want to add too much clutter.

(In reply to Wesley Johnston (:wesj) from comment #27)
> :( I'm not in love with this. I really like how node basically forces you to
> at least acknowledge there might be errors. i.e. I think callers onError
> should be abstract here and I think we should just forward whatever JS error
> object we get up the tree here. I wish I'd done that in the JS api a little
> better. But I'm also not going to hold this up on it either.

I'm not sure I'd want to require callers to have to handle onError since it may not make sense in all situations; for instance, maybe the JS doesn't throw, or maybe it does throw but gets caught on the JS side. I'd be worried about people getting in the habit of using empty onError handlers everywhere, which wouldn't be much of an improvement.

That said, I'm not against including error support...I'm just not sure of the best way to handle it. I filed bug 1052158 so we can think about it more.

https://hg.mozilla.org/integration/fx-team/rev/bbd7ca6452ff
https://hg.mozilla.org/integration/fx-team/rev/5db7e8e8bd6c

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out in https://hg.mozilla.org/integration/fx-team/rev/65b4a34e2218 for robocop failures: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=45702654&tree=Fx-Team

Comment 30

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=65b4a34e2218

Comment 31

[Brian Nicholson (:bnicholson)]

Re-triggered runs show that this is an intermittent failure on 2.3, so the likely culprit was that the timeout wasn't long enough for these slow devices. Upping the timeout goes green as expected: https://tbpl.mozilla.org/?tree=Try&rev=519cb04f1618

https://hg.mozilla.org/integration/fx-team/rev/91b17eac92e9
https://hg.mozilla.org/integration/fx-team/rev/02870060541b

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/91b17eac92e9
https://hg.mozilla.org/mozilla-central/rev/02870060541b