Implement //device/bt changes for notifications.

Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
Committed: https://crrev.com/d1e6dc42512184286e5275eb05a4fb7c8f062aa4
Cr-Commit-Position: refs/heads/master@{#393388}

[rkc, 2016-05-12 04:10:25]

Description was changed from

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
==========

to

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
==========

[scheib, 2016-05-12 16:53:19]

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

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_local_gatt_characteristic_unittest.cc:119:
TEST_F(BluetoothLocalGattCharacteristicTest, StartAndStopNotifications) {
Test Stop as well?

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test.h:265: // Simulates sending a value updated
notification to a remote device. Returns
I think tests should call characteristic_->NotifyValueChanged(), and have test
framework expose a way to check the result. Similar to
TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic).

Then also test combinations of NotifyValueChanged with various flags including
notify/indicate.

[xiyuan, 2016-05-12 19:49:43]

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_local_gatt_characteristic_bluez.cc
(right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_local_gatt_characteristic_bluez.cc:7: #include
<cstdint>
nit: Do we still need a header when it is already included?

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
File device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc
(right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc:110: return
notifications_started_for_characteristic_[characteristic
nit: return found.second; since we have |found|?

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
File device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.h:23:
~TestBluetoothLocalGattServiceDelegate();
nit: make it virtual? Or add a dtor to Delegate interface and override?

[rkc, 2016-05-12 20:12:50]

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

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_local_gatt_characteristic_unittest.cc:119:
TEST_F(BluetoothLocalGattCharacteristicTest, StartAndStopNotifications) {
On 2016/05/12 16:53:19, scheib wrote:
> Test Stop as well?

Done.

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_local_gatt_characteristic_bluez.cc
(right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_local_gatt_characteristic_bluez.cc:7: #include
<cstdint>
On 2016/05/12 19:49:43, xiyuan wrote:
> nit: Do we still need a header when it is already included?

Not sure why eclipse's IWYU tool keeps putting it back.
Removed.

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test.h:265: // Simulates sending a value updated
notification to a remote device. Returns
On 2016/05/12 16:53:19, scheib wrote:
> I think tests should call characteristic_->NotifyValueChanged(), and have test
> framework expose a way to check the result. Similar to
> TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic).
> 
> Then also test combinations of NotifyValueChanged with various flags including
> notify/indicate. 

The BlueZ implementation does check the result. I've updated the comment to
indicate that successful completion of this call includes verification of the
notification being sent.

Since we don't have a delegate for this, having the test get back the actual
value set, will require adding platform specific code. Instead, having this
simulate method do the verification allows us to keep the platform specific code
that actually checks the final value to exist in the bluetooth_test_bluez code.


Added the tests to check for error conditions, including incorrect properties
set.

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
File device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc
(right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.cc:110: return
notifications_started_for_characteristic_[characteristic
On 2016/05/12 19:49:43, xiyuan wrote:
> nit: return found.second; since we have |found|?

Done.

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
File device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/test_...
device/bluetooth/test/test_bluetooth_local_gatt_service_delegate.h:23:
~TestBluetoothLocalGattServiceDelegate();
On 2016/05/12 19:49:43, xiyuan wrote:
> nit: make it virtual? Or add a dtor to Delegate interface and override?

Done.

[scheib, 2016-05-12 20:38:28]

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test.h:265: // Simulates sending a value updated
notification to a remote device. Returns
On 2016/05/12 20:12:49, Rahul Chaturvedi wrote:
> On 2016/05/12 16:53:19, scheib wrote:
> > I think tests should call characteristic_->NotifyValueChanged(), and have
test
> > framework expose a way to check the result. Similar to
> > TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic).
> > 
> > Then also test combinations of NotifyValueChanged with various flags
including
> > notify/indicate. 
> 
> The BlueZ implementation does check the result. I've updated the comment to
> indicate that successful completion of this call includes verification of the
> notification being sent.
> 
> Since we don't have a delegate for this, having the test get back the actual
> value set, will require adding platform specific code. Instead, having this
> simulate method do the verification allows us to keep the platform specific
code
> that actually checks the final value to exist in the bluetooth_test_bluez
code.
> 
> 
> Added the tests to check for error conditions, including incorrect properties
> set.

I'm not feeling good about this approach. The method under test is
characteristic_->NotifyValueChanged(), so I expect to see it in the test code.
Platform test fixture code is needed to verify that the effect of
NotifyValueChanged made it to the lowest level, yes, but that's also true for
e.g. the Read and Write commands. Please follow a test fixture pattern similar
to those.

[rkc, 2016-05-12 21:19:18]

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1973703002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test.h:265: // Simulates sending a value updated
notification to a remote device. Returns
On 2016/05/12 20:38:27, scheib wrote:
> On 2016/05/12 20:12:49, Rahul Chaturvedi wrote:
> > On 2016/05/12 16:53:19, scheib wrote:
> > > I think tests should call characteristic_->NotifyValueChanged(), and have
> test
> > > framework expose a way to check the result. Similar to
> > > TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic).
> > > 
> > > Then also test combinations of NotifyValueChanged with various flags
> including
> > > notify/indicate. 
> > 
> > The BlueZ implementation does check the result. I've updated the comment to
> > indicate that successful completion of this call includes verification of
the
> > notification being sent.
> > 
> > Since we don't have a delegate for this, having the test get back the actual
> > value set, will require adding platform specific code. Instead, having this
> > simulate method do the verification allows us to keep the platform specific
> code
> > that actually checks the final value to exist in the bluetooth_test_bluez
> code.
> > 
> > 
> > Added the tests to check for error conditions, including incorrect
properties
> > set.
> 
> I'm not feeling good about this approach. The method under test is
> characteristic_->NotifyValueChanged(), so I expect to see it in the test code.
> Platform test fixture code is needed to verify that the effect of
> NotifyValueChanged made it to the lowest level, yes, but that's also true for
> e.g. the Read and Write commands. Please follow a test fixture pattern similar
> to those.

Actually we don't really need to simulate a notification update, since that is
an action we take in the test. We just need to check the result of our action.
Changed the code accordingly. So now we call NotifyValueChanged directly, then
just check the last notification value with the fake characteristic provider.

Done.

[scheib, 2016-05-12 21:26:59]

Cross platform device/bluetooth files LGTM with one fix:

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

https://codereview.chromium.org/1973703002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_unittest.cc:152:
BluetoothLocalGattCharacteristic::NOTIFY_PROPERTY_NOT_SET,
I think these top two belong in SendNotificationsWrongProperties below.

[rkc, 2016-05-12 21:42:19]

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

https://codereview.chromium.org/1973703002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_unittest.cc:152:
BluetoothLocalGattCharacteristic::NOTIFY_PROPERTY_NOT_SET,
On 2016/05/12 21:26:59, scheib wrote:
> I think these top two belong in SendNotificationsWrongProperties below.

Done.

[rkc, 2016-05-12 21:42:46]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 21:43:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973703002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973703002/60001

[xiyuan, 2016-05-12 22:35:45]

lgtm

[rkc, 2016-05-12 22:45:22]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2016-05-12 22:45:36]

The CQ bit was checked by rkc@chromium.org

[rkc, 2016-05-12 22:45:36]

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/1973703002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-12 22:46:09]

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

[commit-bot: I haz the power, 2016-05-12 23:12:57]

Description was changed from

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
==========

to

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
==========

[commit-bot: I haz the power, 2016-05-12 23:12:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-12 23:14:29]

Description was changed from

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393
==========

to

==========
Implement //device/bt changes for notifications.

This CL implements the changes to the device::Bluetooth* classes and the
changes to the BlueZ implementations of them for supporting notifications.

Vincent, since this changes device::Bluetooth code and adds tests, an
OWNERs review is needed.

This is part 2 of of 2 CLs
https://codereview.chromium.org/1974633002
https://codereview.chromium.org/1973703002 <<<

R=scheib@chromium.org, xiyuan@chromium.org
BUG=610393

Committed: https://crrev.com/d1e6dc42512184286e5275eb05a4fb7c8f062aa4
Cr-Commit-Position: refs/heads/master@{#393388}
==========

[commit-bot: I haz the power, 2016-05-12 23:14:30]

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