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/1112977Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569879}

components/download/internal/common/in_progress_download_manager.cc

@@ -192,16 +192,12 @@ void InProgressDownloadObserver::OnDownloadRemoved(DownloadItem* download) {
 
 InProgressDownloadManager::InProgressDownloadManager(
     Delegate* delegate,
-    const base::FilePath& metadata_cache_dir,
     const IsOriginSecureCallback& is_origin_secure_cb)
-    : is_initialized_(false),
-      delegate_(delegate),
+    : delegate_(delegate),
       file_factory_(new DownloadFileFactory()),
       download_start_observer_(nullptr),
       is_origin_secure_cb_(is_origin_secure_cb),
-      weak_factory_(this) {
-  Initialize(metadata_cache_dir);
-}
+      weak_factory_(this) {}
 
 InProgressDownloadManager::~InProgressDownloadManager() = default;
 
@@ -274,7 +270,8 @@ void InProgressDownloadManager::InterceptDownloadFromNavigation(
 }
 
 void InProgressDownloadManager::Initialize(
-    const base::FilePath& metadata_cache_dir) {
+    const base::FilePath& metadata_cache_dir,
+    base::OnceClosure callback) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableDownloadDB)) {
     // TODO(qinmin): migrate all the data from InProgressCache into
@@ -282,8 +279,9 @@ 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::OnInitialized, weak_factory_.GetWeakPtr()));
+    download_db_cache_->Initialize(
+        base::BindOnce(&InProgressDownloadManager::OnDownloadDBInitialized,
+                       weak_factory_.GetWeakPtr(), std::move(callback)));
   } else {
     download_metadata_cache_ = std::make_unique<InProgressCacheImpl>(
         metadata_cache_dir.empty()
@@ -292,9 +290,7 @@ void InProgressDownloadManager::Initialize(
         base::CreateSequencedTaskRunnerWithTraits(
             {base::MayBlock(), base::TaskPriority::BACKGROUND,
              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}));
-    download_metadata_cache_->Initialize(base::BindOnce(
-        &InProgressDownloadManager::OnInitialized, weak_factory_.GetWeakPtr(),
-        std::make_unique<std::vector<DownloadDBEntry>>()));
+    download_metadata_cache_->Initialize(std::move(callback));
   }
 }
 
@@ -482,17 +478,14 @@ void InProgressDownloadManager::StartDownloadWithItem(
     download_start_observer_->OnDownloadStarted(download);
 }
 
-void InProgressDownloadManager::OnInitialized(
+void InProgressDownloadManager::OnDownloadDBInitialized(
+    base::OnceClosure callback,
     std::unique_ptr<std::vector<DownloadDBEntry>> entries) {
   for (const auto& entry : *entries)
     in_progress_downloads_.emplace_back(CreateDownloadItemImpl(this, entry));
-  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();
-  }
+  if (delegate_)
+    delegate_->OnInProgressDownloadsLoaded(std::move(in_progress_downloads_));
+  std::move(callback).Run();
 }
 
 DownloadItemImpl* InProgressDownloadManager::GetInProgressDownload(
@@ -504,19 +497,4 @@ 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,11 +66,15 @@ 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.
@@ -96,6 +100,9 @@ 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,
@@ -122,19 +129,10 @@ 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);
   }
@@ -145,11 +143,7 @@ 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,
@@ -160,8 +154,10 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   void OnUrlDownloadHandlerCreated(
       UrlDownloadHandler::UniqueUrlDownloadHandlerPtr downloader) override;
 
-  // Called when the object is initialized.
-  void OnInitialized(std::unique_ptr<std::vector<DownloadDBEntry>> entries);
+  // Called download db is initialized.
+  void OnDownloadDBInitialized(
+      base::OnceClosure callback,
+      std::unique_ptr<std::vector<DownloadDBEntry>> entries);
 
   // Start a DownloadItemImpl.
   void StartDownloadWithItem(
@@ -170,9 +166,6 @@ 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_;
@@ -195,9 +188,6 @@ 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,11 +302,12 @@ 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, IsOffTheRecord() ? base::FilePath() : browser_context_->GetPath(),
-      base::BindRepeating(&IsOriginSecure));
-  in_progress_manager_->NotifyWhenInitialized(base::BindOnce(
-      &DownloadManagerImpl::OnInProgressDownloadManagerInitialized,
-      weak_factory_.GetWeakPtr()));
+      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));
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
@@ -472,9 +473,9 @@ base::FilePath DownloadManagerImpl::GetDefaultDownloadDirectory() {
   return default_download_directory;
 }
 
-void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
-  std::vector<std::unique_ptr<download::DownloadItemImpl>>
-      in_progress_downloads = in_progress_manager_->TakeInProgressDownloads();
+void DownloadManagerImpl::OnInProgressDownloadsLoaded(
+    std::vector<std::unique_ptr<download::DownloadItemImpl>>
+        in_progress_downloads) {
   for (auto& download : in_progress_downloads) {
     DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
     DCHECK(!base::ContainsKey(downloads_, download->GetId()));
@@ -487,7 +488,6 @@ void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
       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;
-
-  // Called when InProgressDownloadManager is initialzed.
-  void OnInProgressDownloadManagerInitialized();
+  void OnInProgressDownloadsLoaded(
+      std::vector<std::unique_ptr<download::DownloadItemImpl>>
+          in_progress_downloads) override;
 
   // Creates a new download item and call |callback|.
   void CreateNewDownloadItemToStart(

content/browser/download/download_manager_impl_unittest.cc

@@ -320,36 +320,6 @@ 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 {
@@ -480,13 +450,9 @@ class DownloadManagerTest : public testing::Test {
             base::Unretained(this)));
   }
 
-  void OnInProgressDownloadManagerInitialized() {
-    download_manager_->OnInProgressDownloadManagerInitialized();
-  }
-
-  void SetInProgressDownloadManager(
-      std::unique_ptr<download::InProgressDownloadManager> manager) {
-    download_manager_->in_progress_manager_ = std::move(manager);
+  void OnInProgressDownloadsLoaded(
+      std::vector<std::unique_ptr<download::DownloadItemImpl>> downloads) {
+    download_manager_->OnInProgressDownloadsLoaded(std::move(downloads));
   }
 
  protected:
@@ -651,10 +617,11 @@ 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.get(), kGuid, 10, base::FilePath(), base::FilePath(),
+      &in_progress_manager, 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",
@@ -665,11 +632,11 @@ TEST_F(DownloadManagerTest, OnInProgressDownloadsLoaded) {
       download::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED, false,
       base::Time::Now(), true,
       std::vector<download::DownloadItem::ReceivedSlice>());
-  in_progress_manager->AddDownloadItem(std::move(in_progress_item));
-  SetInProgressDownloadManager(std::move(in_progress_manager));
+  std::vector<std::unique_ptr<download::DownloadItemImpl>> downloads;
+  downloads.emplace_back(std::move(in_progress_item));
   EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
       .WillOnce(Return());
-  OnInProgressDownloadManagerInitialized();
+  OnInProgressDownloadsLoaded(std::move(downloads));
   ASSERT_TRUE(download_manager_->GetDownloadByGuid(kGuid));
 
   download::DownloadItem* download =