Closed Bug 946344 Opened 6 years ago Closed 6 years ago

Replace GeckoEventResponder with an async callback mechanism

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(3 files, 9 obsolete files)

We should remove GeckoEventResponder. It has three implementors I know of.

Prompts (which don't really need it anymore)

JavaAddonManager, not widely (at all?) used, but allows you to get a response from a java addon file

SharedPrefs, which use it as a quick way to get back pref data

We could replace it with a simpler extension to the current API

sendMessageToJava(msg, callback);

or with promises?

JavaAsyncHandler.send(msg).then(function(data) { } , function(err) { });

The handler would do the business of registering an observer for msg.type + ":Response" and msg.type + ":Error". When it got back a result, it could call all the registered handlers, removing them as it went (if they responded true?).

This might be useful as a way of replacing some of our current redundant logic. I know prompts do a lot of registering response listeners, sending a message, and then waiting. Does anyone else?
Thumbs up!
OS: Linux → Android
Hardware: x86_64 → All
Attached patch Patch 1/? (obsolete) — Splinter Review
I needed this sort of behavior for something else. This just sets up some infrastructure. Creates a Utils.jsm package that just hosts this function (not in an object even for backwards compat?).

Moving everyone over to using it is less trivial. Some of these methods don't transition to async behavior very well. For instance, we have a selector for "Open in App" that queries Android to see if there are any apps for this link. A couple options there

1.) We could write a fancy system where the selectors all run async and we show the context menu once we've heard back from all of them
2.) cut that context menu item,
3.) use something like our Services.tm.processNextEvent trick from the PromptService. Maybe even include a special sendMessageAsync method that would pretend be async behind the scenes, but sync to users....

I assume I'll hit similar questions all over :)
Attachment #8345448 - Flags: review?(mark.finkle)
Comment on attachment 8345448 [details] [diff] [review]
Patch 1/?

>diff --git a/mobile/android/base/EventHelper.java b/mobile/android/base/EventHelper.java

>+public class EventHelper {
>+    private static final String GUID = "__callbackGuid__";

Let's just use "__guid__"

>+    public static void sendResponse(JSONObject message, JSONObject response) {
>+    public static void sendError(JSONObject message, JSONObject error) {

There are no examples of hos this class is used, but I have a pretty good idea. Any code that wants to return data to the caller would use:
EventHelper.sendXxx and pass in the JSON message the caller received from JS.

Why not make this part of EventDispatcher.java? That would keep all the event code in one place.

>diff --git a/mobile/android/modules/Utils.jsm b/mobile/android/modules/Utils.jsm

Let's call it Messaging.jsm (open to other variants too) since Utils.jsm seems a bit too generic.

Message.jsm or Messages.jsm

>+function sendMessageToJava(aMessage, aCallback) {

>+  return Services.androidBridge.handleGeckoMessage(JSON.stringify(aMessage));

I suppose you are still returning here until we migrate the existing code to use the callback. At some point we need to stop returning.

r-, but only to settle the file naming and moving the methods to EventDispatcher. Once we get a decision, it's an easy r+
Attachment #8345448 - Flags: review?(mark.finkle) → review-
Attached patch Patch 1/8 (obsolete) — Splinter Review
This is the same patch. Fixed up.
Attachment #8345448 - Attachment is obsolete: true
Attachment #8346916 - Flags: review?(mark.finkle)
Attached patch Patch 2/8 - Move the dispatcher (obsolete) — Splinter Review
The EventDispatcher is in utils, which is built in a separate jar from Gecko stuff. I like the idea of moving stuff there, so I moved it out of utils.
Attachment #8346917 - Flags: review?(mark.finkle)
Attached patch Patch 3/8 - HelperApps (obsolete) — Splinter Review
And now we start moving stuff. We could make all these API's async, but for now I just made them spin the thread (when we had to).
Attachment #8346918 - Flags: review?(mark.finkle)
Attached patch Patch 4/8 - Move webapps (obsolete) — Splinter Review
Move the webapps code over. I did make this async. Some re-org to make it (more) readable.
Attachment #8346920 - Flags: review?(mark.finkle)
Attached patch Patch 5/8 (obsolete) — Splinter Review
This kills off ALL of the old sync-prompts code. Lots of deleting :) But there's one odd guy in the media code that uses AndroidBridge::ShowFilePicker to show the sync file picker. I moved it to use xpcom. That probably needs review from those guys. Blassey, you've been in there?
Attachment #8346922 - Flags: review?(blassey.bugs)
Attached patch Patch 6/8 - JavaAddonManager (obsolete) — Splinter Review
This moves the java addon manager. It will break java addons AFAICT. I'm fine with that.
Attachment #8346923 - Flags: review?(bugmail.mozilla)
Attached patch Patch 7/8 - Shared prefs (obsolete) — Splinter Review
Shared prefs. Using the thread trick again to avoid having to fix every caller...
Attachment #8346924 - Flags: review?(mark.finkle)
And with that, this class can die.
Attachment #8346925 - Flags: review?(mark.finkle)
Comment on attachment 8346923 [details] [diff] [review]
Patch 6/8 - JavaAddonManager

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

::: mobile/android/base/JavaAddonManager.java
@@ +185,5 @@
> +
> +                JSONObject obj = new JSONObject();
> +                obj.put("response", mBundle.getString("response"));
> +                EventDispatcher.sendResponse(json, obj);
> +                mBundle = null;

You can turn mBundle into a local variable. The only reason it was an instance variable is so that it could be accessed from getResponse
Attachment #8346923 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8346922 [details] [diff] [review]
Patch 5/8

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

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +176,5 @@
> +  nsresult rv = filePicker->Init(nullptr, title, mode);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  filePicker->AppendFilters(nsIFilePicker::filterImages);
> +
> +  // XXX - This API should be made async

file a bug
Attachment #8346922 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8346916 [details] [diff] [review]
Patch 1/8

>diff --git a/mobile/android/base/EventHelper.java b/mobile/android/base/EventHelper.java
This file is not used, right? Remove it?
Attachment #8346916 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #14)
> Comment on attachment 8346916 [details] [diff] [review]
> Patch 1/8
> 
> >diff --git a/mobile/android/base/EventHelper.java b/mobile/android/base/EventHelper.java
> This file is not used, right? Remove it?

You remove it in patch 2
Attachment #8346917 - Flags: review?(mark.finkle) → review+
Comment on attachment 8346918 [details] [diff] [review]
Patch 3/8 - HelperApps

Someday, we should look at the places where we spin the main thread and try to switch to async APIs. File a bug?
Attachment #8346918 - Flags: review?(mark.finkle) → review+
Comment on attachment 8346920 [details] [diff] [review]
Patch 4/8 - Move webapps

This will bitrot Myk a bit. Let's give him a heads up.
Attachment #8346920 - Flags: review?(mark.finkle) → review+
Comment on attachment 8346924 [details] [diff] [review]
Patch 7/8 - Shared prefs

Just a note: Remember to Try server this set of patches.
Attachment #8346924 - Flags: review?(mark.finkle) → review+
Attachment #8346925 - Flags: review?(mark.finkle) → review+
And that's probably not going to be happy. Some fixes and up again:
https://tbpl.mozilla.org/?tree=Try&rev=e9937114bf78

Will put up the fixes fore r? when that seems happy.
Found a bug in prompts and SharedPrefs. Yay tests! Try again:
https://tbpl.mozilla.org/?tree=Try&rev=50cbe55815e4
finally got this to look mostly pretty. I'll split out the new changes and put up one more patch :)
https://tbpl.mozilla.org/?tree=Try&rev=9356c35cdb50
Attached patch PatchSplinter Review
This fixes up some errors that broke tests, as well as removing a bunch of code that I somehow missed. The weirdest problem I ran into was with HelperApps and some sessionRestore tests. We also send off some HelperApps requests on page load.

For some reason, hitting back while the synchronous requests were in transit (and we were spinning an event loop), cause strange races/behavior. I spent a few days but couldn't quite track down the problem. Moving to the async API fixed things. I kept the sync API around for the HelperApps context menu item.

I'd love to know what was happening, but decided it isn't worth the time investment yet.
Attachment #8361327 - Flags: review?(mark.finkle)
Attached patch Rolled up patchSplinter Review
Rolled this up and unbitrotted for landing. mfinkle, can you review the last piece?
Attachment #8346916 - Attachment is obsolete: true
Attachment #8346917 - Attachment is obsolete: true
Attachment #8346918 - Attachment is obsolete: true
Attachment #8346920 - Attachment is obsolete: true
Attachment #8346922 - Attachment is obsolete: true
Attachment #8346923 - Attachment is obsolete: true
Attachment #8346924 - Attachment is obsolete: true
Attachment #8346925 - Attachment is obsolete: true
Flags: needinfo?(mark.finkle)
Comment on attachment 8361327 [details] [diff] [review]
Patch

Moving this review since I know finkle is really busy with MWC stuff.
Attachment #8361327 - Flags: review?(mark.finkle) → review?(margaret.leibovic)
If I'm reading this correctly, this is the mirror of bug 967325, right? That is, this does JS->Java, and bug 967325 does Java->JS.
See Also: → 967325
Comment on attachment 8361327 [details] [diff] [review]
Patch

>+    public void dispatchEvent(JSONObject json) {

>         } catch (Exception e) {
>             Log.e(LOGTAG, "handleGeckoMessage throws " + e, e);
>         }
> 
>-        return "";
>     }

Don't leave the blank line at the bottom of the method

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>-    static native void notifyFilePickerResult(String filePath, long id);
>-
>-    @WrapElementForJNI(stubName = "ShowFilePickerAsyncWrapper")
>-    public static void showFilePickerAsync(String aMimeType, final long id) {
>-        sActivityHelper.showFilePickerAsync(getGeckoInterface().getActivity(), aMimeType, new ActivityHandlerHelper.FileResultHandler() {
>-            public void gotFile(String filename) {
>-                GeckoAppShell.notifyFilePickerResult(filename, id);
>-            }
>-        });
>-    }
>-

This change seems unrelated and should be a separate patch (throughout this patch). I'm gonna let it slide, but keep it in mind for next time.

>diff --git a/mobile/android/base/util/GeckoJarReader.java b/mobile/android/base/util/GeckoJarReader.java

>+            if (zip == null)
>+                return null;
>+

Also looks unrelated. Is this a bug? Was it crashing?
Again, I'll let it slide, but it would be nice to see these things as separate patches. 

>diff --git a/mobile/android/modules/HelperApps.jsm b/mobile/android/modules/HelperApps.jsm

>-  getAppsForUri: function getAppsForUri(uri, flags = { filterHttp: true }) {
>+  getAppsForUri: function getAppsForUri(uri, flags = { filterHttp: true }, callback) {

>-    let data = this._sendMessageSync(msg);
>-    if (!data)
>-      return [];

The above lines are needed to handle GeckoView, which does not support this message and returns nothing.

>+    if (!callback) {
>+      let data = this._sendMessageSync(msg);
>+      return parseData(JSON.parse(data));
>+    } else {
>+      sendMessageToJava(msg, function(data) {
>+        callback(parseData(JSON.parse(data)));
>+      });
>     }

You don't seem to be checking for the no data case anymore

>diff --git a/mobile/android/modules/SharedPreferen

>   _get: function _get(prefs, callback) {
>     let result = null;
>     sendMessageToJava({
>       type: "SharedPreferences:Get",
>       preferences: prefs,
>       branch: this._branch,
>     }, (data) => {
>-      result = JSON.parse(data).values);
>+      result = JSON.parse(data).values;
>     });
> 
>     let thread = Services.tm.currentThread;
>-    while (res == null)
>+    while (result == null)
>       thread.processNextEvent(true);

Looks like a separate bug. Is it? 

>diff --git a/widget/android/AndroidBridge.cpp b/widget/android/AndroidBridge.cpp

>-AndroidBridge::HandleGeckoMessage(const nsAString &aMessage, nsAString &aRet)
>+AndroidBridge::HandleGeckoMessage(const nsAString &aMessage)
> {
>     ALOG_BRIDGE("%s", __PRETTY_FUNCTION__);
> 
>     JNIEnv *env = GetJNIEnv();
>     if (!env)
>         return;
> 
>     AutoLocalJNIFrame jniFrame(env, 1);
>-    jstring returnMessage = GeckoAppShell::HandleGeckoMessageWrapper(aMessage);
>+    GeckoAppShell::HandleGeckoMessageWrapper(aMessage);
>+    return;
> 
>-    if (!returnMessage)
>-        return;
>-
>-    nsJNIString jniStr(returnMessage, env);
>-    aRet.Assign(jniStr);
>     ALOG_BRIDGE("leaving %s", __PRETTY_FUNCTION__);
> }

Why the explicit | return; | ? Why leave the | ALOG_BRIDGE("leaving...") | if it will never be hit?

>diff --git a/widget/android/nsIAndroidBridge.idl b/widget/android/nsIAndroidBridge.idl

> [scriptable, uuid(5aa0cfa5-377c-4f5e-8dcf-59ebd9482d65)]
> interface nsIAndroidBridge : nsISupports
> {
>-  AString handleGeckoMessage(in AString message);
>+  void handleGeckoMessage(in AString message);

Bump the UUID

r- for the cleanup. You don't need to separate out the unrelated code, but I do want the other stuff addressed (fixed or explained)
Attachment #8361327 - Flags: review?(margaret.leibovic) → review-
Attached patch Follow upsSplinter Review
Most of these changes were in the rollup already. I left them in there and just put the review comments here.

(In reply to Mark Finkle (:mfinkle) from comment #27)
> >+            if (zip == null)
> >+                return null;
> >+
> 
> Also looks unrelated. Is this a bug? Was it crashing?
> Again, I'll let it slide, but it would be nice to see these things as
> separate patches. 

Yeah. It was a local crash. Sorry, at one point I had this in a separate patch on my queue or else Fennec wouldn't even start. I removed it here again.

> The above lines are needed to handle GeckoView, which does not support this
> message and returns nothing.
I added this back in.

> Looks like a separate bug. Is it? 
No. This was just a dumb mistake in the original patch that try caught.

> Why the explicit | return; | ? Why leave the | ALOG_BRIDGE("leaving...") |
> if it will never be hit?
Just leftover. Removed it.

> Bump the UUID
Good catch :)
Attachment #8375617 - Flags: review?(mark.finkle)
Attachment #8375617 - Flags: review?(mark.finkle) → review+
Flags: needinfo?(mark.finkle)
https://hg.mozilla.org/mozilla-central/rev/e5ae503d3dba
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Blocks: 917942
At some point between "part 4" (comment 7) and the rollup (comment 24), something went wrong. The code at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=e5ae503d3dba#7149 is not syntactically correct, as far as I can tell.
Depends on: 982182
Depends on: 970300
Depends on: 989098
Depends on: 989109
Depends on: 993195
You need to log in before you can comment on or make changes to this bug.