bluetooth: android: Enable characteristic change notification events.

bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533
Committed: https://crrev.com/988f37c5bc72dfbd524fcb7f9f47b6339a810b3a
Cr-Commit-Position: refs/heads/master@{#369559}

[scheib, 2016-01-13 00:56:53]

Patchset #1 (id:1) has been deleted

[scheib, 2016-01-13 00:57:05]

Patchset #4 (id:80001) has been deleted

[scheib, 2016-01-13 00:57:21]

Patchset #1 (id:20001) has been deleted

[scheib, 2016-01-13 00:57:33]

Patchset #1 (id:40001) has been deleted

[scheib, 2016-01-13 00:59:41]

Patchset #1 (id:60001) has been deleted

[scheib, 2016-01-13 01:00:42]

Description was changed from

==========
bluetooth: android: Enable characteristic change notification events.

PATCH WORK IN PROGRESS --- FILL IN DESCRIPTION BEFORE REVIEW.

BUG=551634, 566533
==========

to

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling notifications, and route
changed values via onCharacteristicChanged.

BUG=551634, 566533
==========

[scheib, 2016-01-13 01:00:52]

Description was changed from

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling notifications, and route
changed values via onCharacteristicChanged.

BUG=551634, 566533
==========

to

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533
==========

[scheib, 2016-01-13 02:41:55]

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

[Jeffrey Yasskin, 2016-01-14 00:13:43]

LGTM after the below fixes.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:78:
&expected_config_descriptor_value,
You mean to pull the bytes out of here, right? Instead, this truncates
expected_config_descriptor_value to uint8_t, and then reads the non-existent
uint16_t after it in memory, and casts that value to uint8_t. To avoid depending
on the endian-ness of Chrome's platform, you should use bitmasks and shifts to
pull the bytes out of expected_config_descriptor_value.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:713:
TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_SynchronousError) {
Is "SynchronousError" the most descriptive name for this kind of failure? I
believe it's failing in the call to setCharacteristicNotification(), so it
should be "FailToSetCharacteristicNotification" or similar.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:717:
SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
If you remove this line, does the test fail? I suspect you need to add the
descriptor in order to get to the attempt to call
setCharacteristicNotification().

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:768: /* properties:
INDICATE */ 0x20,
Please also check when both NOTIFY and INDICATE properties are set. It doesn't
matter which is written, but exactly 1 should be. (Testing that exactly NOTIFY
is written is fine.)

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:810:
SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr,
Please comment that the test here is that nothing crashes.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:232: //
TODO(http://crbug.com/545682): Call GattCharacteristicValueChanged.
https, please.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/test/...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/test/...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:461:
if (chromeCharacteristic == null && sRememberedCharacteristic == null)
Put braces around the 2-line block.

[scheib, 2016-01-14 01:12:27]

Thanks - double check bits because, apparently I need checking on. ;)

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:78:
&expected_config_descriptor_value,
On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> You mean to pull the bytes out of here, right? Instead, this truncates
> expected_config_descriptor_value to uint8_t, and then reads the non-existent
> uint16_t after it in memory, and casts that value to uint8_t. To avoid
depending
> on the endian-ness of Chrome's platform, you should use bitmasks and shifts to
> pull the bytes out of expected_config_descriptor_value.

Thanks - double check this version.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:713:
TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_SynchronousError) {
On 2016/01/14 00:13:43, Jeffrey Yasskin wrote:
> Is "SynchronousError" the most descriptive name for this kind of failure? I
> believe it's failing in the call to setCharacteristicNotification(), so it
> should be "FailToSetCharacteristicNotification" or similar.

Done. Naming was intentionally abstracting platform details - but I've used your
example and commented directly what this translates to for Android. There isn't
an equivalent for Bluez or CoreBluetooth, (or I think Windows, but I'm still
unsure which API surface we'd use there) so this is likely to remain Android
only indefinitely.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:717:
SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> If you remove this line, does the test fail? I suspect you need to add the
> descriptor in order to get to the attempt to call
> setCharacteristicNotification().

It needed the descriptor. Code coverage here is brittle ;(, I'd reorganized for
better error messages. I don't think worth adding more testing instrumentation
for from every block - unsure of a better way.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:768: /* properties:
INDICATE */ 0x20,
On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> Please also check when both NOTIFY and INDICATE properties are set. It doesn't
> matter which is written, but exactly 1 should be. (Testing that exactly NOTIFY
> is written is fine.)

Done.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:810:
SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr,
On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> Please comment that the test here is that nothing crashes.

Done.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:232: //
TODO(http://crbug.com/545682): Call GattCharacteristicValueChanged.
On 2016/01/14 00:13:43, Jeffrey Yasskin wrote:
> https, please.

Done.

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/test/...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/test/...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:461:
if (chromeCharacteristic == null && sRememberedCharacteristic == null)
On 2016/01/14 00:13:43, Jeffrey Yasskin wrote:
> Put braces around the 2-line block.

Done.

[scheib, 2016-01-14 16:53:39]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-01-14 16:53:40]

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

[commit-bot: I haz the power, 2016-01-14 16:54:14]

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

[commit-bot: I haz the power, 2016-01-14 17:09:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 17:09:06]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Jeffrey Yasskin, 2016-01-14 18:43:32]

lgtm

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:717:
SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
On 2016/01/14 01:12:26, scheib wrote:
> On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> > If you remove this line, does the test fail? I suspect you need to add the
> > descriptor in order to get to the attempt to call
> > setCharacteristicNotification().
> 
> It needed the descriptor. Code coverage here is brittle ;(, I'd reorganized
for
> better error messages. I don't think worth adding more testing instrumentation
> for from every block - unsure of a better way.

One way to do it would be to have StartNotifyBoilerplate() take an integer
parameter saying which step to skip (or which failure to force), and then assert
that passing any of those integers causes a failure. Using
StartNotifyBoilerplace means that you also have a built-in test that you're
skipping a step in an otherwise-complete sequence. That said, we don't usually
test this way, so I don't insist on it.

https://codereview.chromium.org/1502833002/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1502833002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:80:
EXPECT_EQ(expected_byte0, last_write_value_[0]);
Looks good. With gmock included, you could also write:
EXPECT_THAT(last_write_value_, ElementsAre(expected_byte0, expected_byte1));

[scheib, 2016-01-14 19:20:57]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-01-14 19:20:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1502833002/#ps180001
(title: "fix API error and getDescriptor NULL error")

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

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

[commit-bot: I haz the power, 2016-01-14 19:44:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 19:44:06]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[scheib, 2016-01-14 20:39:39]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-01-14 20:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1502833002/#ps200001
(title: "fix imports")

[commit-bot: I haz the power, 2016-01-14 20:41:41]

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

[scheib, 2016-01-14 21:02:14]

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1502833002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:717:
SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
On 2016/01/14 18:43:31, Jeffrey Yasskin wrote:
> On 2016/01/14 01:12:26, scheib wrote:
> > On 2016/01/14 00:13:42, Jeffrey Yasskin wrote:
> > > If you remove this line, does the test fail? I suspect you need to add the
> > > descriptor in order to get to the attempt to call
> > > setCharacteristicNotification().
> > 
> > It needed the descriptor. Code coverage here is brittle ;(, I'd reorganized
> for
> > better error messages. I don't think worth adding more testing
instrumentation
> > for from every block - unsure of a better way.
> 
> One way to do it would be to have StartNotifyBoilerplate() take an integer
> parameter saying which step to skip (or which failure to force), and then
assert
> that passing any of those integers causes a failure. Using
> StartNotifyBoilerplace means that you also have a built-in test that you're
> skipping a step in an otherwise-complete sequence. That said, we don't usually
> test this way, so I don't insist on it.

Nice idea - I'll refactor to use it in a follow up.

[commit-bot: I haz the power, 2016-01-14 21:51:55]

Description was changed from

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533
==========

to

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533
==========

[commit-bot: I haz the power, 2016-01-14 21:51:56]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-01-14 22:03:51]

Description was changed from

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533
==========

to

==========
bluetooth: android: Enable characteristic change notification events.

Set Notify or Indicate on remote devices when enabling
notifications, and route changed values via
onCharacteristicChanged.

BUG=551634, 566533

Committed: https://crrev.com/988f37c5bc72dfbd524fcb7f9f47b6339a810b3a
Cr-Commit-Position: refs/heads/master@{#369559}
==========

[commit-bot: I haz the power, 2016-01-14 22:03:52]

Patchset 6 (id:??) landed as
https://crrev.com/988f37c5bc72dfbd524fcb7f9f47b6339a810b3a
Cr-Commit-Position: refs/heads/master@{#369559}