bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900
Committed: https://crrev.com/78ef0595f3bc4a7c97d49b1b2cd0ccc4f9efb67f
Cr-Commit-Position: refs/heads/master@{#383218}

[scheib, 2016-03-04 01:23:23]

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

[ortuno, 2016-03-04 18:55:21]

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

https://codereview.chromium.org/1765773002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:929:
TEST_F(BluetoothGattCharacteristicTest, StartNotifySessionError_Multiple) {
We should add a test for when the gatt objects get deleted before the success
callback.

[scheib, 2016-03-10 00:58:21]

Description was changed from

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

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 GetDescriptorForUUID.
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: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900
==========

[scheib, 2016-03-11 03:20:26]

I've split this patch. Here it is now only the
Refactor GetDescriptorForUUID to GetDescriptorsForUUID

and a new patch for the odds and ends:

bluetooth: Test & make StartNotifySession reentrant.
https://codereview.chromium.org/1779083002/

[scheib, 2016-03-11 04:44:59]

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

https://codereview.chromium.org/1765773002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:929:
TEST_F(BluetoothGattCharacteristicTest, StartNotifySessionError_Multiple) {
On 2016/03/04 18:55:21, ortuno wrote:
> We should add a test for when the gatt objects get deleted before the success
> callback.

Done in https://codereview.chromium.org/1779083002/

[ortuno, 2016-03-13 20:41:45]

lgtm bar some comments.

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java
(right):

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java:158:
String descriptorInstanceId = mInstanceId + "/" + instanceIdCounter++;
Hmm I wonder if we should include the UUID. I think it will make it easier to
identify the descriptors in logs, but then we can just include the UUID in the
log anyway. Up to you.

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.h:164:
std::vector<BluetoothGattDescriptor*> GetDescriptorsForUUID(
nit: Do you think GetDescriptorsByUUID or GetDescriptorsForUUID is better?

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

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:996: EXPECT_EQ(id3,
characteristic2_->GetDescriptorsForUUID(id3).at(1)->GetUUID());
Add
EXPECT_NE(characteristic2_->GetDescriptorsForUUID(id3).at(0)->GetIdentifier(),
             
characteristic2_->GetDescriptorsForUUID(id3).at(1)->GetIdentifier())

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

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:161: if
(ccc_descriptor.size() != 1u) {
Hmm this would fail with GATT NOT SUPPORTED if the characteristic has two ccc
descriptors. I'm OK with it failing with if there are 2 CCC descriptors but GATT
NOT SUPPORTED is misleading.

[scheib, 2016-03-25 00:13:57]

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java
(right):

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java:158:
String descriptorInstanceId = mInstanceId + "/" + instanceIdCounter++;
On 2016/03/13 20:41:44, ortuno wrote:
> Hmm I wonder if we should include the UUID. I think it will make it easier to
> identify the descriptors in logs, but then we can just include the UUID in the
> log anyway. Up to you.

Done: Including the UUID.

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.h:164:
std::vector<BluetoothGattDescriptor*> GetDescriptorsForUUID(
On 2016/03/13 20:41:44, ortuno wrote:
> nit: Do you think GetDescriptorsByUUID or GetDescriptorsForUUID is better?

Done: Renamed to GetDescriptorsByUUID.

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

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:996: EXPECT_EQ(id3,
characteristic2_->GetDescriptorsForUUID(id3).at(1)->GetUUID());
On 2016/03/13 20:41:44, ortuno wrote:
> Add
> EXPECT_NE(characteristic2_->GetDescriptorsForUUID(id3).at(0)->GetIdentifier(),
>              
> characteristic2_->GetDescriptorsForUUID(id3).at(1)->GetIdentifier())

Done.

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

https://codereview.chromium.org/1765773002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:161: if
(ccc_descriptor.size() != 1u) {
On 2016/03/13 20:41:45, ortuno wrote:
> Hmm this would fail with GATT NOT SUPPORTED if the characteristic has two ccc
> descriptors. I'm OK with it failing with if there are 2 CCC descriptors but
GATT
> NOT SUPPORTED is misleading.

Done: Different errors returned for <1 and >1. Test added for multiple CCCDs.

[scheib, 2016-03-25 01:10:28]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-03-25 01:10:29]

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/1765773002/#ps60001
(title: "addressed ortuno's comments")

[commit-bot: I haz the power, 2016-03-25 01:10:45]

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

[commit-bot: I haz the power, 2016-03-25 01:21:52]

Description was changed from

==========
bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900
==========

to

==========
bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900
==========

[commit-bot: I haz the power, 2016-03-25 01:21:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-25 01:23:06]

Description was changed from

==========
bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900
==========

to

==========
bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.

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

Support multiple descriptors with the same UUID.

BUG=584369, 576900

Committed: https://crrev.com/78ef0595f3bc4a7c97d49b1b2cd0ccc4f9efb67f
Cr-Commit-Position: refs/heads/master@{#383218}
==========

[commit-bot: I haz the power, 2016-03-25 01:23:07]

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