Comment 1 Jason Smith [:jsmith] 2012-08-14 12:42:36 PDT

Moving to web apps, as this focuses on web apps in FF Android.

Comment 2 Jason Smith [:jsmith] 2012-12-27 22:40:20 PST

*** Bug 825041 has been marked as a duplicate of this bug. ***

Comment 3 Wesley Johnston (:wesj) 2013-10-15 00:33:40 PDT

Created attachment 817017 [details] [diff] [review]
Early WIP

This is an early WIP to save my place. I created a new interface, nsINativeAppService that has two methods:

jsval getApps(in nsIDOMMozActivityOptions options);
void launch(in jsval appData,
            in nsIDOMMozActivityOptions options,
            in nsIActivityUIGlueCallback onresult);

Still haven't quite figured out how I'll correctly return things to the apps. Android gives us a content provider URI, which can then be used to access the real data. I haven't seen a clear spec for what sort of data apps expect in return, and I imagine there isn't one, so I'll just have to have some special case code to reformat Android apps results into the equivalent Gaia app ones? That sounds painful...

The opposite path will have to be done with synthetic apks, but has similar problems getting the data returned from Gecko into a ContentProvider. Still thinking my way through this....

Comment 4 [:fabrice] Fabrice Desré 2013-10-15 17:35:59 PDT

(In reply to Wesley Johnston (:wesj) from comment #3)

> Still haven't quite figured out how I'll correctly return things to the
> apps. Android gives us a content provider URI, which can then be used to
> access the real data. I haven't seen a clear spec for what sort of data apps
> expect in return, and I imagine there isn't one, so I'll just have to have
> some special case code to reformat Android apps results into the equivalent
> Gaia app ones? That sounds painful...

There's no spec indeed. Each activity is free to define its own data transfer format.

Comment 5 Wesley Johnston (:wesj) 2014-01-07 10:40:15 PST

Josh said he's interested in this.

Comment 6 Gene Lian [:gene] (I already quit Mozilla) 2014-01-07 19:41:34 PST

We need to enable System Message on Android as well, which is the back-end mechanism of Activities.

Comment 7 Josh Dover [:jdover] 2014-01-10 10:48:18 PST

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #6)
> We need to enable System Message on Android as well, which is the back-end
> mechanism of Activities.

Is there a bug for this already?

Comment 8 Josh Dover [:jdover] 2014-01-15 15:39:49 PST

Created attachment 8360716 [details] [diff] [review]
WIP for mapping and launching web activities as intents

A WIP based off of wesj's previous patch. Added a mapping mechanism within NativeAppService to map the Web Activity name, type, and data to the equivalent (or as best as we can) Android intent action, type, and extras. Have not completed the mappings yet, but would like feedback on the way I am doing it.

Comment 9 Josh Dover [:jdover] 2014-01-15 15:41:17 PST

Can test by launching a MozActivity in the main process in Fennec:

  new MozActivity({ name: "send", data: { type:"plain/text", text:"testing" }});

Comment 10 Josh Dover [:jdover] 2014-01-16 17:11:32 PST

Created attachment 8361405 [details] [diff] [review]
Simplified Implementation using Android Intent chooser

Comment 11 Josh Dover [:jdover] 2014-01-16 17:21:38 PST

New WIP with a simplified implementation that uses Android's intent chooser for picking the appropriate activity to handle the WebActivity. This assumes that all webapps will have registered with intent filters with a synthesized APK. 

All that is needed from here is determining rest of the mappings from Web Activities to Intents. wesj / myk, do you know if there is something like this in the synthesized APK work? We need to make sure that the mappings are equal in both (and can share the same code to do it).

Comment 12 Gene Lian [:gene] (I already quit Mozilla) 2014-01-16 19:03:33 PST

(In reply to Josh Dover from comment #7)
> (In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #6)
> > We need to enable System Message on Android as well, which is the back-end
> > mechanism of Activities.
> 
> Is there a bug for this already?

Sorry for the delayed response. Please see bug 895689.

Comment 13 Wesley Johnston (:wesj) 2014-01-17 09:51:04 PST

Comment on attachment 8361405 [details] [diff] [review]
Simplified Implementation using Android Intent chooser

I'm busy. Brian has been doing some dom api stuff. Maybe he can look at this faster than me?

Comment 14 Myk Melez [:myk] [@mykmelez] 2014-01-17 19:24:21 PST

(In reply to Josh Dover from comment #11)
> All that is needed from here is determining rest of the mappings from Web
> Activities to Intents. wesj / myk, do you know if there is something like
> this in the synthesized APK work? We need to make sure that the mappings are
> equal in both (and can share the same code to do it).

I think jhugman has done some work to determine these mappings, or at least a subset of them.  cc:ing him and asking him for info on that.

Comment 15 Brian Nicholson (:bnicholson) 2014-01-21 11:22:45 PST

Comment on attachment 8361405 [details] [diff] [review]
Simplified Implementation using Android Intent chooser

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

I'm not too familiar with web activities, so I think Wes should give another pass at this. The part I'm most unsure about is how we decided which intents to support, which I commented on below.

::: mobile/android/base/GeckoAppShell.java
@@ +1028,5 @@
>  
> +    static Intent getIntentForWebActivity(String action,
> +              @OptionalGeneratedParameter String uri,
> +              @OptionalGeneratedParameter String mime,
> +              @OptionalGeneratedParameter JSONObject extras) {

Are these optionalGeneratedParameter annotations necessary? I thought these were only used with JNI.

@@ +1049,5 @@
> +            }
> +        }
> +
> +        // Setup URIs
> +        if (uri != null && !uri.isEmpty()) {

String#isEmpty() was introduced in Android SDK 9, but we support as low as 8. Instead, use TextUtils#isEmpty (which will also cover the null check for you).

@@ +1054,5 @@
> +            intent.setData(Uri.parse(uri));
> +        }
> +
> +        // Setup type
> +        if (mime != null && !mime.isEmpty()) {

Same here.

::: mobile/android/components/NativeAppService.js
@@ +8,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/HelperApps.jsm");
> +
> +let activityMap = {

Please order these keys alphabetically.

@@ +13,5 @@
> +  "pick": {
> +    action: "android.intent.action.GET_CONTENT",
> +    mime: data => processType(data.type),
> +  },
> +  "activity": {

OK, so here is where I start to get confused. A few of these correspond to the Firefox OS activities Wes linked me to (https://developer.mozilla.org/en-US/docs/WebAPI/Web_Activities#Firefox_OS_activities). But most of them -- activity, createShortcut, delete, edit, insert, main, paste, run, search, send, setWallpaper -- are not. How are we choosing the the activities that get listed here?

The list of all ACTIONs at http://developer.android.com/reference/android/content/Intent.html is quite large. The ones here are just a small subset of these, so I'm curious why these were picked. Is there some other list somewhere outlining the functionality we want to support? Or were these picked arbitrarily?

@@ +58,5 @@
> +  }
> +};
> +
> +function mapActivity(name, data) {
> +  let mappedData = activityMap[name];

We should probably check `name in activityMap` before doing this, and if that condition evaluates to false, throw an error with a clear message. Since this is an API that web pages will be using, we'll want to avoid cryptic exceptions.

@@ +61,5 @@
> +function mapActivity(name, data) {
> +  let mappedData = activityMap[name];
> +
> +  return {
> +    webactivity: true,

What is this property used for?

@@ +71,5 @@
> +}
> +
> +function processType(type) {
> +  // Android only supports one MIME type per intent.
> +  return type instanceof Array ? type[0] : type;

What does FxOS do if multiple MIME types are given? We should probably do our best to make sure behavior matches between platforms as much as possible so apps don't exhibit different behaviors on different platforms. In this case, we might need to iterate through the array, firing a separate request for each, until Android gives us a result back.

Comment 16 James Hugman [:jhugman] [@jhugman] 2014-01-21 12:34:44 PST

From the Android/synthesized APK pov: 

We are able to generate IntentFilters from WebActivities declared in the app's manifest (we don't yet, but it's relatively easy).

Because it is the synthesized APKs that are catching the Android intents, we won't need to provide an Activity chooser of our own – we just use Android's. This is exactly as intended.

However, a couple of things bother me at the moment: 

 * we should provide a standard set of WebActivities, in the same way Android does. Lessons from the Android world would suggest that the community hasn't been very good at adopting community lead standards (see the http://www.openintents.org/intentstable for a repo of intents that no-one uses). My first port of call to find these out would be finding out what use Firefox OS makes of them. WesJ's link is a good start.  
 * WebActivities kinda sorta work like the Intent system in Android, but we'll be unable to support anything but things that invoke a UI. i.e. Android Activities only. AFAIK, headless Gecko doesn't exist yet, and until it does, and is well understood, it's going to be difficult respond to things like ContentResolvers and Broadcast intents. Think: no headless access to data provided by other apps, no responding to phone events, e.g. geo-fencing. The Android Intent docs do provide a bunch of constants, but many of them are not Activity actions.
 * The way we've talked about implementing the synth APK WebActivities mostly depends on the developer registering them in the manifest.json. This does not take into consideration: a) programmatic registration, b) registration by web /sites/. This second point is most difficult, but I don't think this is the right bug to say why.
 * Calling out to WebActivities should not preclude Android Intents: i.e. the mapping should be bi-directional.
 * Whatever mappings we chose should be kept in sync with the APK Factory used to generate APKs from webapp manifests.
 * Especially with this issue, we have an enormous impedance mismatches going on: FirefoxOS/Android. Personally, I think the intents system and putting it at the heart of the OS is one of the few things that Android gets completely right.

More specific feedback for this patch: 
 * I see no reason why the mapping needs to be in Javascript.

Comment 17 Josh Dover [:jdover] 2014-01-22 13:12:16 PST

(In reply to James Hugman [:jhugman] [@jhugman] from comment #16)
> We are able to generate IntentFilters from WebActivities declared in the
> app's manifest (we don't yet, but it's relatively easy).

Is there code anywhere for where this mapping will happen? If not, is this something that should be worked on in this bug, or are you planning on doing this? I think we could at least propose an idl for this mapping service.
 
>  * we should provide a standard set of WebActivities, in the same way
> Android does. Lessons from the Android world would suggest that the
> community hasn't been very good at adopting community lead standards (see
> the http://www.openintents.org/intentstable for a repo of intents that
> no-one uses). My first port of call to find these out would be finding out
> what use Firefox OS makes of them. WesJ's link is a good start. 

Is there a bug/etherpad/wiki to determine these?

>  * Calling out to WebActivities should not preclude Android Intents: i.e.
> the mapping should be bi-directional.

Great, this is what I assumed.

>  * Whatever mappings we chose should be kept in sync with the APK Factory
> used to generate APKs from webapp manifests.

Ideally, we should be able to use the same code to do this, so nothing gets out of sync.

>  * I see no reason why the mapping needs to be in Javascript.

I don't either, but whatever we build should be easily used by both Fennec and the APK synthesizer.

Comment 18 James Hugman [:jhugman] [@jhugman] 2014-01-23 07:32:19 PST

(In reply to Josh Dover from comment #17)
> (In reply to James Hugman [:jhugman] [@jhugman] from comment #16)
> > We are able to generate IntentFilters from WebActivities declared in the
> > app's manifest (we don't yet, but it's relatively easy).
> 
> Is there code anywhere for where this mapping will happen? If not, is this
> something that should be worked on in this bug, or are you planning on doing
> this? I think we could at least propose an idl for this mapping service.
No. The IntentFilters can either be registered programmatically in Java (but would need to be done in the synthesized APK), or via the APK's AndroidManifest.xml, in, ahem, XML.

That's JS => XML, Java, defining the /filter/.
  
> > My first port of call to find these out would be finding out
> > what use Firefox OS makes of them. WesJ's link is a good start. 
> 
> Is there a bug/etherpad/wiki to determine these?
Not that I know of. Would suggest a Wiki page.

> >  * Whatever mappings we chose should be kept in sync with the APK Factory
> > used to generate APKs from webapp manifests.
> 
> Ideally, we should be able to use the same code to do this, so nothing gets
> out of sync.
Yes, that would be nice. 

Conceptually this is: 
/Intent/ => WebActivityHandler

> 
> >  * I see no reason why the mapping needs to be in Javascript.
> 
> I don't either, but whatever we build should be easily used by both Fennec
> and the APK synthesizer.

From an implementation details p.o.v. I'd be wary of premature code sharing – it isn't clear to me that it's possible in the general case: if we see that the two systems are both systems of locks and keys, then we are mapping lock1->lock2, key2->lock1, key1->key2 i.e.

 * in APK-Factory we're mapping WebActivity filters (lock1) to IntentFilters (lock2).
 * in SynthesizedAPK, we're mapping WebActivity filters to IntentFilters again, but this time we must use Java.
 * for incoming intents, in Fennec, we're mapping Intents (key2) that match IntentFilters (lock2) to WebActivity filters (lock1). It's not immediately obvious that Android can tell us which IntentFilter a received Intent matched.
 * for outgoing webactivities, in Fennec, we're mapping MozActivity (key1) to Intent (key2)
 
It should be noted that none of the behaviour described in this comment is implemented.

Comment 19 Wesley Johnston (:wesj) 2014-03-12 11:54:39 PDT

Comment on attachment 8361405 [details] [diff] [review]
Simplified Implementation using Android Intent chooser

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

Talked to josh about this, and I agree that trying to share this is probably premature. I think we're just going to do the initial mapping here. We should try to make it easy to extend/change. A wiki page to detail things would be nice as well. You can just start one somewhere inside https://wiki.mozilla.org/Mobile/Fennec/ . Once we got some basics checked in and the WebApps/Android team are both happy, we can move it over to somewhere near https://developer.mozilla.org/en-US/docs/Web/API/MozActivity.

Comment 20 Josh Dover [:jdover] 2014-03-14 15:30:39 PDT

A mappings table has been added to the project wiki page at [1]. This will serve as a reference spec and place for discussion on:
  1) which actions should be supported
  2) how they (and their associated data) should be translated to Android

[1] https://wiki.mozilla.org/Mobile/Projects/API:_Web_activities#MozActivity_Mapping

Comment 21 Josh Dover [:jdover] 2014-03-18 13:46:39 PDT

Created attachment 8393095 [details] [diff] [review]
01 - Move intent handling to IntentHelper

Comment 22 Josh Dover [:jdover] 2014-03-18 13:47:16 PDT

Created attachment 8393096 [details] [diff] [review]
02 - Send web activities to Java

Comment 23 Josh Dover [:jdover] 2014-03-18 13:47:41 PDT

Created attachment 8393097 [details] [diff] [review]
03 - Map and launch web activities as Android intents

Comment 24 Josh Dover [:jdover] 2014-03-18 14:16:03 PDT

Created attachment 8393114 [details] [diff] [review]
03 - Map and launch web activities as Android intents

Fixed bug in JSON parsing

Comment 25 Wesley Johnston (:wesj) 2014-03-18 22:11:15 PDT

Comment on attachment 8393095 [details] [diff] [review]
01 - Move intent handling to IntentHelper

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

::: mobile/android/base/IntentHelper.java
@@ +23,5 @@
> +
> +public final class IntentHelper implements GeckoEventListener {
> +    private static final String LOGTAG = "GeckoIntentHelper";
> +    private static final String[] EVENTS =
> +        { "Intent:GetHandlers", "Intent:Open", "Intent:OpenForResult" };

Put these on separate lines. r.e.:

EVENTS = {
  EVENT1,
  EVENT2
}

@@ +57,5 @@
> +    }
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        Log.d(LOGTAG, "recieved message: " + message.toString());

Remove logging.

@@ +72,5 @@
> +        }
> +    }
> +
> +    private void getHandlers(JSONObject message) throws JSONException {
> +        final Intent intent = GeckoAppShell.getOpenURIIntent(activity, message.optString("url"),

I like this IntentHelper class. Hopefully we can move these methods in here and cleanup GeckoAppShell some more. Separate bug :) Maybe file and assign to yourself.

@@ +75,5 @@
> +    private void getHandlers(JSONObject message) throws JSONException {
> +        final Intent intent = GeckoAppShell.getOpenURIIntent(activity, message.optString("url"),
> +            message.optString("mime"), message.optString("action"), message.optString("title"));
> +        final String[] handlers = GeckoAppShell.getHandlersForIntent(intent);
> +        final List<String> appList = Arrays.asList(handlers);

Add a blank line here. I hate that we have so many variables here. Maybe this would be nicer if we put these in one line.

final List<String> appList = Arrays.asList(GeckoAppShell.getHandlersForIntent(intent));

@@ +84,5 @@
> +
> +    private void open(JSONObject message) throws JSONException {
> +        GeckoAppShell.openUriExternal(message.optString("url"),
> +            message.optString("mime"), message.optString("packageName"),
> +            message.optString("className"), message.optString("action"), message.optString("title"));

This is hard to read. If we have to break lines, lets just put each argument on its own line.

@@ +89,5 @@
> +    }
> +
> +    private void openForResult(final JSONObject message) throws JSONException {
> +        final Intent intent = GeckoAppShell.getOpenURIIntent(activity, message.optString("url"),
> +            message.optString("mime"), message.optString("action"), message.optString("title"));

Same here

@@ +97,5 @@
> +
> +        final JSONObject originalMessage = message;
> +        ActivityHandlerHelper.startIntentForActivity(activity,
> +                                                     intent,
> +                new ActivityResultHandler() {

Ideally I think we might pull this into an inner class and then call

ActivityHandlerHelper.startIntentForActivity(activity, intent, new Handler(message));

Comment 26 Wesley Johnston (:wesj) 2014-03-18 22:35:37 PDT

Comment on attachment 8393096 [details] [diff] [review]
02 - Send web activities to Java

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

I think we'll probably need to rework this a bit. Lets ping the people who can help tell us how. Fabrice, are they you, or do you know who can help us here?

::: dom/activities/src/ActivitiesService.jsm
@@ +208,2 @@
>        if (aResults.options.length === 0) {
> +        let nativeAppService = Cc["@mozilla.org/native-application-service;1"].createInstance(Ci.nsINativeAppService);

I think we need to move this up into the "find" function above:
http://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm#124

but that's a bit tricky since its using an indexedDB herlper to send results back. However, with the new Synth APK's stuff, we might not need to use the IndexedDB stuff at all. i.e. Any webapps that handle intents will register for them in their APK's, so this whole thing is now un-necessary. We just replace the ActivitiesDB on Android with our own implementation and not use the WebApp DB at all...

If this was XPCom that wouldn't be too hard. Its not thugh. We might need to pull the ActivitesDB into its own file that we can override on Android. Pinging fabrice for advice here.

::: mobile/android/components/NativeAppService.js
@@ +6,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/HelperApps.jsm");

You're not using this anymore.

@@ +10,5 @@
> +Cu.import("resource://gre/modules/HelperApps.jsm");
> +
> +let _callbacks = {};
> +
> +function NativeAppService() { }

If we can override ActivitiesDB, I'd like to remove this file and do everything there instead.

@@ +45,5 @@
> +    }
> +  },
> +
> +  _sendMessage: function(data) {
> +    return Services.androidBridge.handleGeckoMessage(JSON.stringify(data));

I'm pretty biased, but use Messaging.jsm here :)

Comment 27 Wesley Johnston (:wesj) 2014-03-18 23:08:52 PDT

Comment on attachment 8393114 [details] [diff] [review]
03 - Map and launch web activities as Android intents

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

This seems usable. I'd like to try and clean up the mapping a little bit so that, at least for basic use cases, its really simple to add new mappings.

::: mobile/android/base/IntentHelper.java
@@ +128,5 @@
> +                        response.put("extras", bundleToJSON(data.getExtras()));
> +                    }
> +
> +                    response.put("resultCode", resultCode);
> +                    EventDispatcher.sendResponse(originalMessage, response);

I forgot about these returns in my previous review. Getting the result back may mean that we can't just override the database...

::: mobile/android/base/util/WebActivityMapper.java
@@ +25,5 @@
> +            this.name = name;
> +            this.mapping = mapping;
> +        }
> +
> +        public static WebActivityMapping fromName(String name) {

The enum isn't really that helpful if you do things this way. You could remove the name and then use:

WebActivityMapping mapping = Enum.valueOf(Mapping.class, name.toUpperCase()).mapping;

or a HashTable of some sort would probably work just as well:

HashMap<String, WebActivityMapping> map = new HashMap<String, WebActivityMapping>() {{
  put("dial", new DialMapping());
  put("send", new SendMapping());
}};

@@ +49,5 @@
> +            return null;
> +        }
> +
> +        public void putExtras(JSONObject data, Intent intent) throws JSONException {
> +            return;

I'm debating the merits of this over a more basic getIntent(JSONObject) class, but I'm willing to try this out I think...

@@ +78,5 @@
> +
> +    private static class DialMapping extends WebActivityMapping {
> +        @Override
> +        public String getAction() {
> +            return "android.intent.action.DIAL";

Intent.ACTION_DIAL

@@ +87,5 @@
> +            return "tel:" + data.getString("number");
> +        }
> +    }
> +
> +    private static class SendMapping extends WebActivityMapping {

I almost wonder if we're better to abstract this implementation a bit. i.e. the Constructor could take the action to return, and maybe an array of extras... Just trying to figure out if there's a base implementation we can reuse for multiple web activity types.

@@ +90,5 @@
> +
> +    private static class SendMapping extends WebActivityMapping {
> +        @Override
> +        public String getAction() {
> +            return "android.intent.action.SEND";

Intent.ACTION_SEND

@@ +100,5 @@
> +        }
> +
> +        @Override
> +        public void putExtras(JSONObject data, Intent intent) throws JSONException {
> +            optPutExtra("text", "android.intent.extra.TEXT", data, intent);

Intent.EXTRA_TEXT, etc.

Comment 28 Josh Dover [:jdover] 2014-03-24 11:25:37 PDT

Created attachment 8395815 [details] [diff] [review]
02 - Send web activities to Java

(In reply to Wesley Johnston (:wesj) from comment #26)
> Comment on attachment 8393096 [details] [diff] [review]
> 02 - Send web activities to Java
> 
> Review of attachment 8393096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we'll probably need to rework this a bit. Lets ping the people who
> can help tell us how. Fabrice, are they you, or do you know who can help us
> here?
> 
> ::: dom/activities/src/ActivitiesService.jsm
> @@ +208,2 @@
> >        if (aResults.options.length === 0) {
> > +        let nativeAppService = Cc["@mozilla.org/native-application-service;1"].createInstance(Ci.nsINativeAppService);
> 
> I think we need to move this up into the "find" function above:
> http://mxr.mozilla.org/mozilla-central/source/dom/activities/src/
> ActivitiesService.jsm#124
> 
> but that's a bit tricky since its using an indexedDB helper to send results
> back. However, with the new Synth APK's stuff, we might not need to use the
> IndexedDB stuff at all. i.e. Any webapps that handle intents will register
> for them in their APK's, so this whole thing is now un-necessary. We just
> replace the ActivitiesDB on Android with our own implementation and not use
> the WebApp DB at all...
> 
> If this was XPCom that wouldn't be too hard. Its not thugh. We might need to
> pull the ActivitesDB into its own file that we can override on Android.
> Pinging fabrice for advice here.
> 

Still need advice on this part. It seems to me that it would make more sense to override Activities so that all startActivity() did was send the MozActivity options to Java.

> 
> @@ +45,5 @@
> > +    }
> > +  },
> > +
> > +  _sendMessage: function(data) {
> > +    return Services.androidBridge.handleGeckoMessage(JSON.stringify(data));
> 
> I'm pretty biased, but use Messaging.jsm here :)

This greatly simplified things :)

Comment 29 Josh Dover [:jdover] 2014-03-24 11:29:16 PDT

Created attachment 8395818 [details] [diff] [review]
03 - Map and launch web activities as Android intents

(In reply to Wesley Johnston (:wesj) from comment #27)
> Comment on attachment 8393114 [details] [diff] [review]
> 03 - Map and launch web activities as Android intents
> 
> @@ +49,5 @@
> > +            return null;
> > +        }
> > +
> > +        public void putExtras(JSONObject data, Intent intent) throws JSONException {
> > +            return;
> 
> I'm debating the merits of this over a more basic getIntent(JSONObject)
> class, but I'm willing to try this out I think...

I looked at the different web activities used by Gaia and found that 'type' and 'uri'/'url' were used quite often, so I made a BaseMapping class to handle these situations which I used to add more mappings really easily. Having base classes like this was the motivation for doing a less generic method like getIntent(JSONObject).

Let me know what you think!

Comment 30 Josh Dover [:jdover] 2014-03-24 11:30:19 PDT

Created attachment 8395820 [details] [diff] [review]
01 - Move intent handling to IntentHelper

Comment 31 Wesley Johnston (:wesj) 2014-03-27 11:53:50 PDT

Comment on attachment 8395820 [details] [diff] [review]
01 - Move intent handling to IntentHelper

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

Nice :)

Comment 32 Wesley Johnston (:wesj) 2014-03-27 12:14:02 PDT

Comment on attachment 8395815 [details] [diff] [review]
02 - Send web activities to Java

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

We need to get some tests written for all of this code, since its getting quite complex. There's a meeting tomorrow for some JS test stuff. This will be good as a use case...

::: dom/activities/src/ActivitiesService.jsm
@@ +208,3 @@
>        if (aResults.options.length === 0) {
> +        let nativeAppService = Cc["@mozilla.org/native-application-service;1"].createInstance(Ci.nsINativeAppService);
> +        if (nativeAppService) {

We can try leaving this in. Lets just move it down below this successCb, before we even do the match:

http://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm#275

If the NativeService is defined, we can jump the entire line here...

@@ +208,5 @@
>        if (aResults.options.length === 0) {
> +        let nativeAppService = Cc["@mozilla.org/native-application-service;1"].createInstance(Ci.nsINativeAppService);
> +        if (nativeAppService) {
> +          nativeAppService.launchNativeAppPicker(aMsg.options, {
> +            handleEvent: function(result) {

This result will have a bunch of things in it that content probably shouldn't know about. We should clean it up, or maybe it should just contain data that we'll send to the page in a separate object. i.e Java returns:

{ guid: something, type: "Prompt:Return", data: results }

and you'd just pass the data back to the page.

Comment 33 Josh Dover [:jdover] 2014-03-27 18:17:52 PDT

Created attachment 8398259 [details] [diff] [review]
02 - Send web activities to Java

Moved call to NativeAppService. Need JS testing.

Comment 34 Wesley Johnston (:wesj) 2014-03-31 22:43:37 PDT

Comment on attachment 8398259 [details] [diff] [review]
02 - Send web activities to Java

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

I'm changing my mind a bit (sorry), but lets get fabrice's feedback before we do anything.

::: dom/activities/src/ActivitiesService.jsm
@@ +276,5 @@
>      };
>  
> +    // If there is a native app service available, skip the ActivitiesDB, delegating to the platform.
> +    let nativeAppService =
> +        Cc["@mozilla.org/native-application-service;1"].createInstance(Ci.nsINativeAppService);

Sorry to go back and forth here. I think we should wait to get some feedback from fabrice, but after thinking about this a bit, I think we should do this inside the sucessCb, and pass the results we found there to the nativeAppService.

I want to make this a bit general so that it could work on other platforms. Those platforms might not have something like the synthetic APK, so this would let them choose to use or ignore the webapps that were found. Right now they basically have to ignore them (or duplicate this database code...)

::: mobile/android/app/mobile.js
@@ +837,5 @@
>  // which is a test server that always reports all apps as having updates.
>  pref("browser.webapps.updateCheckUrl", "https://controller.apk.firefox.com/app_updates");
>  
> +// System messages needed for Web Activity support
> +pref("dom.sysmsg.enabled", true);

We'll probably lock this to non-release builds initially.

Comment 35 [:fabrice] Fabrice Desré 2014-04-07 11:12:55 PDT

Comment on attachment 8398259 [details] [diff] [review]
02 - Send web activities to Java

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

I need to understand better the android specifics!

::: dom/activities/interfaces/nsINativeAppService.idl
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +interface nsIDOMMozActivityOptions;

this has been removed when switching all the activities code to use webidl. Use jsval instead.

@@ +5,5 @@
> +#include "nsISupports.idl"
> +
> +interface nsIDOMMozActivityOptions;
> +interface nsIActivityUIGlueCallback;
> +

this file needs comments. What is it for?

::: dom/activities/src/Activities.manifest
@@ +14,5 @@
>  contract @mozilla.org/dom/activities/options;1 {ee983dbb-d5ea-4c5b-be98-10a13cac9f9d}
> +
> +component {d5691dc4-cb8b-4f57-bf3f-31215fd5a3ca} NativeAppService.js
> +contract @mozilla.org/native-application-service;1 {d5691dc4-cb8b-4f57-bf3f-31215fd5a3ca}
> +contract @mozilla.org/dom/activities/ui-glue;1 {d5691dc4-cb8b-4f57-bf3f-31215fd5a3ca}

this component is not is this directory. Please remove that.

::: dom/activities/src/ActivitiesService.jsm
@@ +287,5 @@
> +          });
> +        }
> +      });
> +    }
> +

It's not 100% clear to me how all this works, but by doing that you remove all chances to use activities registered from manifests. Is that what we want on Android? Are you converting them to native intents while building the apks?

::: mobile/android/components/NativeAppService.js
@@ +6,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

unused.

Comment 36 Josh Dover [:jdover] 2014-04-08 18:04:47 PDT

Created attachment 8403688 [details] [diff] [review]
02 - Pass activity options instead of name through nsIActivityUIGlue

Some background on how this works with synth-APKs on Android:

All open web apps are packaged up as native APKs (installable Android apps) that have all of their web activities translated into Android intents (this work is separate from this bug). Because of this, we do not use the ActivitiesDB that B2G uses to find eligible apps, so all of our inter-app communications happen via Android's native intent system. This also allows webapps to talk to regular native Android apps as well (yay!).

What's going on in these patches:

My strategy with the modifications to nsIActivitiesUIGlue is to allow an implementer of the interface to have the option to handle the activity by providing all of MozActivity options (rather than just the name) to the implementer as well as a callback to deliver the results (if applicable). This is great for Android and could be applicable to Windows too [1] in the future. Of course for B2G we can safely ignore this new callback.

I tried to comment as much as possible, but definitely point out any areas where it's unclear what I am doing.

[1] Windows app contracts and extensions - http://msdn.microsoft.com/en-us/library/windows/apps/hh464906.aspx

Comment 37 Josh Dover [:jdover] 2014-04-08 18:05:23 PDT

Created attachment 8403689 [details] [diff] [review]
03 - Add onresult callback to nsIActivityUIGlue

Comment 38 Josh Dover [:jdover] 2014-04-08 18:05:52 PDT

Created attachment 8403690 [details] [diff] [review]
04 - Pass web activities to Java

Comment 39 Josh Dover [:jdover] 2014-04-08 18:06:20 PDT

Created attachment 8403691 [details] [diff] [review]
05 - Map and launch web activities as Android intents

Comment 40 [:fabrice] Fabrice Desré 2014-04-09 22:12:18 PDT

Comment on attachment 8403689 [details] [diff] [review]
03 - Add onresult callback to nsIActivityUIGlue

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

::: dom/activities/interfaces/nsIActivityUIGlue.idl
@@ +38,5 @@
>        * @param activities  A json blob which is an array of { "title":"...", "icon":"..." }.
> +      * @param onchoose    The callback to send the index of the choosen activity.
> +                           Send '-1' if no choice is made. Send 'undefined' if activities.length is 0
> +                           and not using alternate activity system.
> +      * @param onresult    The callback to send the result of an externally launched activity.

could we keep a single callback, with a signature like:

handleEvent(in long resultType, in jsval result)

where resultType could be nsIActivityUIGlueChooseCallback.WEBAPPS_ACTIVITY or nsIActivityUIGlueChooseCallback.NATIVE_ACTIVITY

Comment 41 [:fabrice] Fabrice Desré 2014-04-09 22:20:13 PDT

Comment on attachment 8403690 [details] [diff] [review]
04 - Pass web activities to Java

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

::: mobile/android/components/ActivitiesGlue.js
@@ +8,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

unused.

Comment 42 Josh Dover [:jdover] 2014-04-10 16:14:27 PDT

Created attachment 8405048 [details] [diff] [review]
03 - Allow nsIActivityUIGlueCallback to handle native and web activities

(In reply to Fabrice Desré [:fabrice] from comment #40)
> could we keep a single callback, with a signature like:
> 
> handleEvent(in long resultType, in jsval result)
> 
> where resultType could be nsIActivityUIGlueChooseCallback.WEBAPPS_ACTIVITY
> or nsIActivityUIGlueChooseCallback.NATIVE_ACTIVITY

Done, let me know if the comments aren't clear enough.

Comment 43 Josh Dover [:jdover] 2014-04-10 16:15:07 PDT

Created attachment 8405049 [details] [diff] [review]
04 - Pass web activities to Java

Comment 44 Josh Dover [:jdover] 2014-04-10 16:16:57 PDT

Created attachment 8405051 [details] [diff] [review]
05 - Map and launch web activities as Android intents

Made some small modifications to the building of the result JSON to send back to JS:

- If there are no extras, that key will not be present rather than having an empty JS object
- Added the uri to the result (especially important for intents like ACTION_PICK)

Comment 45 Josh Dover [:jdover] 2014-04-10 16:17:45 PDT

Comment on attachment 8403688 [details] [diff] [review]
02 - Pass activity options instead of name through nsIActivityUIGlue

This should be ready for review without any changes.

Comment 46 [:fabrice] Fabrice Desré 2014-04-11 10:23:59 PDT

Comment on attachment 8405048 [details] [diff] [review]
03 - Allow nsIActivityUIGlueCallback to handle native and web activities

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

::: b2g/components/ActivitiesGlue.js
@@ +70,5 @@
>      });
>      Services.tm.currentThread.dispatch(this, Ci.nsIEventTarget.DISPATCH_NORMAL);
>    },
>  
> +  classID: Components.ID("{de25076e-f986-4a6d-bf5a-cda27b9e3ad8}"),

no need to change the classID

::: b2g/components/B2GComponents.manifest
@@ +21,5 @@
>  category xpcom-directory-providers browser-directory-provider @mozilla.org/browser/directory-provider;1
>  
>  # ActivitiesGlue.js
> +component {de25076e-f986-4a6d-bf5a-cda27b9e3ad8} ActivitiesGlue.js
> +contract @mozilla.org/dom/activities/ui-glue;1 {de25076e-f986-4a6d-bf5a-cda27b9e3ad8}

no need to change the classID

Comment 47 [:fabrice] Fabrice Desré 2014-04-11 10:25:31 PDT

Comment on attachment 8405049 [details] [diff] [review]
04 - Pass web activities to Java

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

lgtm, but I know next to nothing about the android code, so wesj should also take a look.

Comment 48 Wesley Johnston (:wesj) 2014-04-14 09:27:40 PDT

Comment on attachment 8405049 [details] [diff] [review]
04 - Pass web activities to Java

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

::: mobile/android/app/mobile.js
@@ +837,5 @@
>  // To test updates, set this to http://apk-update-checker.paas.allizom.org,
>  // which is a test server that always reports all apps as having updates.
>  pref("browser.webapps.updateCheckUrl", "https://controller.apk.firefox.com/app_updates");
>  
> +pref("dom.sysmsg.enabled", true);

Pull this out of the synth-apks ifdef and instead lets wrap it in a #ifdef NIGHTLY_BUILD. I think we'll eventually want to ensure that (at least for the web, maybe not apps), this shows a prompt to the user.

::: mobile/android/components/ActivitiesGlue.js
@@ +17,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIActivityUIGlue]),
> +  classID: Components.ID("{e4deb5f6-d5e3-4fce-bc53-901dd9951c48}"),
> +
> +  // Ignore aActivities results on Android, go straight to Android intents.
> +  chooseActivity: function ap_chooseActivity(aOptions, aActivities, aCallback) {

Nice. Good working finding this :)

@@ +22,5 @@
> +    sendMessageToJava({
> +      type: "WebActivity:Open",
> +      activity: { name: aOptions.name, data: aOptions.data }
> +    }, (result) => {
> +      aCallback.handleEvent(Ci.nsIActivityUIGlueCallback.NATIVE_ACTIVITY, result);

This actually could be the result from a web activity at some point, but we'll deal with that when its possible :)

Comment 49 Wesley Johnston (:wesj) 2014-04-14 09:33:51 PDT

Comment on attachment 8405051 [details] [diff] [review]
05 - Map and launch web activities as Android intents

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

Lets land this :)

::: mobile/android/base/util/WebActivityMapper.java
@@ +133,5 @@
> +        @Override
> +        public String getMime(JSONObject data) {
> +            // MozActivity adds a type 'url' here, we don't want to set the MIME to 'url'.
> +            String type = data.optString("type", null);
> +            if (TextUtils.equals(type, "url") || TextUtils.equals(type, "uri")) {

"url".equals(type) will also get you around any null problems.

Comment 50 Josh Dover [:jdover] 2014-04-14 13:32:44 PDT

Created attachment 8406374 [details] [diff] [review]
03 - Allow nsIActivityUIGlueCallback to handle native and web activities

Comment 51 Josh Dover [:jdover] 2014-04-14 13:33:05 PDT

Created attachment 8406375 [details] [diff] [review]
04 - Pass web activities to Java

Comment 52 Josh Dover [:jdover] 2014-04-14 13:33:28 PDT

Created attachment 8406376 [details] [diff] [review]
05 - Map and launch web activities as Android intents

Comment 53 Josh Dover [:jdover] 2014-04-14 13:34:03 PDT

Before landing, I am running a try build to make sure that nothing in B2G was broken by these changes: https://tbpl.mozilla.org/?tree=Try&rev=8371bd506269

Comment 54 Josh Dover [:jdover] 2014-04-14 17:46:21 PDT

Created attachment 8406527 [details] [diff] [review]
01 - Move intent handling to IntentHelper

Rebased onto fx-team

Comment 55 Josh Dover [:jdover] 2014-04-14 17:47:01 PDT

Created attachment 8406528 [details] [diff] [review]
02 - Pass activity options instead of name through nsIActivityUIGlue

Rebased onto fx-team

Comment 56 Josh Dover [:jdover] 2014-04-14 17:47:24 PDT

Created attachment 8406529 [details] [diff] [review]
03 - Allow nsIActivityUIGlueCallback to handle native and web activities

Rebased onto fx-team

Comment 57 Josh Dover [:jdover] 2014-04-14 17:47:50 PDT

Created attachment 8406530 [details] [diff] [review]
04 - Pass web activities to Java

Rebased onto fx-team

Comment 58 Josh Dover [:jdover] 2014-04-14 17:48:11 PDT

Created attachment 8406531 [details] [diff] [review]
05 - Map and launch web activities as Android intents

Rebased onto fx-team

Comment 59 Carsten Book [:Tomcat] 2014-04-15 02:16:52 PDT

https://hg.mozilla.org/integration/fx-team/rev/33bcd13d52f5
https://hg.mozilla.org/integration/fx-team/rev/f8c51b2092ca
https://hg.mozilla.org/integration/fx-team/rev/2ada01517980
https://hg.mozilla.org/integration/fx-team/rev/ec54368243da
https://hg.mozilla.org/integration/fx-team/rev/46b4833e84c3

Comment 60 Carsten Book [:Tomcat] 2014-04-15 03:17:53 PDT

sorry had to backed this out for a test failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=37826592&tree=Fx-Team

Comment 61 Josh Dover [:jdover] 2014-04-16 15:36:28 PDT

Ongoing discussion on dev.platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/xy0lPhgQCCo

Comment 62 Josh Dover [:jdover] 2014-04-25 16:28:16 PDT

Created attachment 8413034 [details] [diff] [review]
01 - Move intent handling to IntentHelper

rebased onto fx-team

Comment 63 Josh Dover [:jdover] 2014-04-25 16:28:40 PDT

Created attachment 8413035 [details] [diff] [review]
02 - Pass activity options instead of name through nsIActivityUIGlue

rebased onto fx-team

Comment 64 Josh Dover [:jdover] 2014-04-25 16:29:02 PDT

Created attachment 8413036 [details] [diff] [review]
03 - Allow nsIActivityUIGlueCallback to handle native and web activities

rebased onto fx-team

Comment 65 Josh Dover [:jdover] 2014-04-25 16:29:32 PDT

Created attachment 8413037 [details] [diff] [review]
04 - Pass web activities to Java

Comment 66 Josh Dover [:jdover] 2014-04-25 16:29:56 PDT

Created attachment 8413038 [details] [diff] [review]
05 - Map and launch web activities as Android intents

rebased onto fx-team

Comment 67 Josh Dover [:jdover] 2014-04-25 16:30:55 PDT

Created attachment 8413039 [details] [diff] [review]
06 - Pref off MozActivity for only WebappRT

This only exposes MozActivity in webapps running on a nightly build.

Comment 68 Wesley Johnston (:wesj) 2014-04-29 10:07:12 PDT

Comment on attachment 8413039 [details] [diff] [review]
06 - Pref off MozActivity for only WebappRT

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

::: mobile/android/chrome/content/WebappRT.js
@@ +41,5 @@
>      pref("dom.mozTCPSocket.enabled", true),
>      // Don't check for updates in webapp processes to avoid duplicate notifications.
>      pref("browser.webapps.checkForUpdates", 0),
> +
> +    #ifdef NIGHTLY_BUILD

No need for the nightly build stuff even. For webapps, we'll let this ride the trains.

Comment 69 Josh Dover [:jdover] 2014-04-29 14:37:15 PDT

Created attachment 8414791 [details] [diff] [review]
06 - Pref off MozActivity for only WebappRT

Passed on try, marking for checkin!

https://tbpl.mozilla.org/?tree=Try&rev=4343e2943d21

Comment 70 Carsten Book [:Tomcat] 2014-04-30 00:06:08 PDT

https://hg.mozilla.org/integration/fx-team/rev/95742dfbee49
https://hg.mozilla.org/integration/fx-team/rev/c15e4803607d
https://hg.mozilla.org/integration/fx-team/rev/025898d5577c
https://hg.mozilla.org/integration/fx-team/rev/fac267a41e5c
https://hg.mozilla.org/integration/fx-team/rev/774890574009
https://hg.mozilla.org/integration/fx-team/rev/8d7327159983

Comment 71 Carsten Book [:Tomcat] 2014-04-30 06:44:30 PDT

https://hg.mozilla.org/mozilla-central/rev/95742dfbee49
https://hg.mozilla.org/mozilla-central/rev/c15e4803607d
https://hg.mozilla.org/mozilla-central/rev/025898d5577c
https://hg.mozilla.org/mozilla-central/rev/fac267a41e5c
https://hg.mozilla.org/mozilla-central/rev/774890574009
https://hg.mozilla.org/mozilla-central/rev/8d7327159983

Comment 72 Josh Dover [:jdover] 2014-04-30 09:43:24 PDT

Dev docs added: https://developer.mozilla.org/en-US/docs/WebAPI/Web_Activities#Firefox_for_Android_activities