Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697763002:
[CrOS Tether]: Create BleConnectionManager, which manages secure connections between the current de… (Closed)
|
Created:
3 years, 10 months ago by Kyle Horimoto Modified:
3 years, 10 months ago CC:
chromium-reviews, jlklein+watch-tether_chromium.org, tengs+watch-tether_chromium.org, hansberry+watch-tether_chromium.org, jhawkins+watch-tether_chromium.org, oshima+watch_chromium.org, khorimoto+watch-tether_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[CrOS Tether]: Create BleConnectionManager, which manages secure connections between the current device and remote devices.
Once a device is registered, BleConnectionManager attempts to create a connection to it until a connection is created successfully or the device is unregistered. Once an authenticated connection is establisihed, messages can be exchanged between devices.
BleConnectionManager
BUG=672263
Review-Url: https://codereview.chromium.org/2697763002
Cr-Commit-Position: refs/heads/master@{#451502}
Committed: https://chromium.googlesource.com/chromium/src/+/51d2457c22af0cbe490cdee3ad294e2ed6ce1fee
Patch Set 1 #
Total comments: 39
Patch Set 2 : hansberry@ comments. #
Total comments: 6
Patch Set 3 : hansberry@ comments. #
Total comments: 46
Patch Set 4 : hansberry@ comments. #
Total comments: 6
Patch Set 5 : hansberry@ comments. #Patch Set 6 : Fix build breakage. #Patch Set 7 : Fix compile error. #Patch Set 8 : Add missing DEP. #Messages
Total messages: 43 (20 generated)
khorimoto@chromium.org changed reviewers: + hansberry@chromium.org
First pass. Have not looked at the test yet. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:18: const int64_t kAdvertisingTimeoutInSeconds = 12; Where'd this number come from? https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:19: const int64_t kShortErrorTimeoutInSeconds = 1; Assuming this is only used by the test, this can just be 0. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:61: BleConnectionManager::ConnectionMetadata::GetStatus() const { Is this method really necessary if there are already methods like CanSendMessage()? https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:93: int64_t timeout_sec = use_short_error_timeout ? kShortErrorTimeoutInSeconds It would be much better if timeout_sec were a property whose value was decided at construction. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:122: timer_->Stop(); Please name this more descriptively than just timer_, e.g. connection_timeout_timer_, etc. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:162: // ensure that if the callback references this instance, internal state is Why wouldn't the callback reference this instance? https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:198: base::MakeUnique<BleScanner>(local_device_data_provider), BleScanner should be changed to be injected with an adapter. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:281: void BleConnectionManager::SendMessage( This API doesn't make sense to me: if we registered a RemoteDevice for a "ConnectionReason" already, then why do we need to send a message which will be the same thing (e.g. a ConnectTetheringRequest)? Am I reading this wrong? https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:31: // |RegisterRemoteDevice()| and register to observer status change events. Once observe* https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:37: enum class ConnectionReason { We should just use the real proto constants. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:99: class ConnectionMetadata : public cryptauth::SecureChannel::Observer { This inner class is quite large; it needs documentation. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:99: class ConnectionMetadata : public cryptauth::SecureChannel::Observer { Does ConnectionMetadata need to be an inner class? If not, please break it out into its own set of files. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:123: using SecureChannelStatusChangeHandler = base::Callback<void( Is 'using' standard practice? I've only seen 'typedef'. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:123: using SecureChannelStatusChangeHandler = base::Callback<void( Please use '*Callback', not '*Handler' https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:127: using MessageReceivedHandler = Same here. https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... File components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc:46: std::unique_ptr<Connection> Why did you change this? It doesn't really make sense to return Connection from BluetoothLowEnergyWeaveClientConnection's Factory. https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/secure... File components/cryptauth/secure_channel.h (right): https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/secure... components/cryptauth/secure_channel.h:114: SecureChannel( nit: one level too indented
https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:18: const int64_t kAdvertisingTimeoutInSeconds = 12; On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Where'd this number come from? It was derived empirically during Android work. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:19: const int64_t kShortErrorTimeoutInSeconds = 1; On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Assuming this is only used by the test, this can just be 0. This is used under non-test circumstances when there is an error advertising or creating a scan filter. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:61: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Is this method really necessary if there are already methods like > CanSendMessage()? Yes. When a device is unregistered, we need to send out a "current status" => "disconnected" status update, so we need to know what the current status is. You're right that the other methods could be replaced with just calling GetStatus() and comparing that value to the expected one, but I like the more convenient API more. If you have a strong objection, I could remove the convenience methods. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:93: int64_t timeout_sec = use_short_error_timeout ? kShortErrorTimeoutInSeconds On 2017/02/14 18:27:28, Ryan Hansberry wrote: > It would be much better if timeout_sec were a property whose value was decided > at construction. This isn't a constant value, though. If scanning/advertising succeeds, we need to wait the whole advertising timeout. If it could not be initialized, we do not need to wait the full timeout and should proceed to the next device faster. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:122: timer_->Stop(); On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Please name this more descriptively than just timer_, e.g. > connection_timeout_timer_, etc. Done. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:162: // ensure that if the callback references this instance, internal state is On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Why wouldn't the callback reference this instance? Not sure what you mean. The callback being run here is BleConnectionManager::OnSecureChannelStatusChanged(). When this happens, UpdateConnectionAttempts() ends up being called. We want to make sure that if UpdateConnectionAttempts() ends up accessing this ConnectionMetadata object, all of the internal state is consistent. Thus, we reset all of the handlers, etc. before this happens. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:198: base::MakeUnique<BleScanner>(local_device_data_provider), On 2017/02/14 18:27:28, Ryan Hansberry wrote: > BleScanner should be changed to be injected with an adapter. You're absolutely correct. That was a mistake on my part when I first wrote BleScanner. I'll do it in a follow-up CL, though. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:281: void BleConnectionManager::SendMessage( On 2017/02/14 18:27:28, Ryan Hansberry wrote: > This API doesn't make sense to me: if we registered a RemoteDevice for a > "ConnectionReason" already, then why do we need to send a message which will be > the same thing (e.g. a ConnectTetheringRequest)? Am I reading this wrong? Sorry - I probably didn't make things clear enough. Registration/unregistration for a specific reason does not actually send or receive any messages. It just creates a connection. The reason that there is a ConnectionReason (as opposed to just a register/unregister with only a RemoteDevice) is that we want to make sure we do not close down a connection that is active once one operation is finished. For example, imagine that a host scan is in progress when a ConnectTetheringRequest is in the process of being sent. If the host scan finishes first and UnregisterRemoteDevice() is called accordingly, it would break the connection sending the ConnectTetheringRequest. Instead, we keep a |set| of all current ConnectionReasons and only destroy the connection when *all* of the registered ConnectionReasons are unregistered. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:31: // |RegisterRemoteDevice()| and register to observer status change events. Once On 2017/02/14 18:27:28, Ryan Hansberry wrote: > observe* Done. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:37: enum class ConnectionReason { On 2017/02/14 18:27:28, Ryan Hansberry wrote: > We should just use the real proto constants. Done. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:99: class ConnectionMetadata : public cryptauth::SecureChannel::Observer { On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Does ConnectionMetadata need to be an inner class? If not, please break it out > into its own set of files. Yes, it should be an inner class. It is a bookkeeping class specifically tailored to be used as the value of BleConnectionManager's |device_to_metadata_map_| map. I don't think that this class should be visible outside of the class to preserve encapsulation. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:99: class ConnectionMetadata : public cryptauth::SecureChannel::Observer { On 2017/02/14 18:27:28, Ryan Hansberry wrote: > This inner class is quite large; it needs documentation. Done. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:123: using SecureChannelStatusChangeHandler = base::Callback<void( On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Is 'using' standard practice? I've only seen 'typedef'. Yep, pretty common. For example: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:123: using SecureChannelStatusChangeHandler = base::Callback<void( On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Please use '*Callback', not '*Handler' Done. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.h:127: using MessageReceivedHandler = On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Same here. Done. https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... File components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc:46: std::unique_ptr<Connection> On 2017/02/14 18:27:28, Ryan Hansberry wrote: > Why did you change this? It doesn't really make sense to return Connection from > BluetoothLowEnergyWeaveClientConnection's Factory. We need the factory to return |FakeConnection|s in tests. I don't think there's necessarily a problem with return a |Connection| here, though I agree it looks a little weird. In practice, the only place this is used is in BleConnectionManager, where the connection is simply passed to the SecureChannel, which does not care about what type of Connection is used. I'm inclined to leave this as-is, unless you have a better idea. https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/secure... File components/cryptauth/secure_channel.h (right): https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/secure... components/cryptauth/secure_channel.h:114: SecureChannel( On 2017/02/14 18:27:28, Ryan Hansberry wrote: > nit: one level too indented Done.
https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:61: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/14 22:54:45, Kyle Horimoto wrote: > On 2017/02/14 18:27:28, Ryan Hansberry wrote: > > Is this method really necessary if there are already methods like > > CanSendMessage()? > > Yes. When a device is unregistered, we need to send out a "current status" => > "disconnected" status update, so we need to know what the current status is. > You're right that the other methods could be replaced with just calling > GetStatus() and comparing that value to the expected one, but I like the more > convenient API more. If you have a strong objection, I could remove the > convenience methods. IMO the convenience methods just make this class's API harder to understand. Please remove them. https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:161: // Run the handler after the other two handlers have already been reset to s/handler/callback https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:198: base::MakeUnique<BleScanner>(local_device_data_provider), On 2017/02/14 22:54:45, Kyle Horimoto wrote: > On 2017/02/14 18:27:28, Ryan Hansberry wrote: > > BleScanner should be changed to be injected with an adapter. > > You're absolutely correct. That was a mistake on my part when I first wrote > BleScanner. I'll do it in a follow-up CL, though. sgtm https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:431: data->StartConnectionAttemptTimeout( As discussed please add a comment on why we are still calling StartConnectionAttemptTimeout https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:35: // |OnMessageReceived()| observer callback. Please add simple API usage here. https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:73: // |MessageType|s have been unregistered. Types generally don't seem to be surrounded with ||, just vars/params.
https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/1/chromeos/components/tether/... chromeos/components/tether/ble_connection_manager.cc:61: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/15 18:56:02, Ryan Hansberry wrote: > On 2017/02/14 22:54:45, Kyle Horimoto wrote: > > On 2017/02/14 18:27:28, Ryan Hansberry wrote: > > > Is this method really necessary if there are already methods like > > > CanSendMessage()? > > > > Yes. When a device is unregistered, we need to send out a "current status" => > > "disconnected" status update, so we need to know what the current status is. > > You're right that the other methods could be replaced with just calling > > GetStatus() and comparing that value to the expected one, but I like the more > > convenient API more. If you have a strong objection, I could remove the > > convenience methods. > > IMO the convenience methods just make this class's API harder to understand. > Please remove them. Done, except I kept the HasEstablishedConnection() function since it provides different information other than just the status. https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:431: data->StartConnectionAttemptTimeout( On 2017/02/15 18:56:02, Ryan Hansberry wrote: > As discussed please add a comment on why we are still calling > StartConnectionAttemptTimeout Done. https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:35: // |OnMessageReceived()| observer callback. On 2017/02/15 18:56:02, Ryan Hansberry wrote: > Please add simple API usage here. A sample use of the API was too difficult to show since it requires defining an Observer derived class, so instead, I rewrote the description to include more information about how to use the class. https://codereview.chromium.org/2697763002/diff/20001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:73: // |MessageType|s have been unregistered. On 2017/02/15 18:56:02, Ryan Hansberry wrote: > Types generally don't seem to be surrounded with ||, just vars/params. Done.
https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... File components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2697763002/diff/1/components/cryptauth/ble/bl... components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc:46: std::unique_ptr<Connection> On 2017/02/14 22:54:46, Kyle Horimoto wrote: > On 2017/02/14 18:27:28, Ryan Hansberry wrote: > > Why did you change this? It doesn't really make sense to return Connection > from > > BluetoothLowEnergyWeaveClientConnection's Factory. > > We need the factory to return |FakeConnection|s in tests. I don't think there's > necessarily a problem with return a |Connection| here, though I agree it looks a > little weird. In practice, the only place this is used is in > BleConnectionManager, where the connection is simply passed to the > SecureChannel, which does not care about what type of Connection is used. I'm > inclined to leave this as-is, unless you have a better idea. Oh man, lame. If Connection had a factory that this factory could extend, maybe you could inject a Factory into BleConnectionManager, but that's outside the scope of this CL and isn't even worth it. Please just write a comment explaining why this returns Connection. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:19: const int64_t kShortErrorTimeoutMillis = 100; Didn't we agree to make this 0? https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:70: BleConnectionManager::ConnectionMetadata::GetStatus() const { Why not always return secure_channel's status unless it is null? (why does the timer need to factor into this) https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:136: return; returning early is unnecessary. let it fall through to the end of the method to one call of manager_->OnSecureChannelStatusChanged https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:304: PA_LOG(INFO) << "3 " << connection.get(); Are these leftover debug logs? https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:406: data->StartConnectionAttemptTimer(/* use_short_error_timeout */ !success); The param comment should come after the param. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:39: // Then, register device(s) to connect to and wait for the nit: Please state what method to use https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:43: // limit and unregister a device if multiple connection attempts have failed. same here https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager_unittest.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:35: const int64_t kShortErrorTimeoutMillis = 100; I think we should just reuse the constants from BleConnectionManager. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:42: // const char kBluetoothAddress4[] = "44:55:66:77:88:99"; remove this https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:159: : FakeConnection(remote_device, /* should_auto_connect */ false), param comment should come after param https://cs.chromium.org/search/?q=%22*/,%22+package:%5Echromium$&type=cs https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:162: std::string device_address() { return device_address_; } please use GetDeviceAddress https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:259: /* retains_user_task */ false, /* is_repeating */ false); param comment should come after param https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:362: EXPECT_TRUE(data); Throughout this entire cl, please use the variable connection_metadata or metadata. 'data' is simply too generic and means something totally different from metadata. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:368: VerifyTimeoutSet(remote_device, kShortErrorTimeoutMillis); This test will break if the value of this is changed in ble_connection_manager.cc, please reuse the constants. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:416: FakeSecureChannel* ConnectSuccessfully( It seems to me that this method is essentially a helper method to call ConnectChannel and AuthChannel. Please write a comment on each method explaining their relationship to each other. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:507: TEST_F(BleConnectionManagerTest, TestCannotScan) { super nit: since scanning is attempted first, place this test before CannotAdvertise https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:522: // Expected to start a connection attempt, then stop once the timer fires, I can't tell how this test stops and starts again? https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:562: TEST_F(BleConnectionManagerTest, TestRegisterWithNoConnection_TimerFires) { Suggestion: instead of saying "timer fires" you could use "timeout occurs" https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:711: TestSuccessfulConnection_DisconnectsAfterConnection) { Is this test really testing anything that other tests dont cover? https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:734: TEST_F(BleConnectionManagerTest, TwoDevices_NeitherCanScan) { Is this test really necessary? https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:759: TEST_F(BleConnectionManagerTest, TwoDevices_NeitherCanAdvertise) { Same here https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:857: // been achieved. Device 1 is expected to start, then stop once the tiemr timer*
https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:19: const int64_t kShortErrorTimeoutMillis = 100; On 2017/02/16 19:29:35, Ryan Hansberry wrote: > Didn't we agree to make this 0? Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:70: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/16 19:29:35, Ryan Hansberry wrote: > Why not always return secure_channel's status unless it is null? (why does the > timer need to factor into this) If the ConnectionMetadata is associated with a device that is "in line" to attempt a connection (i.e., other devices have occupied the available advertising slots), it is DISCONNECTED. Thus, we need to check if the timer is active to determine if it is CONNECTING. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:136: return; On 2017/02/16 19:29:35, Ryan Hansberry wrote: > returning early is unnecessary. let it fall through to the end of the method to > one call of manager_->OnSecureChannelStatusChanged Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:304: PA_LOG(INFO) << "3 " << connection.get(); On 2017/02/16 19:29:35, Ryan Hansberry wrote: > Are these leftover debug logs? Yes; removed. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:406: data->StartConnectionAttemptTimer(/* use_short_error_timeout */ !success); On 2017/02/16 19:29:35, Ryan Hansberry wrote: > The param comment should come after the param. Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:39: // Then, register device(s) to connect to and wait for the On 2017/02/16 19:29:36, Ryan Hansberry wrote: > nit: Please state what method to use Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:43: // limit and unregister a device if multiple connection attempts have failed. On 2017/02/16 19:29:35, Ryan Hansberry wrote: > same here Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager_unittest.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:35: const int64_t kShortErrorTimeoutMillis = 100; On 2017/02/16 19:29:36, Ryan Hansberry wrote: > I think we should just reuse the constants from BleConnectionManager. Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:42: // const char kBluetoothAddress4[] = "44:55:66:77:88:99"; On 2017/02/16 19:29:36, Ryan Hansberry wrote: > remove this Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:159: : FakeConnection(remote_device, /* should_auto_connect */ false), On 2017/02/16 19:29:36, Ryan Hansberry wrote: > param comment should come after param > > https://cs.chromium.org/search/?q=%22*/,%22+package:%5Echromium$&type=cs Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:162: std::string device_address() { return device_address_; } On 2017/02/16 19:29:36, Ryan Hansberry wrote: > please use GetDeviceAddress Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:259: /* retains_user_task */ false, /* is_repeating */ false); On 2017/02/16 19:29:36, Ryan Hansberry wrote: > param comment should come after param Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:362: EXPECT_TRUE(data); On 2017/02/16 19:29:36, Ryan Hansberry wrote: > Throughout this entire cl, please use the variable connection_metadata or > metadata. 'data' is simply too generic and means something totally different > from metadata. Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:368: VerifyTimeoutSet(remote_device, kShortErrorTimeoutMillis); On 2017/02/16 19:29:36, Ryan Hansberry wrote: > This test will break if the value of this is changed in > ble_connection_manager.cc, please reuse the constants. Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:416: FakeSecureChannel* ConnectSuccessfully( On 2017/02/16 19:29:36, Ryan Hansberry wrote: > It seems to me that this method is essentially a helper method to call > ConnectChannel and AuthChannel. Please write a comment on each method explaining > their relationship to each other. Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:507: TEST_F(BleConnectionManagerTest, TestCannotScan) { On 2017/02/16 19:29:37, Ryan Hansberry wrote: > super nit: since scanning is attempted first, place this test before > CannotAdvertise Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:522: // Expected to start a connection attempt, then stop once the timer fires, On 2017/02/16 19:29:36, Ryan Hansberry wrote: > I can't tell how this test stops and starts again? Oops - that was an incorrect comment. Removed. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:562: TEST_F(BleConnectionManagerTest, TestRegisterWithNoConnection_TimerFires) { On 2017/02/16 19:29:37, Ryan Hansberry wrote: > Suggestion: instead of saying "timer fires" you could use "timeout occurs" Done. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:711: TestSuccessfulConnection_DisconnectsAfterConnection) { On 2017/02/16 19:29:36, Ryan Hansberry wrote: > Is this test really testing anything that other tests dont cover? Yes. This is the only test which tests what should happen when the channel is disconnected due to the channel being broken (e.g., if the devices go out of Bluetooth range) as opposed to the channel being disconnected due to a UnregisterRemoteDevice() call. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:734: TEST_F(BleConnectionManagerTest, TwoDevices_NeitherCanScan) { On 2017/02/16 19:29:36, Ryan Hansberry wrote: > Is this test really necessary? It expands the test coverage for this class, so it has value. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:759: TEST_F(BleConnectionManagerTest, TwoDevices_NeitherCanAdvertise) { On 2017/02/16 19:29:36, Ryan Hansberry wrote: > Same here Same here. https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager_unittest.cc:857: // been achieved. Device 1 is expected to start, then stop once the tiemr On 2017/02/16 19:29:36, Ryan Hansberry wrote: > timer* Done.
lgtm with nits :) https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:70: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/17 01:06:52, Kyle Horimoto wrote: > On 2017/02/16 19:29:35, Ryan Hansberry wrote: > > Why not always return secure_channel's status unless it is null? (why does the > > timer need to factor into this) > > If the ConnectionMetadata is associated with a device that is "in line" to > attempt a connection (i.e., other devices have occupied the available > advertising slots), it is DISCONNECTED. Thus, we need to check if the timer is > active to determine if it is CONNECTING. Got it. Please add a brief comment explaining that. https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:210: std::shared_ptr<ConnectionMetadata> data = nit: Please use the variable name 'metadata' or 'connection_metadata'. https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:406: data->StartConnectionAttemptTimer(!success /* use_short_error_timeout */); What do you think of renaming this from 'use_short_error_timeout' to 'immediately_fail' or something similar? https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:43: // limit and unregister a device via |UnregsterRemoteDevice()| if multiple Methods shouldn't be wrapped in ||, only variables/arguments.
https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/40001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:70: BleConnectionManager::ConnectionMetadata::GetStatus() const { On 2017/02/17 01:59:05, Ryan Hansberry wrote: > On 2017/02/17 01:06:52, Kyle Horimoto wrote: > > On 2017/02/16 19:29:35, Ryan Hansberry wrote: > > > Why not always return secure_channel's status unless it is null? (why does > the > > > timer need to factor into this) > > > > If the ConnectionMetadata is associated with a device that is "in line" to > > attempt a connection (i.e., other devices have occupied the available > > advertising slots), it is DISCONNECTED. Thus, we need to check if the timer is > > active to determine if it is CONNECTING. > > Got it. Please add a brief comment explaining that. Done. https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.cc (right): https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:210: std::shared_ptr<ConnectionMetadata> data = On 2017/02/17 01:59:05, Ryan Hansberry wrote: > nit: Please use the variable name 'metadata' or 'connection_metadata'. Done. https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.cc:406: data->StartConnectionAttemptTimer(!success /* use_short_error_timeout */); On 2017/02/17 01:59:05, Ryan Hansberry wrote: > What do you think of renaming this from 'use_short_error_timeout' to > 'immediately_fail' or something similar? Done. https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... File chromeos/components/tether/ble_connection_manager.h (right): https://codereview.chromium.org/2697763002/diff/60001/chromeos/components/tet... chromeos/components/tether/ble_connection_manager.h:43: // limit and unregister a device via |UnregsterRemoteDevice()| if multiple On 2017/02/17 01:59:05, Ryan Hansberry wrote: > Methods shouldn't be wrapped in ||, only variables/arguments. Done.
The CQ bit was checked by khorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2697763002/#ps80001 (title: "hansberry@ comments.")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
kkhorimoto@chromium.org changed reviewers: + kkhorimoto@chromium.org
lgtm
The CQ bit was checked by khorimoto@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2697763002/#ps100001 (title: "Fix build breakage.")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by khorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2697763002/#ps120001 (title: "Fix compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by khorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2697763002/#ps140001 (title: "Add missing DEP.")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by khorimoto@chromium.org
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by khorimoto@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": 140001, "attempt_start_ts": 1487461526908940,
"parent_rev": "b1730dabdf125160dc23db993e08f453fc648fc8", "commit_rev":
"51d2457c22af0cbe490cdee3ad294e2ed6ce1fee"}
Message was sent while issue was closed.
Description was changed from ========== [CrOS Tether]: Create BleConnectionManager, which manages secure connections between the current device and remote devices. Once a device is registered, BleConnectionManager attempts to create a connection to it until a connection is created successfully or the device is unregistered. Once an authenticated connection is establisihed, messages can be exchanged between devices. BleConnectionManager BUG=672263 ========== to ========== [CrOS Tether]: Create BleConnectionManager, which manages secure connections between the current device and remote devices. Once a device is registered, BleConnectionManager attempts to create a connection to it until a connection is created successfully or the device is unregistered. Once an authenticated connection is establisihed, messages can be exchanged between devices. BleConnectionManager BUG=672263 Review-Url: https://codereview.chromium.org/2697763002 Cr-Commit-Position: refs/heads/master@{#451502} Committed: https://chromium.googlesource.com/chromium/src/+/51d2457c22af0cbe490cdee3ad29... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/51d2457c22af0cbe490cdee3ad29... |
