Android: sync up latest JNI generator changes from downstream.

Android: sync up latest JNI generator changes from downstream.

Quite a few changes were made downstream, keep them in sync:
- specify the #include files.
- inject the common DCHECKs for class and native pointers.
- inject the check exception call.

BUG=

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

[bulach, 2013-12-13 10:55:54]

hey folks,

In order to ensure my patch downstream also work in chrome :) I'd rather have
this as the "authoritative" source.
once we're all happy, I'll sync it up again downstream, where the changes should
be minimal. would you mind taking a look please?

[rmcilroy, 2013-12-13 14:14:51]

One meta-point - I'm not really sure if the eager_called_by_natives /
lazy_called_by_natives is worth the extra complexity involved in having both. 
What would be the pros/cons of switching everything to either lazy or eager so
we only need to support one method registration approach?

Otherwise lgtm other than a couple of minor nits.

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

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:114: # Not a simple map lookup,
because we need to handle generics.
nit - comment might be clearer as something like: "Checking just the start of
the name, rather than a direct comparison, since we need to handle generics"

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
Why the extra args here - does downstream use them?

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator_tests.py:816:
self.assertTextEquals(golden_content, generated_text)
maybe rather than a try/catch just do a 
if not equals...:
 if os.environ.get(REBASELINE_ENV):
   rebaselineraise..."
 raise

[bulach, 2013-12-13 15:23:57]

thanks ross! as for the meta question:
yeah, it's a bit unfortunate to support both mechanisms, but I think it's
unavoidable... and I presume that keeping this generator consistent will be
better than just fork it..

the reason for lazy: in chrome, we have now ~190 files being generated... lazy
is certainly the preferred way here.

the other downstream project have slightly different characteristics (far less
files, synchronous library loading, etc..), so it makes sense for them to be
"eager"...

hopefully the test coverage is good enough to support these mechanisms ;)

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

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:114: # Not a simple map lookup,
because we need to handle generics.
On 2013/12/13 14:14:51, rmcilroy wrote:
> nit - comment might be clearer as something like: "Checking just the start of
> the name, rather than a direct comparison, since we need to handle generics"

Done.

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
On 2013/12/13 14:14:51, rmcilroy wrote:
> Why the extra args here - does downstream use them?

that's right.. downstream has a slightly different mechanism than here.

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/115103002/diff/10001/base/android/jni_generat...
base/android/jni_generator/jni_generator_tests.py:816:
self.assertTextEquals(golden_content, generated_text)
On 2013/12/13 14:14:51, rmcilroy wrote:
> maybe rather than a try/catch just do a 
> if not equals...:
>  if os.environ.get(REBASELINE_ENV):
>    rebaselineraise..."
>  raise

yeah, good point. had to change a bit to expose the comparison without the
assert but yeah, it's clearer. thanks!

[dawalker, 2013-12-13 21:41:08]

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
One comment that Tatsuo made before, and I would also like: Change this to
CHECK_NATIVE_PTR. The reason is that DCHECK_NATIVE_PTR is a bad name, because
it's an unconditional check in our implementation.

I know that has tension with the fact that this really is a DCHECK in Chrome's
implementation of the helper file, but to me that seems like an implementation
detail.

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:22: #define
DCHECK_CLAZZ(clazz, ...) \
ditto

[dawalker, 2013-12-13 21:57:58]

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
On 2013/12/13 21:41:08, dawalker wrote:
> One comment that Tatsuo made before, and I would also like: Change this to
> CHECK_NATIVE_PTR. The reason is that DCHECK_NATIVE_PTR is a bad name, because
> it's an unconditional check in our implementation.
> 
> I know that has tension with the fact that this really is a DCHECK in Chrome's
> implementation of the helper file, but to me that seems like an implementation
> detail.

I made this comment before I saw your comment on the Critique CL. I'm gonna
paste your response here, and then reply to it:

> hehe, we had a long, long debate about this a while ago, let me summarize :)
> 
> - on the java side, we already have a few places doing "if (mNativePtr != 0) {
nativeDoFoo(mNativePtr);".. that's the only sane place to have any meaningful
fallback, down here at the JNI boundary is already too late...

So, we're taking a different approach. In our header file, we do the check here
(in native code) and then throw a Java NullPointerException. It's by no means
too late to do the check natively, and this allows the check to be done
uniformly.

> - adding as "always on" check would then be redundant (also increases binary
size and has a small performance hit)

Yes, it's expected to be a small performance hit for us, but it's dwarfed by the
slowness of crossing the JNI boundary.

> - if it crashes further down the road when "native->" would try to
de-reference any of its member, the crashstack we get from the field (or
tombstones locally) would provide enough information to know what's happening...

We're not using breakpad, so crashes in native code are *much* more painful for
us than uncaught exception crashes.

> - debug builds have this check, and hopefully, enough test coverage....
> 
> So our decision was to keep it lean and mean...
> 
> as mentioned in the description, I'm not changing the semantics for gmm, if
you think it's important to have a CHECK instead of DCHECK, that's totally fine
to be done in the included header file (which will project-dependent
anyways...)!! however, this change is necessary to keep the generator itself in
sync with chromium...

Right. My comment was along the lines that, since this macro enables an
unconditional check, it should be named accordingly. The fact that Chrome, in
its implementation, only does a DCHECK is the implementation detail. :)

[nadiapayet, 2014-01-06 19:22:59]

Hey Marcus,

Any update on this CL? I am blocked on this (and the subsequent change
https://critique.corp.google.com/#review/57432683) to make the final set of
changes to the jni_generator in third_party/jni_generator. Any objections to the
CHECK_ vs. DCHECK_ naming?

[bulach, 2014-01-07 09:23:53]

happy new year everybody! :)

I think the most contentious bits was the (D)CHECK.. I'm convinced by your
argument that it's an implementation detail that chromium does the "D" variant,
so I went ahead and changed the macros to just CHECK..

as for throwing an exception:
""
So, we're taking a different approach. In our header file, we do the check here
(in native code) and then throw a Java NullPointerException. It's by no means
too late to do the check natively, and this allows the check to be done
uniformly.
""

yeah, the whole point of this patch is to allow our different approaches :)

brief history:
in chromium, before we had this generator, we already had tons of places that
was doing the "if (native != 0)" test in the java side and taking the
appropriate steps...
adding that check again on our release builds would be an unnecessary overhead,
so we decided to use "D"CHECK...

breakpad is also part of chrome, for all platforms (you folks should certainly
look into it!!).. we have the luxury of rich bugreports should crashes on the
native pointer happen.

your approach is totally cool as well, specially since you'll probably be able
to start from a clean(er) slate!!!

please take another look, then once this lands, I'll refresh the other side too.

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
On 2013/12/13 21:41:08, dawalker wrote:
> One comment that Tatsuo made before, and I would also like: Change this to
> CHECK_NATIVE_PTR. The reason is that DCHECK_NATIVE_PTR is a bad name, because
> it's an unconditional check in our implementation.
> 
> I know that has tension with the fact that this really is a DCHECK in Chrome's
> implementation of the helper file, but to me that seems like an implementation
> detail.

Done.

https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:22: #define
DCHECK_CLAZZ(clazz, ...) \
On 2013/12/13 21:41:08, dawalker wrote:
> ditto

Done.

[nadiapayet, 2014-01-07 16:40:08]

On 2014/01/07 09:23:53, bulach wrote:
> happy new year everybody! :)
> 
> I think the most contentious bits was the (D)CHECK.. I'm convinced by your
> argument that it's an implementation detail that chromium does the "D"
variant,
> so I went ahead and changed the macros to just CHECK..
> 
> as for throwing an exception:
> ""
> So, we're taking a different approach. In our header file, we do the check
here
> (in native code) and then throw a Java NullPointerException. It's by no means
> too late to do the check natively, and this allows the check to be done
> uniformly.
> ""
> 
> yeah, the whole point of this patch is to allow our different approaches :)
> 
> brief history:
> in chromium, before we had this generator, we already had tons of places that
> was doing the "if (native != 0)" test in the java side and taking the
> appropriate steps...
> adding that check again on our release builds would be an unnecessary
overhead,
> so we decided to use "D"CHECK...
> 
> breakpad is also part of chrome, for all platforms (you folks should certainly
> look into it!!).. we have the luxury of rich bugreports should crashes on the
> native pointer happen.
> 
> your approach is totally cool as well, specially since you'll probably be able
> to start from a clean(er) slate!!!
> 
> please take another look, then once this lands, I'll refresh the other side
too.
> 
>
https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
> File base/android/jni_generator/jni_generator_helper.h (right):
> 
>
https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
> base/android/jni_generator/jni_generator_helper.h:19: #define
> DCHECK_NATIVE_PTR(native_ptr, method_name, ...) \
> On 2013/12/13 21:41:08, dawalker wrote:
> > One comment that Tatsuo made before, and I would also like: Change this to
> > CHECK_NATIVE_PTR. The reason is that DCHECK_NATIVE_PTR is a bad name,
because
> > it's an unconditional check in our implementation.
> > 
> > I know that has tension with the fact that this really is a DCHECK in
Chrome's
> > implementation of the helper file, but to me that seems like an
implementation
> > detail.
> 
> Done.
> 
>
https://codereview.chromium.org/115103002/diff/30001/base/android/jni_generat...
> base/android/jni_generator/jni_generator_helper.h:22: #define
> DCHECK_CLAZZ(clazz, ...) \
> On 2013/12/13 21:41:08, dawalker wrote:
> > ditto
> 
> Done.

LGTM. Thanks for the change.

[nadiapayet, 2014-01-07 16:53:01]

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
CHECK_NATIVE_PTR(native_ptr, method_name, ...) \
We would also need the JNIEnv as well as the object itself here, can we just add
these 2 to the macro signature?

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:22: #define CHECK_CLAZZ(clazz,
...) \
Same comment for this one.

[bulach, 2014-01-07 18:03:51]

thanks nadia! addressed, another look please?

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:19: #define
CHECK_NATIVE_PTR(native_ptr, method_name, ...) \
On 2014/01/07 16:53:01, nadiapayet wrote:
> We would also need the JNIEnv as well as the object itself here, can we just
add
> these 2 to the macro signature?

Done.

https://codereview.chromium.org/115103002/diff/50001/base/android/jni_generat...
base/android/jni_generator/jni_generator_helper.h:22: #define CHECK_CLAZZ(clazz,
...) \
On 2014/01/07 16:53:01, nadiapayet wrote:
> Same comment for this one.

Done.

[nadiapayet, 2014-01-07 18:37:19]

Looking great, thanks! This LGTM now. Let's wait for dawalker's comments before
submitting.

[dawalker, 2014-01-07 21:40:56]

On 2014/01/07 18:37:19, nadiapayet wrote:
> Looking great, thanks! This LGTM now. Let's wait for dawalker's comments
before
> submitting.

LGTM, based on the discussion without looking at the files.

[commit-bot: I haz the power, 2014-01-08 08:59:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/115103002/140001

[commit-bot: I haz the power, 2014-01-08 12:23:10]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-08 13:18:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/115103002/140001

[commit-bot: I haz the power, 2014-01-08 14:41:23]

Change committed as 243558