Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915713002:
Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time … (Closed)
|
Created:
3 years, 6 months ago by Ryan Hansberry Modified:
3 years, 6 months ago Reviewers:
Kyle Horimoto 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, lesliewatkins+watch-tether_chromium.org, khorimoto+watch-tether_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out.
BUG=672263
Review-Url: https://codereview.chromium.org/2915713002
Cr-Commit-Position: refs/heads/master@{#476382}
Committed: https://chromium.googlesource.com/chromium/src/+/e774ca8c21b698fa805b0b45ea90b0fcdcc36384
Patch Set 1 #
Total comments: 40
Patch Set 2 : Rebase. #Patch Set 3 : khorimoto@ comments. #
Total comments: 8
Patch Set 4 : khorimoto@ comments. #
Total comments: 12
Patch Set 5 : khorimoto@ comments. #Patch Set 6 : Remove useless test method. #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. BUG=672263 ========== to ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. Includes a slight refactor to pull TimerFactory out of HostScanCache. BUG=672263 ==========
hansberry@chromium.org changed reviewers: + khorimoto@chromium.org
khorimoto@google.com changed reviewers: + khorimoto@google.com
You also should refactor BleConnectionManager to use the TimerFactory as well. I'd prefer this in a separate CL if you're okay with that. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/host_scan_cache_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/host_scan_cache_unittest.cc:82: class TestTimerFactory : public TimerFactory { Please move this into the anonymous namespace now that it no longer refers to a nested class. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:173: void MessageTransferOperation::StartResponseTimerForDevice( nit: Do you think a log is appropriate in this function? https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:175: if (!wait_for_response_) Only call this function when we should wait for a response. Then, this line should be a DCHECK() instead (except change to DCHECKing the value returned by the ShouldWaitForResponse() function instead of an instance field). Same thing with StopResponseTimerForDevice() as well. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) Instead of returning early if(!remote_device_to_timer_map_[remote_device]), just don't ever call this function if a timer is not active. It should be pretty simple - just remove the remote_device_to_timer_map_.erase(remote_device) call in OnResponseTimeout() below. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:197: PA_LOG(WARNING) << "Timed out while waiting for response from device with id " nit: Capitalize ID. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:197: PA_LOG(WARNING) << "Timed out while waiting for response from device with id " In your log, also include the message type that was sent (the one on which you're waiting for a response). See GetMessageTypeForConnection(). https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:75: void SetWaitForResponse(bool wait_for_response) { Instead, please use the protected function pattern that we've already been using in this class: virtual bool ShouldWaitForResponse() { return true; // The value you currently set for wait_for_response_. } Then override it in derived classes when necessary. Also, add a explanatory comment since it's unclear what "wait for response" means. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:83: void SetTimerFactoryForTest( Please make this private. You can add a friend relationship if you need to, but it looks like this is already done for most tests below. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:98: // SetResponseTimeout(). There is no SetResponseTimeout() - I think you forgot to include this. Please use the same protected function pattern which I outlined above for getting the response timeout, and include a static constant here for the default value to be used. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:99: static uint32_t kResponseTimeoutSeconds; Actually, this is probably more easily expressed using a constexpr here in the header file: static constexpr int kResponseTimeoutSeconds = 10; You can also change kMaxConnectionAttemptsPerDevice above. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:107: base::Timer* GetResponseTimerForDevice( Please avoid exposing private fields via getters like this unless there is no other reasonable approach. Instead, use logic in the test TimerFactory to keep track of this within your test file. I used a similar approach in HostScanCache's test. See [1], specifically my set_tether_network_guid_for_next_timer() function. [1] https://cs.chromium.org/chromium/src/chromeos/components/tether/host_scan_cac... https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. You don't have any tests here for when the operation does NOT want to wait for a response. Please add them. You'll probably want to add another setter function on TestOperation to change this. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:102: return base::MakeUnique<base::MockTimer>(false, false); /* parameter_names */ https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:292: EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0])); Test that timers are created correctly in all tests in which you expect one to be created, not only this one. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:306: // Simulate how subclasses behave after a successful response: unregister the Instead of manually calling UnregisterDevice(), update TestOperation above to include an option for unregistering after a response is received. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:437: AuthenticatesButTimesOutWaitingForResponse) { nit: Change name to reflect the fact that this test authenticates before initialization. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/timer_factory.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/timer_factory.h:18: class TimerFactory { Add a description for classes that are in their own files. You'll probably want to talk about this is injected (for testing purposes) into certain classes which need to create a Timer.
https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/host_scan_cache_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/host_scan_cache_unittest.cc:82: class TestTimerFactory : public TimerFactory { On 2017/05/31 17:26:35, khorimoto wrote: > Please move this into the anonymous namespace now that it no longer refers to a > nested class. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:173: void MessageTransferOperation::StartResponseTimerForDevice( On 2017/05/31 17:26:35, khorimoto wrote: > nit: Do you think a log is appropriate in this function? Doesn't hurt; added. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:175: if (!wait_for_response_) On 2017/05/31 17:26:35, khorimoto wrote: > Only call this function when we should wait for a response. > > Then, this line should be a DCHECK() instead (except change to DCHECKing the > value returned by the ShouldWaitForResponse() function instead of an instance > field). > > Same thing with StopResponseTimerForDevice() as well. Good point, done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) On 2017/05/31 17:26:35, khorimoto wrote: > Instead of returning early if(!remote_device_to_timer_map_[remote_device]), just > don't ever call this function if a timer is not active. It should be pretty > simple - just remove the remote_device_to_timer_map_.erase(remote_device) call > in OnResponseTimeout() below. That can't work, because this method is called from UnregisterDevice, which can be called before OnDeviceAuthenticated is called (when the timer is created). TLDR: it's valid for this method to be called without a timer existing. It also just feels easier to reason about that timers are completely cleaned up after being used. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:197: PA_LOG(WARNING) << "Timed out while waiting for response from device with id " On 2017/05/31 17:26:35, khorimoto wrote: > In your log, also include the message type that was sent (the one on which > you're waiting for a response). See GetMessageTypeForConnection(). Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:75: void SetWaitForResponse(bool wait_for_response) { On 2017/05/31 17:26:35, khorimoto wrote: > Instead, please use the protected function pattern that we've already been using > in this class: > > virtual bool ShouldWaitForResponse() { > return true; // The value you currently set for wait_for_response_. > } > > Then override it in derived classes when necessary. > > Also, add a explanatory comment since it's unclear what "wait for response" > means. Done. Same done for response_timeout_seconds. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:98: // SetResponseTimeout(). On 2017/05/31 17:26:35, khorimoto wrote: > There is no SetResponseTimeout() - I think you forgot to include this. Please > use the same protected function pattern which I outlined above for getting the > response timeout, and include a static constant here for the default value to be > used. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:99: static uint32_t kResponseTimeoutSeconds; On 2017/05/31 17:26:35, khorimoto wrote: > Actually, this is probably more easily expressed using a constexpr here in the > header file: > > static constexpr int kResponseTimeoutSeconds = 10; > > You can also change kMaxConnectionAttemptsPerDevice above. Discussed offline; skipping over this due to linking issues when using EXPECT_EQ with a constexpr. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:107: base::Timer* GetResponseTimerForDevice( On 2017/05/31 17:26:35, khorimoto wrote: > Please avoid exposing private fields via getters like this unless there is no > other reasonable approach. Instead, use logic in the test TimerFactory to keep > track of this within your test file. > > I used a similar approach in HostScanCache's test. See [1], specifically my > set_tether_network_guid_for_next_timer() function. > > [1] > https://cs.chromium.org/chromium/src/chromeos/components/tether/host_scan_cac... I'd prefer not to emulate the logic in host_scan_cache_unittest -- by using MessageTransferOperation's existing map of Timers via this method, I don't need to rewrite it in the test, and additionally don't need to use a custom MockTimer with a destructor callback. This approach is much simpler and easier to reason about. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/31 17:26:36, khorimoto wrote: > You don't have any tests here for when the operation does NOT want to wait for a > response. Please add them. You'll probably want to add another setter function > on TestOperation to change this. Done. Also added a test for different response timeout. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:102: return base::MakeUnique<base::MockTimer>(false, false); On 2017/05/31 17:26:36, khorimoto wrote: > /* parameter_names */ Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:292: EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0])); On 2017/05/31 17:26:36, khorimoto wrote: > Test that timers are created correctly in all tests in which you expect one to > be created, not only this one. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:306: // Simulate how subclasses behave after a successful response: unregister the On 2017/05/31 17:26:36, khorimoto wrote: > Instead of manually calling UnregisterDevice(), update TestOperation above to > include an option for unregistering after a response is received. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation_unittest.cc:437: AuthenticatesButTimesOutWaitingForResponse) { On 2017/05/31 17:26:36, khorimoto wrote: > nit: Change name to reflect the fact that this test authenticates before > initialization. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/timer_factory.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/timer_factory.h:18: class TimerFactory { On 2017/05/31 17:26:36, khorimoto wrote: > Add a description for classes that are in their own files. You'll probably want > to talk about this is injected (for testing purposes) into certain classes which > need to create a Timer. Moved to another CL.
https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) On 2017/05/31 21:19:03, Ryan Hansberry wrote: > On 2017/05/31 17:26:35, khorimoto wrote: > > Instead of returning early if(!remote_device_to_timer_map_[remote_device]), > just > > don't ever call this function if a timer is not active. It should be pretty > > simple - just remove the remote_device_to_timer_map_.erase(remote_device) call > > in OnResponseTimeout() below. > > That can't work, because this method is called from UnregisterDevice, which can > be called before OnDeviceAuthenticated is called (when the timer is created). > TLDR: it's valid for this method to be called without a timer existing. It also > just feels easier to reason about that timers are completely cleaned up after > being used. Instead, can you change your code to make sure there is a timer before calling this? if (ShouldWaitForResponse() && remote_device_to_timer_map_.find(remote_device) != remote_device_to_timer_map_.end()) { StopResponseTimerForDevice(remote_device); } https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:83: void SetTimerFactoryForTest( On 2017/05/31 17:26:35, khorimoto wrote: > Please make this private. You can add a friend relationship if you need to, but > it looks like this is already done for most tests below. Ping. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:107: base::Timer* GetResponseTimerForDevice( On 2017/05/31 21:19:03, Ryan Hansberry wrote: > On 2017/05/31 17:26:35, khorimoto wrote: > > Please avoid exposing private fields via getters like this unless there is no > > other reasonable approach. Instead, use logic in the test TimerFactory to keep > > track of this within your test file. > > > > I used a similar approach in HostScanCache's test. See [1], specifically my > > set_tether_network_guid_for_next_timer() function. > > > > [1] > > > https://cs.chromium.org/chromium/src/chromeos/components/tether/host_scan_cac... > > I'd prefer not to emulate the logic in host_scan_cache_unittest -- by using > MessageTransferOperation's existing map of Timers via this method, I don't need > to rewrite it in the test, and additionally don't need to use a custom MockTimer > with a destructor callback. This approach is much simpler and easier to reason > about. Sorry - I'd still like you to change this. The reason is that this is an implementation detail of your class, and your test shouldn't depend on implementation details. You should be able to change how your class is implemented internally without needing to update the tests. You don't need to use a custom MockTimer with a destructor callback like what was done in HostScanCacheTest. Your implementation will be simpler than in that class - it will just keep track of which device a given Timer corresponds to. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.cc:189: PA_LOG(INFO) << "Starting timer to wait for host response."; This log isn't useful unless you also log the message type you just sent and the device ID you just sent the message to. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:75: // Returns true if we should wait for a response from the host device. Also describe what the consequences of this are (i.e., the timer will be started and will automatically unregister a device if it fires). https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:102: static uint32_t kResponseTimeoutSeconds; Name this kDefaultResponseTimeoutSeconds and edit the description to say that this is the default value. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:355: EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0])); You can combine the checks for having a response timer and asserting that the timeout is correct (below) into one function (e.g., VerifyTimerCreatedWithCorrectDelay()), then call that from each test that needs to test that. You should also move the timeout value (initialized to 5 in TestOperation) to a constant in the anonymous namespace so that you can assert that it is equal.
https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) On 2017/05/31 21:55:33, Kyle Horimoto wrote: > On 2017/05/31 21:19:03, Ryan Hansberry wrote: > > On 2017/05/31 17:26:35, khorimoto wrote: > > > Instead of returning early if(!remote_device_to_timer_map_[remote_device]), > > just > > > don't ever call this function if a timer is not active. It should be pretty > > > simple - just remove the remote_device_to_timer_map_.erase(remote_device) > call > > > in OnResponseTimeout() below. > > > > That can't work, because this method is called from UnregisterDevice, which > can > > be called before OnDeviceAuthenticated is called (when the timer is created). > > TLDR: it's valid for this method to be called without a timer existing. It > also > > just feels easier to reason about that timers are completely cleaned up after > > being used. > > Instead, can you change your code to make sure there is a timer before calling > this? > > if (ShouldWaitForResponse() && > remote_device_to_timer_map_.find(remote_device) != > remote_device_to_timer_map_.end()) { > StopResponseTimerForDevice(remote_device); > } What is the difference between checking if the map holds the timer inside or outside of the method? The only difference I can see is that it's more self-contained within the method. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:83: void SetTimerFactoryForTest( On 2017/05/31 21:55:33, Kyle Horimoto wrote: > On 2017/05/31 17:26:35, khorimoto wrote: > > Please make this private. You can add a friend relationship if you need to, > but > > it looks like this is already done for most tests below. > > Ping. Done. https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.h:107: base::Timer* GetResponseTimerForDevice( On 2017/05/31 21:55:33, Kyle Horimoto wrote: > On 2017/05/31 21:19:03, Ryan Hansberry wrote: > > On 2017/05/31 17:26:35, khorimoto wrote: > > > Please avoid exposing private fields via getters like this unless there is > no > > > other reasonable approach. Instead, use logic in the test TimerFactory to > keep > > > track of this within your test file. > > > > > > I used a similar approach in HostScanCache's test. See [1], specifically my > > > set_tether_network_guid_for_next_timer() function. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/chromeos/components/tether/host_scan_cac... > > > > I'd prefer not to emulate the logic in host_scan_cache_unittest -- by using > > MessageTransferOperation's existing map of Timers via this method, I don't > need > > to rewrite it in the test, and additionally don't need to use a custom > MockTimer > > with a destructor callback. This approach is much simpler and easier to reason > > about. > > Sorry - I'd still like you to change this. The reason is that this is an > implementation detail of your class, and your test shouldn't depend on > implementation details. You should be able to change how your class is > implemented internally without needing to update the tests. > > You don't need to use a custom MockTimer with a destructor callback like what > was done in HostScanCacheTest. Your implementation will be simpler than in that > class - it will just keep track of which device a given Timer corresponds to. Resolved offline -- Done. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.cc:189: PA_LOG(INFO) << "Starting timer to wait for host response."; On 2017/05/31 21:55:33, Kyle Horimoto wrote: > This log isn't useful unless you also log the message type you just sent and the > device ID you just sent the message to. Done. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:75: // Returns true if we should wait for a response from the host device. On 2017/05/31 21:55:33, Kyle Horimoto wrote: > Also describe what the consequences of this are (i.e., the timer will be started > and will automatically unregister a device if it fires). Done. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:102: static uint32_t kResponseTimeoutSeconds; On 2017/05/31 21:55:33, Kyle Horimoto wrote: > Name this kDefaultResponseTimeoutSeconds and edit the description to say that > this is the default value. Done. https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/40001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:355: EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0])); On 2017/05/31 21:55:33, Kyle Horimoto wrote: > You can combine the checks for having a response timer and asserting that the > timeout is correct (below) into one function (e.g., > VerifyTimerCreatedWithCorrectDelay()), then call that from each test that needs > to test that. > > You should also move the timeout value (initialized to 5 in TestOperation) to a > constant in the anonymous namespace so that you can assert that it is equal. Done.
hansberry@chromium.org changed reviewers: - khorimoto@google.com
Description was changed from ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. Includes a slight refactor to pull TimerFactory out of HostScanCache. BUG=672263 ========== to ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. BUG=672263 ==========
lgtm https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) On 2017/06/01 00:31:58, Ryan Hansberry wrote: > On 2017/05/31 21:55:33, Kyle Horimoto wrote: > > On 2017/05/31 21:19:03, Ryan Hansberry wrote: > > > On 2017/05/31 17:26:35, khorimoto wrote: > > > > Instead of returning early > if(!remote_device_to_timer_map_[remote_device]), > > > just > > > > don't ever call this function if a timer is not active. It should be > pretty > > > > simple - just remove the remote_device_to_timer_map_.erase(remote_device) > > call > > > > in OnResponseTimeout() below. > > > > > > That can't work, because this method is called from UnregisterDevice, which > > can > > > be called before OnDeviceAuthenticated is called (when the timer is > created). > > > TLDR: it's valid for this method to be called without a timer existing. It > > also > > > just feels easier to reason about that timers are completely cleaned up > after > > > being used. > > > > Instead, can you change your code to make sure there is a timer before calling > > this? > > > > if (ShouldWaitForResponse() && > > remote_device_to_timer_map_.find(remote_device) != > > remote_device_to_timer_map_.end()) { > > StopResponseTimerForDevice(remote_device); > > } > > What is the difference between checking if the map holds the timer inside or > outside of the method? The only difference I can see is that it's more > self-contained within the method. Yep, I just wanted to make the method more clearly defined. In general, I try to avoid functions called DoSomething() which don't actually do the thing they say they do. If you are set on this design, then change the function name (e.g., StopResponseTimerForDeviceIfActive()). https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/disconnect_tethering_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/disconnect_tethering_operation_unittest.cc:113: TEST_F(DisconnectTetheringOperationTest, TestShouldNotWaitForResponse) { nit: I don't think this test is really necessary. You already test that the operation is finished in the TestSuccess test above. Since ShouldWaitForResponse() is so trivial, this test basically just asserts that a function which returns false returns false, which I don't think provides much value. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:78: // host device will be unregistered. nit: Adjust wording to make it clear that there is a timeout for each device (currently, it says "the host device" which can be misconstrued as meaning that there is only one host device). https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:143: std::string device_id_for_next_timer_ = std::string(); You don't have to initialize this to anything. The default constructor is called by default if you don't use an initializer list in the constructor. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:236: void VerifyTimerCreatedForDevice( nit: Change this to VerifyDefaultTimeoutTimerCreatedForDevice() and pass the constant value as the second parameter to the other function (you'll want to move the "5" constant to the anonymous namespace and reference both from here and from the TestOperation class). As written, this function is a little odd because it will always pass if a timer has been created, regardless of the timeout being set. In the future, someone could edit this test and call this function again, which could cause the test to pass even if it is not expected to. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:367: uint32_t response_timeout_seconds = 90; const uint32_t kResponseTimeoutSeconds https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:431: ; Remove.
https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... File chromeos/components/tether/message_transfer_operation.cc (right): https://codereview.chromium.org/2915713002/diff/1/chromeos/components/tether/... chromeos/components/tether/message_transfer_operation.cc:188: if (!wait_for_response_ || !remote_device_to_timer_map_[remote_device]) On 2017/06/01 18:18:58, Kyle Horimoto wrote: > On 2017/06/01 00:31:58, Ryan Hansberry wrote: > > On 2017/05/31 21:55:33, Kyle Horimoto wrote: > > > On 2017/05/31 21:19:03, Ryan Hansberry wrote: > > > > On 2017/05/31 17:26:35, khorimoto wrote: > > > > > Instead of returning early > > if(!remote_device_to_timer_map_[remote_device]), > > > > just > > > > > don't ever call this function if a timer is not active. It should be > > pretty > > > > > simple - just remove the > remote_device_to_timer_map_.erase(remote_device) > > > call > > > > > in OnResponseTimeout() below. > > > > > > > > That can't work, because this method is called from UnregisterDevice, > which > > > can > > > > be called before OnDeviceAuthenticated is called (when the timer is > > created). > > > > TLDR: it's valid for this method to be called without a timer existing. It > > > also > > > > just feels easier to reason about that timers are completely cleaned up > > after > > > > being used. > > > > > > Instead, can you change your code to make sure there is a timer before > calling > > > this? > > > > > > if (ShouldWaitForResponse() && > > > remote_device_to_timer_map_.find(remote_device) != > > > remote_device_to_timer_map_.end()) { > > > StopResponseTimerForDevice(remote_device); > > > } > > > > What is the difference between checking if the map holds the timer inside or > > outside of the method? The only difference I can see is that it's more > > self-contained within the method. > > Yep, I just wanted to make the method more clearly defined. In general, I try to > avoid functions called DoSomething() which don't actually do the thing they say > they do. > > If you are set on this design, then change the function name (e.g., > StopResponseTimerForDeviceIfActive()). Done. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/disconnect_tethering_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/disconnect_tethering_operation_unittest.cc:113: TEST_F(DisconnectTetheringOperationTest, TestShouldNotWaitForResponse) { On 2017/06/01 18:18:58, Kyle Horimoto wrote: > nit: I don't think this test is really necessary. You already test that the > operation is finished in the TestSuccess test above. Since > ShouldWaitForResponse() is so trivial, this test basically just asserts that a > function which returns false returns false, which I don't think provides much > value. Fair enough, removed. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation.h (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation.h:78: // host device will be unregistered. On 2017/06/01 18:18:58, Kyle Horimoto wrote: > nit: Adjust wording to make it clear that there is a timeout for each device > (currently, it says "the host device" which can be misconstrued as meaning that > there is only one host device). Good catch, clarified. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... File chromeos/components/tether/message_transfer_operation_unittest.cc (right): https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:143: std::string device_id_for_next_timer_ = std::string(); On 2017/06/01 18:18:58, Kyle Horimoto wrote: > You don't have to initialize this to anything. The default constructor is called > by default if you don't use an initializer list in the constructor. Oops :) https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:236: void VerifyTimerCreatedForDevice( On 2017/06/01 18:18:58, Kyle Horimoto wrote: > nit: Change this to VerifyDefaultTimeoutTimerCreatedForDevice() and pass the > constant value as the second parameter to the other function (you'll want to > move the "5" constant to the anonymous namespace and reference both from here > and from the TestOperation class). As written, this function is a little odd > because it will always pass if a timer has been created, regardless of the > timeout being set. In the future, someone could edit this test and call this > function again, which could cause the test to pass even if it is not expected > to. Done. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:367: uint32_t response_timeout_seconds = 90; On 2017/06/01 18:18:59, Kyle Horimoto wrote: > const uint32_t kResponseTimeoutSeconds Done. https://codereview.chromium.org/2915713002/diff/60001/chromeos/components/tet... chromeos/components/tether/message_transfer_operation_unittest.cc:431: ; On 2017/06/01 18:18:58, Kyle Horimoto wrote: > Remove. Done.
The CQ bit was checked by hansberry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2915713002/#ps100001 (title: "Remove useless test method.")
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": 100001, "attempt_start_ts": 1496342770617630,
"parent_rev": "625e0f8156b02d6dd9544d86defcd0f6665ecae9", "commit_rev":
"e774ca8c21b698fa805b0b45ea90b0fcdcc36384"}
Message was sent while issue was closed.
Description was changed from ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. BUG=672263 ========== to ========== Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out. BUG=672263 Review-Url: https://codereview.chromium.org/2915713002 Cr-Commit-Position: refs/heads/master@{#476382} Committed: https://chromium.googlesource.com/chromium/src/+/e774ca8c21b698fa805b0b45ea90... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e774ca8c21b698fa805b0b45ea90... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
