Closed Bug 1192077 Opened 4 years ago Closed 4 years ago

Move AndroidBridge JNIEnv calls to jni/Utils

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(2 files)

Now that we have jni/Utils, functions like GetJNIForThread are more suited there than AndroidBridge.
Calls like GetJNIForThread should now belong in jni/Utils. Moving the
calls also reduce clutter in AndroidBridge.
Attachment #8644689 - Flags: review?(esawin)
Comment on attachment 8644689 [details] [diff] [review]
Move AndroidBridge JNIEnv calls to jni/Utils (v1)

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

Looks good.

::: widget/android/AndroidBridge.cpp
@@ +252,5 @@
>  bool
>  AndroidBridge::SetMainThread(pthread_t thr)
>  {
>      ALOG_BRIDGE("AndroidBridge::SetMainThread");
> +

Whitespace.
Attachment #8644689 - Flags: review?(esawin) → review+
Comment on attachment 8644690 [details] [diff] [review]
Convert AndroidBridge JNIEnv calls (v1)

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

Looks good with some of the nits addressed.

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +404,5 @@
>  
>    bool draining = false;
>    bool waitingEOF = false;
>  
> +  AutoLocalJNIFrame frame(jni::GetEnvForThread(), 1);

Nit: this could be const, if AutoLocalJNIFrame::GetEnv is made const, which it should be.

::: dom/media/webrtc/MediaEngineWebRTC.cpp
@@ +131,5 @@
>  #ifdef MOZ_WIDGET_ANDROID
>    // get the JVM
> +  JavaVM* jvm;
> +  JNIEnv* const env = jni::GetEnvForThread();
> +  MOZ_ALWAYS_TRUE(!env->GetJavaVM(&jvm));

Can we reduce the code duplication by a helper function?

::: dom/plugins/base/PluginPRLibrary.cpp
@@ +36,5 @@
>  nsresult
>  PluginPRLibrary::NP_Initialize(NPNetscapeFuncs* bFuncs,
>  			       NPPluginFuncs* pFuncs, NPError* error)
>  {
> +  JNIEnv* env = jni::GetEnvForThread();

Nit: inconsistent if can made be * const.

::: dom/plugins/base/android/ANPAudio.cpp
@@ +123,5 @@
>  AudioRunnable::Run()
>  {
>    PR_SetCurrentThreadName("Android Audio");
>  
> +  JNIEnv* const jenv = mozilla::jni::GetEnvForThread();

Nit: a using-declaration would save some writing here.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1320,5 @@
>  
>      if (!mJavaView)
>        return false;
>  
> +    mJavaView = (void*)jni::GetGeckoThreadEnv()->NewGlobalRef((jobject)mJavaView);

C++-style type casting should be preferred.

::: mobile/android/base/GeckoAppShell.java
@@ -257,5 @@
>      /* The Android-side API: API methods that Android calls */
>  
>      // Initialization methods
>      public static native void registerJavaUiThread();
> -    public static native void nativeInit(ClassLoader clsLoader, MessageQueue msgQueue);

Shouldn't this go into bug 1189995?

::: widget/android/AndroidJNIWrapper.cpp
@@ +36,5 @@
>    jsjni_FindClass(const char *className) {
>      // FindClass outside the main thread will run into problems due
>      // to missing the classpath
>      MOZ_ASSERT(NS_IsMainThread());
> +    JNIEnv *env = mozilla::jni::GetGeckoThreadEnv();

Nit: a using-declaration or even a using-directive could help here.

@@ +121,5 @@
>    __attribute__ ((visibility("default")))
>    JavaVM* jsjni_GetVM() {
> +    JavaVM* jvm;
> +    JNIEnv* const env = mozilla::jni::GetGeckoThreadEnv();
> +    MOZ_ALWAYS_TRUE(!env->GetJavaVM(&jvm));

Code duplication? I've seen this before.
Attachment #8644690 - Flags: review?(esawin) → review+
https://hg.mozilla.org/mozilla-central/rev/c0e71e22101b
https://hg.mozilla.org/mozilla-central/rev/c9126c9cfc34
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.