Support delayed restore of persisted holding space items

Moves responsibility for validating that holding space items restored
from persistence delegate to file system delegate.
The persistence delegate just de-serializes items from the persistent
storage and adds them to the model as partially initialized items.

File system delegate handles file path validity for non-finalized items,
and removes items whose backing file path cannot be found from the
model. This results in a simpler restoration flow where file validity
checks can be unified.
This CL updates the logic for file existence verification when items
are restored from persistence to better items from mount points that
are not immediately mounted very on startup (for example drive fs).

The file system delegate will leave items whose file system URL cannot
be resolved in partially initialized state until a mount point that
parents the item is mounted.

Note that the items will not remain in partially initialized state
indefinitely - they will be cleaned up if the volume has not been
mounted soon after startup.

Renames ScopedTestDownloadsMountPoint into ScopedTestMountPoint and
makes is a bit more customizable by tests - for example, enables tests
to set up mount point state when the mount point is added to the
external mount points.

BUG=1140150

Change-Id: I924b9b932039d1f1a1498dc3ce4b73ddeac0ddbb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490259Reviewed-by: Josh Nohle <nohle@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823849}

chrome/browser/nearby_sharing/nearby_notification_manager_unittest.cc

@@ -39,7 +39,7 @@
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/ui/ash/holding_space/fake_holding_space_color_provider.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
-#include "chrome/browser/ui/ash/holding_space/scoped_test_downloads_mount_point.h"
+#include "chrome/browser/ui/ash/holding_space/scoped_test_mount_point.h"
 #include "chrome/browser/ui/ash/test_session_controller.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -1188,14 +1188,17 @@ class NearbyFilesHoldingSpaceTest : public testing::Test {
 };
 
 TEST_F(NearbyFilesHoldingSpaceTest, ShowSuccess_Files) {
-  ash::holding_space::ScopedTestDownloadsMountPoint downloads_mount(profile_);
-  ASSERT_TRUE(downloads_mount.IsValid());
+  std::unique_ptr<ash::holding_space::ScopedTestMountPoint> downloads_mount =
+      ash::holding_space::ScopedTestMountPoint::CreateAndMountDownloads(
+          profile_);
+  ASSERT_TRUE(downloads_mount->IsValid());
+
   ShareTarget share_target;
   share_target.is_incoming = true;
 
   const base::FilePath file_virtual_path("Sample.txt");
   base::FilePath file_path =
-      downloads_mount.CreateFile(file_virtual_path, "Sample Text");
+      downloads_mount->CreateFile(file_virtual_path, "Sample Text");
 
   FileAttachment attachment(file_path);
   share_target.file_attachments.push_back(std::move(attachment));

chrome/browser/ui/ash/holding_space/BUILD.gn

@@ -40,8 +40,8 @@ source_set("test_support") {
     "//chrome/browser/ui/ash/holding_space/fake_holding_space_color_provider.h",
     "//chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.cc",
     "//chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.h",
-    "//chrome/browser/ui/ash/holding_space/scoped_test_downloads_mount_point.cc",
-    "//chrome/browser/ui/ash/holding_space/scoped_test_downloads_mount_point.h",
+    "//chrome/browser/ui/ash/holding_space/scoped_test_mount_point.cc",
+    "//chrome/browser/ui/ash/holding_space/scoped_test_mount_point.h",
   ]
 
   deps = [

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

@@ -6,12 +6,13 @@
 
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "base/files/file_path.h"
 #include "base/files/file_path_watcher.h"
 #include "base/sequence_checker.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
-#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -68,10 +69,8 @@ class HoldingSpaceFileSystemDelegate::FileSystemWatcher {
 
 HoldingSpaceFileSystemDelegate::HoldingSpaceFileSystemDelegate(
     Profile* profile,
-    HoldingSpaceModel* model,
-    FileRemovedCallback file_removed_callback)
+    HoldingSpaceModel* model)
     : HoldingSpaceKeyedServiceDelegate(profile, model),
-      file_removed_callback_(file_removed_callback),
       file_system_watcher_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::TaskPriority::BEST_EFFORT})) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -89,18 +88,58 @@ void HoldingSpaceFileSystemDelegate::Init() {
   file_system_watcher_ = std::make_unique<FileSystemWatcher>(
       base::Bind(&HoldingSpaceFileSystemDelegate::OnFilePathChanged,
                  weak_factory_.GetWeakPtr()));
+
+  if (file_manager::VolumeManager::Get(profile()))
+    volume_manager_observer_.Add(file_manager::VolumeManager::Get(profile()));
+
+  // Schedule a task to clean up items that belong to volumes that haven't been
+  // mounted in a reasonable amount of time.
+  // The primary goal of handling the delayed volume mount is to support volumes
+  // that are mounted asynchronously during the startup.
+  clear_non_finalized_items_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromMinutes(1),
+      base::BindOnce(&HoldingSpaceFileSystemDelegate::ClearNonFinalizedItems,
+                     base::Unretained(this)));
+}
+
+void HoldingSpaceFileSystemDelegate::Shutdown() {
+  volume_manager_observer_.RemoveAll();
 }
 
 void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (!item->IsFinalized())
+  if (item->IsFinalized()) {
+    // Watch the directory containing `items`'s backing file. If the directory
+    // is already being watched, this will no-op.
+    AddWatch(item->file_path().DirName());
+    return;
+  }
+
+  // If the item has not yet been finalized, check whether it's path can be
+  // resolved to a file system URL - failure to do so may indicate that the
+  // volume to which it path belongs is not mounted there. If the file system
+  // URL cannot be resolved, leave the item in partially initialized state.
+  // Validity will be checked when the associated mount point is mounted.
+  // NOTE: Items will not be kept in partially initialized state indefinitely
+  // - see `clear_non_finalized_items_timer_`.
+  // NOTE: This does not work well for removable devices, as all removable
+  // devices have the same top level "external" mount point (media/removable),
+  // so a removable device path will be successfully resolved even if the
+  // device is not currently mounted. The logic will have to be updated if
+  // support for restoring items across removable device mounts becomes a
+  // requirement.
+  const GURL file_system_url =
+      holding_space_util::ResolveFileSystemUrl(profile(), item->file_path());
+  if (file_system_url.is_empty())
     return;
 
-  // Watch the directory containing `items`'s backing file. If the directory is
-  // already being watched, this will no-op.
-  AddWatch(item->file_path().DirName());
+  holding_space_util::ValidityRequirement requirements;
+  if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
+    requirements.must_be_newer_than = kMaxFileAge;
+
+  ScheduleFilePathValidityCheck({item->file_path(), requirements});
 }
 
 void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemRemoved(
@@ -126,6 +165,36 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemFinalized(
   AddWatch(item->file_path().DirName());
 }
 
+void HoldingSpaceFileSystemDelegate::OnVolumeMounted(
+    chromeos::MountError error_code,
+    const file_manager::Volume& volume) {
+  holding_space_util::FilePathsWithValidityRequirements
+      file_paths_with_requirements;
+  // Check validity of partially initialized items under the volume's mount
+  // path.
+  for (auto& item : model()->items()) {
+    if (item->IsFinalized())
+      continue;
+    if (!volume.mount_path().IsParent(item->file_path()))
+      continue;
+
+    holding_space_util::ValidityRequirement requirements;
+    if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
+      requirements.must_be_newer_than = kMaxFileAge;
+    ScheduleFilePathValidityCheck({item->file_path(), requirements});
+  }
+}
+
+void HoldingSpaceFileSystemDelegate::OnVolumeUnmounted(
+    chromeos::MountError error_code,
+    const file_manager::Volume& volume) {
+  model()->RemoveIf(base::BindRepeating(
+      [](const base::FilePath& volume_path, const HoldingSpaceItem* item) {
+        return volume_path.IsParent(item->file_path());
+      },
+      volume.mount_path()));
+}
+
 void HoldingSpaceFileSystemDelegate::OnFilePathChanged(
     const base::FilePath& file_path,
     bool error) {
@@ -137,31 +206,69 @@ void HoldingSpaceFileSystemDelegate::OnFilePathChanged(
   // that some, all, or none of these backing files have been removed. We need
   // to verify the existence of these backing files and remove any holding space
   // items that no longer exist.
-
-  holding_space_util::FilePathsWithValidityRequirements
-      file_paths_with_requirements;
   for (const auto& item : model()->items()) {
-    if (file_path.IsParent(item->file_path())) {
-      file_paths_with_requirements.push_back(
-          {item->file_path(), /*requirements=*/{}});
-    }
+    if (file_path.IsParent(item->file_path()))
+      ScheduleFilePathValidityCheck({item->file_path(), /*requirements=*/{}});
   }
+}
+
+void HoldingSpaceFileSystemDelegate::ScheduleFilePathValidityCheck(
+    holding_space_util::FilePathWithValidityRequirement requirement) {
+  pending_file_path_validity_checks_.push_back(std::move(requirement));
+  if (file_path_validity_checks_scheduled_)
+    return;
+
+  file_path_validity_checks_scheduled_ = true;
+
+  // Schedule file validity check for pending items. The check is scheduled
+  // asynchronously so path checks added in quick succession are handled in a
+  // single batch.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &HoldingSpaceFileSystemDelegate::RunPendingFilePathValidityChecks,
+          weak_factory_.GetWeakPtr()));
+}
+
+void HoldingSpaceFileSystemDelegate::RunPendingFilePathValidityChecks() {
+  holding_space_util::FilePathsWithValidityRequirements requirements;
+  requirements.swap(pending_file_path_validity_checks_);
 
+  file_path_validity_checks_scheduled_ = false;
   holding_space_util::PartitionFilePathsByValidity(
-      profile(), std::move(file_paths_with_requirements),
+      profile(), std::move(requirements),
       base::BindOnce(
-          [](const base::WeakPtr<HoldingSpaceFileSystemDelegate>& weak_ptr,
-             std::vector<base::FilePath> valid_file_paths,
-             std::vector<base::FilePath> invalid_file_paths) {
-            if (weak_ptr) {
-              auto file_removed_callback = weak_ptr->file_removed_callback_;
-              for (const auto& invalid_file_path : invalid_file_paths)
-                file_removed_callback.Run(invalid_file_path);
-            }
-          },
+          &HoldingSpaceFileSystemDelegate::OnFilePathValidityChecksComplete,
           weak_factory_.GetWeakPtr()));
 }
 
+void HoldingSpaceFileSystemDelegate::OnFilePathValidityChecksComplete(
+    std::vector<base::FilePath> valid_paths,
+    std::vector<base::FilePath> invalid_paths) {
+  // Remove items with invalid paths.
+  // When `file_path` is removed, we need to remove any associated items.
+  model()->RemoveIf(base::BindRepeating(
+      [](const std::vector<base::FilePath>* invalid_paths,
+         const HoldingSpaceItem* item) {
+        return base::Contains(*invalid_paths, item->file_path());
+      },
+      &invalid_paths));
+
+  std::vector<const HoldingSpaceItem*> items_to_finalize;
+  for (auto& item : model()->items()) {
+    if (!item->IsFinalized() &&
+        base::Contains(valid_paths, item->file_path())) {
+      items_to_finalize.push_back(item.get());
+    }
+  }
+
+  for (auto* item : items_to_finalize) {
+    model()->FinalizeOrRemoveItem(
+        item->id(),
+        holding_space_util::ResolveFileSystemUrl(profile(), item->file_path()));
+  }
+}
+
 void HoldingSpaceFileSystemDelegate::AddWatch(const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   file_system_watcher_runner_->PostTask(
@@ -177,4 +284,9 @@ void HoldingSpaceFileSystemDelegate::RemoveWatch(
                                 file_system_watcher_->GetWeakPtr(), file_path));
 }
 
+void HoldingSpaceFileSystemDelegate::ClearNonFinalizedItems() {
+  model()->RemoveIf(base::BindRepeating(
+      [](const HoldingSpaceItem* item) { return !item->IsFinalized(); }));
+}
+
 }  // namespace ash

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

@@ -8,7 +8,12 @@
 #include <memory>
 
 #include "base/callback.h"
+#include "base/scoped_observer.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/chromeos/file_manager/volume_manager.h"
+#include "chrome/browser/chromeos/file_manager/volume_manager_observer.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 
 namespace base {
 class FilePath;
@@ -16,18 +21,16 @@ class FilePath;
 
 namespace ash {
 
-// A delegate of `HoldingSpaceKeyedService` tasked with monitoring the file
-// system for removal of files backing holding space items.
-class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate {
+// A delegate of `HoldingSpaceKeyedService` tasked with verifying validity of
+// files backing holding space items. The delegate:
+// *  Finalizes partially initialized items loaded from persistent storage once
+//    the validity of the backing file path was verified.
+// *  Monitors the file system for removal of files backing holding space
+//    items.
+class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate,
+                                       file_manager::VolumeManagerObserver {
  public:
-  // Callback to be invoked when a watched file path is removed. The delegate
-  // will watch file paths for all holding space items in the model.
-  using FileRemovedCallback =
-      base::RepeatingCallback<void(const base::FilePath&)>;
-
-  HoldingSpaceFileSystemDelegate(Profile* profile,
-                                 HoldingSpaceModel* model,
-                                 FileRemovedCallback file_removed_callback);
+  HoldingSpaceFileSystemDelegate(Profile* profile, HoldingSpaceModel* model);
   HoldingSpaceFileSystemDelegate(const HoldingSpaceFileSystemDelegate&) =
       delete;
   HoldingSpaceFileSystemDelegate& operator=(
@@ -39,19 +42,45 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate {
 
   // HoldingSpaceKeyedServiceDelegate:
   void Init() override;
+  void Shutdown() override;
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
+  // file_manager::VolumeManagerObserver:
+  void OnVolumeMounted(chromeos::MountError error_code,
+                       const file_manager::Volume& volume) override;
+  void OnVolumeUnmounted(chromeos::MountError error_code,
+                         const file_manager::Volume& volume) override;
+
   // Invoked when the specified `file_path` has changed.
   void OnFilePathChanged(const base::FilePath& file_path, bool error);
 
+  // Adds file path validity requirement to `pending_file_path_validity_checks_`
+  // and schedules a path validity check task (if another task is not already
+  // scheduled).
+  void ScheduleFilePathValidityCheck(
+      holding_space_util::FilePathWithValidityRequirement requirement);
+
+  // Runs validity checks for file paths in
+  // `pending_file_path_validity_checks_`. The checks are performed
+  // asynchronously (with callback `OnFilePathValidityChecksComplete()`).
+  void RunPendingFilePathValidityChecks();
+
+  // Callback for a batch of file path validity checks - it updates the model
+  // depending on the determined file path state.
+  void OnFilePathValidityChecksComplete(
+      std::vector<base::FilePath> valid_paths,
+      std::vector<base::FilePath> invalid_paths);
+
   // Adds/removes a watch for the specified `file_path`.
   void AddWatch(const base::FilePath& file_path);
   void RemoveWatch(const base::FilePath& file_path);
 
-  // Callback to invoke when file removal is detected.
-  FileRemovedCallback file_removed_callback_;
+  // Clears all non-finalized items from holding space model - runs with a delay
+  // after profile initialization to clean up items from volumes that have not
+  // been mounted during startup.
+  void ClearNonFinalizedItems();
 
   // The `file_system_watcher_` is tasked with watching the file system for
   // changes on behalf of the delegate. It does so on a non-UI sequence. As
@@ -61,6 +90,23 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate {
   std::unique_ptr<FileSystemWatcher> file_system_watcher_;
   scoped_refptr<base::SequencedTaskRunner> file_system_watcher_runner_;
 
+  // List of file path validity checks that need to be run.
+  holding_space_util::FilePathsWithValidityRequirements
+      pending_file_path_validity_checks_;
+
+  // Whether a task to run validity checks in
+  // `pending_file_path_validity_checks_` is scheduled.
+  bool file_path_validity_checks_scheduled_ = false;
+
+  // A timer to run clean-up task for items that have not been finalized within
+  // a reasonable amount of time from start-up. (E.g. if the volume they belong
+  // to has not been yet mounted).
+  base::OneShotTimer clear_non_finalized_items_timer_;
+
+  ScopedObserver<file_manager::VolumeManager,
+                 file_manager::VolumeManagerObserver>
+      volume_manager_observer_{this};
+
   base::WeakPtrFactory<HoldingSpaceFileSystemDelegate> weak_factory_{this};
 };
 

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

@@ -224,10 +224,7 @@ void HoldingSpaceKeyedService::OnProfileReady() {
 
   // The `HoldingSpaceFileSystemDelegate` monitors the file system for changes.
   delegates_.push_back(std::make_unique<HoldingSpaceFileSystemDelegate>(
-      profile_, &holding_space_model_,
-      /*file_removed_callback=*/
-      base::BindRepeating(&HoldingSpaceKeyedService::OnFileRemoved,
-                          weak_factory_.GetWeakPtr())));
+      profile_, &holding_space_model_));
 
   // The `HoldingSpacePersistenceDelegate` manages holding space persistence.
   delegates_.push_back(std::make_unique<HoldingSpacePersistenceDelegate>(
@@ -246,15 +243,6 @@ void HoldingSpaceKeyedService::OnProfileReady() {
     delegate->Init();
 }
 
-void HoldingSpaceKeyedService::OnFileRemoved(const base::FilePath& file_path) {
-  // When `file_path` is removed, we need to remove any associated items.
-  holding_space_model_.RemoveIf(base::BindRepeating(
-      [](const base::FilePath& file_path, const HoldingSpaceItem* item) {
-        return item->file_path() == file_path;
-      },
-      std::cref(file_path)));
-}
-
 void HoldingSpaceKeyedService::OnPersistenceRestored() {
   for (auto& delegate : delegates_)
     delegate->NotifyPersistenceRestored();

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

@@ -102,9 +102,6 @@ class HoldingSpaceKeyedService : public KeyedService,
   // Invoked when the associated profile is ready.
   void OnProfileReady();
 
-  // Invoked when the specified `file_path` is removed.
-  void OnFileRemoved(const base::FilePath& file_path);
-
   // Invoked when holding space persistence has been restored.
   void OnPersistenceRestored();
 

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

@@ -40,7 +40,7 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
   Profile* profile() { return profile_; }
 
   // Returns the holding space model owned by `HoldingSpaceKeyedService`.
-  const HoldingSpaceModel* model() const { return model_; }
+  HoldingSpaceModel* model() { return model_; }
 
   // Returns if persistence is being restored.
   bool is_restoring_persistence() const { return is_restoring_persistence_; }
@@ -55,7 +55,7 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
   virtual void OnPersistenceRestored();
 
   Profile* const profile_;
-  const HoldingSpaceModel* const model_;
+  HoldingSpaceModel* const model_;
 
   // If persistence is being restored.
   bool is_restoring_persistence_ = true;

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

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
 
 #include "ash/public/cpp/ash_features.h"
+#include "chrome/browser/chromeos/file_manager/volume_manager_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h"
@@ -24,7 +25,9 @@ HoldingSpaceKeyedServiceFactory::GetInstance() {
 HoldingSpaceKeyedServiceFactory::HoldingSpaceKeyedServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "HoldingSpaceService",
-          BrowserContextDependencyManager::GetInstance()) {}
+          BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(file_manager::VolumeManagerFactory::GetInstance());
+}
 
 HoldingSpaceKeyedService* HoldingSpaceKeyedServiceFactory::GetService(
     content::BrowserContext* context) {

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

@@ -8,7 +8,6 @@
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/scoped_user_pref_update.h"
 
@@ -80,10 +79,6 @@ void HoldingSpacePersistenceDelegate::RestoreModelFromPersistence() {
     return;
   }
 
-  std::vector<HoldingSpaceItemPtr> holding_space_items;
-  holding_space_util::FilePathsWithValidityRequirements
-      file_paths_with_requirements;
-
   for (const auto& persisted_holding_space_item :
        persisted_holding_space_items->GetList()) {
     std::unique_ptr<HoldingSpaceItem> holding_space_item =
@@ -91,56 +86,8 @@ void HoldingSpacePersistenceDelegate::RestoreModelFromPersistence() {
             base::Value::AsDictionaryValue(persisted_holding_space_item),
             base::BindOnce(&holding_space_util::ResolveImage,
                            base::Unretained(thumbnail_loader_)));
-    const GURL file_system_url = holding_space_util::ResolveFileSystemUrl(
-        profile(), holding_space_item->file_path());
-
-    // TODO(https://crbug.com/1140150): Better handle the case an item file
-    // path cannot be resolved to a file system URL.
-    if (!file_system_url.is_empty())
-      holding_space_item->Finalize(file_system_url);
-
-    holding_space_util::ValidityRequirement requirements;
-    if (holding_space_item->type() != HoldingSpaceItem::Type::kPinnedFile)
-      requirements.must_be_newer_than = kMaxFileAge;
-    file_paths_with_requirements.push_back(
-        {holding_space_item->file_path(), requirements});
-
-    holding_space_items.push_back(std::move(holding_space_item));
+    item_restored_callback_.Run(std::move(holding_space_item));
   }
-
-  holding_space_util::PartitionFilePathsByValidity(
-      profile(), std::move(file_paths_with_requirements),
-      base::BindOnce(&HoldingSpacePersistenceDelegate::RestoreModelByValidity,
-                     weak_factory_.GetWeakPtr(),
-                     std::move(holding_space_items)));
-}
-
-void HoldingSpacePersistenceDelegate::RestoreModelByValidity(
-    std::vector<HoldingSpaceItemPtr> holding_space_items,
-    std::vector<base::FilePath> valid_file_paths,
-    std::vector<base::FilePath> invalid_file_paths) {
-  DCHECK(model()->items().empty());
-
-  // Restore valid holding space items.
-  for (auto& holding_space_item : holding_space_items) {
-    if (holding_space_item->IsFinalized() &&
-        base::Contains(valid_file_paths, holding_space_item->file_path())) {
-      item_restored_callback_.Run(std::move(holding_space_item));
-    }
-  }
-
-  // Clean up invalid holding space items from persistence.
-  if (!invalid_file_paths.empty()) {
-    ListPrefUpdate update(profile()->GetPrefs(), kPersistencePath);
-    update->EraseListValueIf(
-        [&invalid_file_paths](const base::Value& persisted_item) {
-          base::FilePath persisted_file_path =
-              HoldingSpaceItem::DeserializeFilePath(
-                  base::Value::AsDictionaryValue(persisted_item));
-          return base::Contains(invalid_file_paths, persisted_file_path);
-        });
-  }
-
   // Notify completion of persistence restoration.
   std::move(persistence_restored_callback_).Run();
 }

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

@@ -10,10 +10,7 @@
 
 #include "base/callback.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h"
-
-namespace base {
-class FilePath;
-}  // namespace base
+#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 
 namespace user_prefs {
 class PrefRegistrySyncable;
@@ -68,10 +65,6 @@ class HoldingSpacePersistenceDelegate
 
   // Restores the holding space model from persistent storage.
   void RestoreModelFromPersistence();
-  void RestoreModelByValidity(
-      std::vector<HoldingSpaceItemPtr> holding_space_items,
-      std::vector<base::FilePath> valid_file_paths,
-      std::vector<base::FilePath> invalid_file_paths);
 
   // Owned by `HoldingSpaceKeyedService`.
   HoldingSpaceThumbnailLoader* const thumbnail_loader_;

chrome/browser/ui/ash/holding_space/scoped_test_downloads_mount_point.cc → chrome/browser/ui/ash/holding_space/scoped_test_mount_point.cc

@@ -2,37 +2,77 @@
 // 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/scoped_test_downloads_mount_point.h"
+#include "chrome/browser/ui/ash/holding_space/scoped_test_mount_point.h"
 
 #include "base/files/file_util.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
+#include "chrome/browser/chromeos/file_manager/volume_manager_factory.h"
 #include "storage/browser/file_system/external_mount_points.h"
 #include "storage/browser/file_system/file_system_context.h"
 
 namespace ash {
 namespace holding_space {
-ScopedTestDownloadsMountPoint::ScopedTestDownloadsMountPoint(Profile* profile)
-    : name_(file_manager::util::GetDownloadsMountPointName(profile)) {
-  if (!temp_dir_.CreateUniqueTempDir())
-    return;
+
+// static
+std::unique_ptr<ScopedTestMountPoint>
+ScopedTestMountPoint::CreateAndMountDownloads(Profile* profile) {
+  auto mount_point = std::make_unique<ScopedTestMountPoint>(
+      file_manager::util::GetDownloadsMountPointName(profile),
+      storage::kFileSystemTypeNativeLocal,
+      file_manager::VOLUME_TYPE_DOWNLOADS_DIRECTORY);
+  mount_point->Mount(profile);
+  return mount_point;
+}
+
+ScopedTestMountPoint::ScopedTestMountPoint(
+    const std::string& name,
+    storage::FileSystemType file_system_type,
+    file_manager::VolumeType volume_type)
+    : name_(name),
+      file_system_type_(file_system_type),
+      volume_type_(volume_type) {
+  CHECK(temp_dir_.CreateUniqueTempDir());
+}
+
+ScopedTestMountPoint::~ScopedTestMountPoint() {
+  if (mounted_) {
+    storage::ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(name_);
+    if (file_manager::VolumeManager::Get(profile_)) {
+      file_manager::VolumeManager::Get(profile_)
+          ->RemoveVolumeForTesting(  // IN-TEST
+              temp_dir_.GetPath(), volume_type_, chromeos::DEVICE_TYPE_UNKNOWN,
+              /*read_only=*/false);
+    }
+  }
+}
+void ScopedTestMountPoint::Mount(Profile* profile) {
+  DCHECK(!profile_);
+  DCHECK(!mounted_);
+  profile_ = profile;
+  mounted_ = true;
 
   storage::ExternalMountPoints::GetSystemInstance()->RegisterFileSystem(
-      name_, storage::kFileSystemTypeNativeLocal,
-      storage::FileSystemMountOption(), temp_dir_.GetPath());
+      name_, file_system_type_, storage::FileSystemMountOption(),
+      temp_dir_.GetPath());
   file_manager::util::GetFileSystemContextForExtensionId(
       profile, file_manager::kFileManagerAppId)
       ->external_backend()
       ->GrantFileAccessToExtension(file_manager::kFileManagerAppId,
                                    base::FilePath(name_));
+  if (file_manager::VolumeManager::Get(profile_)) {
+    file_manager::VolumeManager::Get(profile_)->AddVolumeForTesting(  // IN-TEST
+        temp_dir_.GetPath(), volume_type_, chromeos::DEVICE_TYPE_UNKNOWN,
+        /*read_only=*/false);
+  }
 }
 
-ScopedTestDownloadsMountPoint::~ScopedTestDownloadsMountPoint() {
-  storage::ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(name_);
+bool ScopedTestMountPoint::IsValid() const {
+  return temp_dir_.IsValid();
 }
 
-base::FilePath ScopedTestDownloadsMountPoint::CreateFile(
+base::FilePath ScopedTestMountPoint::CreateFile(
     const base::FilePath& relative_path,
     const std::string& content) {
   const base::FilePath path = GetRootPath().Append(relative_path);
@@ -43,10 +83,10 @@ base::FilePath ScopedTestDownloadsMountPoint::CreateFile(
   return path;
 }
 
-base::FilePath ScopedTestDownloadsMountPoint::CreateArbitraryFile() {
+base::FilePath ScopedTestMountPoint::CreateArbitraryFile() {
   return CreateFile(base::FilePath(base::UnguessableToken::Create().ToString()),
                     /*content=*/std::string());
 }
 
 }  // namespace holding_space
-}  // namespace ash
\ No newline at end of file
+}  // namespace ash

chrome/browser/ui/ash/holding_space/scoped_test_downloads_mount_point.h → chrome/browser/ui/ash/holding_space/scoped_test_mount_point.h

@@ -2,14 +2,14 @@
 // 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_SCOPED_TEST_DOWNLOADS_MOUNT_POINT_H_
-#define CHROME_BROWSER_UI_ASH_HOLDING_SPACE_SCOPED_TEST_DOWNLOADS_MOUNT_POINT_H_
+#ifndef CHROME_BROWSER_UI_ASH_HOLDING_SPACE_SCOPED_TEST_MOUNT_POINT_H_
+#define CHROME_BROWSER_UI_ASH_HOLDING_SPACE_SCOPED_TEST_MOUNT_POINT_H_
 
 #include <memory>
 
 #include "base/files/scoped_temp_dir.h"
+#include "chrome/browser/chromeos/file_manager/volume_manager.h"
 
-class GURL;
 class Profile;
 
 namespace base {
@@ -21,18 +21,33 @@ namespace holding_space {
 
 // Utility class that registers the downloads external file system mount point,
 // and grants file manager app access permission for the mount point.
-class ScopedTestDownloadsMountPoint {
+class ScopedTestMountPoint {
  public:
-  explicit ScopedTestDownloadsMountPoint(Profile* profile);
-  ScopedTestDownloadsMountPoint(const ScopedTestDownloadsMountPoint&) = delete;
-  ScopedTestDownloadsMountPoint& operator=(
-      const ScopedTestDownloadsMountPoint&) = delete;
-  ~ScopedTestDownloadsMountPoint();
+  // Creates and mounts a mount point for downloads.
+  static std::unique_ptr<ScopedTestMountPoint> CreateAndMountDownloads(
+      Profile* profile);
 
-  bool IsValid() const { return temp_dir_.IsValid(); }
+  // Creates a mount point on the file system - the backing mount path existence
+  // will be scoped to the `ScopedTestMountPoint` lifetime, but the mount point
+  // will not be registered as an external mount point by default (so tests can
+  // initialize mount point state before adding it as an external mount point).
+  ScopedTestMountPoint(const std::string& name,
+                       storage::FileSystemType file_system_type,
+                       file_manager::VolumeType volume_type);
+  ScopedTestMountPoint(const ScopedTestMountPoint&) = delete;
+  ScopedTestMountPoint& operator=(const ScopedTestMountPoint&) = delete;
+  ~ScopedTestMountPoint();
 
+  // Mounts the mount point - registarts the mount point as an external system
+  // mount point, and adds it to the file manager's volume manager.
+  void Mount(Profile* profile);
+
+  // Gets the mount point root path on the local file system.
   const base::FilePath& GetRootPath() const { return temp_dir_.GetPath(); }
 
+  // Whether the mount point directory has been successfully created.
+  bool IsValid() const;
+
   const std::string& name() const { return name_; }
 
   // Creates a file under [mount point root path]/|relative_path| with the
@@ -45,11 +60,16 @@ class ScopedTestDownloadsMountPoint {
   base::FilePath CreateArbitraryFile();
 
  private:
-  base::ScopedTempDir temp_dir_;
   std::string name_;
+  const storage::FileSystemType file_system_type_;
+  const file_manager::VolumeType volume_type_;
+
+  Profile* profile_ = nullptr;
+  bool mounted_ = false;
+  base::ScopedTempDir temp_dir_;
 };
 
 }  // namespace holding_space
 }  // namespace ash
 
-#endif  // CHROME_BROWSER_UI_ASH_HOLDING_SPACE_SCOPED_TEST_DOWNLOADS_MOUNT_POINT_H_
\ No newline at end of file
+#endif  // CHROME_BROWSER_UI_ASH_HOLDING_SPACE_SCOPED_TEST_MOUNT_POINT_H_