Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854733002:
bluetooth: macOS: BluetoothLowEnergyDeviceMac::IsConnectable() not implemented (Closed)
Descriptionbluetooth: macOS: BluetoothLowEnergyDeviceMac::IsConnectable() not implemented
Adding NOTIMPLEMENTED() to BluetoothLowEnergyDeviceMac::IsConnectable() and
removing BluetoothLowEnergyDeviceMac::connectable_.
BUG=
Review-Url: https://codereview.chromium.org/2854733002
Cr-Commit-Position: refs/heads/master@{#469458}
Committed: https://chromium.googlesource.com/chromium/src/+/c2c7f4185915b64e17ba5615e0906f4e3ab6b816
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding unittests and implementation #
Total comments: 2
Patch Set 3 : IsConnectable() not implemented #
Total comments: 3
Patch Set 4 : Removing #if defined(OS_MACOSX) from this patch, crrev.com/2863643003 #
Messages
Total messages: 25 (10 generated)
jlebel@chromium.org changed reviewers: + ortuno@chromium.org
Hello Giovanni, Can you review this small patch? Thanks,
Description was changed from ========== Removing uused connectable_ variable. BUG= ========== to ========== bluetooth: macOS: Removing used connectable_ variable. BUG= ==========
https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_low_energy_device_mac.mm:103: return true; Do we need this to return true? Otherwise just add a NOTIMPLEMENTED and return false. Some devices are not connectable so we shouldn't always return true.
On 2017/05/02 01:32:16, ortuno wrote: > https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... > device/bluetooth/bluetooth_low_energy_device_mac.mm:103: return true; > Do we need this to return true? Otherwise just add a NOTIMPLEMENTED and return > false. Some devices are not connectable so we shouldn't always return true. Do we have an example of a bluetooth low energy device on the mac that is not connectable? I'm not sure with the current code this exist.
On 2017/05/02 at 08:28:39, jlebel wrote: > On 2017/05/02 01:32:16, ortuno wrote: > > https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... > > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > > > https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... > > device/bluetooth/bluetooth_low_energy_device_mac.mm:103: return true; > > Do we need this to return true? Otherwise just add a NOTIMPLEMENTED and return > > false. Some devices are not connectable so we shouldn't always return true. > > Do we have an example of a bluetooth low energy device on the mac that is not connectable? I'm not sure with the current code > this exist. Eddystone beacons are non-connectable when not in config mode. There might be other devices that are non-connectable, they usually indicate it by not advertising the connectable flag: https://developer.apple.com/reference/corebluetooth/cbadvertisementdataisconn...
Description was changed from ========== bluetooth: macOS: Removing used connectable_ variable. BUG= ========== to ========== bluetooth: macOS: Implementing BluetoothLowEnergyDeviceMac::IsConnectable(). BUG= ==========
Hello Giovanni, Is this unit test implemented correctly? Thanks, https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2854733002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_low_energy_device_mac.mm:103: return true; On 2017/05/02 01:32:16, ortuno wrote: > Do we need this to return true? Otherwise just add a NOTIMPLEMENTED and return > false. Some devices are not connectable so we shouldn't always return true. Done.
https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:32: connectable_(connectable), Thanks for implementing this. I think we want to keep this method as NOTIMPLEMENTED though. The feature is not supported on Windows and Android so we can't add it to the API :(. It's only there because the APIs are based on BlueZ.
On 2017/05/04 at 00:35:37, ortuno wrote: > https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_low_energy_device_mac.mm:32: connectable_(connectable), > Thanks for implementing this. I think we want to keep this method as NOTIMPLEMENTED though. The feature is not supported on Windows and Android so we can't add it to the API :(. It's only there because the APIs are based on BlueZ. Adding a comment to the method explaining that it's only available on Chrome OS would be a good idea.
Description was changed from ========== bluetooth: macOS: Implementing BluetoothLowEnergyDeviceMac::IsConnectable(). BUG= ========== to ========== bluetooth: macOS: BluetoothLowEnergyDeviceMac::IsConnectable() not implemented Adding NOTIMPLEMENTED() to BluetoothLowEnergyDeviceMac::IsConnectable() and removing BluetoothLowEnergyDeviceMac::connectable_. BUG= ==========
https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2854733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:32: connectable_(connectable), On 2017/05/04 00:35:36, ortuno wrote: > Thanks for implementing this. I think we want to keep this method as > NOTIMPLEMENTED though. The feature is not supported on Windows and Android so we > can't add it to the API :(. It's only there because the APIs are based on BlueZ. Done.
Thanks! lgtm https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:898: #endif // defined(OS_MACOSX) I'm OK with this change here but could you update the description to call out the change and the reason for it?
https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:898: #endif // defined(OS_MACOSX) On 2017/05/04 01:11:49, ortuno wrote: > I'm OK with this change here but could you update the description to call out > the change and the reason for it? Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2854733002/#ps60001 (title: "Removing #if defined(OS_MACOSX) from this patch, crrev.com/2863643003")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_unittest.cc (right): https://codereview.chromium.org/2854733002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_unittest.cc:898: #endif // defined(OS_MACOSX) On 2017/05/04 16:38:49, jlebel wrote: > On 2017/05/04 01:11:49, ortuno wrote: > > I'm OK with this change here but could you update the description to call out > > the change and the reason for it? > > Done. crrev.com/2863643003
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by jlebel@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": 1493930425066140,
"parent_rev": "74fe98e203e687c3c96f25128fc55e94981465a7", "commit_rev":
"c2c7f4185915b64e17ba5615e0906f4e3ab6b816"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: macOS: BluetoothLowEnergyDeviceMac::IsConnectable() not implemented Adding NOTIMPLEMENTED() to BluetoothLowEnergyDeviceMac::IsConnectable() and removing BluetoothLowEnergyDeviceMac::connectable_. BUG= ========== to ========== bluetooth: macOS: BluetoothLowEnergyDeviceMac::IsConnectable() not implemented Adding NOTIMPLEMENTED() to BluetoothLowEnergyDeviceMac::IsConnectable() and removing BluetoothLowEnergyDeviceMac::connectable_. BUG= Review-Url: https://codereview.chromium.org/2854733002 Cr-Commit-Position: refs/heads/master@{#469458} Committed: https://chromium.googlesource.com/chromium/src/+/c2c7f4185915b64e17ba5615e090... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c2c7f4185915b64e17ba5615e090... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
