Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by tommyt Modified:
4 years, 9 months ago Reviewers:
scheib, ortuno CC:
chromium-reviews, ortuno+watch_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: 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}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added the comment that Gio requested #
Total comments: 18
Patch Set 3 : Address Vincent's comments #
Total comments: 25
Created: 4 years, 9 months ago
Dependent Patchsets: Messages
Total messages: 47 (19 generated)
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@opera.com changed reviewers: + ortuno@chromium.org
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.
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
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@chromium.org changed reviewers: + scheib@chromium.org
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
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.
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
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.
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.
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 ==========
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.
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()?
I'll be addressing these review comments on my follow up patch.
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.
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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.
LGTM
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scheib@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scheib@chromium.org
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0348fa65c3d808f854f745fb9f8f185a5b74b922 Cr-Commit-Position: refs/heads/master@{#381088} |
