Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256313002:
bluetooth: android: Implement & test CreateGattConnection. (Closed)
Descriptionbluetooth: android: Implement CreateGattConnection.
Using the base class implementation from previous
patches, this patch completes the Android implementation
of CreateGattConnection and provides tests.
Part of a 4 patch series:
https://crrev.com/1287753002 Remove Disconnect callback
https://crrev.com/1284073002 Add adapter_ to BluetoothDevice
https://crrev.com/1292263002 BluetoothDevice::CreateGattConnection Impl
https://crrev.com/1256313002 Android Impl & tests. <<<
BUG=512643
Committed: https://crrev.com/af7ea91977473d1ae331a6052ca49003134d405e
Cr-Commit-Position: refs/heads/master@{#350359}
Patch Set 1 : Split up patch, this is now just android #Patch Set 2 : Updated patchset dependency #
Total comments: 28
Patch Set 3 : Updated patchset dependency #Patch Set 4 : Merge TOT #Patch Set 5 : addressed jyasskin comments. Added more tests. #
Total comments: 14
Patch Set 6 : address jyasskin #
Total comments: 24
Patch Set 7 : addressed jyasskin #
Total comments: 4
Patch Set 8 : Added 2 more connect/disconnect test blocks #
Total comments: 2
Patch Set 9 : Split tests up into smaller tests #Messages
Total messages: 33 (13 generated)
scheib@chromium.org changed reviewers: + armansito@chromium.org
Arman, Here's a patch forming with default implementation of CreateGattConnection (all mashed up with other Android changes; I'll likely split it up later). It relies on simplifying callbacks by removing the BluetoothGattConnection::Disconnect callback. But, also it requires adding an IsGattConnected method. https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.cc (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.cc:210: if (IsGattConnected()) BluetoothDevice::CreateGattConnection needs to check specifically for IsGattConnected on platforms where the GATT APIs are separated from the classic APIs, e.g. Mac, Android. https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:244: virtual bool IsGattConnected() const = 0; Arman, approve of adding BluetoothDevice::IsGattConnected? https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_chromeos.cc (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_chromeos.cc:244: NOTIMPLEMENTED(); We don't need BluetoothDeviceChromeOS::IsGattConnected implemented for this patch set --- but if you can suggest a low cost way to implement it I happily would.
Arman, To be clear --- I'm only looking for design input right now on the API changes; see my comments.
Patchset #3 (id:40001) has been deleted
Overall I'm fine with this design. See my comments. https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:244: virtual bool IsGattConnected() const = 0; On 2015/08/05 01:09:11, scheib wrote: > Arman, approve of adding BluetoothDevice::IsGattConnected? This is fine by me. https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:476: uint32_t gatt_connection_reference_count_ = 0; Why uint32_t specifically? https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_chromeos.cc (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_chromeos.cc:244: NOTIMPLEMENTED(); On 2015/08/05 01:09:11, scheib wrote: > We don't need BluetoothDeviceChromeOS::IsGattConnected implemented for this > patch set --- but if you can suggest a low cost way to implement it I happily > would. It's not straightforward. Given the current APIs, this will technically be true if the device is connected AND the device is a LE device. I'd prefer adding a proper LowEnergyConnected D-Bus property and expose that from BlueZ. But since you're leaving this empty on ChromeOS, won't this break the logic in BluetoothDevice::CreateGattConnection?
https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:476: uint32_t gatt_connection_reference_count_ = 0; On 2015/08/08 01:51:14, armansito wrote: > Why uint32_t specifically? Need to pick something, and I'd rather pick 32 bits instead of just 'int'. https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_chromeos.cc (right): https://codereview.chromium.org/1256313002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_chromeos.cc:244: NOTIMPLEMENTED(); On 2015/08/08 01:51:14, armansito wrote: > On 2015/08/05 01:09:11, scheib wrote: > > We don't need BluetoothDeviceChromeOS::IsGattConnected implemented for this > > patch set --- but if you can suggest a low cost way to implement it I happily > > would. > > It's not straightforward. Given the current APIs, this will technically be true > if the device is connected AND the device is a LE device. I'd prefer adding a > proper LowEnergyConnected D-Bus property and expose that from BlueZ. > > But since you're leaving this empty on ChromeOS, won't this break the logic in > BluetoothDevice::CreateGattConnection? ChromeOS won't use BluetoothDevice::CreateGattConnection, it overrides it in BluetoothDeviceChromeOS::CreateGattConnection and I'm not planning on changing the implementation since it is so tied to dbus already.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
jyasskin@chromium.org changed reviewers: + jyasskin@chromium.org
https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:103: Log.i(TAG, "connectGatt"); Do we still need all this logging? https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:107: boolean connectResult = mBluetoothGatt.connect(); Calling .connect() appears to ask Android to connect whenever the device comes into range, which looks equivalent to connectGatt(autoConnect=true). I'm not sure whether we want to time out or just let the connection complete whenever: https://github.com/WebBluetoothCG/web-bluetooth/issues/152. I'm leaning toward letting it wait, which implies connectGatt(autoConnect=true) above. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:132: : "Dissconnected"); sp: Dissconnected https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:143: long nativeBluetoothDeviceAndroid, int success, boolean connected); s/success/status/ https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:280: static class BluetoothGattCallbackWrapperCaller extends BluetoothGattCallback { "CallbackWrapperCaller" is awkward. Maybe ForwardBluetoothGattCallbackToWrapper? https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:295: * BluetoothGattCallbackWrapperCaller. The "being called by" clause doesn't make a lot of sense. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:298: * need to wrapp them in a BluetoothGattWrapper. That would be superfluous given sp: wrapp https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:298: * need to wrapp them in a BluetoothGattWrapper. That would be superfluous given BluetoothGattCallback passes the BluetoothGatt objects so that you can have one Callback instance that handles many devices. So the last sentence isn't quite right, even though the interface may be. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:213: return DidFailToConnectGatt(ERROR_UNKNOWN); Log here so we know what error to add a translation for. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:230: DidFailToConnectGatt(ERROR_UNKNOWN); Pick a better error enum. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:143: device->CreateGattConnection(GetGattConnectionCallback(), Can we also see some more complex interleavings of CreateGattConnection and Disconnect calls, to see how the callbacks get fired? https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:146: EXPECT_EQ(1, callback_count_--); I'd rather assign callback_count_ on a separate line here too. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:199: : android.bluetooth.BluetoothGatt.GATT_FAILURE, Make it possible to test more status codes? https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.cc:73: base::Unretained(this)); Generally comment why base::Unretained calls are correct.
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:103: Log.i(TAG, "connectGatt"); On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > Do we still need all this logging? I've reduced some and filed crbug.com/529510 to remember to revisit. These log statements aren't frequent and will offer high value in diagnosing issues on production builds. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:107: boolean connectResult = mBluetoothGatt.connect(); On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > Calling .connect() appears to ask Android to connect whenever the device comes > into range, which looks equivalent to connectGatt(autoConnect=true). > > I'm not sure whether we want to time out or just let the connection complete > whenever: https://github.com/WebBluetoothCG/web-bluetooth/issues/152. I'm > leaning toward letting it wait, which implies connectGatt(autoConnect=true) > above. So, autoconnect=true isn't working on devices I'm testing with. For now, I'm going to create a direct connection. The previous call to mBluetoothGatt.connect() was redundant, so that's removed. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:132: : "Dissconnected"); On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > sp: Dissconnected Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:143: long nativeBluetoothDeviceAndroid, int success, boolean connected); On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > s/success/status/ Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:280: static class BluetoothGattCallbackWrapperCaller extends BluetoothGattCallback { On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > "CallbackWrapperCaller" is awkward. Maybe ForwardBluetoothGattCallbackToWrapper? Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:295: * BluetoothGattCallbackWrapperCaller. On 2015/08/20 21:35:13, Jeffrey Yasskin wrote: > The "being called by" clause doesn't make a lot of sense. Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:298: * need to wrapp them in a BluetoothGattWrapper. That would be superfluous given On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > BluetoothGattCallback passes the BluetoothGatt objects so that you can have one > Callback instance that handles many devices. So the last sentence isn't quite > right, even though the interface may be. Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:298: * need to wrapp them in a BluetoothGattWrapper. That would be superfluous given On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > sp: wrapp Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:213: return DidFailToConnectGatt(ERROR_UNKNOWN); On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > Log here so we know what error to add a translation for. Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:230: DidFailToConnectGatt(ERROR_UNKNOWN); On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > Pick a better error enum. Done. This method no longer has synchronous errors reported because BluetoothGatt.connect() is no longer being used. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:143: device->CreateGattConnection(GetGattConnectionCallback(), On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > Can we also see some more complex interleavings of CreateGattConnection and > Disconnect calls, to see how the callbacks get fired? Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:146: EXPECT_EQ(1, callback_count_--); On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > I'd rather assign callback_count_ on a separate line here too. I think this makes the test harder to read, but OK. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:199: : android.bluetooth.BluetoothGatt.GATT_FAILURE, On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > Make it possible to test more status codes? Done. https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.cc (right): https://codereview.chromium.org/1256313002/diff/100001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.cc:73: base::Unretained(this)); On 2015/08/20 21:35:14, Jeffrey Yasskin wrote: > Generally comment why base::Unretained calls are correct. Done - moved to weak pointers.
https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:30: private long mNativeBluetoothDeviceAndroid; Make this final? https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:34: BluetoothGattCallbackImpl mBluetoothGattCallbackImpl; Make this private and final? https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:290: * Wraper alternative to android.bluetooth.BluetoothGattCallback allowing clients and Fakes to sp: Wraper https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter.cc:207: delete device->second; Erasing device invalidates it, so you need to copy device->second or delete it before the erase() call. I haven't checked whether ~BluetoothDevice can call back into the map. If so, you'd need to copy, erase, delete. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:206: // TODO(scheib) Increase BluetoothDevice::ConnectErrorCode enums for "Increase"? Maybe "Create new"? https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:157: TEST_F(BluetoothTest, BluetoothGattConnection) { TODO(jyasskin): check that this covers all the weird orderings I can think of. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:210: "Multiple callbacks provided to connectGatt unsupported."); "BluetoothGattWrapper doesn't support calls to connectGatt() with multiple distinct callbacks."?
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:30: private long mNativeBluetoothDeviceAndroid; On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > Make this final? OK, but in a patch in the near future it goes back to non-final so that it can be reset to 0 when the C++ object is destroyed. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:34: BluetoothGattCallbackImpl mBluetoothGattCallbackImpl; On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > Make this private and final? Done. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:290: * Wraper alternative to android.bluetooth.BluetoothGattCallback allowing clients and Fakes to On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > sp: Wraper Done. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter.cc:207: delete device->second; On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > Erasing device invalidates it, so you need to copy device->second or delete it > before the erase() call. I haven't checked whether ~BluetoothDevice can call > back into the map. If so, you'd need to copy, erase, delete. Done. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_android.cc:206: // TODO(scheib) Increase BluetoothDevice::ConnectErrorCode enums for On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > "Increase"? Maybe "Create new"? Done. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:157: TEST_F(BluetoothTest, BluetoothGattConnection) { On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > TODO(jyasskin): check that this covers all the weird orderings I can think of. Acknowledged. https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/180001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:210: "Multiple callbacks provided to connectGatt unsupported."); On 2015/09/16 01:40:49, Jeffrey Yasskin wrote: > "BluetoothGattWrapper doesn't support calls to connectGatt() with multiple > distinct callbacks."? Done.
https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:230: // CreateGattConnection, & multiple connection s from platform only invoke sp: connection s https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:251: EXPECT_EQ(1, callback_count_); Please add: EXPECT_FALSE(gatt_connections_[0]->IsConnected()) << "The disconnected connection shouldn't become connected if another connection is created."; EXPECT_TRUE(gatt_connections_[1]->IsConnected()); https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:254: // Disconnect all CreateGattConnection objects. But, don't yet simulate Please also add a test in which the last connection is destroyed instead of having Disconnect() called. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:257: for (auto connection : gatt_connections_) Can you write BluetoothGattConnection* for these for loops too? https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:258: connection->Disconnect(); Can you check that the implementation appropriately asked to disconnect after this loop? https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:261: EXPECT_EQ(1, callback_count_); // Device is assumed still connected. Check that gatt_connections_.back()->IsConnected(). https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:270: EXPECT_FALSE(connection->IsConnected()); These were already false before the CompleteGattDisconnection() call, right? It's only device->IsGattConnected() that I'd expect to change there. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:277: CompleteGattDisconnection(device); Does this happen naturally if you call device->Disconnect() before the gatt connection completes? https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:284: // CreateGattConnection, but error connecting. Also, Multiple errors only s/Multiple/multiple/ https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:206: public Wrappers.BluetoothGattWrapper connectGatt(Context context, boolean autoConnect, I'd like to see this report back to C++ that it was called, so that the C++ test can assert that happened at the right times. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:247: public void disconnect() {} Like connectGatt(), I'd like to see assertions that disconnect() was called at the right times.
https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:230: // CreateGattConnection, & multiple connection s from platform only invoke On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > sp: connection s Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:251: EXPECT_EQ(1, callback_count_); On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Please add: > > EXPECT_FALSE(gatt_connections_[0]->IsConnected()) > << "The disconnected connection shouldn't become connected if another > connection is created."; > EXPECT_TRUE(gatt_connections_[1]->IsConnected()); Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:254: // Disconnect all CreateGattConnection objects. But, don't yet simulate On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Please also add a test in which the last connection is destroyed instead of > having Disconnect() called. Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:257: for (auto connection : gatt_connections_) On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Can you write BluetoothGattConnection* for these for loops too? Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:258: connection->Disconnect(); On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Can you check that the implementation appropriately asked to disconnect after > this loop? Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:261: EXPECT_EQ(1, callback_count_); // Device is assumed still connected. On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Check that gatt_connections_.back()->IsConnected(). Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:270: EXPECT_FALSE(connection->IsConnected()); On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > These were already false before the CompleteGattDisconnection() call, right? > It's only device->IsGattConnected() that I'd expect to change there. The last connection reports connected. (It connected after the others disconnected and initiated the disconnect request but before the device actually disconnected). https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:277: CompleteGattDisconnection(device); On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Does this happen naturally if you call device->Disconnect() before the gatt > connection completes? I was planning on not failing a connect until the device state actually changes. If a connect, then disconnect are sent then what is the state of the device? I don't think we should fire error or disconnected callbacks until the device actually transitions to those states. What if the device does connect in that circumstance but doesn't disconnect for whatever reason? Put another way, I don't think we should make any speculative logic about what a device may actually do; all state and callbacks should reflect OS reported changes in device state. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:284: // CreateGattConnection, but error connecting. Also, Multiple errors only On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > s/Multiple/multiple/ Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:206: public Wrappers.BluetoothGattWrapper connectGatt(Context context, boolean autoConnect, On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > I'd like to see this report back to C++ that it was called, so that the C++ test > can assert that happened at the right times. Done. https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/test/... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:247: public void disconnect() {} On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > Like connectGatt(), I'd like to see assertions that disconnect() was called at > the right times. Done.
https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:277: CompleteGattDisconnection(device); On 2015/09/20 02:53:34, scheib wrote: > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > Does this happen naturally if you call device->Disconnect() before the gatt > > connection completes? > > I was planning on not failing a connect until the device state actually changes. > If a connect, then disconnect are sent then what is the state of the device? I > don't think we should fire error or disconnected callbacks until the device > actually transitions to those states. What if the device does connect in that > circumstance but doesn't disconnect for whatever reason? > > Put another way, I don't think we should make any speculative logic about what a > device may actually do; all state and callbacks should reflect OS reported > changes in device state. Right, that's fine. Calls to CompleteGattDisconnection() from this test simulate what the OS would do. I guess I'm wondering if calling device->CreateGattConnection(); device->Disconnect() results in 1==gatt_connection_attempt_count_==gatt_disconnection_attempt_count_, which might cause the OS to CompleteGattDisconnection() before it ever finishes connecting. But I'm not sure that's worth changing in the test. https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:298: EXPECT_FALSE(connection->IsConnected()); On 2015/09/20 02:53:35, scheib wrote: > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > These were already false before the CompleteGattDisconnection() call, right? > > It's only device->IsGattConnected() that I'd expect to change there. > > The last connection reports connected. (It connected after the others > disconnected and initiated the disconnect request but before the device actually > disconnected). Hm, that's odd. It means that if we close some connections from some tabs, then open a connection from another tab, that tab can see the connection open and then spontaneously close, without the device actually having gone out of range. Should we serialize this a bit and try to give that new tab an actually-open connection?
https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:277: CompleteGattDisconnection(device); On 2015/09/22 01:22:10, Jeffrey Yasskin wrote: > On 2015/09/20 02:53:34, scheib wrote: > > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > > Does this happen naturally if you call device->Disconnect() before the gatt > > > connection completes? > > > > I was planning on not failing a connect until the device state actually > changes. > > If a connect, then disconnect are sent then what is the state of the device? I > > don't think we should fire error or disconnected callbacks until the device > > actually transitions to those states. What if the device does connect in that > > circumstance but doesn't disconnect for whatever reason? > > > > Put another way, I don't think we should make any speculative logic about what > a > > device may actually do; all state and callbacks should reflect OS reported > > changes in device state. > > Right, that's fine. Calls to CompleteGattDisconnection() from this test simulate > what the OS would do. I guess I'm wondering if calling > device->CreateGattConnection(); device->Disconnect() results in > 1==gatt_connection_attempt_count_==gatt_disconnection_attempt_count_, which > might cause the OS to CompleteGattDisconnection() before it ever finishes > connecting. > > But I'm not sure that's worth changing in the test. I've added 2 more test blocks: device->CreateGattConnection(); device->Disconnect(); Connected. device->CreateGattConnection(); device->Disconnect(); Disconnected. https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:298: EXPECT_FALSE(connection->IsConnected()); On 2015/09/22 01:22:10, Jeffrey Yasskin wrote: > On 2015/09/20 02:53:35, scheib wrote: > > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > > These were already false before the CompleteGattDisconnection() call, right? > > > It's only device->IsGattConnected() that I'd expect to change there. > > > > The last connection reports connected. (It connected after the others > > disconnected and initiated the disconnect request but before the device > actually > > disconnected). > > Hm, that's odd. It means that if we close some connections from some tabs, then > open a connection from another tab, that tab can see the connection open and > then spontaneously close, without the device actually having gone out of range. > Should we serialize this a bit and try to give that new tab an actually-open > connection? I'm sticking with no. We can layer queuing on top of things but then we have to invent timeouts. Connections & Disconnections happen. Code has to observe, tolerate, and respond accordingly even if we did have queues. Applications that connect may see a disconnection, and they must handle it. It's possible that we could add a lot of code and complexity to try to not tell an app that a connection that we plan to disconnect is active, and instead wait, but - other edge cases arise (the time out scenarios) anyway. - frequency of this happening is expected to be very, very low.
lgtm with 2 comments. Thanks! https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:298: EXPECT_FALSE(connection->IsConnected()); On 2015/09/22 18:21:48, scheib wrote: > On 2015/09/22 01:22:10, Jeffrey Yasskin wrote: > > On 2015/09/20 02:53:35, scheib wrote: > > > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > > > These were already false before the CompleteGattDisconnection() call, > right? > > > > It's only device->IsGattConnected() that I'd expect to change there. > > > > > > The last connection reports connected. (It connected after the others > > > disconnected and initiated the disconnect request but before the device > > actually > > > disconnected). > > > > Hm, that's odd. It means that if we close some connections from some tabs, > then > > open a connection from another tab, that tab can see the connection open and > > then spontaneously close, without the device actually having gone out of > range. > > Should we serialize this a bit and try to give that new tab an actually-open > > connection? > > I'm sticking with no. We can layer queuing on top of things but then we have to > invent timeouts. Connections & Disconnections happen. Code has to observe, > tolerate, and respond accordingly even if we did have queues. Applications that > connect may see a disconnection, and they must handle it. It's possible that we > could add a lot of code and complexity to try to not tell an app that a > connection that we plan to disconnect is active, and instead wait, but > - other edge cases arise (the time out scenarios) anyway. > - frequency of this happening is expected to be very, very low. I'm happy to deal with this later, but can you file a bug so we come back to it? The low frequency of errors (debugging clues) makes me more worried, not less. https://codereview.chromium.org/1256313002/diff/260001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/260001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:314: // CreateGattConnection & DisconnectGatt, then simulate connection. This test has gotten very long. Can you split it into several TEST_F()s so that it's easier to be sure what the state is at every point? It'd be fine to add a helper function for the initial setup.
https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/240001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:298: EXPECT_FALSE(connection->IsConnected()); On 2015/09/23 00:11:11, Jeffrey Yasskin wrote: > On 2015/09/22 18:21:48, scheib wrote: > > On 2015/09/22 01:22:10, Jeffrey Yasskin wrote: > > > On 2015/09/20 02:53:35, scheib wrote: > > > > On 2015/09/16 23:42:49, Jeffrey Yasskin wrote: > > > > > These were already false before the CompleteGattDisconnection() call, > > right? > > > > > It's only device->IsGattConnected() that I'd expect to change there. > > > > > > > > The last connection reports connected. (It connected after the others > > > > disconnected and initiated the disconnect request but before the device > > > actually > > > > disconnected). > > > > > > Hm, that's odd. It means that if we close some connections from some tabs, > > then > > > open a connection from another tab, that tab can see the connection open and > > > then spontaneously close, without the device actually having gone out of > > range. > > > Should we serialize this a bit and try to give that new tab an actually-open > > > connection? > > > > I'm sticking with no. We can layer queuing on top of things but then we have > to > > invent timeouts. Connections & Disconnections happen. Code has to observe, > > tolerate, and respond accordingly even if we did have queues. Applications > that > > connect may see a disconnection, and they must handle it. It's possible that > we > > could add a lot of code and complexity to try to not tell an app that a > > connection that we plan to disconnect is active, and instead wait, but > > - other edge cases arise (the time out scenarios) anyway. > > - frequency of this happening is expected to be very, very low. > > I'm happy to deal with this later, but can you file a bug so we come back to it? > The low frequency of errors (debugging clues) makes me more worried, not less. armansito, krstmnlsn and I discussed this at length, and I thought I had reviewed that some with you as well. I've filed https://code.google.com/p/chromium/issues/detail?id=535063 if you'd like to discuss further, but I think the alternatives only cause more issues - and even more unpredictably than the scenario you describe above. https://codereview.chromium.org/1256313002/diff/260001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1256313002/diff/260001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:314: // CreateGattConnection & DisconnectGatt, then simulate connection. On 2015/09/23 00:11:11, Jeffrey Yasskin wrote: > This test has gotten very long. Can you split it into several TEST_F()s so that > it's easier to be sure what the state is at every point? It'd be fine to add a > helper function for the initial setup. Done. I already have a clean up CL, I'll merge this into that one and simplify the boilerplate.
lgtm
The CQ bit was checked by scheib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1256313002/#ps280001 (title: "Split tests up into smaller tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256313002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256313002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was unchecked by commit-bot@chromium.org
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/1256313002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256313002/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/af7ea91977473d1ae331a6052ca49003134d405e Cr-Commit-Position: refs/heads/master@{#350359} |
