Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(625)

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

Created:
3 years, 8 months ago by ortuno
Modified:
3 years, 7 months ago
Reviewers:
scheib
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

Messages

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

Powered by Google App Engine
This is Rietveld 408576698