Pass JavaRef to Java methods in device/bluetooth.

Pass JavaRef to Java methods in device/bluetooth.

Update code in device/bluetooth to use JavaRef when calling Java methods
via JNI, instead of passing bare jobject. Various function parameter
types are converted from jobject to JavaRef to enable calls to obj()
higher up the call chain to be removed.

BUG=506850
Committed: https://crrev.com/772283d1079c1271b1dca288a1ae350eae50a719
Cr-Commit-Position: refs/heads/master@{#417283}

[Torne, 2016-09-07 16:50:49]

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

[Torne, 2016-09-07 16:50:50]

https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_service_android.cc (right):

https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_service_android.cc:28: jobject>& /*
BluetoothGattServiceWrapper */ bluetooth_gatt_service_wrapper,
This line and others like it in the change end up blowing the 80 char limit,
because clang-format isn't willing to split the comment apart from the type and
variable name. Are these comments actually useful? The parameters all seem to be
named the exact same thing as the type anyway - removing the comments would make
this code much easier to format sensibly..

[scheib, 2016-09-07 18:00:30]

LGTM -- it would be nice to have the 'why' explained or linked from the change
description, though. Did I code this wrong in the first place? memory leak
concerns? Performance? Changing style guide?

https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_service_android.cc (right):

https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_service_android.cc:28: jobject>& /*
BluetoothGattServiceWrapper */ bluetooth_gatt_service_wrapper,
On 2016/09/07 16:50:50, Torne wrote:
> This line and others like it in the change end up blowing the 80 char limit,
> because clang-format isn't willing to split the comment apart from the type
and
> variable name. Are these comments actually useful? The parameters all seem to
be
> named the exact same thing as the type anyway - removing the comments would
make
> this code much easier to format sensibly..

I believe clang format will be happier if the comments change to // single lines
- it will respect the line breaks.

The comments were added early on at some reviewers requests to keep track of the
data types -- though I agree that the names match the data types, it's still
hard to know what java object the thing is supposed to be. jobject is rather
like passing around a lot of void*s, and to C++ programmers it feels odd.

[Torne, 2016-09-08 10:44:04]

On 2016/09/07 18:00:30, scheib wrote:
> LGTM -- it would be nice to have the 'why' explained or linked from the change
> description, though. Did I code this wrong in the first place? memory leak
> concerns? Performance? Changing style guide?

Ah, yeah. I've explained various bits of what I'm doing in various places, but
not as a single reference anywhere that's easy to link to, which is probably
something I should have done. My plan was to write up a doc describing things
after I'm done with this refactoring and publish it as a PSA, it probably could
also be linked from the style guide - I just haven't done it yet because some of
the changes I'm making (e.g. requiring Java function calls to use JavaRef)
haven't been finished yet - this is part of a larger series of refactorings that
has been dragging on for rather too long as I've had limited time to work on it
:/

The root goal here is to completely stop passing jobject around in the Chromium
code, and to *always* pass things wrapped in one of the JavaRef types. It's to
reduce the chance of people committing various reference lifetime errors (a
jobject doesn't convey anything about the lifetime of the reference, and can
become invalid while you are using it if someone else with the same jobject does
something bad). It's basically the same advice as "use
unique_ptr/ScopedRefPtr/etc instead of bare pointers", but for JNI, where there
are important differences between the lifetimes of local and global refs.

const JavaRef<>& is the equivalent of passing a const reference to a normal C++
object - the purpose is to allow the receiving function to assume "this java
object will be alive for the duration of this call, but if I want to keep a
reference to it, I need to make my own ScopedJavaLocalRef or ScopedJavaGlobalRef
to it".

>
https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
> File device/bluetooth/bluetooth_remote_gatt_service_android.cc (right):
> 
>
https://codereview.chromium.org/2313413002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_remote_gatt_service_android.cc:28: jobject>& /*
> BluetoothGattServiceWrapper */ bluetooth_gatt_service_wrapper,
> On 2016/09/07 16:50:50, Torne wrote:
> > This line and others like it in the change end up blowing the 80 char limit,
> > because clang-format isn't willing to split the comment apart from the type
> and
> > variable name. Are these comments actually useful? The parameters all seem
to
> be
> > named the exact same thing as the type anyway - removing the comments would
> make
> > this code much easier to format sensibly..
> 
> I believe clang format will be happier if the comments change to // single
lines
> - it will respect the line breaks.
> 
> The comments were added early on at some reviewers requests to keep track of
the
> data types -- though I agree that the names match the data types, it's still
> hard to know what java object the thing is supposed to be. jobject is rather
> like passing around a lot of void*s, and to C++ programmers it feels odd.

OK, I'll try to reformat it to // and see if it's happy.

[Torne, 2016-09-08 12:49:58]

OK, using // comments fixed clang-format not being able to deal with it. Thanks
:)

[Torne, 2016-09-08 12:50:02]

The CQ bit was checked by torne@chromium.org

[Torne, 2016-09-08 12:50:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2313413002/#ps20001
(title: "Switch comments to a different form that clang-format will fit in 80
chars")

[commit-bot: I haz the power, 2016-09-08 12:50:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 14:16:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 14:19:13]

Description was changed from

==========
Pass JavaRef to Java methods in device/bluetooth.

Update code in device/bluetooth to use JavaRef when calling Java methods
via JNI, instead of passing bare jobject. Various function parameter
types are converted from jobject to JavaRef to enable calls to obj()
higher up the call chain to be removed.

BUG=506850
==========

to

==========
Pass JavaRef to Java methods in device/bluetooth.

Update code in device/bluetooth to use JavaRef when calling Java methods
via JNI, instead of passing bare jobject. Various function parameter
types are converted from jobject to JavaRef to enable calls to obj()
higher up the call chain to be removed.

BUG=506850

Committed: https://crrev.com/772283d1079c1271b1dca288a1ae350eae50a719
Cr-Commit-Position: refs/heads/master@{#417283}
==========

[commit-bot: I haz the power, 2016-09-08 14:19:14]

Patchset 2 (id:??) landed as
https://crrev.com/772283d1079c1271b1dca288a1ae350eae50a719
Cr-Commit-Position: refs/heads/master@{#417283}