[prev in list] [next in list] [prev in thread] [next in thread] 

List:       rockbox-dev
Subject:    Re: kugel: r29601 - in trunk: apps/hosted/android
From:       Maurus Cuelenaere <mcuelenaere () gmail ! com>
Date:       2011-03-16 19:19:12
Message-ID: 4D810D30.8030407 () gmail ! com
[Download RAW message or body]

Op 16-03-11 15:33, mailer@svn.rockbox.org schreef:
> Date: 2011-03-16 15:33:55 +0100 (Wed, 16 Mar 2011)
> New Revision: 29601
> 
> Log Message:
> Android: Partly revert r29569 and only call the new getJavaEnvironment() when needed.
> 
> The environment is fine to share in general, just not across OS threads, so it's only needed
> for functions which are possibly called from multiple OS threads (only 1 currently).
> 
[snip]
> Modified: trunk/apps/hosted/android/yesno.c
> ===================================================================
> --- trunk/apps/hosted/android/yesno.c	2011-03-16 11:38:49 UTC (rev 29600)
> +++ trunk/apps/hosted/android/yesno.c	2011-03-16 14:33:55 UTC (rev 29601)
> @@ -28,8 +28,8 @@
>  #include "settings.h"
>  #include "lang.h"
>  #include "kernel.h"
> -#include "system.h"
>  
> +extern JNIEnv   *env_ptr;
>  static jobject   RockboxYesno_instance = NULL;
>  static jmethodID yesno_func;
>  static struct semaphore    yesno_done;
> @@ -44,7 +44,7 @@
>      semaphore_release(&yesno_done);
>  }
>  
> -static void yesno_init(JNIEnv *env_ptr)
> +static void yesno_init(void)
>  {
>      JNIEnv e = *env_ptr;
>      static jmethodID yesno_is_usable;
> @@ -74,7 +74,7 @@
>          sleep(HZ/10);
>  }
>  
> -static jstring build_message(JNIEnv *env_ptr, const struct text_message *message)
> +jstring build_message(const struct text_message *message)

I don't think this needed to be reverted?

>  {
>      char msg[1024] = "";
>      JNIEnv e = *env_ptr;
> @@ -98,12 +98,10 @@
>  {
>      (void)yes_message;
>      (void)no_message;
> -    JNIEnv *env_ptr = getJavaEnvironment();
> -
> -    yesno_init(env_ptr);
> -
> +    yesno_init();
> +    
>      JNIEnv e = *env_ptr;
> -    jstring message = build_message(env_ptr, main_message);
> +    jstring message = build_message(main_message);
>      jstring yes = (*env_ptr)->NewStringUTF(env_ptr, str(LANG_SET_BOOL_YES));
>      jstring no  = (*env_ptr)->NewStringUTF(env_ptr, str(LANG_SET_BOOL_NO));
>  
> 
> Modified: trunk/firmware/target/hosted/android/button-android.c
> ===================================================================
> --- trunk/firmware/target/hosted/android/button-android.c	2011-03-16 11:38:49 UTC (rev 29600)
> +++ trunk/firmware/target/hosted/android/button-android.c	2011-03-16 14:33:55 UTC (rev 29601)
> @@ -29,6 +29,7 @@
>  #include "system.h"
>  #include "touchscreen.h"
>  
> +extern JNIEnv *env_ptr;

Same here, no users of env_ptr here?

>  static int last_y, last_x;
>  static int last_btns;
>  
> 
[snip]
> Modified: trunk/firmware/target/hosted/android/system-target.h
> ===================================================================
> --- trunk/firmware/target/hosted/android/system-target.h	2011-03-16 11:38:49 UTC (rev 29600)
> +++ trunk/firmware/target/hosted/android/system-target.h	2011-03-16 14:33:55 UTC (rev 29601)
> @@ -21,8 +21,6 @@
>  #ifndef __SYSTEM_TARGET_H__
>  #define __SYSTEM_TARGET_H__
>  
> -#include <jni.h>
> -
>  #define disable_irq()
>  #define enable_irq()
>  #define disable_irq_save() 0
> @@ -32,20 +30,16 @@
>  void wait_for_interrupt(void);
>  void interrupt(void);
>  
> -/* A JNI environment is specific to its thread, so use the correct way to
> - * obtain it: share a pointer to the JavaVM structure and ask that the JNI
> - * environment attached to the current thread. */
> -static inline JNIEnv* getJavaEnvironment(void)
> -{
> -    extern JavaVM *vm_ptr;
> -    JNIEnv *env = NULL;
> + /* don't pull in jni.h for every user of this file, it should be only needed
> +  * within the target tree (if at all)
> +  * define this before #including system.h or system-target.h */
> +#ifdef _SYSTEM_WITH_JNI
> +#include <jni.h>
> +/*
> + * discover the JNIEnv for this the calling thread in case it's not known */
> +extern JNIEnv* getJavaEnvironment(void);
> +#endif /* _SYSTEM_WITH_JNI */
>  
> -    if (vm_ptr)
> -        (*vm_ptr)->GetEnv(vm_ptr, (void**) &env, JNI_VERSION_1_2);
> -
> -    return env;
> -}
> -


This should be solvable with the following trick:

diff --git a/firmware/target/hosted/android/pcm-android.c
b/firmware/target/hosted/android/pcm-android.c
index cc8bd9c..735956a 100644
--- a/firmware/target/hosted/android/pcm-android.c
+++ b/firmware/target/hosted/android/pcm-android.c
@@ -21,7 +21,6 @@

 #include <jni.h>
 #include <stdbool.h>
-#define _SYSTEM_WITH_JNI /* for getJavaEnvironment */
 #include <system.h>
 #include "debug.h"
 #include "pcm.h"
diff --git a/firmware/target/hosted/android/system-target.h
b/firmware/target/hosted/android/system-target.h
index 325c101..c547a58 100644
--- a/firmware/target/hosted/android/system-target.h
+++ b/firmware/target/hosted/android/system-target.h
@@ -30,15 +30,11 @@ void power_off(void);
 void wait_for_interrupt(void);
 void interrupt(void);

- /* don't pull in jni.h for every user of this file, it should be only needed
-  * within the target tree (if at all)
-  * define this before #including system.h or system-target.h */
-#ifdef _SYSTEM_WITH_JNI
-#include <jni.h>
-/*
- * discover the JNIEnv for this the calling thread in case it's not known */
+struct JNINativeInterface;
+typedef const struct JNINativeInterface* JNIEnv;
+
+/* discover the JNIEnv for this the calling thread in case it's not known */
 extern JNIEnv* getJavaEnvironment(void);
-#endif /* _SYSTEM_WITH_JNI */



>  #endif /* __SYSTEM_TARGET_H__ */
>  
>  #define NEED_GENERIC_BYTESWAPS
> 

Also, is not wanting to include jni.h for everyone the same reason for that ugly
"extern JNIEnv *env_ptr;" all over the place? If so, that can be solved the same
way as above.
[prev in list] [next in list] [prev in thread] [next in thread]