Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by ortuno
Modified:
3 months, 3 weeks ago
CC:
chromium-reviews, ortuno+watch_chromium.org, scheib+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

bluetooth: Make in progress errors consistent across android Concurrent operations' results vary depending on the platform. See https://docs.google.com/document/d/1ClMet28ElV4VA4iRYG1fnIwmDcNk2mw8KrNHHC-QSjg/edit 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/bluetooth_device.cc 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/bluetooth_remote_gatt_characteristic.cc 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/bluetooth_remote_gatt_characteristic_android.cc View 5 chunks +8 lines, -20 lines 1 comment Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc View 1 2 3 chunks +306 lines, -53 lines 6 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 14 (11 generated)
ortuno
scheib: PTAL
4 months ago (2017-04-21 05:55:54 UTC) #10
scheib - Prefer gerrit tool
This is an important moment to consider if we do notable work to make a ...
3 months, 4 weeks ago (2017-04-21 23:33:11 UTC) #13
ortuno
3 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.

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

https://codereview.chromium.org/2826383002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:89: void
BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristic(
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
pending_gatt_operation_.
> 
> 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.

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

https://codereview.chromium.org/2826383002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:39:
base::RunLoop().RunUntilIdle();
On 2017/04/21 at 23:33:11, scheib wrote:
> Heads up: This pattern is changing:
>
https://codereview.chromium.org/2818533003/diff/240001/device/bluetooth/bluez...
> 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:

base::RunLoop().RunUntilIdle();
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b