Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in

Issue 2826383002: bluetooth: Make in progress errors consistent across android

12 months ago by ortuno
11 months, 3 weeks ago
Target Ref:


bluetooth: Make in progress errors consistent across android Concurrent operations' results vary depending on the platform. See This is the first patch in a series of patches to make the results consistent. Introduces BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristicImpl that implementations of the interface need to implement. BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristic will call this once it performs the necessary checks to ensure there are no other operations in progress. BUG=657921

Patch Set 1 #

Patch Set 2 : clean up #

Patch Set 3 : Fix windows #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+400 lines, -84 lines) Patch
M device/bluetooth/bluetooth_device.h View 2 chunks +13 lines, -0 lines 0 comments Download
M device/bluetooth/ View 2 chunks +9 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic.h View 1 chunk +22 lines, -4 lines 0 comments Download
M device/bluetooth/ View 1 3 chunks +35 lines, -0 lines 2 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_android.h View 2 chunks +7 lines, -7 lines 0 comments Download
M device/bluetooth/ View 5 chunks +8 lines, -20 lines 1 comment Download
M device/bluetooth/ View 1 2 3 chunks +306 lines, -53 lines 6 comments Download


Total messages: 14 (11 generated)
scheib: PTAL
12 months ago (2017-04-21 05:55:54 UTC) #10
This is an important moment to consider if we do notable work to make a ...
12 months ago (2017-04-21 23:33:11 UTC) #13
11 months, 3 weeks ago (2017-04-28 03:47:12 UTC) #14
Will revisit the rest of the comments once the reentrancy tests are done.
File device/bluetooth/ (right):
device/bluetooth/ void
On 2017/04/21 at 23:33:10, scheib wrote:
> Consider keeping all logic in the base classes, never referring to
PendingGattOperation in the platform specific sub-classes. Ideally the sub class
___Impl methods do the minimal OS specific work to perform the task and then
fire back the callbacks.
> The methods on device would check and set the pending_gatt_operation_ state,
and wrap callbacks so that when the sub-classes Run them they clear
> There may be benefit from having generic methods
> something roughly like:
> BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristic(callback,
error_callback) {
>   if (ReserveGattOperationOrRunError(error_callback)) {
>     ReadRemoteCharacteristicImpl(
>         MakeValueCallbackAfterGattOperation(callback),
>         MakeErrorCallbackAfterGattOperation(error_callback));
>   }
> where MakeValueCallbackAfterGattOperation come from BluetoothDevice and
generically return a callback that wraps another callback and clears
pending_gatt_operation_ before running the inner callback.

I was exploring something like that at some point albeit only at the
characteristic level. I ended up abandoning that approach because it could
affect reentrancy and we have no tests for it. Since this is somewhat orthogonal
I'll do it in a separate CL before continuing this.
File device/bluetooth/ (right):
On 2017/04/21 at 23:33:11, scheib wrote:
> Heads up: This pattern is changing:
> In that change this unittest file isn't modified, but perhaps we should start
using new pattern.

Those tests used a slightly different pattern in which they actually quit the
runloop in their callbacks. We just let the runloop go idle in our callbacks. It
could be useful when testing reentrancy but for the rest of the tests I like the
simplicity of just calling:


Powered by Google App Engine
This is Rietveld 408576698