Move downloads logic to `HoldingSpaceDownloadsDelegate`.

As was previously done for file watching and persistence logic, moving
downloads logic out of `HoldingSpaceKeyedService` into its own delegate.

Bug: 1119496
Change-Id: Ibba263b73bda846b9c076361d72d0f05b231da8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386396
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803396}

chrome/browser/ui/BUILD.gn

@@ -1870,6 +1870,8 @@ static_library("ui") {
       "ash/clipboard_util.h",
       "ash/holding_space/holding_space_client_impl.cc",
       "ash/holding_space/holding_space_client_impl.h",
+      "ash/holding_space/holding_space_downloads_delegate.cc",
+      "ash/holding_space/holding_space_downloads_delegate.h",
       "ash/holding_space/holding_space_file_system_delegate.cc",
       "ash/holding_space/holding_space_file_system_delegate.h",
       "ash/holding_space/holding_space_keyed_service.cc",

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.cc

+// Copyright 2020 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 "chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h"
+
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_context.h"
+
+namespace ash {
+
+namespace {
+
+content::DownloadManager* download_manager_for_testing = nullptr;
+
+}  // namespace
+
+HoldingSpaceDownloadsDelegate::HoldingSpaceDownloadsDelegate(
+    Profile* profile,
+    HoldingSpaceModel* model,
+    ItemDownloadedCallback item_downloaded_callback)
+    : HoldingSpaceKeyedServiceDelegate(profile, model),
+      item_downloaded_callback_(item_downloaded_callback) {}
+
+HoldingSpaceDownloadsDelegate::~HoldingSpaceDownloadsDelegate() = default;
+
+// static
+void HoldingSpaceDownloadsDelegate::SetDownloadManagerForTesting(
+    content::DownloadManager* download_manager) {
+  download_manager_for_testing = download_manager;
+}
+
+void HoldingSpaceDownloadsDelegate::Init() {
+  download_manager_observer_.Add(
+      download_manager_for_testing
+          ? download_manager_for_testing
+          : content::BrowserContext::GetDownloadManager(profile()));
+}
+
+void HoldingSpaceDownloadsDelegate::Shutdown() {
+  RemoveObservers();
+}
+
+void HoldingSpaceDownloadsDelegate::OnHoldingSpaceModelRestored() {
+  content::DownloadManager* download_manager =
+      download_manager_for_testing
+          ? download_manager_for_testing
+          : content::BrowserContext::GetDownloadManager(profile());
+
+  download::SimpleDownloadManager::DownloadVector downloads;
+  download_manager->GetAllDownloads(&downloads);
+
+  for (auto* download : downloads) {
+    switch (download->GetState()) {
+      case download::DownloadItem::COMPLETE:
+        item_downloaded_callback_.Run(download->GetFullPath());
+        break;
+      case download::DownloadItem::IN_PROGRESS:
+        download_item_observer_.Add(download);
+        break;
+      case download::DownloadItem::CANCELLED:
+      case download::DownloadItem::INTERRUPTED:
+      case download::DownloadItem::MAX_DOWNLOAD_STATE:
+        break;
+    }
+  }
+}
+
+void HoldingSpaceDownloadsDelegate::ManagerGoingDown(
+    content::DownloadManager* manager) {
+  RemoveObservers();
+}
+
+void HoldingSpaceDownloadsDelegate::OnDownloadCreated(
+    content::DownloadManager* manager,
+    download::DownloadItem* item) {
+  download_item_observer_.Add(item);
+}
+
+void HoldingSpaceDownloadsDelegate::OnDownloadUpdated(
+    download::DownloadItem* item) {
+  switch (item->GetState()) {
+    case download::DownloadItem::COMPLETE:
+      OnDownloadCompleted(item);
+      FALLTHROUGH;
+    case download::DownloadItem::CANCELLED:
+    case download::DownloadItem::INTERRUPTED:
+      download_item_observer_.Remove(item);
+      break;
+    case download::DownloadItem::IN_PROGRESS:
+    case download::DownloadItem::MAX_DOWNLOAD_STATE:
+      break;
+  }
+}
+
+void HoldingSpaceDownloadsDelegate::OnDownloadCompleted(
+    download::DownloadItem* item) {
+  if (!is_restoring())
+    item_downloaded_callback_.Run(item->GetFullPath());
+}
+
+void HoldingSpaceDownloadsDelegate::RemoveObservers() {
+  download_manager_observer_.RemoveAll();
+  download_item_observer_.RemoveAll();
+}
+
+}  // namespace ash

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_DOWNLOADS_DELEGATE_H_
+#define CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_DOWNLOADS_DELEGATE_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/scoped_observer.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
+#include "components/download/public/common/download_item.h"
+#include "content/public/browser/download_manager.h"
+
+namespace base {
+class FilePath;
+}  // namespace base
+
+namespace ash {
+
+// A delegate of `HoldingSpaceKeyedService` tasked with monitoring the status of
+// of downloads and notifying a callback on download completion.
+class HoldingSpaceDownloadsDelegate : public HoldingSpaceKeyedServiceDelegate,
+                                      public content::DownloadManager::Observer,
+                                      public download::DownloadItem::Observer {
+ public:
+  // Callback to be invoked when a download is completed. Note that this
+  // callback will only be invoked after the holding space model is restored.
+  using ItemDownloadedCallback =
+      base::RepeatingCallback<void(const base::FilePath&)>;
+
+  HoldingSpaceDownloadsDelegate(
+      Profile* profile,
+      HoldingSpaceModel* model,
+      ItemDownloadedCallback item_downloaded_callback);
+  HoldingSpaceDownloadsDelegate(const HoldingSpaceDownloadsDelegate&) = delete;
+  HoldingSpaceDownloadsDelegate& operator=(
+      const HoldingSpaceDownloadsDelegate&) = delete;
+  ~HoldingSpaceDownloadsDelegate() override;
+
+  // Sets the `content::DownloadManager` to be used for testing.
+  // NOTE: This method must be called prior to delegate initialization.
+  static void SetDownloadManagerForTesting(
+      content::DownloadManager* download_manager);
+
+ private:
+  // HoldingSpaceKeyedServiceDelegate:
+  void Init() override;
+  void Shutdown() override;
+  void OnHoldingSpaceModelRestored() override;
+
+  // content::DownloadManager::Observer:
+  void ManagerGoingDown(content::DownloadManager* manager) override;
+  void OnDownloadCreated(content::DownloadManager* manager,
+                         download::DownloadItem* item) override;
+
+  // download::DownloadItem::Observer:
+  void OnDownloadUpdated(download::DownloadItem* item) override;
+
+  // Invoked when the specified `item` has completed downloading.
+  void OnDownloadCompleted(download::DownloadItem* item);
+
+  // Removes all observers.
+  void RemoveObservers();
+
+  // Callback to invoke when a download is completed.
+  ItemDownloadedCallback item_downloaded_callback_;
+
+  ScopedObserver<content::DownloadManager, content::DownloadManager::Observer>
+      download_manager_observer_{this};
+
+  ScopedObserver<download::DownloadItem, download::DownloadItem::Observer>
+      download_item_observer_{this};
+};
+
+}  // namespace ash
+
+#endif  // CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_DOWNLOADS_DELEGATE_H_

chrome/browser/ui/ash/holding_space/holding_space_file_system_delegate.h

@@ -28,7 +28,6 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate {
   HoldingSpaceFileSystemDelegate(Profile* profile,
                                  HoldingSpaceModel* model,
                                  FileRemovedCallback file_removed_callback);
-
   HoldingSpaceFileSystemDelegate(const HoldingSpaceFileSystemDelegate&) =
       delete;
   HoldingSpaceFileSystemDelegate& operator=(

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_file_system_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_persistence_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
@@ -28,7 +29,6 @@ ProfileManager* GetProfileManager() {
 
 }  // namespace
 
-// TODO(dmblack): Add a delegate for downloads.
 HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
                                                    const AccountId& account_id)
     : profile_(profile),
@@ -112,16 +112,9 @@ void HoldingSpaceKeyedService::AddItem(std::unique_ptr<HoldingSpaceItem> item) {
   holding_space_model_.AddItem(std::move(item));
 }
 
-void HoldingSpaceKeyedService::SetDownloadManagerForTesting(
-    content::DownloadManager* manager) {
-  RemoveDownloadManagerObservers();
-  download_manager_ = manager;
-  download_manager_->AddObserver(this);
-  RetrieveDownloadHistory();
-}
-
 void HoldingSpaceKeyedService::Shutdown() {
-  RemoveDownloadManagerObservers();
+  for (auto& delegate : delegates_)
+    delegate->Shutdown();
 }
 
 void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
@@ -131,40 +124,13 @@ void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
   }
 }
 
-void HoldingSpaceKeyedService::ManagerGoingDown(
-    content::DownloadManager* manager) {
-  RemoveDownloadManagerObservers();
-  download_manager_ = nullptr;
-}
-
-void HoldingSpaceKeyedService::OnDownloadCreated(
-    content::DownloadManager* manager,
-    download::DownloadItem* item) {
-  download_items_observer_.Add(item);
-}
-
-void HoldingSpaceKeyedService::OnDownloadUpdated(download::DownloadItem* item) {
-  switch (item->GetState()) {
-    case download::DownloadItem::COMPLETE:
-      AddDownload(item->GetFullPath());
-      FALLTHROUGH;
-    case download::DownloadItem::CANCELLED:
-    case download::DownloadItem::INTERRUPTED:
-      download_items_observer_.Remove(item);
-      break;
-    case download::DownloadItem::IN_PROGRESS:
-    case download::DownloadItem::MAX_DOWNLOAD_STATE:
-      break;
-  }
-}
-
-void HoldingSpaceKeyedService::RemoveDownloadManagerObservers() {
-  if (download_manager_)
-    download_manager_->RemoveObserver(this);
-  download_items_observer_.RemoveAll();
-}
-
 void HoldingSpaceKeyedService::OnProfileReady() {
+  // The `HoldingSpaceDownloadsDelegate` monitors the status of downloads.
+  delegates_.push_back(std::make_unique<HoldingSpaceDownloadsDelegate>(
+      profile_, &holding_space_model_, /*item_downloaded_callback=*/
+      base::BindRepeating(&HoldingSpaceKeyedService::AddDownload,
+                          weak_factory_.GetWeakPtr())));
+
   // The `HoldingSpaceFileSystemDelegate` monitors the file system for changes.
   delegates_.push_back(std::make_unique<HoldingSpaceFileSystemDelegate>(
       profile_, &holding_space_model_,
@@ -204,29 +170,6 @@ void HoldingSpaceKeyedService::OnModelRestored() {
 
   HoldingSpaceController::Get()->RegisterClientAndModelForUser(
       account_id_, &holding_space_client_, &holding_space_model_);
-
-  // NOTE: `download_manager_` may have already been set in tests.
-  if (download_manager_)
-    return;
-
-  // Once the `holding_space_model_` has been restored from persistence, we can
-  // start to observe the `download_manager_` to track downloaded files.
-  download_manager_ = content::BrowserContext::GetDownloadManager(profile_);
-  download_manager_->AddObserver(this);
-  RetrieveDownloadHistory();
-}
-
-void HoldingSpaceKeyedService::RetrieveDownloadHistory() {
-  DCHECK(download_manager_);
-  download::SimpleDownloadManager::DownloadVector downloads;
-  download_manager_->GetAllDownloads(&downloads);
-  for (auto* download : downloads) {
-    download::DownloadItem::DownloadState state = download->GetState();
-    if (state == download::DownloadItem::COMPLETE)
-      AddDownload(download->GetFullPath());
-    else if (state == download::DownloadItem::IN_PROGRESS)
-      download_items_observer_.Add(download);
-  }
 }
 
 }  // namespace ash

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h

@@ -17,9 +17,7 @@
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_thumbnail_loader.h"
 #include "components/account_id/account_id.h"
-#include "components/download/public/common/download_item.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "content/public/browser/download_manager.h"
 #include "url/gurl.h"
 
 class GURL;
@@ -46,9 +44,7 @@ namespace ash {
 // *   Manages the temporary holding space per-profile data model.
 // *   Serves as an entry point to add holding space items from Chrome.
 class HoldingSpaceKeyedService : public KeyedService,
-                                 public ProfileManagerObserver,
-                                 public content::DownloadManager::Observer,
-                                 public download::DownloadItem::Observer {
+                                 public ProfileManagerObserver {
  public:
   HoldingSpaceKeyedService(Profile* profile, const AccountId& account_id);
   HoldingSpaceKeyedService(const HoldingSpaceKeyedService& other) = delete;
@@ -98,8 +94,6 @@ class HoldingSpaceKeyedService : public KeyedService,
     return &thumbnail_loader_;
   }
 
-  void SetDownloadManagerForTesting(content::DownloadManager* manager);
-
  private:
   // KeyedService:
   void Shutdown() override;
@@ -107,19 +101,6 @@ class HoldingSpaceKeyedService : public KeyedService,
   // ProfileManagerObserver:
   void OnProfileAdded(Profile* profile) override;
 
-  // content::DownloadManager::Observer:
-  void ManagerGoingDown(content::DownloadManager* manager) override;
-  void OnDownloadCreated(content::DownloadManager* manager,
-                         download::DownloadItem* item) override;
-
-  // download::DownloadItem::Observer:
-  void OnDownloadUpdated(download::DownloadItem* item) override;
-
-  // Removes all observers from:
-  // - `download_manager_`
-  // - `download_items_observer_`.
-  void RemoveDownloadManagerObservers();
-
   // Invoked when the associated profile is ready.
   void OnProfileReady();
 
@@ -129,8 +110,6 @@ class HoldingSpaceKeyedService : public KeyedService,
   // Invoked when the holding space model has been restored from persistence.
   void OnModelRestored();
 
-  void RetrieveDownloadHistory();
-
   Profile* const profile_;
   const AccountId account_id_;
 
@@ -147,10 +126,6 @@ class HoldingSpaceKeyedService : public KeyedService,
   ScopedObserver<ProfileManager, ProfileManagerObserver>
       profile_manager_observer_{this};
 
-  content::DownloadManager* download_manager_ = nullptr;
-  ScopedObserver<download::DownloadItem, download::DownloadItem::Observer>
-      download_items_observer_{this};
-
   base::WeakPtrFactory<HoldingSpaceKeyedService> weak_factory_{this};
 };
 

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.cc

@@ -19,6 +19,8 @@ ProfileManager* GetProfileManager() {
 
 HoldingSpaceKeyedServiceDelegate::~HoldingSpaceKeyedServiceDelegate() = default;
 
+void HoldingSpaceKeyedServiceDelegate::Shutdown() {}
+
 void HoldingSpaceKeyedServiceDelegate::NotifyHoldingSpaceModelRestored() {
   DCHECK(is_restoring_);
   is_restoring_ = false;

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h

@@ -25,6 +25,10 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
   // to do so during or anytime after initialization.
   virtual void Init() = 0;
 
+  // Invoked by `HoldingSpaceKeyedService` when the service is shutting down.
+  // Delegates should perform any necessary clean up.
+  virtual void Shutdown();
+
   // Invoked by `HoldingSpaceKeyedService` to notify delegates when the holding
   // space model has been restored from persistence.
   void NotifyHoldingSpaceModelRestored();
@@ -47,7 +51,7 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
 
   // Invoked when the holding space model has been restored from persistence.
-  void OnHoldingSpaceModelRestored();
+  virtual void OnHoldingSpaceModelRestored();
 
   Profile* const profile_;
   const HoldingSpaceModel* const model_;

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_persistence_delegate.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
@@ -140,14 +141,34 @@ class HoldingSpaceModelAttachedWaiter : public HoldingSpaceControllerObserver {
   std::unique_ptr<base::RunLoop> wait_loop_;
 };
 
+// A mock `content::DownloadManager` which can notify observers of events.
+class MockDownloadManager : public content::MockDownloadManager {
+ public:
+  // content::MockDownloadManager:
+  void AddObserver(Observer* observer) override {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(Observer* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  void NotifyDownloadCreated(download::DownloadItem* item) {
+    for (auto& observer : observers_)
+      observer.OnDownloadCreated(this, item);
+  }
+
+ private:
+  base::ObserverList<content::DownloadManager::Observer>::Unchecked observers_;
+};
+
 }  // namespace
 
 class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
  public:
   HoldingSpaceKeyedServiceTest()
       : fake_user_manager_(new chromeos::FakeChromeUserManager),
-        user_manager_enabler_(base::WrapUnique(fake_user_manager_)),
-        download_manager_(new testing::NiceMock<content::MockDownloadManager>) {
+        user_manager_enabler_(base::WrapUnique(fake_user_manager_)) {
     scoped_feature_list_.InitAndEnableFeature(features::kTemporaryHoldingSpace);
   }
   HoldingSpaceKeyedServiceTest(const HoldingSpaceKeyedServiceTest& other) =
@@ -281,34 +302,10 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
     return result;
   }
 
-  std::unique_ptr<download::MockDownloadItem> CreateMockDownloadItem(
-      base::FilePath full_file_path) {
-    std::unique_ptr<download::MockDownloadItem> item(
-        new testing::NiceMock<download::MockDownloadItem>());
-    ON_CALL(*item, GetId()).WillByDefault(testing::Return(1));
-    ON_CALL(*item, GetGuid())
-        .WillByDefault(testing::ReturnRefOfCopy(
-            std::string("14CA04AF-ECEC-4B13-8829-817477EFAB83")));
-    ON_CALL(*item, GetFullPath())
-        .WillByDefault(testing::ReturnRefOfCopy(full_file_path));
-    ON_CALL(*item, GetURL())
-        .WillByDefault(testing::ReturnRefOfCopy(GURL("foo/bar")));
-    ON_CALL(*item, GetMimeType()).WillByDefault(testing::Return(std::string()));
-    content::DownloadItemUtils::AttachInfo(item.get(), GetProfile(), nullptr);
-
-    return item;
-  }
-
-  content::MockDownloadManager* download_manager() {
-    return download_manager_.get();
-  }
-
  private:
   chromeos::FakeChromeUserManager* fake_user_manager_;
   user_manager::ScopedUserManager user_manager_enabler_;
 
-  std::unique_ptr<content::MockDownloadManager> download_manager_;
-
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
@@ -551,7 +548,52 @@ TEST_F(HoldingSpaceKeyedServiceTest, RestorePersistentStorage) {
             persisted_holding_space_items_after_restoration);
 }
 
-TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
+class HoldingSpaceKeyedServiceDownloadsTest
+    : public HoldingSpaceKeyedServiceTest {
+ public:
+  HoldingSpaceKeyedServiceDownloadsTest()
+      : download_manager_(
+            std::make_unique<testing::NiceMock<MockDownloadManager>>()) {}
+
+  std::unique_ptr<download::MockDownloadItem> CreateMockDownloadItem(
+      base::FilePath full_file_path) {
+    auto item =
+        std::make_unique<testing::NiceMock<download::MockDownloadItem>>();
+    ON_CALL(*item, GetId()).WillByDefault(testing::Return(1));
+    ON_CALL(*item, GetGuid())
+        .WillByDefault(testing::ReturnRefOfCopy(
+            std::string("14CA04AF-ECEC-4B13-8829-817477EFAB83")));
+    ON_CALL(*item, GetFullPath())
+        .WillByDefault(testing::ReturnRefOfCopy(full_file_path));
+    ON_CALL(*item, GetURL())
+        .WillByDefault(testing::ReturnRefOfCopy(GURL("foo/bar")));
+    ON_CALL(*item, GetMimeType()).WillByDefault(testing::Return(std::string()));
+    content::DownloadItemUtils::AttachInfo(item.get(), GetProfile(), nullptr);
+    return item;
+  }
+
+  MockDownloadManager* download_manager() { return download_manager_.get(); }
+
+ private:
+  // HoldingSpaceKeyedServiceTest:
+  void SetUp() override {
+    SetUpDownloadManager();
+    HoldingSpaceKeyedServiceTest::SetUp();
+  }
+
+  void SetUpDownloadManager() {
+    // The `content::DownloadManager` needs to be set prior to initialization
+    // of the `HoldingSpaceDownloadsDelegate`. This must happen before the
+    // `HoldingSpaceKeyedService` is created for the profile under test.
+    MockDownloadManager* mock_download_manager = download_manager();
+    HoldingSpaceDownloadsDelegate::SetDownloadManagerForTesting(
+        mock_download_manager);
+  }
+
+  std::unique_ptr<MockDownloadManager> download_manager_;
+};
+
+TEST_F(HoldingSpaceKeyedServiceDownloadsTest, RetrieveHistory) {
   TestingProfile* profile = GetProfile();
   // Create a test downloads mount point.
   ScopedDownloadsMountPoint downloads_mount(profile);
@@ -591,15 +633,10 @@ TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
   EXPECT_CALL(*mock_download_manager, GetAllDownloads(testing::_))
       .WillOnce(testing::SetArgPointee<0>(download_items_mock));
 
-  HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
-  ASSERT_EQ(0u, model->items().size());
-
-  // Set the testing download manager, it should retrieve history and add the
-  // completed downloads to the model.
-  HoldingSpaceKeyedService* const holding_space_service =
-      HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(profile);
-  holding_space_service->SetDownloadManagerForTesting(mock_download_manager);
+  CreateSecondaryProfile();
+  ActivateSecondaryProfile();
 
+  HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
   ASSERT_EQ(2u, model->items().size());
 
   for (int i = 0; i < static_cast<int>(model->items().size()); ++i) {
@@ -625,36 +662,29 @@ TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
     delete download_items_mock[i];
 }
 
-TEST_F(HoldingSpaceKeyedServiceTest, AddDownloadItem) {
+TEST_F(HoldingSpaceKeyedServiceDownloadsTest, AddDownloadItem) {
   TestingProfile* profile = GetProfile();
   // Create a test downloads mount point.
   ScopedDownloadsMountPoint downloads_mount(profile);
   ASSERT_TRUE(downloads_mount.IsValid());
 
-  const base::FilePath download_item_virtual_path("Download 1.png");
   // Create a fake download file on the local file system - later parts of the
   // test will try to resolve the file's file system URL, which fails if the
   // file does not exist.
+  const base::FilePath download_item_virtual_path("Download 1.png");
   const base::FilePath download_item_full_path =
       CreateFile(downloads_mount, download_item_virtual_path, "download 1");
 
-  content::MockDownloadManager* mock_download_manager = download_manager();
+  MockDownloadManager* mock_download_manager = download_manager();
   std::unique_ptr<download::MockDownloadItem> item(
       CreateMockDownloadItem(download_item_full_path));
 
-  HoldingSpaceKeyedService* const holding_space_service =
-      HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(profile);
-  holding_space_service->SetDownloadManagerForTesting(mock_download_manager);
-
   download::MockDownloadItem* mock_download_item = item.get();
   EXPECT_CALL(*mock_download_manager, MockCreateDownloadItem(testing::_))
       .WillRepeatedly(testing::DoAll(
-          testing::InvokeWithoutArgs([&holding_space_service,
-                                      mock_download_manager,
+          testing::InvokeWithoutArgs([mock_download_manager,
                                       mock_download_item]() {
-            static_cast<content::DownloadManager::Observer*>(
-                holding_space_service)
-                ->OnDownloadCreated(mock_download_manager, mock_download_item);
+            mock_download_manager->NotifyDownloadCreated(mock_download_item);
           }),
           testing::Return(item.get())));
 

chrome/browser/ui/ash/holding_space/holding_space_persistence_delegate.h

@@ -43,7 +43,6 @@ class HoldingSpacePersistenceDelegate
       HoldingSpaceModel* model,
       ItemRestoredCallback item_restored_callback,
       ModelRestoredCallback model_restored_callback);
-
   HoldingSpacePersistenceDelegate(const HoldingSpacePersistenceDelegate&) =
       delete;
   HoldingSpacePersistenceDelegate& operator=(