Clean up FtpTransactionFacory ownership.

net/url_request/ftp_protocol_handler.cc

 // Copyright (c) 2012 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 "net/url_request/ftp_protocol_handler.h"
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "net/base/net_errors.h"
 #include "net/base/port_util.h"
 #include "net/ftp/ftp_auth_cache.h"
+#include "net/ftp/ftp_network_layer.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_error_job.h"
 #include "net/url_request/url_request_ftp_job.h"
 #include "url/gurl.h"
 
 namespace net {
 
-FtpProtocolHandler::FtpProtocolHandler(
-    FtpTransactionFactory* ftp_transaction_factory)
-    : ftp_transaction_factory_(ftp_transaction_factory),
-      ftp_auth_cache_(new FtpAuthCache) {
-  DCHECK(ftp_transaction_factory_);
+std::unique_ptr<FtpProtocolHandler> FtpProtocolHandler::Create(
+    HostResolver* host_resolver) {

[eroman, 2016/10/19 16:29:51]

It would be good to have DCHECK that |host_resolver| is not null somewhere.

(Since we plumb it down a bunch of layers and may not surface as a crash until
FTP is used, which may be rarely)

Perhaps a DCHECK here, or in FtpNetworkLayer, or in FtpNetworkSession ?

[mmenke, 2016/10/19 17:09:35]

SGTM, done.  Added to FtpNetworkLayer (Wider coverage than in
FtpProtocolHandler.  Not sure FtpNetworkSession really should exist in the first
place).

+  return base::WrapUnique(

[eroman, 2016/10/19 16:29:51]

style: this line uses both WrapUnique and MakeUnique -- would read more
consistently it if only used one pattern for constructing the two classes.

[mmenke, 2016/10/19 17:09:35]

Switched to WrapUnique (Due to the private constructor, can't use MakeUnique to
make an FtpProtocolHandler, without make MakeUnique a friend, which I prefer to
avoid).

+      new FtpProtocolHandler(base::MakeUnique<FtpNetworkLayer>(host_resolver)));
+}
+
+std::unique_ptr<FtpProtocolHandler> FtpProtocolHandler::CreateForTesting(
+    std::unique_ptr<FtpTransactionFactory> ftp_transaction_factory) {
+  return base::WrapUnique(
+      new FtpProtocolHandler(std::move(ftp_transaction_factory)));
 }
 
 FtpProtocolHandler::~FtpProtocolHandler() {
 }
 
 URLRequestJob* FtpProtocolHandler::MaybeCreateJob(
     URLRequest* request, NetworkDelegate* network_delegate) const {
   DCHECK_EQ("ftp", request->url().scheme());
 
   if (!IsPortAllowedForScheme(request->url().EffectiveIntPort(),
                               request->url().scheme())) {
     return new URLRequestErrorJob(request, network_delegate, ERR_UNSAFE_PORT);
   }
 
-  return new URLRequestFtpJob(request,
-                              network_delegate,
-                              ftp_transaction_factory_,
+  return new URLRequestFtpJob(request, network_delegate,
+                              ftp_transaction_factory_.get(),
                               ftp_auth_cache_.get());
 }
 
+FtpProtocolHandler::FtpProtocolHandler(
+    std::unique_ptr<FtpTransactionFactory> ftp_transaction_factory)
+    : ftp_transaction_factory_(std::move(ftp_transaction_factory)),
+      ftp_auth_cache_(new FtpAuthCache) {
+  DCHECK(ftp_transaction_factory_);
+}
+
 }  // namespace net