575750 - Implement support for OS protocol handlers on Android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: patch

We currently don't implement any of functions used to query the OS about protocol handlers.  Most of the functionality for this already exists for querying handlers for mime types.

Mwu, could you please review the widget parts of this patch and Benjamin the uriloader and xpcom parts? Thanks.

Comment 1

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

please disregard lines 94 and 95 of nsAndroidHandlerApp.cpp, I will delete them before pushing

Comment 2

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

Attachment: patch

with function names per mwu's request

Comment 3

[Michael Wu [:mwu]]

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


>@@ -280,10 +306,17 @@ class GeckoAppShell
>         return android.webkit.MimeTypeMap.getSingleton().getMimeTypeFromExtension(aFileExt);
>     }
> 
>-    static boolean openUriExternal(String aUriSpec, String aMimeType) {
>+    static boolean openUriExternal(String aUriSpec, String aMimeType, 
>+                                   String aPackageName, String aClassName) {
>         // XXX: It's not clear if we should set the action to view or leave it open
>         Intent intent = new Intent(Intent.ACTION_VIEW);
>-        intent.setDataAndType(android.net.Uri.parse(aUriSpec), aMimeType);
>+        if (aMimeType.length() > 0)
>+            intent.setDataAndType(android.net.Uri.parse(aUriSpec), aMimeType);
>+        else
>+            intent.setData(android.net.Uri.parse(aUriSpec));
>+        if (aPackageName.length() > 0 && aClassName.length() > 0)
>+            intent.setClassName(aPackageName, aClassName);
>+
>         intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
>         try {
>             GeckoApp.surfaceView.getContext().startActivity(intent);
Looks like android.net.Uri is used enough that you should just import it.

>diff -r 5298bae2eb91 -r 70022c7d6d48 uriloader/exthandler/android/nsOSHelperAppService.cpp
>--- a/uriloader/exthandler/android/nsOSHelperAppService.cpp	Mon Jun 28 03:12:07 2010 -0400
>+++ b/uriloader/exthandler/android/nsOSHelperAppService.cpp	Tue Jun 29 16:35:31 2010 -0400
>@@ -37,6 +37,7 @@
> 
> #include "nsOSHelperAppService.h"
> #include "nsMIMEInfoAndroid.h"
>+#include "AndroidBridge.h"
> 
> nsOSHelperAppService::nsOSHelperAppService() : nsExternalHelperAppService()
> {
>@@ -66,6 +67,20 @@ nsresult
> nsOSHelperAppService::OSProtocolHandlerExists(const char* aScheme,
>                                               PRBool* aExists)
> {
>-    *aExists = PR_FALSE;
>+    mozilla::AndroidBridge* bridge = mozilla::AndroidBridge::Bridge();
>+    nsStringArray stringArray;
>+    bridge->GetHandlersForProtocol(nsCAutoString(aScheme).get(), &stringArray);
>+    
>+    nsString empty = EmptyString();
>+    PRInt32 len = stringArray.Count();
>+    *aExists = len != 0;
>     return NS_OK;
> }
Can we have GetHandlersForProtocol return true/false if there are any handlers? Then there will be no need to create a stringArray that ends up just getting counted and thrown away. Also, nsCAutoString should not be needed here..

>diff -r 5298bae2eb91 -r 70022c7d6d48 widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h	Mon Jun 28 03:12:07 2010 -0400
>+++ b/widget/src/android/AndroidBridge.h	Tue Jun 29 16:35:31 2010 -0400
>@@ -104,9 +104,12 @@ public:
>     void SetSurfaceView(jobject jobj);
>     AndroidGeckoSurfaceView& SurfaceView() { return mSurfaceView; }
> 
>+    void GetHandlersForProtocol(const char *aScheme, nsStringArray* aStringArray);
>+
>     void GetHandlersForMimeType(const char *aMimeType, nsStringArray* aStringArray);
> 
>-    PRBool OpenUriExternal(nsCString& aUriSpec, nsCString& aMimeType);
>+    PRBool OpenUriExternal(const nsCString& aUriSpec, const nsCString& aMimeType, 
>+                           const nsString& aPackageName, const nsString& aClassName);
> 
>     void GetMimeTypeFromExtension(const nsCString& aFileExt, nsCString& aMimeType);
> 
Can we set defaults for aPackageName and aClassName? Also, aren't we suppose to use the abstract string classes? That will let us use nsAutoString in the callers or whatever concrete string class the caller would prefer to use.

Comment 4

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

Attachment: patch

Comment 5

[Michael Wu [:mwu]]

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

The NS_PRECONDITION in GetHandlersForProtocol is probably not needed anymore. I'm not too familiar with what the code needs to do but it looks reasonable enough.

Comment 6

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

(In reply to comment #5)
> (From update of attachment 455053 [details] [diff] [review])
> The NS_PRECONDITION in GetHandlersForProtocol is probably not needed anymore.
> I'm not too familiar with what the code needs to do but it looks reasonable
> enough.

Ah yes, that should definitely go. Thanks for catching it.

Comment 7

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

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

bsmedberg suggested that bz would be a better reviewer or could suggest a better reviewer

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

>+    nsString empty = EmptyString();
>+    nsCString emptyC = EmptyCString();

Make those const refs; no need to construct separate objects.

> +  // the chooser dialog currently causes a crash on Android, avoid this by returning false here
> +  // but be sure to return mAlwaysAsk when that gets fixed

This comment should contain a bug number.

Note that not asking when you need to is a likely security issue, since it leads to automatic handling of content by possibly-insecure apps...

> +  mMimeType(aMIMEType), mAlwaysAsk(PR_FALSE), 

Why this change?  I don't think we should change that.

r=me with those fixed.

Comment 10

[Matt Brubeck (:mbrubeck)]

pushed with nits from comment 8 fixed:
http://hg.mozilla.org/mozilla-central/rev/aeaac11e5c73