Hook components/gcm_driver up to JNI Generator

Hook components/gcm_driver up to JNI Generator

(modelled on https://codereview.chromium.org/183893029)

This will be used when implementing GCMDriverAndroid.

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

[johnme, 2014-05-15 12:56:58]

bulach: please review overall correct usage of JNI generator (and OWNERS
approval of chrome/browser/android/chrome_jni_registrar.cc)

jochen: please add OWNERS approval of remaining files

Thanks!

[bulach, 2014-05-15 13:05:15]

lgtm, but I'm not an owner. just checking: does the gypi rule work fine without
the actual java files? if it does, go for it...

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver.gypi#n...
components/gcm_driver.gypi:59: 'jni_generator_ptr_type': 'long',
nit: you can remove the jni_generator_ptr_type, sorry about that.
I'm about to clean it up, the transition is, long, gone.

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/androi...
File components/gcm_driver/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/androi...
components/gcm_driver/android/component_jni_registrar.cc:15: static
base::android::RegistrationMethod kGCMDriverRegisteredMethods[] = { };
ultra nit:
put the }; in the following line, diffs will be smaller.

[jochen (gone - plz use gerrit), 2014-05-15 14:20:21]

lgtm

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/DEPS
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/DEPS#n...
components/gcm_driver/DEPS:4: "+jni",
is there also a jni gyp target you should depend on?

[fgorski, 2014-05-15 17:38:02]

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gy...
components/gcm_driver.gypi:26: 'gcm_driver/gcm_driver_android.cc',
Will it be correct to build jni* and android* sources outside of Android?

[johnme, 2014-05-15 17:39:47]

Thanks both! Addressed comments.

Marcus wrote:
> just checking: does the gypi rule work fine without
the actual java files? if it does, go for it...

Ah, I checked, and indeed the Generator requires you to have one or more Java
files. The Java files also must have one or more methods exposed to JNI.
Accordingly I've added a Java file which exposes a doNothing method, and
TODO(johnme)s to replace this with the real implementation of GCMDriver, which
I'm working on next.

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver.gypi#n...
components/gcm_driver.gypi:59: 'jni_generator_ptr_type': 'long',
On 2014/05/15 13:05:16, bulach wrote:
> nit: you can remove the jni_generator_ptr_type, sorry about that.
> I'm about to clean it up, the transition is, long, gone.

Done.

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/DEPS
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/DEPS#n...
components/gcm_driver/DEPS:4: "+jni",
On 2014/05/15 14:20:21, jochen wrote:
> is there also a jni gyp target you should depend on?

No, these are files generated by the JNI Generator, due to the Java targets
added to gcm_driver.gypi

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/androi...
File components/gcm_driver/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/285203005/diff/1/components/gcm_driver/androi...
components/gcm_driver/android/component_jni_registrar.cc:15: static
base::android::RegistrationMethod kGCMDriverRegisteredMethods[] = { };
On 2014/05/15 13:05:16, bulach wrote:
> ultra nit:
> put the }; in the following line, diffs will be smaller.

Done.

[bulach, 2014-05-15 17:49:08]

lgtm, but one suggestion

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_android.cc:14: void doNothing() {
nit: s/do/Do/
also, unindent below.
how come it's ok for this to be unused?

I think you can get away with:
$decl __attribute__ ((unused));

(where $decl is the gcm::Java_GCMDriver_doNothing declaration..)

[johnme, 2014-05-15 17:54:14]

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gy...
components/gcm_driver.gypi:26: 'gcm_driver/gcm_driver_android.cc',
On 2014/05/15 17:38:02, fgorski wrote:
> Will it be correct to build jni* and android* sources outside of Android?

Files whose path includes (^|/)android/ or that end in _android.cc don't get
built on Android, and the targets below are guarded by an OS=="android"
condition. So none of the changes to this GYP file have effect on desktop.

[fgorski, 2014-05-15 18:01:17]

On 2014/05/15 17:54:14, johnme wrote:
>
https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gypi
> File components/gcm_driver.gypi (right):
> 
>
https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gy...
> components/gcm_driver.gypi:26: 'gcm_driver/gcm_driver_android.cc',
> On 2014/05/15 17:38:02, fgorski wrote:
> > Will it be correct to build jni* and android* sources outside of Android?
> 
> Files whose path includes (^|/)android/ or that end in _android.cc don't get
> built on Android, and the targets below are guarded by an OS=="android"
> condition. So none of the changes to this GYP file have effect on desktop.

lgtm then.

you meant don't get build outside of Android, right?

[jianli, 2014-05-15 18:13:38]

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gy...
components/gcm_driver.gypi:41: ['OS=="android"', {
nit: space before and after '=='

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
File components/gcm_driver/android/component_jni_registrar.h (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.h:8: #include <jni.h>
nit: probably you don't need to include this header file. Instead, you can
forward declare JNIEnv.

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.h:10: namespace gcm {
Wrong namespace. I am also going to update namespace in other gcm component
files to use gcm_driver namespace.

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_android.h (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_android.h:10: namespace gcm {
ditto

[jianli, 2014-05-15 19:04:43]

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
File components/gcm_driver/android/component_jni_registrar.h (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.h:10: namespace gcm {
On 2014/05/15 18:13:39, jianli wrote:
> Wrong namespace. I am also going to update namespace in other gcm component
> files to use gcm_driver namespace.

We decide not to update namespace to gcm_driver at this moment. We're going to
do it after all files have been componentized. So you can keep using gcm
namespace.

[jianli, 2014-05-15 19:05:26]

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
File components/gcm_driver/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.cc:29: }  // namespace
gcm_driver
nit: in a few places you have inconsistency in the namespace comment

[johnme, 2014-05-15 19:29:41]

Thanks everyone, addressed the review comments.

Filip wrote:
> you meant don't get build outside of Android, right?

Yes, exactly.

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver.gy...
components/gcm_driver.gypi:41: ['OS=="android"', {
On 2014/05/15 18:13:39, jianli wrote:
> nit: space before and after '=='

Done (though technically, no spaces is 3x more common in the codebase).

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
File components/gcm_driver/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.cc:29: }  // namespace
gcm_driver
On 2014/05/15 19:05:26, jianli wrote:
> nit: in a few places you have inconsistency in the namespace comment

Done (just here; I couldn't find any other places?).

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
File components/gcm_driver/android/component_jni_registrar.h (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/component_jni_registrar.h:8: #include <jni.h>
On 2014/05/15 18:13:39, jianli wrote:
> nit: probably you don't need to include this header file. Instead, you can
> forward declare JNIEnv.

Unfortunately, JNIEnv is defined as:

struct _JNIEnv;
#if defined(__cplusplus)
typedef _JNIEnv JNIEnv;
#else
typedef const struct JNINativeInterface* JNIEnv;
#endif

and you can't forward declare a typedef without exposing implementation
details[1] :-|

[1]:
http://stackoverflow.com/questions/804894/forward-declaration-of-a-typedef-in-c

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/285203005/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_android.cc:14: void doNothing() {
On 2014/05/15 17:49:09, bulach wrote:
> nit: s/do/Do/
> also, unindent below.
> how come it's ok for this to be unused?
> 
> I think you can get away with:
> $decl __attribute__ ((unused));
> 
> (where $decl is the gcm::Java_GCMDriver_doNothing declaration..)

Done - I now forward declare it with ALLOW_UNUSED (and got your verbal lgtm on
that).

[johnme, 2014-05-15 19:38:52]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-05-15 19:40:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/[email protected]/285203005/40001

[commit-bot: I haz the power, 2014-05-15 22:23:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to [email protected].
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-15 23:14:06]

Change committed as 270843