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

chromeos/components/tether/message_transfer_operation.h

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROMEOS_COMPONENTS_TETHER_MESSAGE_TRANSFER_OPERATION_H_
 #define CHROMEOS_COMPONENTS_TETHER_MESSAGE_TRANSFER_OPERATION_H_
 
 #include <map>
 #include <vector>
 
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chromeos/components/tether/ble_connection_manager.h"
 
 namespace chromeos {
 
 namespace tether {
 
 class MessageWrapper;
+class TimerFactory;
 
 // Abstract base class used for operations which send and/or receive messages
 // from remote devices.
 class MessageTransferOperation : public BleConnectionManager::Observer {
  public:
   MessageTransferOperation(
       const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
       BleConnectionManager* connection_manager);
   virtual ~MessageTransferOperation();
 
@@ skipping 34 matching lines @@
   // Callback executed when the operation has started (i.e., in Initialize()).
   virtual void OnOperationStarted() {}
 
   // Callback executed when the operation has finished (i.e., when all devices
   // have been unregistered).
   virtual void OnOperationFinished() {}
 
   // 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;
   friend class HostScannerOperationTest;
   friend class MessageTransferOperationTest;
 
   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);
 };
 
 }  // namespace tether
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_COMPONENTS_TETHER_MESSAGE_TRANSFER_OPERATION_H_