Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1215303006:
bluetooth: android: Initial BluetoothDeviceAndroid implementation. (Closed)
Descriptionbluetooth: android: Initial BluetoothDeviceAndroid implementation.
Devices can now be discovered and have basic data fields accessed:
GetBluetoothClass(), GetAddress(), GetName(), IsPaired(), GetUUIDs().
Some data fields are not supported on Android, and return 0:
GetVendorIDSource(), GetVendorID(), GetProductID(), GetDeviceID()
BUG=488579, 488575
Committed: https://crrev.com/59e28a2e1672ade6d9e08e7e1699b47f0851f0df
Cr-Commit-Position: refs/heads/master@{#338604}
Patch Set 1 : #
Total comments: 20
Patch Set 2 : jyasskin comments addressed. Pass UUIDs by array. Added more tests. #
Total comments: 20
Patch Set 3 : Fixes from upstream patch; addressed jyasskin request for type comments. #Patch Set 4 : Merge TOT #
Total comments: 2
Patch Set 5 : armansito addressed: TODO for UUID merge #Patch Set 6 : Split out string changes to https://codereview.chromium.org/1224273008/ #Patch Set 7 : Merge TOT #Depends on Patchset: Messages
Total messages: 33 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
scheib@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin, please take a first pass review.
https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:94: for (int i = 0; i < number_of_uuids; i++) { Use AppendJavaStringArrayToStringVector instead.
lgtm https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:246: // Binds to BluetoothAdapterAndroid::GetChromeBluetoothDevice. Why does "CreateOrUpdateDeviceOnScan" bind to "GetChromeBluetoothDevice"? https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:247: // 'Object' type must be used because inner class Wrappers.BluetoothDeviceWrapper reference is "must be used for |bluetoothDeviceWrapper| because" https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:250: String jaddress, Object bluetoothDeviceWrapper, List<ParcelUuid> advertisedUuids); "jaddress"? https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:76: // Implements BluetoothDeviceAndroid::GetUUIDs. More "Used in implementation of GetUUIDs", but I guess you're switching this to return an array of strings? https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.cc (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.cc:137: jobject obj, |obj| isn't used? Wildly guessing at why it's there, does it go away if you make 'nativeCreateOrUpdateDeviceOnScan' static? ... http://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/design.html#wp715 says you get a reference to the class if you make it static, which isn't really an improvement. Oh well. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.h (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:30: // Java class BluetoothAdapter via j_adapter_. A tree of additional C++ objects "The adapter also owns a tree of ..." https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:91: jobject obj, What's |obj|? Maybe comment the expected types for all three jobjects? https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/test/a... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/test/a... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:181: return ADDRESS; We're going to need to be able to return multiple devices and check that, e.g. an update to one gets routed to the right BluetoothDeviceAndroid. I guess you don't need that in this patch.
https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:246: // Binds to BluetoothAdapterAndroid::GetChromeBluetoothDevice. On 2015/07/07 00:11:13, Jeffrey Yasskin wrote: > Why does "CreateOrUpdateDeviceOnScan" bind to "GetChromeBluetoothDevice"? Done. - failed to update comment after updating method name. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:247: // 'Object' type must be used because inner class Wrappers.BluetoothDeviceWrapper reference is On 2015/07/07 00:11:13, Jeffrey Yasskin wrote: > "must be used for |bluetoothDeviceWrapper| because" Done. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:250: String jaddress, Object bluetoothDeviceWrapper, List<ParcelUuid> advertisedUuids); On 2015/07/07 00:11:13, Jeffrey Yasskin wrote: > "jaddress"? Done. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:76: // Implements BluetoothDeviceAndroid::GetUUIDs. On 2015/07/07 00:11:13, Jeffrey Yasskin wrote: > More "Used in implementation of GetUUIDs", but I guess you're switching this to > return an array of strings? True, yes. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.cc (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.cc:137: jobject obj, On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > |obj| isn't used? Wildly guessing at why it's there, does it go away if you make > 'nativeCreateOrUpdateDeviceOnScan' static? ... > http://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/design.html#wp715 says > you get a reference to the class if you make it static, which isn't really an > improvement. Oh well. :( Yes, this seems unfortunate that JNI presumes this on all signatures instead of having Java calling code provide object references explicitly when needed. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.h (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:30: // Java class BluetoothAdapter via j_adapter_. A tree of additional C++ objects On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > "The adapter also owns a tree of ..." Done. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:91: jobject obj, On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > What's |obj|? Maybe comment the expected types for all three jobjects? The first two parameters are standard JNI form. I've kicked off https://codereview.chromium.org/1212443006/ to improve that. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:94: for (int i = 0; i < number_of_uuids; i++) { On 2015/07/06 23:10:43, scheib wrote: > Use AppendJavaStringArrayToStringVector instead. Done. https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/test/a... File device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/test/a... device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:181: return ADDRESS; On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > We're going to need to be able to return multiple devices and check that, e.g. > an update to one gets routed to the right BluetoothDeviceAndroid. I guess you > don't need that in this patch. Added multiple addresses and tests that use them.
scheib@chromium.org changed reviewers: + armansito@chromium.org
Thanks jyasskin. +armansito. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: armansito, confirm this is what you believe should happen if AD packets stop including UUIDs.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 00:54:09, scheib wrote: > armansito, confirm this is what you believe should happen if AD packets stop > including UUIDs. On Chrome OS this will be populated with the complete list of UUIDs after we perform service discovery over GATT, so GetUUIDs should also behave that way on Android (so don't only return them based on scan results). I believe http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... does exactly that. So you should combine that list with mUuidsFromScan in ChromeBluetoothDevice.java.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 03:12:09, armansito wrote: > On 2015/07/08 00:54:09, scheib wrote: > > armansito, confirm this is what you believe should happen if AD packets stop > > including UUIDs. > > On Chrome OS this will be populated with the complete list of UUIDs after we > perform service discovery over GATT, so GetUUIDs should also behave that way on > Android (so don't only return them based on scan results). I believe > http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... > does exactly that. So you should combine that list with mUuidsFromScan in > ChromeBluetoothDevice.java. On Android, device.getUuids() will be empty unless we have explicitly requested service discovery via fetchUuidsWithSdp() and waited for results. I see reports on e.g. stack overflow that this will only work if a scan is stopped and then SDP performed. I'm perhaps surprised that ChromeOS would do a SDP before higher level code connects to a device. Isn't SDP fairly expensive? Also, from a Web point of view, if a site starts a scan with requestDevice and we collect only AD packets, aren't we able to divulge less information, e.g. is a scan for AD packets passive only? If we perform SDP then a site can cause a user's computer to connect with devices (granted, a user must have clicked on the site).
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 05:09:10, scheib wrote: > On 2015/07/08 03:12:09, armansito wrote: > > On 2015/07/08 00:54:09, scheib wrote: > > > armansito, confirm this is what you believe should happen if AD packets stop > > > including UUIDs. > > > > On Chrome OS this will be populated with the complete list of UUIDs after we > > perform service discovery over GATT, so GetUUIDs should also behave that way > on > > Android (so don't only return them based on scan results). I believe > > > http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... > > does exactly that. So you should combine that list with mUuidsFromScan in > > ChromeBluetoothDevice.java. > > On Android, device.getUuids() will be empty unless we have explicitly requested > service discovery via fetchUuidsWithSdp() and waited for results. I see reports > on e.g. stack overflow that this will only work if a scan is stopped and then > SDP performed. > > I'm perhaps surprised that ChromeOS would do a SDP before higher level code > connects to a device. Isn't SDP fairly expensive? > > Also, from a Web point of view, if a site starts a scan with requestDevice and > we collect only AD packets, aren't we able to divulge less information, e.g. is > a scan for AD packets passive only? If we perform SDP then a site can cause a > user's computer to connect with devices (granted, a user must have clicked on > the site). SDP is a Bluetooth classic protocol, so it wouldn't be done in this case (though I would expect getUuids() to contain services from GATT as well). Regardless of LE or classic, upon connection all services will be discovered via SDP or GATT. This is just done normally after you've scanned for devices and decided to connect to one. Both on Android and Chrome OS, GATT service discovery is always performed when the ATT bearer is connected. Until that's done you get the UUIDs from AD or scan response but once discovery is done the whole list is populated; that's about it. So for example you do discovery ad you get one or two UUIDs from GetUUIDs(). Then you connect, now the list has 4-5 UUIDs.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 06:00:13, armansito wrote: > On 2015/07/08 05:09:10, scheib wrote: > > On 2015/07/08 03:12:09, armansito wrote: > > > On 2015/07/08 00:54:09, scheib wrote: > > > > armansito, confirm this is what you believe should happen if AD packets > stop > > > > including UUIDs. > > > > > > On Chrome OS this will be populated with the complete list of UUIDs after we > > > perform service discovery over GATT, so GetUUIDs should also behave that way > > on > > > Android (so don't only return them based on scan results). I believe > > > > > > http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... > > > does exactly that. So you should combine that list with mUuidsFromScan in > > > ChromeBluetoothDevice.java. > > > > On Android, device.getUuids() will be empty unless we have explicitly > requested > > service discovery via fetchUuidsWithSdp() and waited for results. I see > reports > > on e.g. stack overflow that this will only work if a scan is stopped and then > > SDP performed. > > > > I'm perhaps surprised that ChromeOS would do a SDP before higher level code > > connects to a device. Isn't SDP fairly expensive? > > > > Also, from a Web point of view, if a site starts a scan with requestDevice and > > we collect only AD packets, aren't we able to divulge less information, e.g. > is > > a scan for AD packets passive only? If we perform SDP then a site can cause a > > user's computer to connect with devices (granted, a user must have clicked on > > the site). > > SDP is a Bluetooth classic protocol, so it wouldn't be done in this case (though > I would expect getUuids() to contain services from GATT as well). Regardless of > LE or classic, upon connection all services will be discovered via SDP or GATT. > This is just done normally after you've scanned for devices and decided to > connect to one. > > Both on Android and Chrome OS, GATT service discovery is always performed when > the ATT bearer is connected. Until that's done you get the UUIDs from AD or scan > response but once discovery is done the whole list is populated; that's about > it. So for example you do discovery ad you get one or two UUIDs from GetUUIDs(). > Then you connect, now the list has 4-5 UUIDs. OK, to be clear, upon a DiscoverySession we only expect to report UUIDS from AD packets. We should not, just because of a DiscoverySession, connect and discover services. (I see API for that in Android: connectGATT, discoverServices). However, once connected GetUUIDs should report the merged list of UUIDs from AD packets and discovered services. For THIS patch, where we do not yet connect, should the UUIDs from AD packets be cleared if a new AD packet is seen with different or empty UUIDs?
https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.h (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:91: jobject obj, On 2015/07/08 00:49:10, scheib wrote: > On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > > What's |obj|? Maybe comment the expected types for all three jobjects? > > The first two parameters are standard JNI form. I've kicked off > https://codereview.chromium.org/1212443006/ to improve that. Cool, thanks. Should bluetooth_device_wrapper and advertised_uuids also mention their expected types? https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:83: return string_array; Just "return mUuidsFromScan.toArray(new String[mUuidsFromScan.size()]);" "return mUuidsFromScan.toArray(new String[0]);" would also work. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { I'm not sure I like the wrapper having logic in it, even as simple as creating a ScanSettings. I'm not dead-set against it though. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 16:42:51, scheib wrote: > On 2015/07/08 06:00:13, armansito wrote: > > On 2015/07/08 05:09:10, scheib wrote: > > > On 2015/07/08 03:12:09, armansito wrote: > > > > On 2015/07/08 00:54:09, scheib wrote: > > > > > armansito, confirm this is what you believe should happen if AD packets > > stop > > > > > including UUIDs. > > > > > > > > On Chrome OS this will be populated with the complete list of UUIDs after > we > > > > perform service discovery over GATT, so GetUUIDs should also behave that > way > > > on > > > > Android (so don't only return them based on scan results). I believe > > > > > > > > > > http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... > > > > does exactly that. So you should combine that list with mUuidsFromScan in > > > > ChromeBluetoothDevice.java. > > > > > > On Android, device.getUuids() will be empty unless we have explicitly > > requested > > > service discovery via fetchUuidsWithSdp() and waited for results. I see > > reports > > > on e.g. stack overflow that this will only work if a scan is stopped and > then > > > SDP performed. > > > > > > I'm perhaps surprised that ChromeOS would do a SDP before higher level code > > > connects to a device. Isn't SDP fairly expensive? > > > > > > Also, from a Web point of view, if a site starts a scan with requestDevice > and > > > we collect only AD packets, aren't we able to divulge less information, e.g. > > is > > > a scan for AD packets passive only? If we perform SDP then a site can cause > a > > > user's computer to connect with devices (granted, a user must have clicked > on > > > the site). > > > > SDP is a Bluetooth classic protocol, so it wouldn't be done in this case > (though > > I would expect getUuids() to contain services from GATT as well). Regardless > of > > LE or classic, upon connection all services will be discovered via SDP or > GATT. > > This is just done normally after you've scanned for devices and decided to > > connect to one. > > > > Both on Android and Chrome OS, GATT service discovery is always performed when > > the ATT bearer is connected. Until that's done you get the UUIDs from AD or > scan > > response but once discovery is done the whole list is populated; that's about > > it. So for example you do discovery ad you get one or two UUIDs from > GetUUIDs(). > > Then you connect, now the list has 4-5 UUIDs. > > OK, to be clear, upon a DiscoverySession we only expect to report UUIDS from AD > packets. We should not, just because of a DiscoverySession, connect and discover > services. (I see API for that in Android: connectGATT, discoverServices). > > However, once connected GetUUIDs should report the merged list of UUIDs from AD > packets and discovered services. > > For THIS patch, where we do not yet connect, should the UUIDs from AD packets be > cleared if a new AD packet is seen with different or empty UUIDs? You're not really testing that situation here. You're faking the BLE device above the OS layer, so you're directly controlling what UUIDs it returns. With this sort of test, we can't test how the OS responds to a device interleaving multiple different AD packets; we'd need a real radio for that. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:94: AppendJavaStringArrayToStringVector( Returning the array is an improvement, but maybe iterating over it yourself makes sense, since you need the loop to convert it to BluetoothUUIDs anyway.
https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_android.h (right): https://codereview.chromium.org/1215303006/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_android.h:91: jobject obj, On 2015/07/08 17:19:38, Jeffrey Yasskin wrote: > On 2015/07/08 00:49:10, scheib wrote: > > On 2015/07/07 00:11:14, Jeffrey Yasskin wrote: > > > What's |obj|? Maybe comment the expected types for all three jobjects? > > > > The first two parameters are standard JNI form. I've kicked off > > https://codereview.chromium.org/1212443006/ to improve that. > > Cool, thanks. > > Should bluetooth_device_wrapper and advertised_uuids also mention their expected > types? OK. /shrug/ this is a bit of a mess, and the C++ code only passes these objects through, but why not. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:83: return string_array; On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > Just "return mUuidsFromScan.toArray(new String[mUuidsFromScan.size()]);" > > "return mUuidsFromScan.toArray(new String[0]);" would also work. Doesn't work. These are ParcelUuids and don't convert to String without calling toString() explicitly. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > I'm not sure I like the wrapper having logic in it, even as simple as creating a > ScanSettings. > > I'm not dead-set against it though. The alternative I thought of is to create a ScanSettingsWrapper that internally has logic to capture options and then build an Android ScanSettings... by the time I got that far I realized I had logic anyway and might as well do the same without building a wrapper class to do it. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > On 2015/07/08 16:42:51, scheib wrote: > > On 2015/07/08 06:00:13, armansito wrote: > > > On 2015/07/08 05:09:10, scheib wrote: > > > > On 2015/07/08 03:12:09, armansito wrote: > > > > > On 2015/07/08 00:54:09, scheib wrote: > > > > > > armansito, confirm this is what you believe should happen if AD > packets > > > stop > > > > > > including UUIDs. > > > > > > > > > > On Chrome OS this will be populated with the complete list of UUIDs > after > > we > > > > > perform service discovery over GATT, so GetUUIDs should also behave that > > way > > > > on > > > > > Android (so don't only return them based on scan results). I believe > > > > > > > > > > > > > > > http://developer.android.com/reference/android/bluetooth/BluetoothDevice.html... > > > > > does exactly that. So you should combine that list with mUuidsFromScan > in > > > > > ChromeBluetoothDevice.java. > > > > > > > > On Android, device.getUuids() will be empty unless we have explicitly > > > requested > > > > service discovery via fetchUuidsWithSdp() and waited for results. I see > > > reports > > > > on e.g. stack overflow that this will only work if a scan is stopped and > > then > > > > SDP performed. > > > > > > > > I'm perhaps surprised that ChromeOS would do a SDP before higher level > code > > > > connects to a device. Isn't SDP fairly expensive? > > > > > > > > Also, from a Web point of view, if a site starts a scan with requestDevice > > and > > > > we collect only AD packets, aren't we able to divulge less information, > e.g. > > > is > > > > a scan for AD packets passive only? If we perform SDP then a site can > cause > > a > > > > user's computer to connect with devices (granted, a user must have clicked > > on > > > > the site). > > > > > > SDP is a Bluetooth classic protocol, so it wouldn't be done in this case > > (though > > > I would expect getUuids() to contain services from GATT as well). Regardless > > of > > > LE or classic, upon connection all services will be discovered via SDP or > > GATT. > > > This is just done normally after you've scanned for devices and decided to > > > connect to one. > > > > > > Both on Android and Chrome OS, GATT service discovery is always performed > when > > > the ATT bearer is connected. Until that's done you get the UUIDs from AD or > > scan > > > response but once discovery is done the whole list is populated; that's > about > > > it. So for example you do discovery ad you get one or two UUIDs from > > GetUUIDs(). > > > Then you connect, now the list has 4-5 UUIDs. > > > > OK, to be clear, upon a DiscoverySession we only expect to report UUIDS from > AD > > packets. We should not, just because of a DiscoverySession, connect and > discover > > services. (I see API for that in Android: connectGATT, discoverServices). > > > > However, once connected GetUUIDs should report the merged list of UUIDs from > AD > > packets and discovered services. > > > > For THIS patch, where we do not yet connect, should the UUIDs from AD packets > be > > cleared if a new AD packet is seen with different or empty UUIDs? > > You're not really testing that situation here. You're faking the BLE device > above the OS layer, so you're directly controlling what UUIDs it returns. With > this sort of test, we can't test how the OS responds to a device interleaving > multiple different AD packets; we'd need a real radio for that. I don't know what Android does for sure. Our test peripheral changes UUIDs, but when it does it also gets a new address. However, this test should cover all possible situations (changing, and changing to empty), even if Android isn't that harsh to us. So, for the sake of determining what to do in the harshest case, what should we do if Android updates to sending us an empty list? I think we discard the old AD UUIDs, but speak up if you think otherwise. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:94: AppendJavaStringArrayToStringVector( On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > Returning the array is an improvement, but maybe iterating over it yourself > makes sense, since you need the loop to convert it to BluetoothUUIDs anyway. I thought about it, and I'm hesitant to duplicate the code to do so, as it has some details and I don't think we're incurring much performance loss here. This code is easier to understand with the JNI details encapsulated within AppendJavaStringArrayToStringVector.
lgtm still. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:83: return string_array; On 2015/07/08 23:02:34, scheib wrote: > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > Just "return mUuidsFromScan.toArray(new String[mUuidsFromScan.size()]);" > > > > "return mUuidsFromScan.toArray(new String[0]);" would also work. > > Doesn't work. These are ParcelUuids and don't convert to String without calling > toString() explicitly. Oops. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { On 2015/07/08 23:02:35, scheib wrote: > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > I'm not sure I like the wrapper having logic in it, even as simple as creating > a > > ScanSettings. > > > > I'm not dead-set against it though. > > The alternative I thought of is to create a ScanSettingsWrapper that internally > has logic to capture options and then build an Android ScanSettings... by the > time I got that far I realized I had logic anyway and might as well do the same > without building a wrapper class to do it. You don't need to fake the ScanSettings, since it's just a value object. The test can assert against https://developer.android.com/reference/android/bluetooth/le/ScanSettings.htm.... That said, I still don't hate what you have. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:557: // Expect empty UUIDs: On 2015/07/08 23:02:35, scheib wrote: > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > You're not really testing that situation here. You're faking the BLE device > > above the OS layer, so you're directly controlling what UUIDs it returns. With > > this sort of test, we can't test how the OS responds to a device interleaving > > multiple different AD packets; we'd need a real radio for that. > > I don't know what Android does for sure. Our test peripheral changes UUIDs, but > when it does it also gets a new address. However, this test should cover all > possible situations (changing, and changing to empty), even if Android isn't > that harsh to us. > > So, for the sake of determining what to do in the harshest case, what should we > do if Android updates to sending us an empty list? I think we discard the old AD > UUIDs, but speak up if you think otherwise. I think that's right, until someone files a bug with a reason otherwise. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:94: AppendJavaStringArrayToStringVector( On 2015/07/08 23:02:35, scheib wrote: > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > Returning the array is an improvement, but maybe iterating over it yourself > > makes sense, since you need the loop to convert it to BluetoothUUIDs anyway. > > I thought about it, and I'm hesitant to duplicate the code to do so, as it has > some details and I don't think we're incurring much performance loss here. This > code is easier to understand with the JNI details encapsulated within > AppendJavaStringArrayToStringVector. SGTM.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { On 2015/07/08 23:18:24, Jeffrey Yasskin wrote: > On 2015/07/08 23:02:35, scheib wrote: > > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > > I'm not sure I like the wrapper having logic in it, even as simple as > creating > > a > > > ScanSettings. > > > > > > I'm not dead-set against it though. > > > > The alternative I thought of is to create a ScanSettingsWrapper that > internally > > has logic to capture options and then build an Android ScanSettings... by the > > time I got that far I realized I had logic anyway and might as well do the > same > > without building a wrapper class to do it. > > You don't need to fake the ScanSettings, since it's just a value object. The > test can assert against > https://developer.android.com/reference/android/bluetooth/le/ScanSettings.htm.... > That said, I still don't hate what you have. Previous code that used ScanSettings while using Fakes threw an exception, NoClassDefFoundError IIRC. When using Fakes we can't instantiate a ScanSettings, so I included it only in Wrapper code that runs when not under test.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { On 2015/07/08 23:29:05, scheib wrote: > On 2015/07/08 23:18:24, Jeffrey Yasskin wrote: > > On 2015/07/08 23:02:35, scheib wrote: > > > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > > > I'm not sure I like the wrapper having logic in it, even as simple as > > creating > > > a > > > > ScanSettings. > > > > > > > > I'm not dead-set against it though. > > > > > > The alternative I thought of is to create a ScanSettingsWrapper that > > internally > > > has logic to capture options and then build an Android ScanSettings... by > the > > > time I got that far I realized I had logic anyway and might as well do the > > same > > > without building a wrapper class to do it. > > > > You don't need to fake the ScanSettings, since it's just a value object. The > > test can assert against > > > https://developer.android.com/reference/android/bluetooth/le/ScanSettings.htm.... > > That said, I still don't hate what you have. > > Previous code that used ScanSettings while using Fakes threw an exception, > NoClassDefFoundError IIRC. When using Fakes we can't instantiate a ScanSettings, > so I included it only in Wrapper code that runs when not under test. Ah, right, none of the Android classes are available in unittest mode? This seems good then.
https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java (right): https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:135: List<ScanFilter> filters, int scanSettingsScanMode, ScanCallbackWrapper callback) { On 2015/07/09 17:04:09, Jeffrey Yasskin wrote: > On 2015/07/08 23:29:05, scheib wrote: > > On 2015/07/08 23:18:24, Jeffrey Yasskin wrote: > > > On 2015/07/08 23:02:35, scheib wrote: > > > > On 2015/07/08 17:19:39, Jeffrey Yasskin wrote: > > > > > I'm not sure I like the wrapper having logic in it, even as simple as > > > creating > > > > a > > > > > ScanSettings. > > > > > > > > > > I'm not dead-set against it though. > > > > > > > > The alternative I thought of is to create a ScanSettingsWrapper that > > > internally > > > > has logic to capture options and then build an Android ScanSettings... by > > the > > > > time I got that far I realized I had logic anyway and might as well do the > > > same > > > > without building a wrapper class to do it. > > > > > > You don't need to fake the ScanSettings, since it's just a value object. The > > > test can assert against > > > > > > https://developer.android.com/reference/android/bluetooth/le/ScanSettings.htm.... > > > That said, I still don't hate what you have. > > > > Previous code that used ScanSettings while using Fakes threw an exception, > > NoClassDefFoundError IIRC. When using Fakes we can't instantiate a > ScanSettings, > > so I included it only in Wrapper code that runs when not under test. > > Ah, right, none of the Android classes are available in unittest mode? This > seems good then. Classes introduced in later Android versions may not be available on all testing devices. For non tests, there is an Android version check when constructing the adapter, and we just don't run code if it's an old version. For tests with fakes, I want them to run on testers even if it's an old version of android. So all fake based code must not use any newer classes.
https://codereview.chromium.org/1215303006/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:79: private String[] getUuids() { Right, my previous comment was basically that GetUUIDs() should return UUIDs that are discovered through GATT/SDP once there is a connection later as well, but I don't see that addressed here. It's OK to not do it in this CL yet but please add a TODO here that you will merge the UUIDs from scan with the UUIDs from discovery later.
Thanks https://codereview.chromium.org/1215303006/diff/100001/device/bluetooth/andro... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/1215303006/diff/100001/device/bluetooth/andro... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:79: private String[] getUuids() { On 2015/07/09 18:54:22, armansito wrote: > Right, my previous comment was basically that GetUUIDs() should return UUIDs > that are discovered through GATT/SDP once there is a connection later as well, > but I don't see that addressed here. It's OK to not do it in this CL yet but > please add a TODO here that you will merge the UUIDs from scan with the UUIDs > from discovery later. Done.
Thanks, lgtm!
thakis, OWNERS review please: chrome/chrome_resources.gyp chrome/tools/build/repack_locales.py
scheib@chromium.org changed reviewers: + thakis@chromium.org
thakis, OWNERS review please: chrome/chrome_resources.gyp chrome/tools/build/repack_locales.py
On 2015/07/09 20:35:18, scheib wrote:
> thakis, OWNERS review please:
> chrome/chrome_resources.gyp
> chrome/tools/build/repack_locales.py
Do you need to change any chrome/ .gn files?
How much binary size does this add?
Since this used to be behind enable_extensions: How is this going to be exposed?
Regular web apis? ("bluetooth: android: Implement BluetoothDeviceAndroid for Web
Bluetooth's GATT's needs." is codeword-heavy enough that I don't understand what
it means
scheib@chromium.org changed reviewers: - thakis@chromium.org
Split out string changes to https://codereview.chromium.org/1224273008/
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, armansito@chromium.org Link to the patchset: https://codereview.chromium.org/1215303006/#ps160001 (title: "Merge TOT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215303006/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/59e28a2e1672ade6d9e08e7e1699b47f0851f0df Cr-Commit-Position: refs/heads/master@{#338604} |
