All callbacks to notify when InProgressDownloadManager is initialized

Currently DownloadManagerImpl intializes InProgressDownloadManager.
However, when only ServiceManager is running, DownloadManagerImpl will
not be created.
As a result, some other class will initialize InProgressDownloadManager.
And when chrome fully starts, DownloadManagerImpl needs to know whether
InProgressManager has been initialized.
This CL allows InProgressDownloadManager to take more callbacks to
notify when initialized.
So multiple classes can observe the initialization event.

BUG=842245

Change-Id: I5881b10db0f93f98262ab1baebc67621c2743fbe
Reviewed-on: https://chromium-review.googlesource.com/1111027
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569849}

components/download/internal/common/in_progress_download_manager.cc

@@ -192,12 +192,16 @@ void InProgressDownloadObserver::OnDownloadRemoved(DownloadItem* download) {
 
 InProgressDownloadManager::InProgressDownloadManager(
     Delegate* delegate,
+    const base::FilePath& metadata_cache_dir,
     const IsOriginSecureCallback& is_origin_secure_cb)
-    : delegate_(delegate),
+    : is_initialized_(false),
+      delegate_(delegate),
       file_factory_(new DownloadFileFactory()),
       download_start_observer_(nullptr),
       is_origin_secure_cb_(is_origin_secure_cb),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  Initialize(metadata_cache_dir);
+}
 
 InProgressDownloadManager::~InProgressDownloadManager() = default;
 
@@ -270,8 +274,7 @@ void InProgressDownloadManager::InterceptDownloadFromNavigation(
 }
 
 void InProgressDownloadManager::Initialize(
-    const base::FilePath& metadata_cache_dir,
-    base::OnceClosure callback) {
+    const base::FilePath& metadata_cache_dir) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableDownloadDB)) {
     // TODO(qinmin): migrate all the data from InProgressCache into
@@ -279,9 +282,8 @@ void InProgressDownloadManager::Initialize(
     download_db_cache_ =
         std::make_unique<DownloadDBCache>(std::make_unique<DownloadDBImpl>(
             DownloadNamespace::NAMESPACE_BROWSER_DOWNLOAD, metadata_cache_dir));
-    download_db_cache_->Initialize(
-        base::BindOnce(&InProgressDownloadManager::OnDownloadDBInitialized,
-                       weak_factory_.GetWeakPtr(), std::move(callback)));
+    download_db_cache_->Initialize(base::BindOnce(
+        &InProgressDownloadManager::OnInitialized, weak_factory_.GetWeakPtr()));
   } else {
     download_metadata_cache_ = std::make_unique<InProgressCacheImpl>(
         metadata_cache_dir.empty()
@@ -290,7 +292,9 @@ void InProgressDownloadManager::Initialize(
         base::CreateSequencedTaskRunnerWithTraits(
             {base::MayBlock(), base::TaskPriority::BACKGROUND,
              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}));
-    download_metadata_cache_->Initialize(std::move(callback));
+    download_metadata_cache_->Initialize(base::BindOnce(
+        &InProgressDownloadManager::OnInitialized, weak_factory_.GetWeakPtr(),
+        std::make_unique<std::vector<DownloadDBEntry>>()));
   }
 }
 
@@ -478,14 +482,17 @@ void InProgressDownloadManager::StartDownloadWithItem(
     download_start_observer_->OnDownloadStarted(download);
 }
 
-void InProgressDownloadManager::OnDownloadDBInitialized(
-    base::OnceClosure callback,
+void InProgressDownloadManager::OnInitialized(
     std::unique_ptr<std::vector<DownloadDBEntry>> entries) {
   for (const auto& entry : *entries)
     in_progress_downloads_.emplace_back(CreateDownloadItemImpl(this, entry));
-  if (delegate_)
-    delegate_->OnInProgressDownloadsLoaded(std::move(in_progress_downloads_));
-  std::move(callback).Run();
+  is_initialized_ = true;
+  auto iter = on_initialized_callbacks_.begin();
+  while (iter != on_initialized_callbacks_.end()) {
+    std::unique_ptr<base::OnceClosure> callback = std::move(*iter);
+    on_initialized_callbacks_.erase(iter);
+    std::move(*callback).Run();
+  }
 }
 
 DownloadItemImpl* InProgressDownloadManager::GetInProgressDownload(
@@ -497,4 +504,19 @@ DownloadItemImpl* InProgressDownloadManager::GetInProgressDownload(
   return nullptr;
 }
 
+void InProgressDownloadManager::NotifyWhenInitialized(
+    base::OnceClosure on_initialized_cb) {
+  if (is_initialized_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(on_initialized_cb));
+    return;
+  }
+  on_initialized_callbacks_.emplace_back(
+      std::make_unique<base::OnceClosure>(std::move(on_initialized_cb)));
+}
+
+std::vector<std::unique_ptr<download::DownloadItemImpl>>
+InProgressDownloadManager::TakeInProgressDownloads() {
+  return std::move(in_progress_downloads_);
+}
 }  // namespace download

components/download/public/common/in_progress_download_manager.h

@@ -66,15 +66,11 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
     // TODO(qinmin): remove this once network service is fully enabled.
     virtual net::URLRequestContextGetter* GetURLRequestContextGetter(
         const DownloadCreateInfo& download_create_info) = 0;
-
-    // Called when all in-progress downloads are loaded from the database.
-    virtual void OnInProgressDownloadsLoaded(
-        std::vector<std::unique_ptr<download::DownloadItemImpl>>
-            in_progress_downloads) = 0;
   };
 
   using IsOriginSecureCallback = base::RepeatingCallback<bool(const GURL&)>;
   InProgressDownloadManager(Delegate* delegate,
+                            const base::FilePath& metadata_cache_dir,
                             const IsOriginSecureCallback& is_origin_secure_cb);
   ~InProgressDownloadManager() override;
   // Called to start a download.
@@ -100,9 +96,6 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
       network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints,
       scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter);
 
-  void Initialize(const base::FilePath& metadata_cache_dir,
-                  base::OnceClosure callback);
-
   void StartDownload(
       std::unique_ptr<DownloadCreateInfo> info,
       std::unique_ptr<InputStream> stream,
@@ -129,10 +122,19 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   // Called to retrieve an in-progress download.
   DownloadItemImpl* GetInProgressDownload(const std::string& guid);
 
+  // Run |on_initialized_cb| once this object is initialized.
+  void NotifyWhenInitialized(base::OnceClosure on_initialized_cb);
+
   void set_download_start_observer(DownloadStartObserver* observer) {
     download_start_observer_ = observer;
   }
 
+  // Called to get all in-progress DownloadItemImpl.
+  // TODO(qinmin): remove this method once InProgressDownloadManager owns
+  // all in-progress downloads.
+  virtual std::vector<std::unique_ptr<download::DownloadItemImpl>>
+  TakeInProgressDownloads();
+
   void set_file_factory(std::unique_ptr<DownloadFileFactory> file_factory) {
     file_factory_ = std::move(file_factory);
   }
@@ -143,7 +145,11 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
     url_loader_factory_getter_ = std::move(url_loader_factory_getter);
   }
 
+  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
+
  private:
+  void Initialize(const base::FilePath& metadata_cache_dir);
+
   // UrlDownloadHandler::Delegate implementations.
   void OnUrlDownloadStarted(
       std::unique_ptr<DownloadCreateInfo> download_create_info,
@@ -154,10 +160,8 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   void OnUrlDownloadHandlerCreated(
       UrlDownloadHandler::UniqueUrlDownloadHandlerPtr downloader) override;
 
-  // Called download db is initialized.
-  void OnDownloadDBInitialized(
-      base::OnceClosure callback,
-      std::unique_ptr<std::vector<DownloadDBEntry>> entries);
+  // Called when the object is initialized.
+  void OnInitialized(std::unique_ptr<std::vector<DownloadDBEntry>> entries);
 
   // Start a DownloadItemImpl.
   void StartDownloadWithItem(
@@ -166,6 +170,9 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
       std::unique_ptr<DownloadCreateInfo> info,
       DownloadItemImpl* download);
 
+  // Whether |download_db_cache_| is initialized.
+  bool is_initialized_;
+
   // Active download handlers.
   std::vector<UrlDownloadHandler::UniqueUrlDownloadHandlerPtr>
       url_download_handlers_;
@@ -188,6 +195,9 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   // Observer to notify when a download starts.
   DownloadStartObserver* download_start_observer_;
 
+  // Callbacks to call once this object is initialized.
+  std::vector<std::unique_ptr<base::OnceClosure>> on_initialized_callbacks_;
+
   // callback to check if an origin is secure.
   IsOriginSecureCallback is_origin_secure_cb_;
 

content/browser/download/download_manager_impl.cc

@@ -302,12 +302,11 @@ DownloadManagerImpl::DownloadManagerImpl(BrowserContext* browser_context)
   if (!base::FeatureList::IsEnabled(network::features::kNetworkService))
     download::UrlDownloadHandlerFactory::Install(new UrlDownloaderFactory());
   in_progress_manager_ = std::make_unique<download::InProgressDownloadManager>(
-      this, base::BindRepeating(&IsOriginSecure));
-  in_progress_manager_->Initialize(
-      IsOffTheRecord() ? base::FilePath() : browser_context_->GetPath(),
-      base::BindOnce(&DownloadManagerImpl::PostInitialization,
-                     weak_factory_.GetWeakPtr(),
-                     DOWNLOAD_INITIALIZATION_DEPENDENCY_IN_PROGRESS_CACHE));
+      this, IsOffTheRecord() ? base::FilePath() : browser_context_->GetPath(),
+      base::BindRepeating(&IsOriginSecure));
+  in_progress_manager_->NotifyWhenInitialized(base::BindOnce(
+      &DownloadManagerImpl::OnInProgressDownloadManagerInitialized,
+      weak_factory_.GetWeakPtr()));
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
@@ -473,9 +472,9 @@ base::FilePath DownloadManagerImpl::GetDefaultDownloadDirectory() {
   return default_download_directory;
 }
 
-void DownloadManagerImpl::OnInProgressDownloadsLoaded(
-    std::vector<std::unique_ptr<download::DownloadItemImpl>>
-        in_progress_downloads) {
+void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
+  std::vector<std::unique_ptr<download::DownloadItemImpl>>
+      in_progress_downloads = in_progress_manager_->TakeInProgressDownloads();
   for (auto& download : in_progress_downloads) {
     DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
     DCHECK(!base::ContainsKey(downloads_, download->GetId()));
@@ -488,6 +487,7 @@ void DownloadManagerImpl::OnInProgressDownloadsLoaded(
       observer.OnDownloadCreated(this, item);
     DVLOG(20) << __func__ << "() download = " << item->DebugString(true);
   }
+  PostInitialization(DOWNLOAD_INITIALIZATION_DEPENDENCY_IN_PROGRESS_CACHE);
 }
 
 void DownloadManagerImpl::StartDownloadItem(

content/browser/download/download_manager_impl.h

@@ -203,9 +203,9 @@ class CONTENT_EXPORT DownloadManagerImpl
       override;
   net::URLRequestContextGetter* GetURLRequestContextGetter(
       const download::DownloadCreateInfo& info) override;
-  void OnInProgressDownloadsLoaded(
-      std::vector<std::unique_ptr<download::DownloadItemImpl>>
-          in_progress_downloads) override;
+
+  // Called when InProgressDownloadManager is initialzed.
+  void OnInProgressDownloadManagerInitialized();
 
   // Creates a new download item and call |callback|.
   void CreateNewDownloadItemToStart(

content/browser/download/download_manager_impl_unittest.cc

@@ -320,6 +320,36 @@ class MockByteStreamReader : public ByteStreamReader {
   MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
 };
 
+class TestInProgressManager : public download::InProgressDownloadManager {
+ public:
+  TestInProgressManager();
+  ~TestInProgressManager() override = default;
+
+  std::vector<std::unique_ptr<download::DownloadItemImpl>>
+  TakeInProgressDownloads() override;
+
+  void AddDownloadItem(std::unique_ptr<download::DownloadItemImpl> item);
+
+ private:
+  std::vector<std::unique_ptr<download::DownloadItemImpl>> download_items_;
+};
+
+TestInProgressManager::TestInProgressManager()
+    : download::InProgressDownloadManager(
+          nullptr,
+          base::FilePath(),
+          download::InProgressDownloadManager::IsOriginSecureCallback()) {}
+
+void TestInProgressManager::AddDownloadItem(
+    std::unique_ptr<download::DownloadItemImpl> item) {
+  download_items_.emplace_back(std::move(item));
+}
+
+std::vector<std::unique_ptr<download::DownloadItemImpl>>
+TestInProgressManager::TakeInProgressDownloads() {
+  return std::move(download_items_);
+}
+
 }  // namespace
 
 class DownloadManagerTest : public testing::Test {
@@ -450,9 +480,13 @@ class DownloadManagerTest : public testing::Test {
             base::Unretained(this)));
   }
 
-  void OnInProgressDownloadsLoaded(
-      std::vector<std::unique_ptr<download::DownloadItemImpl>> downloads) {
-    download_manager_->OnInProgressDownloadsLoaded(std::move(downloads));
+  void OnInProgressDownloadManagerInitialized() {
+    download_manager_->OnInProgressDownloadManagerInitialized();
+  }
+
+  void SetInProgressDownloadManager(
+      std::unique_ptr<download::InProgressDownloadManager> manager) {
+    download_manager_->in_progress_manager_ = std::move(manager);
   }
 
  protected:
@@ -617,11 +651,10 @@ TEST_F(DownloadManagerTest, RemoveDownloadsByURL) {
 // Confirm that in-progress downloads will be taken and managed by
 // DownloadManager.
 TEST_F(DownloadManagerTest, OnInProgressDownloadsLoaded) {
+  auto in_progress_manager = std::make_unique<TestInProgressManager>();
   const char kGuid[] = "8DF158E8-C980-4618-BB03-EBA3242EB48B";
-  download::InProgressDownloadManager in_progress_manager(
-      nullptr, download::InProgressDownloadManager::IsOriginSecureCallback());
   auto in_progress_item = std::make_unique<download::DownloadItemImpl>(
-      &in_progress_manager, kGuid, 10, base::FilePath(), base::FilePath(),
+      in_progress_manager.get(), kGuid, 10, base::FilePath(), base::FilePath(),
       std::vector<GURL>(), GURL("http://example.com/a"),
       GURL("http://example.com/a"), GURL("http://example.com/a"),
       GURL("http://example.com/a"), "application/octet-stream",
@@ -632,11 +665,11 @@ TEST_F(DownloadManagerTest, OnInProgressDownloadsLoaded) {
       download::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED, false,
       base::Time::Now(), true,
       std::vector<download::DownloadItem::ReceivedSlice>());
-  std::vector<std::unique_ptr<download::DownloadItemImpl>> downloads;
-  downloads.emplace_back(std::move(in_progress_item));
+  in_progress_manager->AddDownloadItem(std::move(in_progress_item));
+  SetInProgressDownloadManager(std::move(in_progress_manager));
   EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
       .WillOnce(Return());
-  OnInProgressDownloadsLoaded(std::move(downloads));
+  OnInProgressDownloadManagerInitialized();
   ASSERT_TRUE(download_manager_->GetDownloadByGuid(kGuid));
 
   download::DownloadItem* download =