Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time …

chromeos/components/tether/message_transfer_operation.h

Index: chromeos/components/tether/message_transfer_operation.h
diff --git a/chromeos/components/tether/message_transfer_operation.h b/chromeos/components/tether/message_transfer_operation.h
index 3265214002750504c3e40f3c9e71205d82c4d08f..7e1d88a6a1931f9efde1cb13e5eeb1b047ddb216 100644
--- a/chromeos/components/tether/message_transfer_operation.h
+++ b/chromeos/components/tether/message_transfer_operation.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chromeos/components/tether/ble_connection_manager.h"
 
 namespace chromeos {
@@ -16,6 +17,7 @@ namespace chromeos {
 namespace tether {
 
 class MessageWrapper;
+class TimerFactory;
 
 // Abstract base class used for operations which send and/or receive messages
 // from remote devices.
@@ -70,10 +72,17 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   // Returns the type of message that this operation intends to send.
   virtual MessageType GetMessageTypeForConnection() = 0;
 
+  void SetWaitForResponse(bool wait_for_response) {

[khorimoto, 2017/05/31 17:26:35]

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.

[Ryan Hansberry, 2017/05/31 21:19:04]

Done. Same done for response_timeout_seconds.

+    wait_for_response_ = wait_for_response;
+  }
+
   std::vector<cryptauth::RemoteDevice>& remote_devices() {
     return remote_devices_;
   }
 
+  void SetTimerFactoryForTest(

[khorimoto, 2017/05/31 17:26:35]

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.

[Kyle Horimoto, 2017/05/31 21:55:33]

Ping.

[Ryan Hansberry, 2017/06/01 00:31:58]

Done.

+      std::unique_ptr<TimerFactory> timer_factory_for_test);
+
  private:
   friend class ConnectTetheringOperationTest;
   friend class DisconnectTetheringOperationTest;
@@ -82,12 +91,36 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
 
   static uint32_t kMaxConnectionAttemptsPerDevice;
 
+  // The number of seconds the client should generally wait for a response from
+  // the host once an authenticated connection is established. Once this amount
+  // of time passes, the connection will be closed. Subclasses of
+  // MessageTransferOperation may desire a shorter or longer duration; see
+  // SetResponseTimeout().

[khorimoto, 2017/05/31 17:26:35]

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.

[Ryan Hansberry, 2017/05/31 21:19:04]

Done.

+  static uint32_t kResponseTimeoutSeconds;

[khorimoto, 2017/05/31 17:26:35]

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.

[Ryan Hansberry, 2017/05/31 21:19:04]

Discussed offline; skipping over this due to linking issues when using EXPECT_EQ
with a constexpr.

+
+  void StartResponseTimerForDevice(
+      const cryptauth::RemoteDevice& remote_device);
+  void StopResponseTimerForDevice(const cryptauth::RemoteDevice& remote_device);
+  void OnResponseTimeout(const cryptauth::RemoteDevice& remote_device);
+
+  // Exposed for testing.
+  base::Timer* GetResponseTimerForDevice(

[khorimoto, 2017/05/31 17:26:35]

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...

[Ryan Hansberry, 2017/05/31 21:19:03]

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.

[Kyle Horimoto, 2017/05/31 21:55:33]

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.

[Ryan Hansberry, 2017/06/01 00:31:58]

Resolved offline -- Done.

+      const cryptauth::RemoteDevice& remote_device) {
+    return remote_device_to_timer_map_[remote_device].get();
+  }
+
   std::vector<cryptauth::RemoteDevice> remote_devices_;
   BleConnectionManager* connection_manager_;
+  std::unique_ptr<TimerFactory> timer_factory_;
 
   bool initialized_;
+  bool wait_for_response_;
+  uint32_t response_timeout_seconds_;
   std::map<cryptauth::RemoteDevice, uint32_t>
       remote_device_to_num_attempts_map_;
+  std::map<cryptauth::RemoteDevice, std::unique_ptr<base::Timer>>
+      remote_device_to_timer_map_;
+  base::WeakPtrFactory<MessageTransferOperation> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MessageTransferOperation);
 };