1016747 - add android protocol handler to proxy input streams to Gecko

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: android_protocol.patch

Comment 1

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

Attachment: android_protocol.patch

this uses NIO to avoid copying the buffer

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8429780 [details] [diff] [review]
android_protocol.patch

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

Minus mostly for nits. Looks cool, but what does android:// actually refer to? Do we need to define that still?

::: mobile/android/base/GeckoAppShell.java
@@ +2722,5 @@
>          toast.show();
>      }
>  
> +    @WrapElementForJNI(stubName = "InputStreamCreateWrapper")
> +    static InputStream inputStreamCreate(URLConnection connection) throws IOException {

createInputStream seems to be closer to convention in the file

@@ +2744,5 @@
> +        return new URL(spec).openConnection();
> +    }
> +
> +    @WrapElementForJNI(stubName = "ConnectionGetMimeTypeWrapper")
> +    static String connectionGetMimeType(URLConnection connection) {

getConnectionMimeType?

::: widget/android/AndroidBridge.cpp
@@ +206,4 @@
>          jEGLSurfacePointerField = 0;
>      }
>  
> +    jGeckoAppShell = getClassGlobalRef("org/mozilla/gecko/GeckoAppShell");

Isn't this stuff in GeneratedJNIWrappers?

@@ +2117,5 @@
>      }
>      return -1;
>  }
> +
> +jobject AndroidBridge::GetConnection(nsIURI* aURI) {

Do we need this in addition to the generated one?

@@ +2131,5 @@
> +    }
> +    return env->NewGlobalRef(connection);
> +}
> +
> +jobject AndroidBridge::InputStreamCreate(jobject connection) {

Same

::: widget/android/nsAndroidProtocolHandler.cpp
@@ +65,5 @@
> +    AndroidChannel(nsIURI *aURI, jobject aConnection) :
> +        mConnection(aConnection) {
> +        mURI = aURI;
> +        nsCString type;
> +        if (NS_OK == mozilla::AndroidBridge::ConnectionGetMimeType(mConnection, type))

NS_SUCCEDED(mozilla::AndroidBridge::..) perhaps

Comment 3

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> ::: widget/android/AndroidBridge.cpp
> @@ +206,4 @@
> >          jEGLSurfacePointerField = 0;
> >      }
> >  
> > +    jGeckoAppShell = getClassGlobalRef("org/mozilla/gecko/GeckoAppShell");
> 
> Isn't this stuff in GeneratedJNIWrappers?
It is, but its private.
> 
> @@ +2117,5 @@
> >      }
> >      return -1;
> >  }
> > +
> > +jobject AndroidBridge::GetConnection(nsIURI* aURI) {
> 
> Do we need this in addition to the generated one?
The generated one uses GetJNIEnv(), which only works on the main gecko thread. These functions need to run background threads and are implemented with GetJNIForThread()

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> > ::: widget/android/AndroidBridge.cpp
> > @@ +206,4 @@
> > >          jEGLSurfacePointerField = 0;
> > >      }
> > >  
> > > +    jGeckoAppShell = getClassGlobalRef("org/mozilla/gecko/GeckoAppShell");
> > 
> > Isn't this stuff in GeneratedJNIWrappers?
> It is, but its private.
> > 
> > @@ +2117,5 @@
> > >      }
> > >      return -1;
> > >  }
> > > +
> > > +jobject AndroidBridge::GetConnection(nsIURI* aURI) {
> > 
> > Do we need this in addition to the generated one?
> The generated one uses GetJNIEnv(), which only works on the main gecko
> thread. These functions need to run background threads and are implemented
> with GetJNIForThread()

The @WrapElementForJNI annotation supports an 'allowMultithread' property which should fix that for you.

If there are other things preventing you from using the generated wrappers we should try to fix that too. Otherwise you can just replace @WrapElementForJNI with @JNITarget since you aren't using the generated code at all, AFAICT.

Comment 5

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

Attachment: android_protocol.patch

Comment 6

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

Attachment: android_protocol.patch

Comment 7

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

Attachment: narrow_char_support.patch

This patch avoids some character conversions by annotating the generated functions to use narrow chars (UTF-8) and adding an nsJNICString class.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8430467 [details] [diff] [review]
narrow_char_support.patch

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

nice, how did we not already need this?

Comment 9

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

Attachment: android_protocol.patch

Comment 10

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

Attachment: android_protocol.patch

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c54b7fe8941

Comment 12

[Nick Alexander :nalexander [he/him]]

snorp: I can't see any consumers of "android://" in the tree.  (Not even tests!)  This code looks like it allows to load App icons from arbitrary packages, which I imagine had something to do with the WebAppRT.  Is that correct?  Can we remove this now?

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

I actually can't remember why we added this, but I don't see anything using it either. Brad, do you remember why we have this?

Comment 14

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

This was for getting icons from the system, originally for a gecko-based homescreen. Fabrice, is that still a thing?

Comment 15

[[:fabrice] Fabrice Desré]

No, we don't need that anymore.

Comment 16

[Nick Alexander :nalexander [he/him]]

blassey, fabrice: thanks for confirming.

This code isn't used right now, but it's a valuable demonstration of how to add an Android specific Gecko protocol.  I added resource://android/assets for this reason, more or less, but it would be nice to support Webview's file:///android_asset and file:///android_res (see https://developer.android.com/reference/android/webkit/WebSettings.html) exactly.  Maybe this code would let us do that?