Allow HelperApps to return a result if requested

RESOLVED FIXED in Firefox 30

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: murph, Assigned: murph)

Tracking

Trunk
Firefox 30
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Attachment #8377697 - Flags: review?(wjohnston)
Summary: Allow HelperApps to request a result → Allow HelperApps to return a result if requested
Assignee: nobody → murph
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8377697 - Flags: review?(wjohnston) → review+
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.
Attachment #8377697 - Attachment is obsolete: true
Attachment #8378635 - Flags: review?(wjohnston)
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
Attachment #8378635 - Flags: review?(wjohnston) → review+
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.
Attachment #8378635 - Attachment is obsolete: true
Attachment #8380032 - Flags: checkin?
Keywords: checkin-needed
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 :)
Attachment #8380032 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/0ed9572044c1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.