Add Download Files to Holding Space Model

Holding space keyed service now observes downloads. As new downloads are
completed, they are added to the holding space model.

Bug: 1111995
Change-Id: Id45b276a3ada06d94f7efd42bc35cf89cc3fe070
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2378648
Commit-Queue: Ana Salazar <anasalazar@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802335}

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

@@ -9,6 +9,7 @@
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/files/file_path.h"
 #include "base/guid.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -40,10 +41,27 @@ HoldingSpaceKeyedService::HoldingSpaceKeyedService(
   holding_space_model_observer_.Add(&holding_space_model_);
   HoldingSpaceController::Get()->RegisterClientAndModelForUser(
       account_id, &holding_space_client_, &holding_space_model_);
+
+  // Observe the profile manager to get notified when the profile creation
+  // finishes to start handling user downloads. The keyed service is created
+  // with the profile, and at this stage the download manager may not be
+  // ready to be used.
+  if (g_browser_process->profile_manager()->IsValidProfile(
+          Profile::FromBrowserContext(browser_context_))) {
+    download_manager_ =
+        content::BrowserContext::GetDownloadManager(browser_context_);
+    download_manager_->AddObserver(this);
+  } else {
+    observed_profile_manager_.Add(g_browser_process->profile_manager());
+  }
 }
 
 HoldingSpaceKeyedService::~HoldingSpaceKeyedService() = default;
 
+void HoldingSpaceKeyedService::Shutdown() {
+  RemoveDownloadManagerObservers();
+}
+
 // static
 void HoldingSpaceKeyedService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
@@ -63,6 +81,18 @@ void HoldingSpaceKeyedService::AddScreenshot(
   holding_space_model_.AddItem(std::move(item));
 }
 
+void HoldingSpaceKeyedService::AddDownload(
+    const base::FilePath& download_file) {
+  GURL file_system_url = ResolveFileSystemUrl(download_file);
+  if (file_system_url.is_empty())
+    return;
+
+  auto item = HoldingSpaceItem::CreateFileBackedItem(
+      HoldingSpaceItem::Type::kDownload, download_file, file_system_url,
+      ResolveImage(download_file));
+  holding_space_model_.AddItem(std::move(item));
+}
+
 void HoldingSpaceKeyedService::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
   // `kDownload` type holding space items have their own persistence mechanism.
@@ -120,4 +150,66 @@ gfx::ImageSkia HoldingSpaceKeyedService::ResolveImage(
   return GetIconForPath(file_path);
 }
 
+void HoldingSpaceKeyedService::SetDownloadManagerForTesting(
+    content::DownloadManager* manager) {
+  RemoveDownloadManagerObservers();
+  download_manager_ = manager;
+  download_manager_->AddObserver(this);
+}
+
+void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
+  if (!profile->IsSameOrParent(Profile::FromBrowserContext(browser_context_)))
+    return;
+
+  observed_profile_manager_.RemoveAll();
+
+  // Download Manager may have been already set in tests.
+  if (download_manager_)
+    return;
+
+  download_manager_ =
+      content::BrowserContext::GetDownloadManager(browser_context_);
+  download_manager_->AddObserver(this);
+}
+
+void HoldingSpaceKeyedService::OnDownloadCreated(
+    content::DownloadManager* manager,
+    download::DownloadItem* item) {
+  download_items_observer_.Add(item);
+}
+
+void HoldingSpaceKeyedService::OnDownloadDropped(
+    content::DownloadManager* manager) {}
+
+void HoldingSpaceKeyedService::OnManagerInitialized() {}
+
+void HoldingSpaceKeyedService::ManagerGoingDown(
+    content::DownloadManager* manager) {
+  RemoveDownloadManagerObservers();
+  download_manager_ = nullptr;
+}
+
+void HoldingSpaceKeyedService::OnDownloadUpdated(download::DownloadItem* item) {
+  download::DownloadItem::DownloadState state = item->GetState();
+  if (state == download::DownloadItem::COMPLETE ||
+      state == download::DownloadItem::CANCELLED ||
+      state == download::DownloadItem::INTERRUPTED) {
+    // Stop observing now to ensure we only send one complete/fail notification.
+    download_items_observer_.Remove(item);
+
+    if (state == download::DownloadItem::COMPLETE) {
+      const base::FilePath download_path = item->GetFullPath();
+      AddDownload(download_path);
+    }
+  }
+}
+
+void HoldingSpaceKeyedService::RemoveDownloadManagerObservers() {
+  if (!download_manager_)
+    return;
+
+  download_manager_->RemoveObserver(this);
+  download_items_observer_.RemoveAll();
+}
+
 }  // namespace ash

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

@@ -9,9 +9,13 @@
 #include "ash/public/cpp/holding_space/holding_space_model_observer.h"
 #include "base/scoped_observer.h"
 #include "base/strings/string16.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_client_impl.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"
 
 class GURL;
 
@@ -37,7 +41,10 @@ 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 HoldingSpaceModelObserver {
+                                 public HoldingSpaceModelObserver,
+                                 public ProfileManagerObserver,
+                                 public content::DownloadManager::Observer,
+                                 public download::DownloadItem::Observer {
  public:
   // Preference path at which holding space items are persisted.
   // NOTE: Any changes to persistence must be backwards compatible.
@@ -53,11 +60,31 @@ class HoldingSpaceKeyedService : public KeyedService,
   // Registers profile preferences for holding space.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  // Adds a screenshot item backed by the provided absolute file path.
+  // Methods to add an item backed by the provided absolute file path.
   // The path is expected to be under a mount point path recognized by the file
   // manager app (otherwise, the item will be dropped silently).
   void AddScreenshot(const base::FilePath& screenshot_path,
                      const gfx::ImageSkia& image);
+  void AddDownload(const base::FilePath& download_path);
+
+  void RemoveDownloadManagerObservers();
+
+  // KeyedService:
+  void Shutdown() override;
+
+  // ProfileManagerObserver:
+  void OnProfileAdded(Profile* profile) override;
+
+  // content::DownloadManager::Observer implementation:
+  void OnDownloadCreated(content::DownloadManager* manager,
+                         download::DownloadItem* item) override;
+  void OnDownloadDropped(content::DownloadManager* manager) override;
+  void OnManagerInitialized() override;
+  void ManagerGoingDown(content::DownloadManager* manager) override;
+
+  // download::DownloadItem::Observer implementation:
+  void OnDownloadUpdated(download::DownloadItem* item) override;
+
 
   const HoldingSpaceClient* client_for_testing() const {
     return &holding_space_client_;
@@ -66,6 +93,7 @@ class HoldingSpaceKeyedService : public KeyedService,
   const HoldingSpaceModel* model_for_testing() const {
     return &holding_space_model_;
   }
+  void SetDownloadManagerForTesting(content::DownloadManager* manager);
 
  private:
   // HoldingSpaceModelObserver:
@@ -85,6 +113,12 @@ class HoldingSpaceKeyedService : public KeyedService,
 
   ScopedObserver<HoldingSpaceModel, HoldingSpaceModelObserver>
       holding_space_model_observer_{this};
+
+  content::DownloadManager* download_manager_ = nullptr;
+  ScopedObserver<ProfileManager, ProfileManagerObserver>
+      observed_profile_manager_{this};
+  ScopedObserver<download::DownloadItem, download::DownloadItem::Observer>
+      download_items_observer_{this};
 };
 
 }  // namespace ash

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

@@ -13,6 +13,7 @@
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/guid.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
@@ -20,18 +21,24 @@
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #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_keyed_service_factory.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/account_id/account_id.h"
+#include "components/download/public/common/mock_download_item.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/sync_preferences/pref_service_mock_factory.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "components/user_manager/scoped_user_manager.h"
+#include "content/public/browser/download_item_utils.h"
+#include "content/public/test/mock_download_manager.h"
 #include "storage/browser/file_system/external_mount_points.h"
 #include "storage/browser/file_system/file_system_context.h"
 #include "storage/browser/file_system/file_system_url.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_unittest_util.h"
@@ -96,7 +103,8 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
  public:
   HoldingSpaceKeyedServiceTest()
       : fake_user_manager_(new chromeos::FakeChromeUserManager),
-        user_manager_enabler_(base::WrapUnique(fake_user_manager_)) {
+        user_manager_enabler_(base::WrapUnique(fake_user_manager_)),
+        download_manager_(new testing::NiceMock<content::MockDownloadManager>) {
     scoped_feature_list_.InitAndEnableFeature(features::kTemporaryHoldingSpace);
   }
   HoldingSpaceKeyedServiceTest(const HoldingSpaceKeyedServiceTest& other) =
@@ -230,10 +238,32 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
     return result;
   }
 
+  std::unique_ptr<download::MockDownloadItem> CreateMockInProgressDownload(
+      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* 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_;
 };
 
@@ -446,4 +476,70 @@ TEST_F(HoldingSpaceKeyedServiceTest, RestorePersistentStorage) {
   }
 }
 
+TEST_F(HoldingSpaceKeyedServiceTest, 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_full_path =
+      CreateFile(downloads_mount, download_item_virtual_path, "download 1");
+
+  content::MockDownloadManager* mock_download_manager = manager();
+  std::unique_ptr<download::MockDownloadItem> item(
+      CreateMockInProgressDownload(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,
+                                                     mock_download_item]() {
+                           holding_space_service->OnDownloadCreated(
+                               mock_download_manager, mock_download_item);
+                         }),
+                         testing::Return(item.get())));
+
+  std::vector<GURL> url_chain;
+  url_chain.push_back(item->GetURL());
+  mock_download_manager->CreateDownloadItem(
+      base::GenerateGUID(), item->GetId(), item->GetFullPath(),
+      item->GetFullPath(), url_chain, GURL(), GURL(), GURL(), GURL(),
+      url::Origin(), item->GetMimeType(), item->GetMimeType(),
+      base::Time::Now(), base::Time::Now(), "", "", 10, 10, "",
+      download::DownloadItem::IN_PROGRESS,
+      download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+      download::DOWNLOAD_INTERRUPT_REASON_NONE, false, base::Time::Now(), false,
+      std::vector<download::DownloadItem::ReceivedSlice>());
+
+  HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
+  ASSERT_EQ(0u, model->items().size());
+
+  EXPECT_CALL(*item, GetState())
+      .WillRepeatedly(testing::Return(download::DownloadItem::IN_PROGRESS));
+  item->NotifyObserversDownloadUpdated();
+
+  ASSERT_EQ(0u, model->items().size());
+
+  EXPECT_CALL(*item, GetState())
+      .WillRepeatedly(testing::Return(download::DownloadItem::COMPLETE));
+  item->NotifyObserversDownloadUpdated();
+
+  ASSERT_EQ(1u, model->items().size());
+
+  const HoldingSpaceItem* download_item = model->items()[0].get();
+  EXPECT_EQ(download_item_full_path, download_item->file_path());
+  EXPECT_EQ(download_item_virtual_path,
+            GetVirtualPathFromUrl(download_item->file_system_url(),
+                                  downloads_mount.name()));
+}
+
 }  // namespace ash