bluetooth: android: Initial basic Descriptors implementation.

bluetooth: android: Initial basic Descriptors implementation.

Initial basic implementation of BluetoothRemoteGattDescriptorAndroid and ChromeBluetoothRemoteGattDescriptor.

BUG=566533
Committed: https://crrev.com/81a0ff269b3e54580b15c52da806249435e4e206
Cr-Commit-Position: refs/heads/master@{#369059}

[scheib, 2016-01-10 05:12:06]

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

[scheib, 2016-01-10 05:13:15]

Patchset #1 (id:1) has been deleted

[scheib, 2016-01-12 19:45:51]

Patchset #2 (id:40001) has been deleted

[Jeffrey Yasskin, 2016-01-12 21:53:54]

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java:136:
private void ensureDescriptorsCreated() {
I'm not sure 'ensure' is the right word here. It looks like this unconditionally
creates the descriptors, instead of bailing out if they aren't already created.

Looking at the C++ that calls this, yeah, call this 'createDescriptors' or
similar, since the C++ is the part that bails out.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java
(right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:62:
Object bluetoothGattDescriptorWrapper, Object chromeDevice) {
Can you use ChromeBluetoothDevice here? I thought the JNI bug was only for
nested types.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:77:
// BluetoothAdapterDevice C++ methods declared for access from java:
Remove this section boundary.

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:738:
EXPECT_EQ(c1_uuid1, characteristic1_->GetDescriptor(c1_id1)->GetUUID());
Also check that the pointer is the same for multiple retrievals, I think.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_descriptor_unittest.cc (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:26: // 2 devices to
verify unique IDs across them.
s/verify unique IDs across them/verify that descriptors on them have distinct
IDs/?

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:61: // 6 descriptors
(same UUID), 1 on each characteristic
The Characteristic Presentation Format descriptor can appear multiple times, but
it looks like the Android API doesn't provide instance IDs for you to use to
distinguish them. :( Could you comment that having duplicate UUIDs be
indistinguishable is a limitation of the Android API?

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:256: jobject>&
/* chromeBluetoothDevice */ chrome_bluetooth_device) {
Should 'chrome' be capitalized too? It's a type name, right?

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:276: if
(!descriptors_.empty())
Are we going to need to do something else here to handle Android discovering new
descriptors within a characteristic?

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.h (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.h:85: // Creates a
Bluetooth GATT descriptor object and adds it to |descriptors_|
Add a comma before 'DCHECKing'.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:115: :
adapter_(adapter), instance_id_(instance_id) {}
I don't see any uses of adapter_. What's it for?

[scheib, 2016-01-12 23:16:56]

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattCharacteristic.java:136:
private void ensureDescriptorsCreated() {
On 2016/01/12 21:53:53, Jeffrey Yasskin wrote:
> I'm not sure 'ensure' is the right word here. It looks like this
unconditionally
> creates the descriptors, instead of bailing out if they aren't already
created.
> 
> Looking at the C++ that calls this, yeah, call this 'createDescriptors' or
> similar, since the C++ is the part that bails out.

Done.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java
(right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:62:
Object bluetoothGattDescriptorWrapper, Object chromeDevice) {
On 2016/01/12 21:53:53, Jeffrey Yasskin wrote:
> Can you use ChromeBluetoothDevice here? I thought the JNI bug was only for
> nested types.

Done - Yep, fixed that and the name as well.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:77:
// BluetoothAdapterDevice C++ methods declared for access from java:
On 2016/01/12 21:53:53, Jeffrey Yasskin wrote:
> Remove this section boundary.

Done.

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:738:
EXPECT_EQ(c1_uuid1, characteristic1_->GetDescriptor(c1_id1)->GetUUID());
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> Also check that the pointer is the same for multiple retrievals, I think.

Done.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_descriptor_unittest.cc (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:26: // 2 devices to
verify unique IDs across them.
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> s/verify unique IDs across them/verify that descriptors on them have distinct
> IDs/?

Done.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:61: // 6 descriptors
(same UUID), 1 on each characteristic
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> The Characteristic Presentation Format descriptor can appear multiple times,
but
> it looks like the Android API doesn't provide instance IDs for you to use to
> distinguish them. :( Could you comment that having duplicate UUIDs be
> indistinguishable is a limitation of the Android API?

TODOs added to code. Though Android doesn't provide an instance ID - we can
handle this because getDescriptors should return the unique descriptors with
duplicate UUIDs, and we can assign our own instance IDs. I'll leave that for
later, though.

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

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:256: jobject>&
/* chromeBluetoothDevice */ chrome_bluetooth_device) {
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> Should 'chrome' be capitalized too? It's a type name, right?

Done.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:276: if
(!descriptors_.empty())
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> Are we going to need to do something else here to handle Android discovering
new
> descriptors within a characteristic?

Yes, created issue http://crbug.com/576906 for this and added TODO in
ChromeBluetoothDevice

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.h (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.h:85: // Creates a
Bluetooth GATT descriptor object and adds it to |descriptors_|
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> Add a comma before 'DCHECKing'.

Done.

[Jeffrey Yasskin, 2016-01-12 23:25:59]

LGTM after a couple tweaks.

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:115: :
adapter_(adapter), instance_id_(instance_id) {}
On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> I don't see any uses of adapter_. What's it for?

Ping.

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

https://codereview.chromium.org/1574773002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:744:
EXPECT_EQ(characteristic1_->GetDescriptor(c1_id1)->GetUUID(),
Remove "->GetUUID()".

Could you also check that it's the same as what GetDescriptors()[0] returns?

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

https://codereview.chromium.org/1574773002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:142:
EXPECT_EQ(service->GetCharacteristic(char_id1)->GetUUID(),
Remove "->GetUUID()".

[scheib, 2016-01-12 23:54:46]

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc (right):

https://codereview.chromium.org/1574773002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:115: :
adapter_(adapter), instance_id_(instance_id) {}
On 2016/01/12 23:25:59, Jeffrey Yasskin wrote:
> On 2016/01/12 21:53:54, Jeffrey Yasskin wrote:
> > I don't see any uses of adapter_. What's it for?
> 
> Ping.

Done (removed). It's for a later patch where we will need to notify observers of
descriptor value changes.

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

https://codereview.chromium.org/1574773002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:744:
EXPECT_EQ(characteristic1_->GetDescriptor(c1_id1)->GetUUID(),
On 2016/01/12 23:25:59, Jeffrey Yasskin wrote:
> Remove "->GetUUID()".
> 
> Could you also check that it's the same as what GetDescriptors()[0] returns?

Done.

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

https://codereview.chromium.org/1574773002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:142:
EXPECT_EQ(service->GetCharacteristic(char_id1)->GetUUID(),
On 2016/01/12 23:25:59, Jeffrey Yasskin wrote:
> Remove "->GetUUID()".

Done.

[scheib, 2016-01-12 23:54:54]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-01-12 23:54:54]

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/1574773002/#ps100001
(title: "addressed j again")

[commit-bot: I haz the power, 2016-01-12 23:55:37]

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

[commit-bot: I haz the power, 2016-01-13 00:58:38]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-01-13 00:59:43]

Description was changed from

==========
bluetooth: android: Initial basic Descriptors implementation.

Initial basic implementation of BluetoothRemoteGattDescriptorAndroid and
ChromeBluetoothRemoteGattDescriptor.

BUG=566533
==========

to

==========
bluetooth: android: Initial basic Descriptors implementation.

Initial basic implementation of BluetoothRemoteGattDescriptorAndroid and
ChromeBluetoothRemoteGattDescriptor.

BUG=566533

Committed: https://crrev.com/81a0ff269b3e54580b15c52da806249435e4e206
Cr-Commit-Position: refs/heads/master@{#369059}
==========

[commit-bot: I haz the power, 2016-01-13 00:59:44]

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