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!

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 :)

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+

This is the same patch. Fixed up.

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.

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).

Move the webapps code over. I did make this async. Some re-org to make it (more) readable.

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?

This moves the java addon manager. It will break java addons AFAICT. I'm fine with that.

Shared prefs. Using the thread trick again to avoid having to fix every caller...

And with that, this class can die.

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

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

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?

(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

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?

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.

Comment on attachment 8346924 [details] [diff] [review] Patch 7/8 - Shared prefs Just a note: Remember to Try server this set of patches.

To try: https://tbpl.mozilla.org/?tree=Try&rev=7727fdce884f

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

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.

Rolled this up and unbitrotted for landing. mfinkle, can you review the last piece?

Comment on attachment 8361327 [details] [diff] [review] Patch Moving this review since I know finkle is really busy with MWC stuff.

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.

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)

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 :)

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.