bluetooth: android: Confirm the notify session after the descriptor has been written.

bluetooth: android: Confirm the notify session after the descriptor has been written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

[tommyt, 2016-02-18 15:27:46]

Description was changed from

==========
Confirm the notify session after the descriptor has been written

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

to

==========
Confirm the notify session after the descriptor has been written

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

[tommyt, 2016-02-18 15:27:46]

tommyt@opera.com changed reviewers:
+ ortuno@chromium.org

[tommyt, 2016-02-18 15:30:20]

Here's my proposed fix for issue 584369.

It's a bit more code than it strictly needs to be, because I also implemented
BluetoothRemoteGattDescriptorAndroid::WriteRemoteDescriptor and
BluetoothRemoteGattDescriptorAndroid::ReadRemoteDescriptor since I thought I was
going to need those. They ended up not being essential to the implementation,
but since they presumably need to be implemented anyway, I left them in the
commit.

[ortuno, 2016-02-19 20:51:02]

looking good! Just one small comment. As mentioned before you will have to wait
until scheib is back next week.

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

https://codereview.chromium.org/1712593002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:219: if (status
== 0) {
Comment what 0 means

[tommyt, 2016-02-22 08:59:56]

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

https://codereview.chromium.org/1712593002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:219: if (status
== 0) {
On 2016/02/19 20:51:02, ortuno wrote:
> Comment what 0 means

Done.

[scheib, 2016-02-26 04:27:00]

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

[scheib, 2016-02-26 04:27:01]

Great to see this!

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:269:
Log.v(TAG, "onDescriptorRead when chromeDescriptor == null.");
Copy the comments from characteristics above as well explaining why this
situation happens.

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:284:
UUID.fromString("00002902-0000-1000-8000-00805F9B34FB"))) {
// If this is the Client Configuration Characteristic descriptor check
// the associated characteristic to see if startNotifySession is waiting
// for the descriptor write to be completed. 

Move the constant value to a static member on
ChromeBluetoothRemoteGattCharacteristic and use it here and in
startNotifySession.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:107:

Add a comment section divider:
//
---------------------------------------------------------------------------------------------
// BluetoothRemoteGattDescriptorAndroid C++ methods declared for access from
java:

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:139: if
(pending_start_notify_calls_.empty()) {
Starting notify, reading & writing the CCC descriptor operations are all
mutually exclusive. Please add a check that the CCC descriptor doesn't have an
in-flight read or write operation. And when notify is being started please flag
the CCC descriptor to reject any read/write.

It's possible that the logic that does the descriptor write in Java should just
be moved to C++ here.  Then the Java code wouldn't even need to know that the
start notify was pending, etc. It would just be a 'normal' descriptor write. 
And the descriptor C++ code would just guard as already implemented for
read|write in progress.

Either way, please also add tests for startnotify and read/write operations
colliding.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.h:83: // Callback
after StartNotifySession operation completes.
Called when StartNotifySession operation completes.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:155: //
TODO(https://crbug.com/545682): Call GattDescriptorValueChanged.
update to descriptor bug 584369

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:177: //
TODO(https://crbug.com/545682): Call GattDescriptorValueChanged.
update to descriptor bug 584369

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.h:53: // Callback
after Read operation completes.
Called when

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.h:59: // Callback
after Write operation completes.
Called when

[tommyt, 2016-03-01 14:45:15]

I've made a new changeset where I've tried to address Vincent's comments. The
biggest change is the move of the startNotifySession logic from java to c++.

I'm still struggling with a failing unittest, though. There seems to be some JNI
related problem when running on the trybots, but I'm unfortunately not able to
reproduce it locally.

In case someone who sees this has a useful hint for me, here's the exception I'm
getting:

java.lang.UnsatisfiedLinkError: Native method not found:
org.chromium.device.bluetooth.ChromeBluetoothRemoteGattDescriptor.nativeOnRead:(JI[B)V

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:269:
Log.v(TAG, "onDescriptorRead when chromeDescriptor == null.");
On 2016/02/26 04:27:01, scheib wrote:
> Copy the comments from characteristics above as well explaining why this
> situation happens.

Done.

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:284:
UUID.fromString("00002902-0000-1000-8000-00805F9B34FB"))) {
On 2016/02/26 04:27:01, scheib wrote:
> // If this is the Client Configuration Characteristic descriptor check
> // the associated characteristic to see if startNotifySession is waiting
> // for the descriptor write to be completed. 
> 
> Move the constant value to a static member on
> ChromeBluetoothRemoteGattCharacteristic and use it here and in
> startNotifySession.

Done.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothRemoteGattDescriptor.java:107:

On 2016/02/26 04:27:01, scheib wrote:
> Add a comment section divider:
> //
>
---------------------------------------------------------------------------------------------
> // BluetoothRemoteGattDescriptorAndroid C++ methods declared for access from
> java:

Done.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:139: if
(pending_start_notify_calls_.empty()) {
On 2016/02/26 04:27:01, scheib wrote:
> Starting notify, reading & writing the CCC descriptor operations are all
> mutually exclusive. Please add a check that the CCC descriptor doesn't have an
> in-flight read or write operation. And when notify is being started please
flag
> the CCC descriptor to reject any read/write.
> 
> It's possible that the logic that does the descriptor write in Java should
just
> be moved to C++ here.  Then the Java code wouldn't even need to know that the
> start notify was pending, etc. It would just be a 'normal' descriptor write. 
> And the descriptor C++ code would just guard as already implemented for
> read|write in progress.
> 
> Either way, please also add tests for startnotify and read/write operations
> colliding.

I agree with moving this to C++. My newest changeset does this, and I think it's
quite a bit nicer.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.h:83: // Callback
after StartNotifySession operation completes.
On 2016/02/26 04:27:01, scheib wrote:
> Called when StartNotifySession operation completes.

Done.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:155: //
TODO(https://crbug.com/545682): Call GattDescriptorValueChanged.
On 2016/02/26 04:27:01, scheib wrote:
> update to descriptor bug 584369

Done.

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:177: //
TODO(https://crbug.com/545682): Call GattDescriptorValueChanged.
On 2016/02/26 04:27:01, scheib wrote:
> update to descriptor bug 584369

Done.

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

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.h:53: // Callback
after Read operation completes.
On 2016/02/26 04:27:01, scheib wrote:
> Called when

Done.

https://codereview.chromium.org/1712593002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.h:59: // Callback
after Write operation completes.
On 2016/02/26 04:27:01, scheib wrote:
> Called when

Done.

[scheib, 2016-03-01 19:13:48]

On 2016/03/01 14:45:15, tommyt wrote:
> I've made a new changeset where I've tried to address Vincent's comments. The
> biggest change is the move of the startNotifySession logic from java to c++.
> 
> I'm still struggling with a failing unittest, though. There seems to be some
JNI
> related problem when running on the trybots, but I'm unfortunately not able to
> reproduce it locally.
> 
> In case someone who sees this has a useful hint for me, here's the exception
I'm
> getting:
> 
> java.lang.UnsatisfiedLinkError: Native method not found:
>
org.chromium.device.bluetooth.ChromeBluetoothRemoteGattDescriptor.nativeOnRead:(JI[B)V

You'll need to update device/bluetooth/android/bluetooth_jni_registrar.cc

[scheib, 2016-03-02 06:15:20]

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.cc:28: BluetoothGattDescriptor*
BluetoothGattCharacteristic::GetDescriptorForUUID(
We should add a test to bluetooth_gatt_characteristic_unittest.cc for this now.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:255: while
(!pending_start_notify_calls_.empty()) {
Protect against reentrant code by swapping the member vector to a local vector,
then looping over that.

[scheib, 2016-03-02 22:22:22]

I will land this patch and perform the follow up requests in a patch I submit,
as I hear tommyt is not available to work on this right now.

[scheib, 2016-03-04 01:18:56]

Description was changed from

==========
Confirm the notify session after the descriptor has been written

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

to

==========
bluetooth: android: Confirm the notify session after the descriptor has been
written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

[tommyt, 2016-03-04 14:35:26]

    
Thanks Vincent! Yes, I will be on leave/vacation for about a month due to the
birth of my child.
I actually have a change sitting around for implementing stopNotifySession too,
but since we ended up moving a lot of the startNotifySession code to c++, this
change would need so much rework that I think someone might as well implement it
from scratch.
Br,Tommy

Sent from my Samsung device

-------- Original message --------
From: scheib@chromium.org 
Date: 02/03/2016  23:22  (GMT+01:00) 
To: tommyt@opera.com, ortuno@chromium.org 
Cc: chromium-reviews@chromium.org, ortuno+watch@chromium.org,
	scheib+watch@chromium.org 
Subject: Re: Confirm the notify session after the descriptor has been written
(issue 1712593002 by tommyt@opera.com) 



{
  "@context": "http://schema.org",
  "@type": "EmailMessage",
  "potentialAction": {
    "@type": "ViewAction",
    "name": "Open CL",
    "url": "https://codereview.chromium.org/1712593002/"
  },
  "description": ""
}



I will land this patch and perform the follow up requests in a patch I submit,
as I hear tommyt is not available to work on this right now.

https://codereview.chromium.org/1712593002/


-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ortuno, 2016-03-04 17:39:48]

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.cc:35: return NULL;
nit: nullptr

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.h:161: // Returns the GATT
characteristic descriptor that matches |uuid|.
I think this should be GetDescriptorsByUUID (plural). We have a similar function
in BluetoothDispatcherHost except for services. Users of the function can
decided if they only want the first descriptor.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:203:
SimulateGattDescriptorWrite(descriptor1_);
Shouldn't we test that duplicated write reported doesn't cause any troubles?

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:218:
DeleteDevice(device_);
I wonder if we should explicitly delete the descriptor. Otherwise we might miss
bugs because the chain of deletions has a bug. Also we have no tests to make
sure deleting the characteristic deletes its children.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:139: if
(!pending_start_notify_calls_.empty()) {
Why are we allowing multiple notify session calls? I thought we would just fail
with IN PROGRESS. I'm OK with going for a queue but hopefully some thought have
been give as to how Stop is going to be implemented since this will clearly
increase the complexity.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:154:
BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
I think this should be GATT_ERROR_NOT_SUPPORTED.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:167:
BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
Same here. I think GATT_ERROR_NOT_SUPPORTED is more appropriate.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:181:
std::vector<uint8_t> value;
nit: value(2) // The Client Characteristic Configuration descriptor is 16bits
long

Also this would be nicer if we were allowed to use initialization syntax:

std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:189:
base::Unretained(this)),
Is this safe? I guess it is since the characteristic owns the descriptor.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:255: while
(!pending_start_notify_calls_.empty()) {
On 2016/03/02 at 06:15:20, scheib wrote:
> Protect against reentrant code by swapping the member vector to a local
vector, then looping over that.

I just realized how easy it is to miss this. Do we have any tests for this?

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:152: &&
!read_callback.is_null()) {
Why do we need this is_null()?

[scheib, 2016-03-04 18:48:25]

I'll be addressing these review comments on my follow up patch.

[scheib, 2016-03-11 03:17:31]

These issues are being addressed in follow up patches.

bluetooth: Refactor GetDescriptorForUUID to GetDescriptorsForUUID.
https://codereview.chromium.org/1765773002/

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

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.cc:28: BluetoothGattDescriptor*
BluetoothGattCharacteristic::GetDescriptorForUUID(
On 2016/03/02 06:15:20, scheib wrote:
> We should add a test to bluetooth_gatt_characteristic_unittest.cc for this
now.

Done.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.cc:35: return NULL;
On 2016/03/04 17:39:47, ortuno wrote:
> nit: nullptr

Done.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic.h:161: // Returns the GATT
characteristic descriptor that matches |uuid|.
On 2016/03/04 17:39:47, ortuno wrote:
> I think this should be GetDescriptorsByUUID (plural). We have a similar
function
> in BluetoothDispatcherHost except for services. Users of the function can
> decided if they only want the first descriptor.

Done.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:203:
SimulateGattDescriptorWrite(descriptor1_);
On 2016/03/04 17:39:47, ortuno wrote:
> Shouldn't we test that duplicated write reported doesn't cause any troubles?

Done.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_unittest.cc:218:
DeleteDevice(device_);
On 2016/03/04 17:39:47, ortuno wrote:
> I wonder if we should explicitly delete the descriptor. Otherwise we might
miss
> bugs because the chain of deletions has a bug. Also we have no tests to make
> sure deleting the characteristic deletes its children.

We don't test deletions of GATT objects yet. Android doesn't support a service
change and there's an issue for that:
https://bugs.chromium.org/p/chromium/issues/detail?id=576906
I'm not thinking of a good way to test a bug in the test framework's
DeleteDevice and chain of our bluetooth object deletion other than memory leak
detection. No good hooks for that. I think the right solution is to wait for
576906. I've noted at these points that we should update tests to delete objects
more narrowly.

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:139: if
(!pending_start_notify_calls_.empty()) {
On 2016/03/04 17:39:47, ortuno wrote:
> Why are we allowing multiple notify session calls? I thought we would just
fail
> with IN PROGRESS. I'm OK with going for a queue but hopefully some thought
have
> been give as to how Stop is going to be implemented since this will clearly
> increase the complexity.

Unlike a read or write that must happen serially, StartNotify is more like
StartDiscoverySession. Stop is called per BluetoothGattNotifySession and
decrements a reference count. Only when all references are released is the stop
carried out. I don't think there's much complexity here that we can't get out
of.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:154:
BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
On 2016/03/04 17:39:47, ortuno wrote:
> I think this should be GATT_ERROR_NOT_SUPPORTED.

Done.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:167:
BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
On 2016/03/04 17:39:47, ortuno wrote:
> Same here. I think GATT_ERROR_NOT_SUPPORTED is more appropriate.

Done.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:181:
std::vector<uint8_t> value;
On 2016/03/04 17:39:47, ortuno wrote:
> nit: value(2) // The Client Characteristic Configuration descriptor is 16bits
> long
> 
> Also this would be nicer if we were allowed to use initialization syntax:
> 
> std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

Done.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:189:
base::Unretained(this)),
On 2016/03/04 17:39:47, ortuno wrote:
> Is this safe? I guess it is since the characteristic owns the descriptor.

Acknowledged.

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:255: while
(!pending_start_notify_calls_.empty()) {
On 2016/03/04 17:39:47, ortuno wrote:
> On 2016/03/02 at 06:15:20, scheib wrote:
> > Protect against reentrant code by swapping the member vector to a local
> vector, then looping over that.
> 
> I just realized how easy it is to miss this. Do we have any tests for this?

Not yet -- we should. Here's a new issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=593554

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

https://codereview.chromium.org/1712593002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_descriptor_android.cc:152: &&
!read_callback.is_null()) {
On 2016/03/04 17:39:47, ortuno wrote:
> Why do we need this is_null()?

Can't call .Run on null callbacks, and it is valid to call
BluetoothGattDescriptor::ReadRemoteDescriptor() with null callbacks.

[scheib, 2016-03-11 03:19:12]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:19:40]

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

[commit-bot: I haz the power, 2016-03-11 03:19:41]

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

[commit-bot: I haz the power, 2016-03-11 03:19:42]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

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

LGTM

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

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:21:30]

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

[commit-bot: I haz the power, 2016-03-11 03:28:44]

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

[commit-bot: I haz the power, 2016-03-11 03:28:45]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[scheib, 2016-03-11 04:12:23]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-11 04:12:46]

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

[commit-bot: I haz the power, 2016-03-11 04:44:08]

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

[commit-bot: I haz the power, 2016-03-11 04:44:09]

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

[scheib, 2016-03-11 04:45:49]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-11 04:46:25]

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

[commit-bot: I haz the power, 2016-03-11 07:34:02]

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

[commit-bot: I haz the power, 2016-03-11 07:34:03]

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

[scheib, 2016-03-11 16:07:05]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-11 16:07:23]

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

[commit-bot: I haz the power, 2016-03-11 18:50:42]

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

[commit-bot: I haz the power, 2016-03-11 18:50:43]

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

[scheib, 2016-03-12 20:47:23]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-12 20:47:35]

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

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

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

[commit-bot: I haz the power, 2016-03-12 23:20:59]

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

[scheib, 2016-03-14 20:43:03]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-03-14 20:43:24]

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

[commit-bot: I haz the power, 2016-03-14 22:12:42]

Description was changed from

==========
bluetooth: android: Confirm the notify session after the descriptor has been
written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

to

==========
bluetooth: android: Confirm the notify session after the descriptor has been
written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

[commit-bot: I haz the power, 2016-03-14 22:12:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-14 22:15:09]

Description was changed from

==========
bluetooth: android: Confirm the notify session after the descriptor has been
written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

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

to

==========
bluetooth: android: Confirm the notify session after the descriptor has been
written.

This change also implements WriteRemoteDescriptor and
ReadRemoteDescriptor. Because of this, I've also added quite a few
descriptor unit tests. These tests are pretty much the same as the
read/write tests for characteristics.

BUG=584369

Committed: https://crrev.com/0348fa65c3d808f854f745fb9f8f185a5b74b922
Cr-Commit-Position: refs/heads/master@{#381088}
==========

[commit-bot: I haz the power, 2016-03-14 22:15:10]

Patchset 3 (id:??) landed as
https://crrev.com/0348fa65c3d808f854f745fb9f8f185a5b74b922
Cr-Commit-Position: refs/heads/master@{#381088}