Make class lookup lazy in jni_generator when using lazy method lookup.

Make class lookup lazy in jni_generator when using lazy method lookup.

This removes the eager class registration from RegisterNatives when possible.

BUG=402003
TBR=sievers@chromium.org, brettw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290810

[mkosiba (inactive), 2014-08-15 16:53:50]

PTAL

https://codereview.chromium.org/472553002/diff/60001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

https://codereview.chromium.org/472553002/diff/60001/base/android/jni_android...
base/android/jni_android.cc:134: ScopedJavaLocalRef<jobject> class_loader =
GetClassLoader(env);
having jni_android depend on jni initialization having already been performed is
not ideal. The alternative is to pass the instance in as an argument, but that
seems silly, since we're requiring the caller to 'connect the dots' for us.

https://codereview.chromium.org/472553002/diff/60001/ui/gl/android/scoped_jav...
File ui/gl/android/scoped_java_surface.cc (left):

https://codereview.chromium.org/472553002/diff/60001/ui/gl/android/scoped_jav...
ui/gl/android/scoped_java_surface.cc:35: DCHECK(env->IsInstanceOf(surface.obj(),
g_Surface_clazz));
I was thinking of making this a method (g_Sufrace_clazz() ?) and having the JNI
generator generate the right code.

Any preferences/ideas on how to address this?

[mkosiba (inactive), 2014-08-18 11:01:22]

https://codereview.chromium.org/472553002/diff/60001/ui/gl/android/scoped_jav...
File ui/gl/android/scoped_java_surface.cc (left):

https://codereview.chromium.org/472553002/diff/60001/ui/gl/android/scoped_jav...
ui/gl/android/scoped_java_surface.cc:35: DCHECK(env->IsInstanceOf(surface.obj(),
g_Surface_clazz));
On 2014/08/15 16:53:50, mkosiba wrote:
> I was thinking of making this a method (g_Sufrace_clazz() ?) and having the
JNI
> generator generate the right code.
> 
> Any preferences/ideas on how to address this?

Should be now fixed

[Torne, 2014-08-18 14:54:40]

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:179: subtle::AtomicWord value =
base::subtle::NoBarrier_Load(atomic_class_id);
Can't we just return old here?

[rmcilroy, 2014-08-18 15:16:41]

Looks good overall, a couple of suggestions.

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/JNIUtils.java (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/JNIUtils.java:10: public class JNIUtils
{
nit - I think this should be JniUtils according to the style guide.

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:125: void InitFallbackClassLoader(JNIEnv* env) {
Maybe call this InitReplacementClassLoader (since the classloader used here
replaces all classloaders.  Also, I think it might be better to just pass the
classloader as an argument to this function, with the caller itself using the
JniUtils GetClassLoader call to get the correct classloader - that way you don't
need the warning on the function description that this needs to be done after
registering the JNI for base, WDYT?

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:176: // the ref in |atomic_class_id|.
Maybe mention that this is so that we can store it as a raw pointer in
|atomic_class_id| and to avoid the destructor being called on the JavaRef?

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:179: subtle::AtomicWord value =
base::subtle::NoBarrier_Load(atomic_class_id);
Hmm, I'm not sure about the semantics of a NoBarrier_Load after a
Release_CompareAndSwap.  The definition of Release operations is "all previous
memory stores are globally visible, and all previous memory loads have been
satisfied, but following memory reads are not prevented from being speculated to
before the barrier".  

I think this should still be OK since the two operations are on the same memory
location, so the NoBarrier_Load shouldn't be speculated above the
Release_CompareAndSwap, but these things are subtle.

Would it not be OK just to return the old value here?

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:1128: first_param_in_call =
('%s_clazz(env)' % (java_class,))
nit - remove unnecessary ","

[Torne, 2014-08-18 15:27:21]

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:1128: first_param_in_call =
('%s_clazz(env)' % (java_class,))
On 2014/08/18 15:16:41, rmcilroy wrote:
> nit - remove unnecessary ","

The , is what makes it a 1-element tuple instead of just a parenthesised
expression. It will work without it because % specialcases formatting a string
that only needs one param, but in that case you may as well also leave out the
parens.

[rmcilroy, 2014-08-18 15:32:17]

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:1128: first_param_in_call =
('%s_clazz(env)' % (java_class,))
On 2014/08/18 15:27:21, Torne wrote:
> On 2014/08/18 15:16:41, rmcilroy wrote:
> > nit - remove unnecessary ","
> 
> The , is what makes it a 1-element tuple instead of just a parenthesised
> expression. It will work without it because % specialcases formatting a string
> that only needs one param, but in that case you may as well also leave out the
> parens.

That makes sense, thanks.  I still think it doesn't need the params or extra
comma, but I don't particularly mind.

[mkosiba (inactive), 2014-08-19 09:50:15]

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/JNIUtils.java (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/JNIUtils.java:10: public class JNIUtils
{
On 2014/08/18 15:16:41, rmcilroy wrote:
> nit - I think this should be JniUtils according to the style guide.

base has two *JNI* classes (JNINamespace and JNIAdditionalImport) but zero *Jni*
classes. Just following suit here.

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:125: void InitFallbackClassLoader(JNIEnv* env) {
On 2014/08/18 15:16:41, rmcilroy wrote:
> Maybe call this InitReplacementClassLoader (since the classloader used here
> replaces all classloaders.  Also, I think it might be better to just pass the
> classloader as an argument to this function, with the caller itself using the
> JniUtils GetClassLoader call to get the correct classloader - that way you
don't
> need the warning on the function description that this needs to be done after
> registering the JNI for base, WDYT?

Sounds good. As I was commenting earlier - it seemed to me that having a method
in base:: ask the caller to give it the result of calling another method in
base:: seemed a bit ugly. It does clarify when it is appropriate to call the
method though so I guess I'll just go with that.

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:176: // the ref in |atomic_class_id|.
On 2014/08/18 15:16:41, rmcilroy wrote:
> Maybe mention that this is so that we can store it as a raw pointer in
> |atomic_class_id| and to avoid the destructor being called on the JavaRef?

Done.

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_android...
base/android/jni_android.cc:179: subtle::AtomicWord value =
base::subtle::NoBarrier_Load(atomic_class_id);
On 2014/08/18 15:16:41, rmcilroy wrote:
> Would it not be OK just to return the old value here?

Yeath, using old should be fine here.

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:1128: first_param_in_call =
('%s_clazz(env)' % (java_class,))
On 2014/08/18 15:32:17, rmcilroy wrote:
> On 2014/08/18 15:27:21, Torne wrote:
> > On 2014/08/18 15:16:41, rmcilroy wrote:
> > > nit - remove unnecessary ","
> > 
> > The , is what makes it a 1-element tuple instead of just a parenthesised
> > expression. It will work without it because % specialcases formatting a
string
> > that only needs one param, but in that case you may as well also leave out
the
> > parens.
> 
> That makes sense, thanks.  I still think it doesn't need the params or extra
> comma, but I don't particularly mind.

Done.

[rmcilroy, 2014-08-19 10:42:00]

lgtm, thanks!

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/JNIUtils.java (right):

https://codereview.chromium.org/472553002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/JNIUtils.java:10: public class JNIUtils
{
On 2014/08/19 09:50:15, mkosiba wrote:
> On 2014/08/18 15:16:41, rmcilroy wrote:
> > nit - I think this should be JniUtils according to the style guide.
> 
> base has two *JNI* classes (JNINamespace and JNIAdditionalImport) but zero
*Jni*
> classes. Just following suit here.

Fair enough :).

[Torne, 2014-08-19 10:58:08]

lgtm

[mkosiba (inactive), 2014-08-19 16:29:02]

Ross, please take a quick look at the most recent changes. I had to suppress
unused-function warnings. The culprit is AutofillDialogResult - it's a class
that has some inner classes. All the CalledByNative methods are on the inner
classes but the jni_generator pulls in the outer class as well.

[rmcilroy, 2014-08-19 17:03:51]

On 2014/08/19 16:29:02, mkosiba wrote:
> Ross, please take a quick look at the most recent changes. I had to suppress
> unused-function warnings. The culprit is AutofillDialogResult - it's a class
> that has some inner classes. All the CalledByNative methods are on the inner
> classes but the jni_generator pulls in the outer class as well.

yuck, but I guess there isn't really much way around this. lgtm.

[mkosiba (inactive), 2014-08-20 09:15:44]

TBR'ing brettw for base/ .gyp and .gn changes
TBR'ing sievers for ui/gl

[mkosiba (inactive), 2014-08-20 09:15:51]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-08-20 09:17:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/472553002/160001

[commit-bot: I haz the power, 2014-08-20 10:19:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-20 11:19:59]

Committed patchset #9 (160001) as 290810

[mkosiba (inactive), 2014-08-20 13:52:51]

this was reverted in https://codereview.chromium.org/492713002 since it broke
the build on the Android 64 bit builder.