Stop DownloadManagerService from inheriting InProgressDownloadManager::Delegate

DownloadManagerService already listens to AllDownloadEventNofitier, so
there is no need to implement OnDownloadInitialized().
The service manager connector can also be provided to
InProgressDownloadManager ctor, so there is no need to implement
GetServiceConnector().

BUG=942770

Change-Id: Ibc143b832c734e3a1fac0af6044aba99f8d7c1b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1606182Reviewed-by: Tao Bai <michaelbai@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658755}

android_webview/browser/aw_browser_context.cc

@@ -390,7 +390,7 @@ AwBrowserContext::RetriveInProgressDownloadManager() {
   return new download::InProgressDownloadManager(
       nullptr, base::FilePath(),
       base::BindRepeating(&IgnoreOriginSecurityCheck),
-      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe));
+      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe), nullptr);
 }
 
 web_restrictions::WebRestrictionsClient*

chrome/browser/android/download/download_manager_service.cc

@@ -251,9 +251,6 @@ DownloadManagerService::RetriveInProgressDownloadManager(
     content::BrowserContext* context) {
   if (in_progress_manager_) {
     DCHECK(!context->IsOffTheRecord());
-    // We are no longer interested in listening to |in_progress_manager_|,
-    // remove this object from being the delegate.
-    in_progress_manager_->SetDelegate(nullptr);
     // Set |is_pending_downloads_loaded_| to false so that we need to wait for
     // download history to initialize before performing new download actions.
     is_pending_downloads_loaded_ = false;
@@ -436,8 +433,10 @@ void DownloadManagerService::CancelDownload(
 void DownloadManagerService::OnDownloadsInitialized(
     download::SimpleDownloadManagerCoordinator* coordinator,
     bool active_downloads_only) {
-  if (active_downloads_only)
+  if (active_downloads_only) {
+    OnPendingDownloadsLoaded();
     return;
+  }
   is_manager_initialized_ = true;
   OnPendingDownloadsLoaded();
 
@@ -699,11 +698,13 @@ void DownloadManagerService::CreateInProgressDownloadManager() {
     return;
   base::FilePath data_dir;
   base::android::GetDataDirectory(&data_dir);
+  service_manager::Connector* connector = service_binding_.GetConnector();
   in_progress_manager_ = std::make_unique<download::InProgressDownloadManager>(
-      this, data_dir.Append(chrome::kInitialProfile),
+      nullptr, data_dir.Append(chrome::kInitialProfile),
       base::BindRepeating(&IgnoreOriginSecurityCheck),
-      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe));
-  content::GetNetworkServiceFromConnector(service_binding_.GetConnector());
+      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe),
+      connector);
+  content::GetNetworkServiceFromConnector(connector);
   scoped_refptr<network::SharedURLLoaderFactory> factory =
       SystemNetworkContextManager::GetInstance()->GetSharedURLLoaderFactory();
   in_progress_manager_->set_url_loader_factory_getter(
@@ -762,18 +763,6 @@ void DownloadManagerService::OnPendingDownloadsLoaded() {
   pending_actions_.clear();
 }
 
-void DownloadManagerService::OnDownloadsInitialized() {
-  OnPendingDownloadsLoaded();
-}
-
-std::unique_ptr<service_manager::Connector>
-DownloadManagerService::GetServiceConnector() {
-  service_manager::Connector* connector = service_binding_.GetConnector();
-  if (connector)
-    return connector->Clone();  // Clone for use on a different thread.
-  return nullptr;
-}
-
 content::DownloadManager* DownloadManagerService::GetDownloadManager(
     bool is_off_the_record) {
   Profile* profile = ProfileManager::GetActiveUserProfile();

chrome/browser/android/download/download_manager_service.h

@@ -36,7 +36,6 @@ class SimpleDownloadManagerCoordinator;
 // Java object.
 class DownloadManagerService
     : public download::AllDownloadEventNotifier::Observer,
-      public download::InProgressDownloadManager::Delegate,
       public content::NotificationObserver,
       public service_manager::Service {
  public:
@@ -237,10 +236,6 @@ class DownloadManagerService
   // Called when all pending downloads are loaded.
   void OnPendingDownloadsLoaded();
 
-  // download::InProgressDownloadManager::Delegate implementations.
-  void OnDownloadsInitialized() override;
-  std::unique_ptr<service_manager::Connector> GetServiceConnector() override;
-
   typedef base::Callback<void(bool)> ResumeCallback;
   void set_resume_callback_for_testing(const ResumeCallback& resume_cb) {
     resume_callback_for_testing_ = resume_cb;

chrome/browser/download/download_manager_utils.cc

@@ -12,6 +12,7 @@
 #include "components/download/public/common/in_progress_download_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_request_utils.h"
+#include "content/public/common/service_manager_connection.h"
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/download/download_manager_service.h"
@@ -33,6 +34,13 @@ void GetDownloadManagerOnProfileCreation(Profile* profile) {
   DCHECK(manager);
 }
 
+service_manager::Connector* GetServiceConnector() {
+  auto* connection = content::ServiceManagerConnection::GetForProcess();
+  if (!connection)
+    return nullptr;
+  return connection->GetConnector();
+}
+
 }  // namespace
 
 download::InProgressDownloadManager*
@@ -50,7 +58,8 @@ DownloadManagerUtils::RetrieveInProgressDownloadManager(Profile* profile) {
       nullptr,
       profile->IsOffTheRecord() ? base::FilePath() : profile->GetPath(),
       base::BindRepeating(&IgnoreOriginSecurityCheck),
-      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe));
+      base::BindRepeating(&content::DownloadRequestUtils::IsURLSafe),
+      GetServiceConnector());
 }
 
 void DownloadManagerUtils::InitializeSimpleDownloadManager(

components/download/internal/common/in_progress_download_manager.cc

@@ -168,13 +168,15 @@ InProgressDownloadManager::InProgressDownloadManager(
     Delegate* delegate,
     const base::FilePath& in_progress_db_dir,
     const IsOriginSecureCallback& is_origin_secure_cb,
-    const URLSecurityPolicy& url_security_policy)
+    const URLSecurityPolicy& url_security_policy,
+    service_manager::Connector* connector)
     : delegate_(delegate),
       file_factory_(new DownloadFileFactory()),
       download_start_observer_(nullptr),
       is_origin_secure_cb_(is_origin_secure_cb),
       url_security_policy_(url_security_policy),
       use_empty_db_(in_progress_db_dir.empty()),
+      connector_(connector),
       weak_factory_(this) {
   Initialize(in_progress_db_dir);
 }
@@ -252,15 +254,14 @@ void InProgressDownloadManager::BeginDownload(
     const GURL& tab_referrer_url) {
   std::unique_ptr<network::ResourceRequest> request =
       CreateResourceRequest(params.get());
-  auto connector = delegate_ ? delegate_->GetServiceConnector() : nullptr;
   GetIOTaskRunner()->PostTask(
       FROM_HERE,
-      base::BindOnce(&BeginResourceDownload, std::move(params),
-                     std::move(request), std::move(url_loader_factory_getter),
-                     url_security_policy_, is_new_download,
-                     weak_factory_.GetWeakPtr(), site_url, tab_url,
-                     tab_referrer_url, std::move(connector),
-                     base::ThreadTaskRunnerHandle::Get()));
+      base::BindOnce(
+          &BeginResourceDownload, std::move(params), std::move(request),
+          std::move(url_loader_factory_getter), url_security_policy_,
+          is_new_download, weak_factory_.GetWeakPtr(), site_url, tab_url,
+          tab_referrer_url, connector_ ? connector_->Clone() : nullptr,
+          base::ThreadTaskRunnerHandle::Get()));
 }
 
 void InProgressDownloadManager::InterceptDownloadFromNavigation(
@@ -275,17 +276,17 @@ void InProgressDownloadManager::InterceptDownloadFromNavigation(
     net::CertStatus cert_status,
     network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints,
     scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter) {
-  auto connector = delegate_ ? delegate_->GetServiceConnector() : nullptr;
   GetIOTaskRunner()->PostTask(
       FROM_HERE,
-      base::BindOnce(
-          &CreateDownloadHandlerForNavigation, weak_factory_.GetWeakPtr(),
-          std::move(resource_request), render_process_id, render_frame_id,
-          site_url, tab_url, tab_referrer_url, std::move(url_chain),
-          std::move(response), std::move(cert_status),
-          std::move(url_loader_client_endpoints),
-          std::move(url_loader_factory_getter), url_security_policy_,
-          std::move(connector), base::ThreadTaskRunnerHandle::Get()));
+      base::BindOnce(&CreateDownloadHandlerForNavigation,
+                     weak_factory_.GetWeakPtr(), std::move(resource_request),
+                     render_process_id, render_frame_id, site_url, tab_url,
+                     tab_referrer_url, std::move(url_chain),
+                     std::move(response), std::move(cert_status),
+                     std::move(url_loader_client_endpoints),
+                     std::move(url_loader_factory_getter), url_security_policy_,
+                     connector_ ? connector_->Clone() : nullptr,
+                     base::ThreadTaskRunnerHandle::Get()));
 }
 
 void InProgressDownloadManager::Initialize(
@@ -572,14 +573,12 @@ void InProgressDownloadManager::OnDownloadsInitialized() {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(&InProgressDownloadManager::NotifyDownloadsInitialized,
-                       weak_factory_.GetWeakPtr(),
-                       base::Unretained(delegate_)));
+                       weak_factory_.GetWeakPtr()));
   }
 }
 
-void InProgressDownloadManager::NotifyDownloadsInitialized(Delegate* delegate) {
-  // Do nothing if |delegate_| changes while posing the task.
-  if (delegate_ == delegate)
+void InProgressDownloadManager::NotifyDownloadsInitialized() {
+  if (delegate_)
     delegate_->OnDownloadsInitialized();
 }
 

components/download/public/common/in_progress_download_manager.h

@@ -59,7 +59,7 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   class COMPONENTS_DOWNLOAD_EXPORT Delegate {
    public:
     // Called when in-progress downloads are initialized.
-    virtual void OnDownloadsInitialized() = 0;
+    virtual void OnDownloadsInitialized() {}
 
     // Intercepts the download to another system if applicable. Returns true if
     // the download was intercepted.
@@ -81,16 +81,14 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
     // TODO(qinmin): remove this once network service is fully enabled.
     virtual net::URLRequestContextGetter* GetURLRequestContextGetter(
         const DownloadCreateInfo& download_create_info);
-
-    virtual std::unique_ptr<service_manager::Connector>
-    GetServiceConnector() = 0;
   };
 
   using IsOriginSecureCallback = base::RepeatingCallback<bool(const GURL&)>;
   InProgressDownloadManager(Delegate* delegate,
                             const base::FilePath& in_progress_db_dir,
                             const IsOriginSecureCallback& is_origin_secure_cb,
-                            const URLSecurityPolicy& url_security_policy);
+                            const URLSecurityPolicy& url_security_policy,
+                            service_manager::Connector* connector);
   ~InProgressDownloadManager() override;
 
   // SimpleDownloadManager implementation.
@@ -218,8 +216,8 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   // Called when downloads are initialized.
   void OnDownloadsInitialized();
 
-  // Called to notify |delegate| that downloads are initialized.
-  void NotifyDownloadsInitialized(Delegate* delegate);
+  // Called to notify |delegate_| that downloads are initialized.
+  void NotifyDownloadsInitialized();
 
   // Active download handlers.
   std::vector<UrlDownloadHandler::UniqueUrlDownloadHandlerPtr>
@@ -271,6 +269,9 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   // Whether this object uses an empty database and no history will be saved.
   bool use_empty_db_;
 
+  // Connector to the service manager.
+  service_manager::Connector* connector_;
+
   base::WeakPtrFactory<InProgressDownloadManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(InProgressDownloadManager);

content/browser/download/download_manager_impl.cc

@@ -362,7 +362,7 @@ DownloadManagerImpl::DownloadManagerImpl(BrowserContext* browser_context)
     in_progress_manager_ =
         std::make_unique<download::InProgressDownloadManager>(
             this, base::FilePath(), base::BindRepeating(&IsOriginSecure),
-            base::BindRepeating(&DownloadRequestUtils::IsURLSafe));
+            base::BindRepeating(&DownloadRequestUtils::IsURLSafe), nullptr);
   } else {
     in_progress_manager_->SetDelegate(this);
     in_progress_manager_->set_download_start_observer(nullptr);
@@ -678,16 +678,6 @@ net::URLRequestContextGetter* DownloadManagerImpl::GetURLRequestContextGetter(
                            : nullptr;
 }
 
-std::unique_ptr<service_manager::Connector>
-DownloadManagerImpl::GetServiceConnector() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  service_manager::Connector* connector = GetServiceManagerConnector();
-  if (connector)
-    return connector->Clone();  // Clone for use on a different thread.
-  return nullptr;
-}
-
 service_manager::Connector* DownloadManagerImpl::GetServiceManagerConnector() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (auto* connection = ServiceManagerConnection::GetForProcess())

content/browser/download/download_manager_impl.h

@@ -210,7 +210,6 @@ class CONTENT_EXPORT DownloadManagerImpl
       override;
   net::URLRequestContextGetter* GetURLRequestContextGetter(
       const download::DownloadCreateInfo& info) override;
-  std::unique_ptr<service_manager::Connector> GetServiceConnector() override;
 
   // Creates a new download item and call |callback|.
   void CreateNewDownloadItemToStart(

content/browser/download/download_manager_impl_unittest.cc

@@ -383,7 +383,8 @@ TestInProgressManager::TestInProgressManager()
           nullptr,
           base::FilePath(),
           download::InProgressDownloadManager::IsOriginSecureCallback(),
-          base::BindRepeating(&URLAlwaysSafe)) {}
+          base::BindRepeating(&URLAlwaysSafe),
+          nullptr) {}
 
 void TestInProgressManager::AddDownloadItem(
     std::unique_ptr<download::DownloadItemImpl> item) {