bluetooth: android: Implement StartNotifySession.

bluetooth: android: Implement StartNotifySession.

Basic implementation only starts session. They can not yet be stopped
and the characteristic updates are not propagated yet.

BUG=551634
Committed: https://crrev.com/f184bd5c60373345d36545af55bb05ad493945c1
Cr-Commit-Position: refs/heads/master@{#358259}

[scheib, 2015-11-06 01:57:47]

Patchset #2 (id:20001) has been deleted

[scheib, 2015-11-06 01:58:01]

Patchset #1 (id:1) has been deleted

[scheib, 2015-11-06 01:59:06]

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

[ortuno, 2015-11-06 02:30:06]

A couple of comments otherwise lgtm

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:127: if
(Java_ChromeBluetoothRemoteGattCharacteristic_startNotifySession(
Should we check the characteristic's properties before trying to subscribe? That
would allow us to return a better error message. You can add a TODO if so.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:132: FROM_HERE,
base::Bind(callback, base::Passed(&notify_session)));
nit: I don't think you need the '&'. Passed takes an object as well:

https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:133: } else {
nit: you can save yourself some indentation if you just return here instead of
using the else.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/test/a...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/test/a...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:515:
// Binds to BluetoothAdapterAndroid::OnFakeBluetoothGattReadCharacteristic.
Update the comment :)

[scheib, 2015-11-06 02:53:50]

Patchset #2 (id:60001) has been deleted

[scheib, 2015-11-06 02:55:41]

Patchset #2 (id:80001) has been deleted

[scheib, 2015-11-06 02:56:05]

Patchset #2 (id:100001) has been deleted

[scheib, 2015-11-06 02:56:57]

Thanks.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:127: if
(Java_ChromeBluetoothRemoteGattCharacteristic_startNotifySession(
On 2015/11/06 02:30:06, ortuno wrote:
> Should we check the characteristic's properties before trying to subscribe?
That
> would allow us to return a better error message. You can add a TODO if so.

Done.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:132: FROM_HERE,
base::Bind(callback, base::Passed(&notify_session)));
On 2015/11/06 02:30:06, ortuno wrote:
> nit: I don't think you need the '&'. Passed takes an object as well:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

Options are:
base::Passed(&thing)
base::Passed(thing.Pass())
and I believe the former is preferred because it's a tiny bit shorter and has
fewer words. See comments at the link you provided.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:133: } else {
On 2015/11/06 02:30:06, ortuno wrote:
> nit: you can save yourself some indentation if you just return here instead of
> using the else.

/shrug/ I sort of prefer the if __ else __ symmetry for small blocks. The
indentation doesn't save us anything here.

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/test/a...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1422463012/diff/40001/device/bluetooth/test/a...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:515:
// Binds to BluetoothAdapterAndroid::OnFakeBluetoothGattReadCharacteristic.
On 2015/11/06 02:30:06, ortuno wrote:
> Update the comment :)

Done.

[scheib, 2015-11-06 02:57:02]

The CQ bit was checked by scheib@chromium.org

[scheib, 2015-11-06 02:57:03]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/1422463012/#ps120001
(title: "addressed ortuno")

[commit-bot: I haz the power, 2015-11-06 02:58:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422463012/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422463012/120001

[commit-bot: I haz the power, 2015-11-06 05:00:15]

Committed patchset #2 (id:120001)

[commit-bot: I haz the power, 2015-11-06 05:01:07]

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

[perezju, 2015-11-06 11:11:23]

A revert of this CL (patchset #2 id:120001) has been created in
https://codereview.chromium.org/1413383008/ by perezju@chromium.org.

The reason for reverting is: Broke downstream tot builder. See bug for details..