Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1094273003:
Implementing a BLE connection finder. (Closed)
DescriptionImplementing a BLE connection finder.
This CL adds a connection finder class. The connection finder scans for
Bluetooth Low Energy devices advertising a given target service.
The current version only works when the remote device advertising the
target service is sending all its primary services UUID.
BUG=479673
R=msarda
TBR=tengs
Committed: https://crrev.com/f56835d5211fc533a02f143e06eb310764f6b901
Cr-Commit-Position: refs/heads/master@{#326807}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Nits. #Patch Set 3 : Rebasing. #Patch Set 4 : Reverting build file. #
Total comments: 31
Patch Set 5 : Error handling and nits. #
Total comments: 4
Patch Set 6 : ensuring a single connection #
Total comments: 12
Patch Set 7 : string to char[] #
Messages
Total messages: 17 (3 generated)
https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:20: namespace proximity_auth { This function is internal. Use an anonymous namespace for it: namespace { .... https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:21: Kill empty line. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:32: if (adapter_.get()) { You are starting discovery after the adapter is set. I think we should destroy them in the opposite order - first stop discovery settings ans then stop the observer. I also think you do not need the .get() in this if (I think if (adapater_) { should work). https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:44: DVLOG(1) << "Finding connection"; Move this below the IsBluetoothAvailable line. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:47: VLOG(1) << "[BCF] Bluetooth is unsupported on this platform. Aborting."; LOG(WARNING) https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:59: const ConnectionCallback& connection_callback) { I would expect this would be the method that we should call. Why do we have a different method than the ConnectionFinder? https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:92: discovery_session_.reset(discovery_session.release()); discovery_session_ = discovery_session.Pass(); https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:121: discovery_session_->Stop(base::Bind(base::DoNothing), Should you reset the discovery_session_ object when the Stop function is over? https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& Why this method? I think we should only have a single method Find that returns a proximity_auth::Connection object. That way we could actually interface with the BT classic case. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:50: // Starts a discovery session for |adaptor_|. s/adaptor_/adapter_
PTAL. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:20: namespace proximity_auth { On 2015/04/22 08:55:14, msarda wrote: > This function is internal. Use an anonymous namespace for it: > namespace { > .... Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:21: On 2015/04/22 08:55:14, msarda wrote: > Kill empty line. Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:32: if (adapter_.get()) { On 2015/04/22 08:55:14, msarda wrote: > You are starting discovery after the adapter is set. I think we should destroy > them in the opposite order - first stop discovery settings ans then stop the > observer. > > I also think you do not need the .get() in this if (I think if (adapater_) { > should work). Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:32: if (adapter_.get()) { On 2015/04/22 08:55:14, msarda wrote: > You are starting discovery after the adapter is set. I think we should destroy > them in the opposite order - first stop discovery settings ans then stop the > observer. > > I also think you do not need the .get() in this if (I think if (adapater_) { > should work). Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:44: DVLOG(1) << "Finding connection"; On 2015/04/22 08:55:14, msarda wrote: > Move this below the IsBluetoothAvailable line. Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:47: VLOG(1) << "[BCF] Bluetooth is unsupported on this platform. Aborting."; On 2015/04/22 08:55:14, msarda wrote: > LOG(WARNING) I'm being consistent with the current proximity/bluetooth_connection_finder implementation. I think we should keep like this for now. Later we can change everything to LOG(WARNING). https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:59: const ConnectionCallback& connection_callback) { On 2015/04/22 08:55:14, msarda wrote: > I would expect this would be the method that we should call. Why do we have a > different method than the ConnectionFinder? I agree. The problem is that the current callback types don't match. The callback required for BluetoothDevice::CreateGattConnection is not the same as ConnectionCallback. I'm planing to change this once the BluetoothLowEnergyConnection is implemented. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:92: discovery_session_.reset(discovery_session.release()); On 2015/04/22 08:55:14, msarda wrote: > discovery_session_ = discovery_session.Pass(); Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:121: discovery_session_->Stop(base::Bind(base::DoNothing), On 2015/04/22 08:55:14, msarda wrote: > Should you reset the discovery_session_ object when the Stop function is over? Done. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& On 2015/04/22 08:55:15, msarda wrote: > Why this method? I think we should only have a single method Find that returns a > proximity_auth::Connection object. That way we could actually interface with the > BT classic case. I agree. This is a temporary solution while BLE connection class is not implemented. https://codereview.chromium.org/1094273003/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:50: // Starts a discovery session for |adaptor_|. On 2015/04/22 08:55:15, msarda wrote: > s/adaptor_/adapter_ Done.
You will probably need to add bluetooth_low_energy_connection_finder.h and bluetooth_low_energy_connection_finder.cc to components/proximity_auth/ble/BUILD.gn https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth.gypi (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth.gypi:26: "proximity_auth/ble/proximity_auth_ble_system.cc", This order is broken. Let's move these 4 entries up before proximity_auth/bluetooth https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:61: const ConnectionCallback& connection_callback) { This method does not do what the user expects. Please add a NOTREACHED() in here. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:82: for (auto iter = devices.begin(); iter != devices.end(); iter++) { I think we should have a single method to handle new devices. I see 2 options: * have a method called HandleDeviceAdded that is called from here and from DeviceAdded * call DeviceAdded from here. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:97: void BluetoothLowEnergyConnectionFinder::StartDiscoverySession() { I expect this method should not be called when adapter_ is NULL. If so then prefer using DCHECK(adapter_) instead of the if. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:97: void BluetoothLowEnergyConnectionFinder::StartDiscoverySession() { How can we be sure that this method is not called twice? I think in that case we would call StartDiscoverySession twice and this may lead to errors. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:109: base::Bind(base::DoNothing)); We need error handling. At least let's log the errors as otherwise this will be impossible to debug. It may actually be wise to use VLOG(1) instead of DVLOG(1) for the BLE connection to be able to investigate this in release. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:123: discovery_session_->Stop(base::Bind(base::DoNothing), I think it is wrong to reset the discovery_session_ here (it will probably be destructed, so the callback will never be called). We should only reset it after the callback ends. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:146: base::Bind(&DoNothingErrorCallback)); Ditto: Same here about error handling. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:23: // This ConnectionFinder implementation tries to find a Bluetooth connection to This comment must be explicit about the fact that this class uses a Bluetooth Low Energy connection. How about: A connection finder that is specialized in finding a Bluetooth Low Energy remote device. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& I somehow feel we need to add an empty implementation of proximity_auth::Connection and use the Find method. I think that would avoid unnecessary changes in ProximityAuthBleSystem. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& Please add a comment explaining that this method can be called only once. BTW, is that the contract for proximity_auth::ConnectionFinder? https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:44: void OnAdapterInitialized(scoped_refptr<device::BluetoothAdapter> adapter); Why is this private and DeviceAdded is protected? https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:59: // Creates a connection with |remote_device|, the |connection_callback_| will s/a connection/a GATT connection s/, the |coonnection_callback_|/. |connection_callback_| https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:26: DVLOG(1) << "Starting Proximity Auth over Bluetooth Low Energy."; Let's keep the VLOG for now. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:33: DVLOG(1) << "Stopping Proximity over Bluetooth Low Energy."; VLOG here as well.
I do not see your patch. You need to address my last comments (otherwise there is nothing to review for me).
https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth.gypi (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth.gypi:26: "proximity_auth/ble/proximity_auth_ble_system.cc", On 2015/04/22 15:58:12, msarda wrote: > This order is broken. Let's move these 4 entries up before > proximity_auth/bluetooth Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:61: const ConnectionCallback& connection_callback) { On 2015/04/22 15:58:13, msarda wrote: > This method does not do what the user expects. Please add a NOTREACHED() in > here. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:82: for (auto iter = devices.begin(); iter != devices.end(); iter++) { On 2015/04/22 15:58:12, msarda wrote: > I think we should have a single method to handle new devices. I see 2 options: > * have a method called HandleDeviceAdded that is called from here and from > DeviceAdded > * call DeviceAdded from here. Done. I think the first option is better, because differentiating if a device was already known or just discovery makes debugging easier. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:97: void BluetoothLowEnergyConnectionFinder::StartDiscoverySession() { On 2015/04/22 15:58:12, msarda wrote: > I expect this method should not be called when adapter_ is NULL. If so then > prefer using DCHECK(adapter_) instead of the if. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:97: void BluetoothLowEnergyConnectionFinder::StartDiscoverySession() { On 2015/04/22 15:58:13, msarda wrote: > How can we be sure that this method is not called twice? I think in that case we > would call StartDiscoverySession twice and this may lead to errors. This method does exactly what the method "BluetoothEventRouter::StartDiscoverySession" (which implements the startDiscovery in the bluetooth javascript API) does. That is, they also test if there is an active discovery session before calling StartDiscoverySession, but they also don't consider the case where we have two calls and the second call is made while the discovery callback has not been called yet. Anyway, I see your point, and I think it would be better to have the Find method in the class constructor, that way we guarantee that "BluetoothLowEnergyConnectionFinder::StartDiscoverySession" is only called once. But this doesn't match the ConnectionFinder interface. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:109: base::Bind(base::DoNothing)); On 2015/04/22 15:58:13, msarda wrote: > We need error handling. At least let's log the errors as otherwise this will be > impossible to debug. > > It may actually be wise to use VLOG(1) instead of DVLOG(1) for the BLE > connection to be able to investigate this in release. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:123: discovery_session_->Stop(base::Bind(base::DoNothing), On 2015/04/22 15:58:13, msarda wrote: > I think it is wrong to reset the discovery_session_ here (it will probably be > destructed, so the callback will never be called). We should only reset it after > the callback ends. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:146: base::Bind(&DoNothingErrorCallback)); On 2015/04/22 15:58:13, msarda wrote: > Ditto: Same here about error handling. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:23: // This ConnectionFinder implementation tries to find a Bluetooth connection to On 2015/04/22 15:58:13, msarda wrote: > This comment must be explicit about the fact that this class uses a Bluetooth > Low Energy connection. How about: > A connection finder that is specialized in finding a Bluetooth Low Energy remote > device. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& On 2015/04/22 15:58:13, msarda wrote: > Please add a comment explaining that this method can be called only once. BTW, > is that the contract for proximity_auth::ConnectionFinder? Yes. Actually, I think it would be better to have the Find method in the class constructor, that way we guarantee that "BluetoothLowEnergyConnectionFinder::StartDiscoverySession" is only called once. But this doesn't match the ConnectionFinder interface. I think they will do it at some point, there is a comment about it in ConnectionFinder, when they do it, I'll reflect the changes here. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& On 2015/04/22 15:58:13, msarda wrote: > I somehow feel we need to add an empty implementation of > proximity_auth::Connection and use the Find method. I think that would avoid > unnecessary changes in ProximityAuthBleSystem. My next CL already implements proximity_auth::Connection and there we only need a single Find method. Can we leave like this for the moment? https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:44: void OnAdapterInitialized(scoped_refptr<device::BluetoothAdapter> adapter); On 2015/04/22 15:58:13, msarda wrote: > Why is this private and DeviceAdded is protected? DeviceAdded is part of the BluetoothAdapter::Observer interface. The current object is added as an observer of |adapter_| to receive the notification when a new device is added. So, DeviceAdded should be called from outside the BluetoothLowEnergyConnectionFinder, whereas OnAdapterInitialized is only called internally. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:59: // Creates a connection with |remote_device|, the |connection_callback_| will On 2015/04/22 15:58:13, msarda wrote: > s/a connection/a GATT connection > s/, the |coonnection_callback_|/. |connection_callback_| Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:26: DVLOG(1) << "Starting Proximity Auth over Bluetooth Low Energy."; On 2015/04/22 15:58:13, msarda wrote: > Let's keep the VLOG for now. Done. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:33: DVLOG(1) << "Stopping Proximity over Bluetooth Low Energy."; On 2015/04/22 15:58:13, msarda wrote: > VLOG here as well. Done.
LGTM with some nits. https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:33: void Find(const device::BluetoothDevice::GattConnectionCallback& On 2015/04/24 14:04:57, sacomoto wrote: > On 2015/04/22 15:58:13, msarda wrote: > > I somehow feel we need to add an empty implementation of > > proximity_auth::Connection and use the Find method. I think that would avoid > > unnecessary changes in ProximityAuthBleSystem. > > My next CL already implements proximity_auth::Connection and there we only need > a single Find method. Can we leave like this for the moment? Ok. https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:84: CreateConnection(remote_device); This seems to potentially create multiple concurrent connections. Is that something you want? https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:142: if (remote_device) { Can this actually be nullptr? If not, then I think it would be better to use DCHECK(remote_device) here.
https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:84: CreateConnection(remote_device); On 2015/04/24 14:12:16, msarda wrote: > This seems to potentially create multiple concurrent connections. Is that > something you want? You are right. Modified. https://codereview.chromium.org/1094273003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:142: if (remote_device) { On 2015/04/24 14:12:16, msarda wrote: > Can this actually be nullptr? If not, then I think it would be better to use > DCHECK(remote_device) here. I think it can. When the observer interface method DeviceAdded is called there is no guarantee that the device is non-NULL.
sacomoto@chromium.org changed reviewers: + tengs@chromium.org
Hi Tim, I added some files to proximity_auth.gypi. Cheers, Gustavo
The CQ bit was checked by sacomoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/1094273003/#ps100001 (title: "ensuring a single connection")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094273003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f56835d5211fc533a02f143e06eb310764f6b901 Cr-Commit-Position: refs/heads/master@{#326807}
Message was sent while issue was closed.
Please write a unit test for this class. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:31: if (discovery_session_.get()) { nit: you don't need the .get() to check if the pointer is non-null. Same with all the cases below. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:164: connection_callback_.Run(connection.Pass()); You should stop the discovery session before running the callback. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:5: #ifndef COMPONENTS_PROXIMITY_AUTH_BLUETOOTH_LOW_ENERGY_CONNECTION_FINDER_H nit: keep headers consistent with path (missing BLE directory). https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:37: protected: Why declare a protected method? https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:38: // Observer for device::BluetoothAdapter::Observer nit: for consistency use // device::BluetoothAdapter::Observer: https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/proximity_auth_ble_system.cc:27: BluetoothLowEnergyConnectionFinder* connection_finder = Use a scoped_ptr member variable to hold this pointer.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1105863003/ by jdonnelly@chromium.org. The reason for reverting is: This change broke the chromium perf dashboard: https://build.chromium.org/p/chromium/builders/Linux/builds/61221 by adding a new static initializer: const std::string kSmartLockServiceUUID = "b3b7e28e-a000-3e17-bd86-6e97b9e28c11"; I think you want static const char* instead..
Message was sent while issue was closed.
Hi, I addressed Tim's comments and change the string to char[] that caused the CL to be reverted. The new CL corresponding to the latest version of this CL is here: https://codereview.chromium.org/1102093002. @Tim, I still didn't add unit tests, but it's next thing I'm going to do. I want to land it to unblock Mihai. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:31: if (discovery_session_.get()) { On 2015/04/24 15:56:55, Tim Song wrote: > nit: you don't need the .get() to check if the pointer is non-null. Same with > all the cases below. Done. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:164: connection_callback_.Run(connection.Pass()); On 2015/04/24 15:56:55, Tim Song wrote: > You should stop the discovery session before running the callback. Done. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:5: #ifndef COMPONENTS_PROXIMITY_AUTH_BLUETOOTH_LOW_ENERGY_CONNECTION_FINDER_H On 2015/04/24 15:56:55, Tim Song wrote: > nit: keep headers consistent with path (missing BLE directory). Done. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:37: protected: On 2015/04/24 15:56:55, Tim Song wrote: > Why declare a protected method? I'm being consistent with BluetoothConnection. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:38: // Observer for device::BluetoothAdapter::Observer On 2015/04/24 15:56:55, Tim Song wrote: > nit: for consistency use > > // device::BluetoothAdapter::Observer: Done. https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1094273003/diff/100001/components/proximity_a... components/proximity_auth/ble/proximity_auth_ble_system.cc:27: BluetoothLowEnergyConnectionFinder* connection_finder = On 2015/04/24 15:56:56, Tim Song wrote: > Use a scoped_ptr member variable to hold this pointer. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
