bluetooth: Initial JNI setup for device/bluetooth.

bluetooth: Initial JNI setup for device/bluetooth.

BUG=471536
Committed: https://crrev.com/1f48f15dda8892c7d57e8155a011d9b4395299ba
Cr-Commit-Position: refs/heads/master@{#328678}

[scheib, 2015-05-01 19:34:19]

Patchset #1 (id:1) has been deleted

[scheib, 2015-05-05 04:22:15]

scheib@chromium.org changed reviewers:
+ armansito@chromium.org

[scheib, 2015-05-05 04:24:05]

scheib@chromium.org changed reviewers:
+ jam@chromium.org

[scheib, 2015-05-05 04:24:05]

armansito: PTAL device/*
jam: PTAL content/*

[scheib, 2015-05-05 04:24:17]

Patchset #1 (id:20001) has been deleted

[scheib, 2015-05-05 04:24:24]

Patchset #1 (id:30010) has been deleted

[scheib, 2015-05-05 04:24:32]

Patchset #1 (id:50001) has been deleted

[jam, 2015-05-05 14:58:09]

lgtm

[armansito, 2015-05-06 01:57:07]

mostly nits.

https://codereview.chromium.org/1119113002/diff/70001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/1119113002/diff/70001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:69: if
(!device::android::RegisterBluetoothJni(env))
Can you document somewhere that calling this is necessary for device/bluetooth
to work on Android?

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
I don't know much about GN, so maybe you should add someone with more GN
knowledge to review these bits.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
File device/bluetooth/android/bluetooth_jni_registrar.h (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
device/bluetooth/android/bluetooth_jni_registrar.h:15: DEVICE_BLUETOOTH_EXPORT
bool RegisterBluetoothJni(JNIEnv* env);
Please document what this function does.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/BluetoothAdapter.java
(right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/BluetoothAdapter.java:18:
@JNINamespace("device")
It would be good to have a Java reviewer to take a look at these to make sure
that the coding style is correct.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_android.h (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:24: static
base::WeakPtr<BluetoothAdapterAndroid> CreateAdapter();
Shouldn't this be <BluetoothAdapter>?

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:29: // True if this app has android
permissions necessary for bluetooth.
s/bluetooth/Bluetooth/

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:30: bool has_bluetooth_permission()
{ return has_bluetooth_permission_; }
nit: make this a const method.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:88: bool has_bluetooth_permission_
= false;
Is this allowed in Chromium (or C++ for that matter)? Otherwise initialize in
the constructor.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_android_unittest.cc (left):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android_unittest.cc:21:
EXPECT_TRUE(adapter_.get());
Add ASSERT_TRUE(adapter_.get()); or even better ASSERT_NE(adapter_.get(), NULL);

[scheib, 2015-05-06 04:26:19]

scheib@chromium.org changed reviewers:
+ timvolodine@chromium.org

[scheib, 2015-05-06 04:30:15]

Thanks armansito@.

timvolodine@, this JNI infrastructure setup mostly follows the example from your
vibration patch https://codereview.chromium.org/1129683002/ Would you review
with more JNI/android/java experience.

https://codereview.chromium.org/1119113002/diff/70001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/1119113002/diff/70001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:69: if
(!device::android::RegisterBluetoothJni(env))
On 2015/05/06 01:57:06, armansito wrote:
> Can you document somewhere that calling this is necessary for device/bluetooth
> to work on Android?

https://www.chromium.org/developers/design-documents/android-jni and the details
of the sample JNI code sort of cover this. It's not super friendly doc, I had to
ask some questions of android devs, I plan on improving it some, but details are
there when you dig.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
On 2015/05/06 01:57:06, armansito wrote:
> I don't know much about GN, so maybe you should add someone with more GN
> knowledge to review these bits.

Done.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
File device/bluetooth/android/bluetooth_jni_registrar.h (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
device/bluetooth/android/bluetooth_jni_registrar.h:15: DEVICE_BLUETOOTH_EXPORT
bool RegisterBluetoothJni(JNIEnv* env);
On 2015/05/06 01:57:06, armansito wrote:
> Please document what this function does.

Done.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/BluetoothAdapter.java
(right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/BluetoothAdapter.java:18:
@JNINamespace("device")
On 2015/05/06 01:57:06, armansito wrote:
> It would be good to have a Java reviewer to take a look at these to make sure
> that the coding style is correct.

Done.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_android.h (right):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:24: static
base::WeakPtr<BluetoothAdapterAndroid> CreateAdapter();
On 2015/05/06 01:57:06, armansito wrote:
> Shouldn't this be <BluetoothAdapter>?

No, changed to BluetoothAdapterAndroid so that Android unit tests can
instantiate and test the Android implementation. The
BluetoothAdapter::CreateAdapter method returns a BluetoothAdapter (by calling
this method).

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:29: // True if this app has android
permissions necessary for bluetooth.
On 2015/05/06 01:57:06, armansito wrote:
> s/bluetooth/Bluetooth/

Done.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:30: bool has_bluetooth_permission()
{ return has_bluetooth_permission_; }
On 2015/05/06 01:57:06, armansito wrote:
> nit: make this a const method.

Done.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android.h:88: bool has_bluetooth_permission_
= false;
On 2015/05/06 01:57:06, armansito wrote:
> Is this allowed in Chromium (or C++ for that matter)? Otherwise initialize in
> the constructor.

Yes, https://chromium-cpp.appspot.com/ "Non-Static Class Member Initializers".
Though, I've removed it from this patch.

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_android_unittest.cc (left):

https://codereview.chromium.org/1119113002/diff/70001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_android_unittest.cc:21:
EXPECT_TRUE(adapter_.get());
On 2015/05/06 01:57:06, armansito wrote:
> Add ASSERT_TRUE(adapter_.get()); or even better ASSERT_NE(adapter_.get(),
NULL);

Done. (_NE .. NULL is tricky with types, gets verbose, so I stuck with just
_TRUE)

[scheib, 2015-05-06 16:34:26]

Patchset #6 (id:170001) has been deleted

[scheib, 2015-05-06 16:34:31]

Patchset #5 (id:150001) has been deleted

[scheib, 2015-05-06 16:34:37]

Patchset #4 (id:130001) has been deleted

[scheib, 2015-05-06 16:35:03]

Patchset #3 (id:110001) has been deleted

[scheib, 2015-05-06 16:35:51]

Correction on the vibration patch with related, that's
https://codereview.chromium.org/583663003

[scheib, 2015-05-06 16:42:39]

scheib@chromium.org changed reviewers:
+ cjhopman@chromium.org

[scheib, 2015-05-06 16:42:39]

cjhopman@, PTAL as someone more familar with JNI/GN setup, armansito@'s
requested someone with more familiarity there. This change is mostly patterned
after device/vibration, JNI landed there in
https://codereview.chromium.org/583663003.

[cjhopman, 2015-05-06 16:56:14]

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
It looks like you need this because you have the jni stuff generated from the
java BluetoothAdapter being implemented/registered in
bluetooth_adapter_android.cc. I would maybe add something like a
java_bluetooth_adapter.h/.cc (or maybe bluetooth_adapter_jni.h/.cc) in android/
that did that and then you wouldn't need to allow circular includes.

[timvolodine, 2015-05-06 17:27:10]

I am not an owner here, but looks lgtm to me modulo 'circular include'.

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
On 2015/05/06 16:56:14, cjhopman wrote:
> It looks like you need this because you have the jni stuff generated from the
> java BluetoothAdapter being implemented/registered in
> bluetooth_adapter_android.cc. I would maybe add something like a
> java_bluetooth_adapter.h/.cc (or maybe bluetooth_adapter_jni.h/.cc) in
android/
> that did that and then you wouldn't need to allow circular includes.

+1 if you can avoid 'allow_circular_includes'. I don't remember needing that in
the vibration patch.

[cjhopman, 2015-05-06 17:51:21]

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
On 2015/05/06 17:27:10, timvolodine wrote:
> On 2015/05/06 16:56:14, cjhopman wrote:
> > It looks like you need this because you have the jni stuff generated from
the
> > java BluetoothAdapter being implemented/registered in
> > bluetooth_adapter_android.cc. I would maybe add something like a
> > java_bluetooth_adapter.h/.cc (or maybe bluetooth_adapter_jni.h/.cc) in
> android/
> > that did that and then you wouldn't need to allow circular includes.
> 
> +1 if you can avoid 'allow_circular_includes'. I don't remember needing that
in
> the vibration patch.

It does look like the vibration change introduces an include from
device/vibration/android to device/vibration. That violates the dependency
relationship, but there's no existing include from device/vibration to
device/vibration/android and so there's no circular includes.

That include without a corresponding dep caused build failures and required this
change: https://codereview.chromium.org/826593003 (which doesn't actually add
the real dep because that would cause a dependency cycle).

[scheib, 2015-05-06 17:59:34]

Patchset #3 (id:190001) has been deleted

[scheib, 2015-05-06 19:14:08]

Thanks, PTAL with merged GN targets. cjhopman, also please take a look at the
.java file.

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1119113002/diff/90001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:131: allow_circular_includes_from = [
"//device/bluetooth/android" ]
On 2015/05/06 17:51:20, cjhopman wrote:
> On 2015/05/06 17:27:10, timvolodine wrote:
> > On 2015/05/06 16:56:14, cjhopman wrote:
> > > It looks like you need this because you have the jni stuff generated from
> the
> > > java BluetoothAdapter being implemented/registered in
> > > bluetooth_adapter_android.cc. I would maybe add something like a
> > > java_bluetooth_adapter.h/.cc (or maybe bluetooth_adapter_jni.h/.cc) in
> > android/
> > > that did that and then you wouldn't need to allow circular includes.
> > 
> > +1 if you can avoid 'allow_circular_includes'. I don't remember needing that
> in
> > the vibration patch.
> 
> It does look like the vibration change introduces an include from
> device/vibration/android to device/vibration. That violates the dependency
> relationship, but there's no existing include from device/vibration to
> device/vibration/android and so there's no circular includes.
> 
> That include without a corresponding dep caused build failures and required
this
> change: https://codereview.chromium.org/826593003 (which doesn't actually add
> the real dep because that would cause a dependency cycle).

I've merged the GN files, with just one target this is no longer an issue.

[cjhopman, 2015-05-06 20:29:02]

lgtm

[armansito, 2015-05-06 22:00:39]

lgtm with one nit. Thanks!

https://codereview.chromium.org/1119113002/diff/210001/device/bluetooth/andro...
File device/bluetooth/android/bluetooth_jni_registrar.h (right):

https://codereview.chromium.org/1119113002/diff/210001/device/bluetooth/andro...
device/bluetooth/android/bluetooth_jni_registrar.h:16: // See
https://www.chromium.org/developers/design-documents/android-jni
nit: I would just briefly add something along the lines of "This function must
be called early on in an application that wishes to use this library". I'm
approaching this with the notion that device_bluetooth is a stand-alone library
here.

[scheib, 2015-05-06 22:57:11]

Thanks

https://codereview.chromium.org/1119113002/diff/210001/device/bluetooth/andro...
File device/bluetooth/android/bluetooth_jni_registrar.h (right):

https://codereview.chromium.org/1119113002/diff/210001/device/bluetooth/andro...
device/bluetooth/android/bluetooth_jni_registrar.h:16: // See
https://www.chromium.org/developers/design-documents/android-jni
On 2015/05/06 22:00:39, armansito wrote:
> nit: I would just briefly add something along the lines of "This function must
> be called early on in an application that wishes to use this library". I'm
> approaching this with the notion that device_bluetooth is a stand-alone
library
> here.

Done.

[scheib, 2015-05-06 22:57:35]

The CQ bit was checked by scheib@chromium.org

[scheib, 2015-05-06 22:57:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
timvolodine@chromium.org, cjhopman@chromium.org, armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1119113002/#ps230001
(title: "Additional comment on RegisterBluetoothJni")

[commit-bot: I haz the power, 2015-05-06 22:58:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119113002/230001

[commit-bot: I haz the power, 2015-05-07 01:59:34]

Committed patchset #4 (id:230001)

[commit-bot: I haz the power, 2015-05-07 02:00:31]

Patchset 4 (id:??) landed as
https://crrev.com/1f48f15dda8892c7d57e8155a011d9b4395299ba
Cr-Commit-Position: refs/heads/master@{#328678}