Closed Bug 993195 Opened 6 years ago Closed 6 years ago

Add support for sending callbacks from NativeJSObject listeners

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(2 files, 2 obsolete files)

EventDispatcher#sendResponse takes a JSONObject message as its first parameter, meaning it can be used only with the deprecated GeckoEventListener API. We should update sendResponse to support NativeJSObject.
Minor fix to set up one listener for responses instead of two.
Attachment #8403011 - Flags: review?(wjohnston)
My first attempt simply copied sendResponse/sendError to accept a NativeJSObject in addition to JSONObject, but that won't work since the callbacks may be executed after the handleMessage(), meaning the message may have been disposed.

Since we're changing our handleMessage API anyway, this seemed like a good opportunity to build in proper event response callbacks. With this patch, handleMessage now takes a third argument, a GeckoCallback, which listeners can use to send a response back to Gecko.

This patch has several changes from our old approach:
1) We no longer have to hold onto the raw message given to us to use sendResponse.
2) If we try to send a response or error more than once, we throw an exception.
3) If there aren't any listeners on the Java side to respond to the callback, we make sure to send a cancel response to avoid leaking observers. We could consider throwing here too, though I figured there may be valid situations where response listeners aren't attached. Also note that this still isn't perfect; the existence of listeners doesn't guarantee that any of those listeners will actually send a response. For debug builds, we might want to consider overriding finalize() in GeckoCallback to enforce that sent is true when it's destroyed.

Note that in our build system, classes in util/ cannot reference other o.m.g. classes. The weird GeckoCallback interface split here is used as a workaround.
Attachment #8403022 - Flags: review?(wjohnston)
Attachment #8403022 - Flags: review?(nchen)
Minor fix to use optString for the GUID.
Attachment #8403022 - Attachment is obsolete: true
Attachment #8403022 - Flags: review?(wjohnston)
Attachment #8403022 - Flags: review?(nchen)
Attachment #8403023 - Flags: review?(wjohnston)
Attachment #8403023 - Flags: review?(nchen)
Comment on attachment 8403011 [details] [diff] [review]
Add only one observer for messages with callbacks

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

::: mobile/android/base/EventDispatcher.java
@@ +210,1 @@
>              wrapper.put("response", response);

Result and response are confusing. "success"? "error"?
Attachment #8403011 - Flags: review?(wjohnston) → review+
Comment on attachment 8403023 [details] [diff] [review]
Add GeckoCallback parameter to handleMessage, v2

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

Looks good; just one comment and some nits.

::: mobile/android/base/EventDispatcher.java
@@ +148,5 @@
>                  listeners = mGeckoThreadNativeListeners.get(type);
>              }
> +
> +            GeckoCallback callback = null;
> +            String guid = message.optString(GUID, null);

final

@@ +166,5 @@
>                  return;
> +            } else {
> +                // If there are no listeners, execute the callback immediately to prevent
> +                // Gecko-side observers from being leaked.
> +                callback.sendCancel();

I think we should call sendCancel in dispatchEvent(JSONObject) instead. If there's no native listener, we try dispatching to JSON listeners next. If there's no JSON listener either, we can then call sendCancel.

@@ +230,5 @@
>      }
> +
> +    private static class GeckoEventCallback implements GeckoCallback {
> +        private String guid;
> +        private String type;

final for guid and type

@@ +250,5 @@
> +        public void sendCancel() {
> +            sendHelper(RESULT_CANCEL, null);
> +        }
> +
> +        private void sendHelper(String result, Object response) {

final args

@@ +252,5 @@
> +        }
> +
> +        private void sendHelper(String result, Object response) {
> +            if (sent) {
> +                throw new IllegalStateException("Callback has already been executed for type=" + type + ", guid=" + guid);

Wrap line

@@ +262,5 @@
> +                final JSONObject wrapper = new JSONObject();
> +                wrapper.put(GUID, guid);
> +                wrapper.put("result", result);
> +                wrapper.put("response", response);
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(type + ":Response", wrapper.toString()));

Wrap line

@@ +263,5 @@
> +                wrapper.put(GUID, guid);
> +                wrapper.put("result", result);
> +                wrapper.put("response", response);
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(type + ":Response", wrapper.toString()));
> +            } catch (JSONException e) {

final

::: mobile/android/base/util/GeckoCallback.java
@@ +1,3 @@
> +package org.mozilla.gecko.util;
> +
> +public interface GeckoCallback {

I'd rather it be called EventCallback than GeckoCallback.

@@ +2,5 @@
> +
> +public interface GeckoCallback {
> +    public void sendResponse(Object response);
> +    public void sendError(Object response);
> +    public void sendCancel();

Please add some docs.
Attachment #8403023 - Flags: review?(wjohnston) → feedback+
Thanks for the review; comments addressed.
Attachment #8403023 - Attachment is obsolete: true
Attachment #8403023 - Flags: review?(nchen)
Attachment #8403524 - Flags: review?(nchen)
Comment on attachment 8403524 [details] [diff] [review]
Add GeckoCallback parameter to handleMessage, v3

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

LGTM. Thanks!
Attachment #8403524 - Flags: review?(nchen) → review+
https://hg.mozilla.org/mozilla-central/rev/5af035625dc0
https://hg.mozilla.org/mozilla-central/rev/655f3cf12f45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.