Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706763002:
bluetooth: Increase min api for Android (Closed)
Descriptionbluetooth: Increase min api for Android
In order to be able to use new features of the BLE API this patch increases the min required version.
Also makes use of a >M API to specify preferred transport method.
Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low.
BUG=694332
Review-Url: https://codereview.chromium.org/2706763002
Cr-Commit-Position: refs/heads/master@{#465052}
Committed: https://chromium.googlesource.com/chromium/src/+/049e6ec2d2ce7baf077601acf2755eb0cb0eafe3
Patch Set 1 #Patch Set 2 : bluetooth: Set min version to M #
Total comments: 2
Patch Set 3 : bluetooth: Increase min API #
Total comments: 3
Patch Set 4 : Increase min API for other classes #
Messages
Total messages: 34 (23 generated)
Description was changed from ========== bluetooth: Android: Prefer LE when connecting to dual mode devices Due to lower energy consumption we prefer to connect over LE on dual mode devices. BUG=693937 ========== to ========== DO NOT SUMMIT bluetooth: Increase min api 3 changes to see how bots react: 1. Raise the min API from 21 to 23 2. Use a function from API 23 3. Break a test to make sure tests are actually running. BUG=693937 ==========
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
scheib@chromium.org changed reviewers: + scheib@chromium.org
https://codereview.chromium.org/2706763002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2706763002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1260: EXPECT_EQ(0, gatt_discovery_attempts_); Hmm, know why this changed?
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2706763002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2706763002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1260: EXPECT_EQ(0, gatt_discovery_attempts_); On 2017/04/08 at 04:05:34, scheib wrote: > Hmm, know why this changed? Should've added a comment. This change just breaks the test to make sure the tests will run on the trybots.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== DO NOT SUMMIT bluetooth: Increase min api 3 changes to see how bots react: 1. Raise the min API from 21 to 23 2. Use a function from API 23 3. Break a test to make sure tests are actually running. BUG=693937 ========== to ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=693937 ==========
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scheib: PTAL
Oh, we should update the upfront check, to ensure we don't try to execute the java code that only works on M. This line in particular: https://cs.chromium.org/chromium/src/device/bluetooth/android/java/src/org/ch... But, probably all instances of "Build.VERSION_CODES.LOLLIPOP" should be removed under device/bluetooth. https://cs.chromium.org/search/?q=f:device/bluetooth+Build.VERSION_CODES.LOLL... Also, unless another client was relying on this, the uses-permission ... BLUETOOTH lines from the manifest can be removed. Perhaps as a follow up patch to make reverting easy in case we goof it. https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?... https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:112: // Prefer LE for dual-mode devices due to lower energy consumption. Let's explain that we don't have any use of Bluetooth classic, the only bluetooth client on Android so far is GATT for Web Bluetooth.
> Oh, we should update the upfront check, to ensure we don't try to execute the java code that only works on M. > > This line in particular: > https://cs.chromium.org/chromium/src/device/bluetooth/android/java/src/org/ch... > This patch already changed that line. Did you mean a different line? > But, probably all instances of "Build.VERSION_CODES.LOLLIPOP" should be removed under device/bluetooth. > https://cs.chromium.org/search/?q=f:device/bluetooth+Build.VERSION_CODES.LOLL... > ChromeBluetoothAdapter and ChromeBluetoothRemoteGattCharacteristic changed. > Also, unless another client was relying on this, the > uses-permission ... BLUETOOTH > lines from the manifest can be removed. Perhaps as a follow up patch to make reverting easy in case we goof it. > https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?... Will do in follow up. https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:112: // Prefer LE for dual-mode devices due to lower energy consumption. On 2017/04/11 at 18:08:19, scheib wrote: > Let's explain that we don't have any use of Bluetooth classic, the only bluetooth client on Android so far is GATT for Web Bluetooth. I am tad confused here 🤔. What exactly should we comment? This is only about the preferred transport when establishing a GATT connection. It shouldn't matter that Web Bluetooth is the only client. Unlike our change to filter out non-LE devices.
Description was changed from ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=693937 ========== to ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=694332 ==========
On 2017/04/11 23:12:47, ortuno wrote: > > Oh, we should update the upfront check, to ensure we don't try to execute the > java code that only works on M. > > > > This line in particular: > > > https://cs.chromium.org/chromium/src/device/bluetooth/android/java/src/org/ch... > > > > This patch already changed that line. Did you mean a different line? Oops. I was thinking about the manifest, then thought about the check, and had not been attentive to the change already being done.
lgtm https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... File device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java (right): https://codereview.chromium.org/2706763002/diff/40001/device/bluetooth/androi... device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:112: // Prefer LE for dual-mode devices due to lower energy consumption. On 2017/04/11 23:12:47, ortuno wrote: > On 2017/04/11 at 18:08:19, scheib wrote: > > Let's explain that we don't have any use of Bluetooth classic, the only > bluetooth client on Android so far is GATT for Web Bluetooth. > > I am tad confused here 🤔. What exactly should we comment? This is only about the > preferred transport when establishing a GATT connection. It shouldn't matter > that Web Bluetooth is the only client. Unlike our change to filter out non-LE > devices. Oops, I had forgotten this was just the preferred transport, not a selection of only doing one kind of transport. Fine as is.
Thanks! If it's OK with you I'll land this after branch point.
On 2017/04/12 01:33:33, ortuno wrote: > Thanks! If it's OK with you I'll land this after branch point. SGTM
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ortuno@chromium.org
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492469265379960,
"parent_rev": "487d548c3639002dbee792c672e0460da257bb17", "commit_rev":
"049e6ec2d2ce7baf077601acf2755eb0cb0eafe3"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=694332 ========== to ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=694332 Review-Url: https://codereview.chromium.org/2706763002 Cr-Commit-Position: refs/heads/master@{#465052} Committed: https://chromium.googlesource.com/chromium/src/+/049e6ec2d2ce7baf077601acf275... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/049e6ec2d2ce7baf077601acf275...
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=694332 Review-Url: https://codereview.chromium.org/2706763002 Cr-Commit-Position: refs/heads/master@{#465052} Committed: https://chromium.googlesource.com/chromium/src/+/049e6ec2d2ce7baf077601acf275... ========== to ========== bluetooth: Increase min api for Android In order to be able to use new features of the BLE API this patch increases the min required version. Also makes use of a >M API to specify preferred transport method. Users on Android L would have had to find an install chromium in order to use this API so chances are the usage was very low. BUG=694332 Review-Url: https://codereview.chromium.org/2706763002 Cr-Commit-Position: refs/heads/master@{#465052} Committed: https://chromium.googlesource.com/chromium/src/+/049e6ec2d2ce7baf077601acf275... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
