[CrOS MultiDevice] Refactor PendingConnectionRequest.

The class has been split between a pure-virtual class, an implementation
class, and a test double. The test double will be used in a follow-up
CL.

Note: This CL also removes the is_active() function from the class. As
I was implementing contents for a future CL, I realized it was never
actually used.

Bug: 824568, 752273
Change-Id: I748b5400ee02f86d1978fa866febf85132acb153
Reviewed-on: https://chromium-review.googlesource.com/1050951
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557262}

chromeos/services/secure_channel/BUILD.gn

@@ -7,6 +7,7 @@ assert(is_chromeos, "Non-ChromeOS builds cannot depend on //chromeos")
 static_library("secure_channel") {
   sources = [
     "pending_connection_request.h",
+    "pending_connection_request_base.h",
     "pending_connection_request_delegate.cc",
     "pending_connection_request_delegate.h",
   ]
@@ -24,6 +25,8 @@ static_library("test_support") {
   sources = [
     "fake_connection_delegate.cc",
     "fake_connection_delegate.h",
+    "fake_pending_connection_request.cc",
+    "fake_pending_connection_request.h",
     "fake_pending_connection_request_delegate.cc",
     "fake_pending_connection_request_delegate.h",
   ]
@@ -39,7 +42,7 @@ source_set("unit_tests") {
   testonly = true
 
   sources = [
-    "pending_connection_request_unittest.cc",
+    "pending_connection_request_base_unittest.cc",
   ]
 
   deps = [

chromeos/services/secure_channel/fake_pending_connection_request.cc

+// Copyright 2018 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.
+
+#include "chromeos/services/secure_channel/fake_pending_connection_request.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+FakePendingConnectionRequest::FakePendingConnectionRequest(
+    PendingConnectionRequestDelegate* delegate)
+    : PendingConnectionRequest<std::string>(delegate) {}
+
+FakePendingConnectionRequest::~FakePendingConnectionRequest() = default;
+
+void FakePendingConnectionRequest::HandleConnectionFailure(
+    std::string failure_detail) {
+  handled_failure_details_.push_back(failure_detail);
+}
+
+}  // namespace secure_channel
+
+}  // namespace chromeos

chromeos/services/secure_channel/fake_pending_connection_request.h

+// Copyright 2018 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_SERVICES_SECURE_CHANNEL_FAKE_PENDING_CONNECTION_REQUEST_H_
+#define CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_PENDING_CONNECTION_REQUEST_H_
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "chromeos/services/secure_channel/pending_connection_request.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+class PendingConnectionRequestDelegate;
+
+// Fake PendingConnectionRequest implementation, whose FailureDetailType is
+// std::string.
+class FakePendingConnectionRequest
+    : public PendingConnectionRequest<std::string> {
+ public:
+  FakePendingConnectionRequest(PendingConnectionRequestDelegate* delegate);
+  ~FakePendingConnectionRequest() override;
+
+  const std::vector<std::string>& handled_failure_details() const {
+    return handled_failure_details_;
+  }
+
+  // Make NotifyRequestFinishedWithoutConnection() public for testing.
+  using PendingConnectionRequest<
+      std::string>::NotifyRequestFinishedWithoutConnection;
+
+ private:
+  // PendingConnectionRequest<std::string>:
+  void HandleConnectionFailure(std::string failure_detail) override;
+
+  std::vector<std::string> handled_failure_details_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakePendingConnectionRequest);
+};
+
+}  // namespace secure_channel
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_PENDING_CONNECTION_REQUEST_H_

chromeos/services/secure_channel/pending_connection_request.h

@@ -7,25 +7,16 @@
 
 #include "base/guid.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/services/secure_channel/pending_connection_request_delegate.h"
-#include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
 
 namespace chromeos {
 
 namespace secure_channel {
 
 // Encapsulates metadata for a pending request for a connection to a remote
-// device. Every PendingConnectionRequest starts out active (i.e., there exists
-// an ongoing attempt to create a connection). The client of this class can
-// cancel an active attempt by disconnecting the ConnectionDelegatePtr passed
-// PendingConnectionRequest's constructor; likewise, a PendingConnectionRequest
-// can become inactive due to connection failures.
-//
-// PendingConnectionRequest is templatized so that each derived class can
-// specify its own error-handling for connection failures; for instance, some
-// derived classes may choose to continue an ongoing connection attempt
+// device. PendingConnectionRequest is templatized so that each derived class
+// can specify its own error-handling for connection failures; for instance,
+// some derived classes may choose to continue an ongoing connection attempt
 // indefinitely, while others may choose to handle connection failures by giving
 // up on the request entirely.
 template <typename FailureDetailType>
@@ -37,8 +28,6 @@ class PendingConnectionRequest {
   // trying to connect after some number of failures.
   virtual void HandleConnectionFailure(FailureDetailType failure_detail) = 0;
 
-  bool is_active() { return is_active_; }
-
   // Note: Request ID is guaranteed to be unique among all requests.
   const std::string& GetRequestId() {
     static const std::string kRequestId = base::GenerateGUID();
@@ -46,60 +35,18 @@ class PendingConnectionRequest {
   }
 
  protected:
-  PendingConnectionRequest(const std::string& feature,
-                           const std::string& readable_request_type_for_logging,
-                           PendingConnectionRequestDelegate* delegate,
-                           mojom::ConnectionDelegatePtr connection_delegate_ptr)
-      : feature_(feature),
-        readable_request_type_for_logging_(readable_request_type_for_logging),
-        delegate_(delegate),
-        connection_delegate_ptr_(std::move(connection_delegate_ptr)),
-        weak_ptr_factory_(this) {
-    DCHECK(!feature_.empty());
+  PendingConnectionRequest(PendingConnectionRequestDelegate* delegate)
+      : delegate_(delegate) {
     DCHECK(delegate_);
-    DCHECK(connection_delegate_ptr_);
-
-    // If the client disconnects its delegate, the client is signaling that the
-    // connection request has been canceled.
-    connection_delegate_ptr_.set_connection_error_handler(
-        base::BindOnce(&PendingConnectionRequest::
-                           SetInactiveAndNotifyRequestFinishedWithoutConnection,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       PendingConnectionRequestDelegate::
-                           FailedConnectionReason::kRequestCanceledByClient));
   }
 
-  // Derived classes should invoke this function if they would like to give up
-  // on the request due to connection failures.
-  void StopRequestDueToConnectionFailures() {
-    SetInactiveAndNotifyRequestFinishedWithoutConnection(
-        PendingConnectionRequestDelegate::FailedConnectionReason::
-            kRequestFailed);
-  }
-
- private:
-  void SetInactiveAndNotifyRequestFinishedWithoutConnection(
+  void NotifyRequestFinishedWithoutConnection(
       PendingConnectionRequestDelegate::FailedConnectionReason reason) {
-    // If the request is already inactive, there is nothing else to do.
-    if (!is_active_)
-      return;
-
-    PA_LOG(INFO) << "Request finished without connection; notifying delegate. "
-                 << "Feature: \"" << feature_ << "\""
-                 << ", Reason: " << reason << ", Request type: \""
-                 << readable_request_type_for_logging_ << "\"";
-
-    is_active_ = false;
     delegate_->OnRequestFinishedWithoutConnection(GetRequestId(), reason);
   }
 
-  const std::string feature_;
-  const std::string readable_request_type_for_logging_;
+ private:
   PendingConnectionRequestDelegate* delegate_;
-  mojom::ConnectionDelegatePtr connection_delegate_ptr_;
-
-  bool is_active_ = true;
-  base::WeakPtrFactory<PendingConnectionRequest> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingConnectionRequest);
 };

chromeos/services/secure_channel/pending_connection_request_base.h

+// Copyright 2018 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_SERVICES_SECURE_CHANNEL_PENDING_CONNECTION_REQUEST_BASE_H_
+#define CHROMEOS_SERVICES_SECURE_CHANNEL_PENDING_CONNECTION_REQUEST_BASE_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chromeos/components/proximity_auth/logging/logging.h"
+#include "chromeos/services/secure_channel/pending_connection_request.h"
+#include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+// Encapsulates metadata for a pending request for a connection to a remote
+// device. Every PendingConnectionRequestBase starts out active (i.e., there
+// exists an ongoing attempt to create a connection). The client of this class
+// can cancel an active attempt by disconnecting the ConnectionDelegatePtr
+// passed PendingConnectionRequestBase's constructor; likewise, a
+// PendingConnectionRequestBase can become inactive due to connection failures.
+//
+// Each connection type should implement its own pending request class deriving
+// from PendingConnectionRequestBase.
+template <typename FailureDetailType>
+class PendingConnectionRequestBase
+    : public PendingConnectionRequest<FailureDetailType> {
+ public:
+  ~PendingConnectionRequestBase() override = default;
+
+ protected:
+  PendingConnectionRequestBase(
+      const std::string& feature,
+      const std::string& readable_request_type_for_logging,
+      PendingConnectionRequestDelegate* delegate,
+      mojom::ConnectionDelegatePtr connection_delegate_ptr)
+      : PendingConnectionRequest<FailureDetailType>(delegate),
+        feature_(feature),
+        readable_request_type_for_logging_(readable_request_type_for_logging),
+        connection_delegate_ptr_(std::move(connection_delegate_ptr)),
+        weak_ptr_factory_(this) {
+    DCHECK(!feature_.empty());
+    DCHECK(connection_delegate_ptr_);
+
+    // If the client disconnects its delegate, the client is signaling that the
+    // connection request has been canceled.
+    connection_delegate_ptr_.set_connection_error_handler(base::BindOnce(
+        &PendingConnectionRequestBase::OnFinishedWithoutConnection,
+        weak_ptr_factory_.GetWeakPtr(),
+        PendingConnectionRequestDelegate::FailedConnectionReason::
+            kRequestCanceledByClient));
+  }
+
+  // Derived classes should invoke this function if they would like to give up
+  // on the request due to connection failures.
+  void StopRequestDueToConnectionFailures() {
+    OnFinishedWithoutConnection(PendingConnectionRequestDelegate::
+                                    FailedConnectionReason::kRequestFailed);
+  }
+
+ private:
+  // Make NotifyRequestFinishedWithoutConnection() inaccessible to derived
+  // types, which should use StopRequestDueToConnectionFailures() instead.
+  using PendingConnectionRequest<
+      FailureDetailType>::NotifyRequestFinishedWithoutConnection;
+
+  void OnFinishedWithoutConnection(
+      PendingConnectionRequestDelegate::FailedConnectionReason reason) {
+    PA_LOG(INFO) << "Request finished without connection; notifying delegate. "
+                 << "Feature: \"" << feature_ << "\""
+                 << ", Reason: " << reason << ", Request type: \""
+                 << readable_request_type_for_logging_ << "\"";
+    NotifyRequestFinishedWithoutConnection(reason);
+  }
+
+  const std::string feature_;
+  const std::string readable_request_type_for_logging_;
+  mojom::ConnectionDelegatePtr connection_delegate_ptr_;
+
+  base::WeakPtrFactory<PendingConnectionRequestBase> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PendingConnectionRequestBase);
+};
+
+}  // namespace secure_channel
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_PENDING_CONNECTION_REQUEST_BASE_H_

chromeos/services/secure_channel/pending_connection_request_unittest.cc → chromeos/services/secure_channel/pending_connection_request_base_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/services/secure_channel/pending_connection_request.h"
+#include "chromeos/services/secure_channel/pending_connection_request_base.h"
 
 #include <memory>
 
@@ -26,23 +26,23 @@ enum class TestFailureDetail {
   kReasonWhichDoesNotCauseRequestToBecomeInactive
 };
 
-// Since PendingConnectionRequest is templatized, a concrete implementation is
-// needed for its test.
+// Since PendingConnectionRequestBase is templatized, a concrete implementation
+// is needed for its test.
 class TestPendingConnectionRequest
-    : public PendingConnectionRequest<TestFailureDetail> {
+    : public PendingConnectionRequestBase<TestFailureDetail> {
  public:
   TestPendingConnectionRequest(
       const std::string& feature,
       mojom::ConnectionDelegatePtr connection_delegate_ptr,
       PendingConnectionRequestDelegate* delegate)
-      : PendingConnectionRequest<TestFailureDetail>(
+      : PendingConnectionRequestBase<TestFailureDetail>(
             feature,
             kTestReadableRequestTypeForLogging,
             delegate,
             std::move(connection_delegate_ptr)) {}
   ~TestPendingConnectionRequest() override = default;
 
-  // PendingConnectionRequest<TestFailureDetail>:
+  // PendingConnectionRequestBase<TestFailureDetail>:
   void HandleConnectionFailure(TestFailureDetail failure_detail) override {
     switch (failure_detail) {
       case TestFailureDetail::kReasonWhichCausesRequestToBecomeInactive:
@@ -57,10 +57,10 @@ class TestPendingConnectionRequest
 
 }  // namespace
 
-class SecureChannelPendingConnectionRequestTest : public testing::Test {
+class SecureChannelPendingConnectionRequestBaseTest : public testing::Test {
  protected:
-  SecureChannelPendingConnectionRequestTest() = default;
-  ~SecureChannelPendingConnectionRequestTest() override = default;
+  SecureChannelPendingConnectionRequestBaseTest() = default;
+  ~SecureChannelPendingConnectionRequestBaseTest() override = default;
 
   void SetUp() override {
     fake_connection_delegate_ = std::make_unique<FakeConnectionDelegate>();
@@ -102,35 +102,32 @@ class SecureChannelPendingConnectionRequestTest : public testing::Test {
   std::unique_ptr<TestPendingConnectionRequest>
       test_pending_connection_request_;
 
-  DISALLOW_COPY_AND_ASSIGN(SecureChannelPendingConnectionRequestTest);
+  DISALLOW_COPY_AND_ASSIGN(SecureChannelPendingConnectionRequestBaseTest);
 };
 
-TEST_F(SecureChannelPendingConnectionRequestTest,
+TEST_F(SecureChannelPendingConnectionRequestBaseTest,
        HandleConnectionFailureWhichCausesRequestToBecomeInactive) {
   test_pending_connection_request()->HandleConnectionFailure(
       TestFailureDetail::kReasonWhichCausesRequestToBecomeInactive);
-  EXPECT_FALSE(test_pending_connection_request()->is_active());
   EXPECT_EQ(
       PendingConnectionRequestDelegate::FailedConnectionReason::kRequestFailed,
       *GetFailedConnectionReason());
 }
 
-TEST_F(SecureChannelPendingConnectionRequestTest,
+TEST_F(SecureChannelPendingConnectionRequestBaseTest,
        HandleConnectionFailureWhichDoesNotCauseRequestToBecomeInactive) {
   // Repeat 5 connection failures, none of which should cause the request to
   // become inactive.
   for (int i = 0; i < 5; ++i) {
     test_pending_connection_request()->HandleConnectionFailure(
         TestFailureDetail::kReasonWhichDoesNotCauseRequestToBecomeInactive);
-    EXPECT_TRUE(test_pending_connection_request()->is_active());
     EXPECT_FALSE(GetFailedConnectionReason());
   }
 }
 
-TEST_F(SecureChannelPendingConnectionRequestTest,
+TEST_F(SecureChannelPendingConnectionRequestBaseTest,
        ConnectionDelegateInvalidated) {
   DisconnectConnectionDelegatePtr();
-  EXPECT_FALSE(test_pending_connection_request()->is_active());
   EXPECT_EQ(PendingConnectionRequestDelegate::FailedConnectionReason::
                 kRequestCanceledByClient,
             *GetFailedConnectionReason());