bluetooth: Test & make StartNotifySession reentrant.

bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369
Committed: https://crrev.com/fc2e764e46a19ec3ef3fce0deffb261424cd69f3
Cr-Commit-Position: refs/heads/master@{#383285}

[scheib, 2016-03-11 04:50:54]

Description was changed from

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Adds unit test for StartNotifySession failure condition.
o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.

BUG=584369
==========

to

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369
==========

[scheib, 2016-03-11 04:50:54]

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

[ortuno, 2016-03-13 22:45:04]

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:319:
DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
nit: This should follow the same pattern as other TODOs in file:

TODO(crbug.com/576906)

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:341:
DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
Same here. TODO should follow same pattern as other TODOs.

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:915: // Tests
multiple StartNotifySession success.
Wrong test description.

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

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:140: void
BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
We need an issue to track IsNotifying(). Otherwise we will write to the
descriptor every time we call StartNotifySession.

[scheib, 2016-03-14 20:00:16]

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:319:
DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
On 2016/03/13 22:45:03, ortuno wrote:
> nit: This should follow the same pattern as other TODOs in file:
> 
> TODO(crbug.com/576906)

Done, by fixing the other TODOs to the shorter form. Keeping more descriptive
text on a one line is better than redundant crbug.com.

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:341:
DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
On 2016/03/13 22:45:03, ortuno wrote:
> Same here. TODO should follow same pattern as other TODOs.

Done.

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:915: // Tests
multiple StartNotifySession success.
On 2016/03/13 22:45:03, ortuno wrote:
> Wrong test description.

Done.

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

https://codereview.chromium.org/1779083002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:140: void
BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
On 2016/03/13 22:45:03, ortuno wrote:
> We need an issue to track IsNotifying(). Otherwise we will write to the
> descriptor every time we call StartNotifySession.

Done. https://bugs.chromium.org/p/chromium/issues/detail?id=594665

[scheib, 2016-03-25 00:14:25]

ortuno PTAL

[ortuno, 2016-03-25 02:07:31]

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:922: // Tests
multiple StartNotifySession success.
Wrong test description.

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:970: ASSERT_EQ(0u,
notify_sessions_.size());
Is there a specific error we should expect?

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1011: empty_vector);
I think we should add some observers to the adapter and expect no characteristic
value changed events.

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

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:53:
OnStartNotifySessionError(device::BluetoothGattService::GATT_ERROR_FAILED);
Talked offline. This can happen when we call startNotifications and the device
disconnects. So we agreed that it's a good pattern that we should follow for the
rest of the GATT operations.

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:158: 
remove new line.

[scheib, 2016-03-25 02:34:43]

Patchset #6 (id:100001) has been deleted

[scheib, 2016-03-25 02:35:01]

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:922: // Tests
multiple StartNotifySession success.
On 2016/03/25 02:07:31, ortuno wrote:
> Wrong test description.

Done.

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:970: ASSERT_EQ(0u,
notify_sessions_.size());
On 2016/03/25 02:07:31, ortuno wrote:
> Is there a specific error we should expect?

Done.

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1011: empty_vector);
On 2016/03/25 02:07:31, ortuno wrote:
> I think we should add some observers to the adapter and expect no
characteristic
> value changed events.

Done.

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

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:53:
OnStartNotifySessionError(device::BluetoothGattService::GATT_ERROR_FAILED);
On 2016/03/25 02:07:31, ortuno wrote:
> Talked offline. This can happen when we call startNotifications and the device
> disconnects. So we agreed that it's a good pattern that we should follow for
the
> rest of the GATT operations.

Acknowledged.

https://codereview.chromium.org/1779083002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:158: 
On 2016/03/25 02:07:31, ortuno wrote:
> remove new line.

Done.

[ortuno, 2016-03-25 02:36:15]

lgtm

[scheib, 2016-03-25 02:50:13]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-25 02:50:27]

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

[commit-bot: I haz the power, 2016-03-25 04:22:14]

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

[commit-bot: I haz the power, 2016-03-25 04:22:15]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scheib, 2016-03-25 15:21:38]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-25 15:21:54]

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

[commit-bot: I haz the power, 2016-03-25 16:25:16]

Description was changed from

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369
==========

to

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369
==========

[commit-bot: I haz the power, 2016-03-25 16:25:17]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-03-25 16:26:44]

Description was changed from

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369
==========

to

==========
bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369

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

[commit-bot: I haz the power, 2016-03-25 16:26:45]

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