Refactor SyncServerConnectionManager construction signature

The constructor takes ownership of the injected HttpPostProviderFactory
so let's trivially adopt std::unique_ptr to make this obvious.

While doing this, some member fields are trivially marked as const.

Bug: None
Change-Id: Ieb199dc48c71d68fe6354f1f075a7977f72e7299
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626416
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662594}

components/sync/engine_impl/net/server_connection_manager.h

@@ -179,10 +179,6 @@ class ServerConnectionManager {
 
   const std::string client_id() const { return client_id_; }
 
-  // Factory method to create an Connection object we can use for
-  // communication with the server.
-  virtual std::unique_ptr<Connection> MakeConnection();
-
   void set_client_id(const std::string& client_id) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DCHECK(client_id_.empty());
@@ -203,6 +199,10 @@ class ServerConnectionManager {
   // changed.
   void SetServerResponse(const HttpResponse& server_response);
 
+  // Factory method to create an Connection object we can use for communication
+  // with the server. The returned object must not outlive |*this|.
+  virtual std::unique_ptr<Connection> MakeConnection();
+
   // NOTE: Tests rely on this protected function being virtual.
   //
   // Internal PostBuffer base function.
@@ -213,7 +213,8 @@ class ServerConnectionManager {
   void ClearAccessToken();
 
   // Helper to check terminated flags and build a Connection object. If this
-  // ServerConnectionManager has been terminated, this will return null.
+  // ServerConnectionManager has been terminated, this will return null. The
+  // returned object must not outlive |*this|.
   std::unique_ptr<Connection> MakeActiveConnection();
 
  private:

components/sync/engine_impl/net/sync_server_connection_manager.cc

@@ -6,6 +6,8 @@
 
 #include <stdint.h>
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "components/sync/base/cancelation_signal.h"
@@ -22,14 +24,16 @@ SyncBridgedConnection::SyncBridgedConnection(
     CancelationSignal* cancelation_signal)
     : Connection(scm),
       factory_(factory),
-      cancelation_signal_(cancelation_signal) {
-  post_provider_ = factory_->Create();
+      cancelation_signal_(cancelation_signal),
+      post_provider_(factory_->Create()) {
+  DCHECK(scm);
+  DCHECK(factory);
+  DCHECK(cancelation_signal);
+  DCHECK(post_provider_);
 }
 
 SyncBridgedConnection::~SyncBridgedConnection() {
-  DCHECK(post_provider_);
   factory_->Destroy(post_provider_);
-  post_provider_ = nullptr;
 }
 
 bool SyncBridgedConnection::Init(const char* path,
@@ -103,12 +107,13 @@ SyncServerConnectionManager::SyncServerConnectionManager(
     const std::string& server,
     int port,
     bool use_ssl,
-    HttpPostProviderFactory* factory,
+    std::unique_ptr<HttpPostProviderFactory> factory,
     CancelationSignal* cancelation_signal)
     : ServerConnectionManager(server, port, use_ssl, cancelation_signal),
-      post_provider_factory_(factory),
+      post_provider_factory_(std::move(factory)),
       cancelation_signal_(cancelation_signal) {
   DCHECK(post_provider_factory_);
+  DCHECK(cancelation_signal_);
 }
 
 SyncServerConnectionManager::~SyncServerConnectionManager() = default;

components/sync/engine_impl/net/sync_server_connection_manager.h

@@ -24,6 +24,7 @@ class HttpPostProviderInterface;
 class SyncBridgedConnection : public ServerConnectionManager::Connection,
                               public CancelationObserver {
  public:
+  // All pointers must not be null and must outlive this object.
   SyncBridgedConnection(ServerConnectionManager* scm,
                         HttpPostProviderFactory* factory,
                         CancelationSignal* cancelation_signal);
@@ -40,13 +41,13 @@ class SyncBridgedConnection : public ServerConnectionManager::Connection,
  private:
   // Pointer to the factory we use for creating HttpPostProviders. We do not
   // own |factory_|.
-  HttpPostProviderFactory* factory_;
+  HttpPostProviderFactory* const factory_;
 
   // Cancelation signal is signalled when engine shuts down. Current blocking
   // operation should be aborted.
-  CancelationSignal* cancelation_signal_;
+  CancelationSignal* const cancelation_signal_;
 
-  HttpPostProviderInterface* post_provider_;
+  HttpPostProviderInterface* const post_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncBridgedConnection);
 };
@@ -55,14 +56,16 @@ class SyncBridgedConnection : public ServerConnectionManager::Connection,
 // instance of the HttpPostProviderFactory class.
 class SyncServerConnectionManager : public ServerConnectionManager {
  public:
-  // Takes ownership of factory.
+  // |factory| and |cancelation_signal| must not be null, and the latter must
+  // outlive this object.
   SyncServerConnectionManager(const std::string& server,
                               int port,
                               bool use_ssl,
-                              HttpPostProviderFactory* factory,
+                              std::unique_ptr<HttpPostProviderFactory> factory,
                               CancelationSignal* cancelation_signal);
   ~SyncServerConnectionManager() override;
 
+ protected:
   // ServerConnectionManager overrides.
   std::unique_ptr<Connection> MakeConnection() override;
 
@@ -79,7 +82,7 @@ class SyncServerConnectionManager : public ServerConnectionManager {
 
   // Cancelation signal is signalled when engine shuts down. Current blocking
   // operation should be aborted.
-  CancelationSignal* cancelation_signal_;
+  CancelationSignal* const cancelation_signal_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncServerConnectionManager);
 };

components/sync/engine_impl/net/sync_server_connection_manager_unittest.cc

@@ -71,8 +71,8 @@ class BlockingHttpPostFactory : public HttpPostProviderFactory {
 TEST(SyncServerConnectionManagerTest, VeryEarlyAbortPost) {
   CancelationSignal signal;
   signal.Signal();
-  SyncServerConnectionManager server("server", 0, true,
-                                     new BlockingHttpPostFactory(), &signal);
+  SyncServerConnectionManager server(
+      "server", 0, true, std::make_unique<BlockingHttpPostFactory>(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
 
@@ -86,8 +86,8 @@ TEST(SyncServerConnectionManagerTest, VeryEarlyAbortPost) {
 // Ask the ServerConnectionManager to stop before its first request is made.
 TEST(SyncServerConnectionManagerTest, EarlyAbortPost) {
   CancelationSignal signal;
-  SyncServerConnectionManager server("server", 0, true,
-                                     new BlockingHttpPostFactory(), &signal);
+  SyncServerConnectionManager server(
+      "server", 0, true, std::make_unique<BlockingHttpPostFactory>(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
 
@@ -102,8 +102,8 @@ TEST(SyncServerConnectionManagerTest, EarlyAbortPost) {
 // Ask the ServerConnectionManager to stop during a request.
 TEST(SyncServerConnectionManagerTest, AbortPost) {
   CancelationSignal signal;
-  SyncServerConnectionManager server("server", 0, true,
-                                     new BlockingHttpPostFactory(), &signal);
+  SyncServerConnectionManager server(
+      "server", 0, true, std::make_unique<BlockingHttpPostFactory>(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
 
@@ -178,8 +178,8 @@ class FailingHttpPostFactory : public HttpPostProviderFactory {
 TEST(SyncServerConnectionManagerTest, FailPostWithTimedOut) {
   CancelationSignal signal;
   SyncServerConnectionManager server(
-      "server", 0, true, new FailingHttpPostFactory(net::ERR_TIMED_OUT),
-      &signal);
+      "server", 0, true,
+      std::make_unique<FailingHttpPostFactory>(net::ERR_TIMED_OUT), &signal);
 
   ServerConnectionManager::PostBufferParams params;
 

components/sync/engine_impl/sync_manager_impl.cc

@@ -384,8 +384,8 @@ void SyncManagerImpl::Init(InitArgs* args) {
     connection_manager_ = std::make_unique<SyncServerConnectionManager>(
         args->service_url.host() + args->service_url.path(),
         args->service_url.EffectiveIntPort(),
-        args->service_url.SchemeIsCryptographic(), args->post_factory.release(),
-        args->cancelation_signal);
+        args->service_url.SchemeIsCryptographic(),
+        std::move(args->post_factory), args->cancelation_signal);
   }
   connection_manager_->set_client_id(directory()->cache_guid());
   connection_manager_->AddListener(this);