974001 - Allow HelperApps to return a result if requested

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[murph]

Attachment: AllowHelperAppsToReturnResults.patch

Currently, addons can use HelperApps.jsm to launch activities, but cannot get a result back.

It'd be good if they could.

I've attached a patch that adds this functionality. It's my first patch for the project, so all feedback is more than welcome.

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 8377697 [details] [diff] [review]
AllowHelperAppsToReturnResults.patch

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

Nice. Seems like a good addition. Thanks :)

::: mobile/android/base/GeckoApp.java
@@ +703,5 @@
> +                            @Override
> +                            public void onActivityResult (int resultCode, Intent data) {
> +                                JSONObject resultJSON = new JSONObject();
> +
> +                                if (resultCode == RESULT_OK && data != null) {

Lets just early return here. I find it easier to read. Also, if the resultCode isn't OK, we can send an error back to the caller.

Then I'd kinda recommend we split this up a bit and move all the extras stuff into its own private function

if (data == null) {
  sendResponse(orig, result);
  return;
}

appendExtras(result, data.getExtras());
appendCursor(result, data.getData());
sendResponse(orig, result);

@@ +715,5 @@
> +                                            }
> +                                        }
> +                                    }
> +                                } 
> +                                EventDispatcher.sendResponse(originalMessage, resultJSON);

For some intents we'll also need to query the uri returned here. i.e.

Uri uri = data.getData();
Cursor c =  getContentResolver().query(uri, null, null, null, null);
// loop over all the columns in the cursor and add the the resultJSON.

I think we could do that in a separate patch if you want.

Comment 2

[murph]

Attachment: AllowHelperAppsToReturnResults.patch

Instead of returning a JSONObject made from the extras, this updated patch nests it within another JSONObject. This allows us to include the resultCode directly and leaves room for adding the values from the cursor in a later patch. Otherwise we would have to worry about the keys colliding.

I also created a seperate function for converting the extras bundle into JSON.

The activityResultHandler is made a bit ugly by the JSONException try/catch. I don't know if there's any way to avoid that, though.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8378635 [details] [diff] [review]
AllowHelperAppsToReturnResults.patch

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

Seems great :)

::: mobile/android/base/GeckoApp.java
@@ +705,5 @@
> +                                JSONObject response = new JSONObject();
> +
> +                                try {
> +                                    if (data != null) {
> +                                        response.put("extras", bundleToJSON(data.getExtras()));

You could do optPut if you wanted. bundleToJSON would have to return null for errors in order to not contain an empty object. That might be nice actually though...

@@ +711,5 @@
> +                                    response.put("resultCode", resultCode);
> +                                } catch (JSONException e) {
> +                                    Log.w(LOGTAG, "Error building JSON response.", e);
> +                                }
> +                                

Nit some whitespace here.

@@ +877,5 @@
> +        }
> +
> +        for (String key : bundle.keySet()) {
> +            try {
> +                json.put(key, bundle.get(key));

Same here with optPut

Comment 4

[murph]

Attachment: AllowHelperAppsToReturnResults.patch

Same as the last patch with the whitespace nit fixed.

I don't think there is a optPut. There's a putOpt, but it still throws JSONException, so it doesn't make the code cleaner.

That being said, I'd like to request the patch checked in, if that's alright.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8380032 [details] [diff] [review]
AllowHelperAppsToReturnResults.patch

https://hg.mozilla.org/integration/fx-team/rev/0ed9572044c1

In the future, please just use the checkin-needed keyword :)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ed9572044c1