582616 - Sharing back-end for Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Allow the new Sharing feature in Fennec 2.0 to discover, list, and launch third-party Android applications.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch

This works, bug I'm putting this up to get feedback on the js api.  For now I've added getHandlerAppsForSharingMIMEType() to nsIDOMWindowUtils.

The example use is pretty simple:
let handlers = winUtils.getHandlerAppsForSharingMIMEType("image/png");

I tested it by enumerating the sharing handlers for PNGs on about:home as buttons and launching them.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

getHandlerAppsForSharingMIMEType seems like a good API. I initially thought of returning a nsIHandlerInfo (or nsIMIMEInfo) and retrieving the list of nsIHandlerApp from that.

We could implement an interface like this for Maemo too, but it might need a little hacking.

getHandlerAppsForSharing(...) might be enough of a name, but I think we need to find a better home for the method. Any ideas for that?

nsIExternalHelperAppService seems like a good place, but the IDL file seems weak. nsIExternalProtocolService seems like it's a good interface, but it's all protocol based.

Maybe nsIMIMEService?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 468591 [details] [diff] [review]
WIP patch

fb+ for the API direction

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

nsIExternalHelperAppService's idl is kinda weak, but I think its the most appropriate place to hang this function.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 468623 [details] [diff] [review]
patch

Let's change to the spec outlined here:
http://etherpad.mozilla.com:9000/sharing-interface

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 477413 [details] [diff] [review]
patch

The non-Android parts look OK to me.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 477413 [details] [diff] [review]
patch

Hm, will these reviews be enough? We're touching docshell code and adding interfaces.. might need a sr?

>diff --git a/docshell/build/nsDocShellModule.cpp b/docshell/build/nsDocShellModule.cpp
>--- a/docshell/build/nsDocShellModule.cpp
>+++ b/docshell/build/nsDocShellModule.cpp
>@@ -110,6 +113,9 @@ NS_GENERIC_FACTORY_CONSTRUCTOR(PlatformL
> #ifdef MOZ_ENABLE_DBUS
> NS_GENERIC_FACTORY_CONSTRUCTOR(nsDBusHandlerApp)
> #endif 
>+#if defined(ANDROID)
>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsExternalSharingAppService)
>+#endif
#ifdef for consistency with the rest of the file.

> 
> // session history
> NS_GENERIC_FACTORY_CONSTRUCTOR(nsSHEntry)
>@@ -134,6 +140,9 @@ NS_DEFINE_NAMED_CID(NS_LOCALHANDLERAPP_C
> #ifdef MOZ_ENABLE_DBUS
> NS_DEFINE_NAMED_CID(NS_DBUSHANDLERAPP_CID);
> #endif
>+#if defined(ANDROID)
>+NS_DEFINE_NAMED_CID(NS_EXTERNALSHARINGAPPSERVICE_CID);
>+#endif
#ifdef

> NS_DEFINE_NAMED_CID(NS_SHENTRY_CID);
> NS_DEFINE_NAMED_CID(NS_HISTORYENTRY_CID);
> NS_DEFINE_NAMED_CID(NS_SHTRANSACTION_CID);
>@@ -158,6 +167,9 @@ const mozilla::Module::CIDEntry kDocShel
> #ifdef MOZ_ENABLE_DBUS
>   { &kNS_DBUSHANDLERAPP_CID, false, NULL, nsDBusHandlerAppConstructor },
> #endif
>+#if defined(ANDROID)
>+  { &kNS_EXTERNALSHARINGAPPSERVICE_CID, false, NULL, nsExternalSharingAppServiceConstructor },
>+#endif
#ifdef

>   { &kNS_SHENTRY_CID, false, NULL, nsSHEntryConstructor },
>   { &kNS_HISTORYENTRY_CID, false, NULL, nsSHEntryConstructor },
>   { &kNS_SHTRANSACTION_CID, false, NULL, nsSHTransactionConstructor },
>@@ -199,6 +211,9 @@ const mozilla::Module::ContractIDEntry k
> #ifdef MOZ_ENABLE_DBUS
>   { NS_DBUSHANDLERAPP_CONTRACTID, &kNS_DBUSHANDLERAPP_CID },
> #endif
>+#if defined(ANDROID)
>+  { NS_EXTERNALSHARINGAPPSERVICE_CONTRACTID, &kNS_EXTERNALSHARINGAPPSERVICE_CID },
>+#endif
#ifdef

>diff --git a/uriloader/exthandler/android/nsExternalSharingAppService.cpp b/uriloader/exthandler/android/nsExternalSharingAppService.cpp
>new file mode 100644
>--- /dev/null
>+++ b/uriloader/exthandler/android/nsExternalSharingAppService.cpp
>diff --git a/uriloader/exthandler/android/nsExternalSharingAppService.h b/uriloader/exthandler/android/nsExternalSharingAppService.h
>new file mode 100644

namespace mozilla for this file so we can shorten some things.

>--- /dev/null
>+++ b/uriloader/exthandler/android/nsExternalSharingAppService.h
>+  /* additional members */
>+};
>+
>+#endif
#endif /* NS_EXTERNAL_SHARING_APP_SERVICE_H */

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 477413 [details] [diff] [review]
patch

>+NS_IMETHODIMP
>+nsExternalSharingAppService::GetSharingApps(const nsAString & aMIMEType,
>+                                            PRUint32 *aLen NS_OUTPARAM,
>+                                            nsISharingHandlerApp ***aHandlers NS_OUTPARAM)
>+{
>+  nsresult rv;
>+  NS_NAMED_LITERAL_STRING(sendAction, "android.intent.action.SEND");
>+  nsCOMPtr<nsIMutableArray> array = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ConvertUTF16toUTF8 nMimeType(aMIMEType);
>+  if (mozilla::AndroidBridge::Bridge()) {
if (!AndroidBridge::Bridge())
    return NS_OK;

>+    mozilla::AndroidBridge::Bridge()->GetHandlersForMimeType(nMimeType.get(),
>+                                                             array, nsnull, 
>+                                                             sendAction);
>+    array->GetLength(aLen);
>+    *aHandlers = 
>+      static_cast<nsISharingHandlerApp**>(NS_Alloc(sizeof(nsISharingHandlerApp*)
>+                                                   * *aLen));
>+    for (int i = 0; i < *aLen; i++) {
>+      rv = array->QueryElementAt(i, nsISharingHandlerApp::GetIID(), 
>+                                 (void**)(*aHandlers + i));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+    return NS_OK;
extra return.

>+  }
>+  return NS_OK;
>+}

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #9)
> Comment on attachment 477413 [details] [diff] [review]
> patch
> 
> Hm, will these reviews be enough? We're touching docshell code and adding
> interfaces.. might need a sr?
> 
> >diff --git a/docshell/build/nsDocShellModule.cpp b/docshell/build/nsDocShellModule.cpp
> >--- a/docshell/build/nsDocShellModule.cpp
> >+++ b/docshell/build/nsDocShellModule.cpp
> >@@ -110,6 +113,9 @@ NS_GENERIC_FACTORY_CONSTRUCTOR(PlatformL
> > #ifdef MOZ_ENABLE_DBUS
> > NS_GENERIC_FACTORY_CONSTRUCTOR(nsDBusHandlerApp)
> > #endif 
> >+#if defined(ANDROID)
> >+NS_GENERIC_FACTORY_CONSTRUCTOR(nsExternalSharingAppService)
> >+#endif
> #ifdef for consistency with the rest of the file.

using #if defined(ANDROID) because maemo will slot right in here when bug 582621 lands

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 477413 [details] [diff] [review]
patch

I don't think sr is strictly necessary, but it can't hurt either

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/8a6554580c4e

Comment 14

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b7pre) Gecko/20101008 Firefox/4.0b7pre Fennec/2.0b2pre

Comment 15

[Tony Chung [:tchung]]

https://litmus.mozilla.org/show_test.cgi?id=12770
https://litmus.mozilla.org/show_test.cgi?id=12771

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fwiw, the classid/contractid should have gone in nsDocShellCID.h, not in the IDL....