Reland "All callbacks to notify when InProgressDownloadManager is initialized"

This reverts commit c7496eca.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Revert "All callbacks to notify when InProgressDownloadManager is initialized"
> 
> This reverts commit 39c14237.
> 
> Reason for revert:
> Broke many tests on many bots, eg. https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win10%20Tests%20x64%20%28dbg%29/1473
> 
> Original change's description:
> > 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}
> 
> TBR=dtrainor@chromium.org,qinmin@chromium.org
> 
> Change-Id: Ic2e8b09dc8aca3680c7a5939fdfb545e0dfec82a
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 842245
> Reviewed-on: https://chromium-review.googlesource.com/1112977
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#569879}

TBR=thakis@chromium.org,dtrainor@chromium.org,qinmin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 842245
Change-Id: I9a2d1e2215f755785190400e554408f3bf2e7de5
Reviewed-on: https://chromium-review.googlesource.com/1113839
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570140}

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,14 @@ 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;
+  for (auto& callback : on_initialized_callbacks_)
+    std::move(*callback).Run();
+  on_initialized_callbacks_.clear();
 }
 
 DownloadItemImpl* InProgressDownloadManager::GetInProgressDownload(
@@ -497,4 +501,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 =