Revert "Introduce partially initialized holding space items"

This reverts commit 314e453b.

Reason for revert: HoldingSpaceTrayTest.ShowTrayButtonOnFirstUse is crashing on linux-chromeos-dbg:
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/21023


Original change's description:
> Introduce partially initialized holding space items
>
> Introduces a concept of partially initialized HoldingSpaceItem - this
> state will be used when loading items from persistent storage in case
> the item file system URL cannot immediately be resolved (e.g. if the
> associated mount point hasn't been loaded yet).
> HoldingSpacePersistenceDelegate will deserialize persisted items into
> partial state when they are first loaded from the storage, and
> "finalize" them when their file system URL can be resolved.
>
> Compared to an alternative approach where items whose initialization has
> to be deferred, this approach keeps the order in which items are added
> to the model.
>
> Updates holding space UI not to show partially initialized items.
>
> BUG=1140150
>
> Change-Id: Ie19997ec02f6323434f4053f6ba41a2998e72efa
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493323
> Commit-Queue: Toni Baržić <tbarzic@chromium.org>
> Reviewed-by: David Black <dmblack@google.com>
> Cr-Commit-Position: refs/heads/master@{#821024}

TBR=tbarzic@chromium.org,dmblack@google.com

Change-Id: Ie916d1b4f15eab360b7124c481105733736c671c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1140150
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2500688Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821111}

ash/BUILD.gn

@@ -2129,7 +2129,6 @@ test("ash_unittests") {
     "system/bluetooth/tray_bluetooth_helper_legacy_unittest.cc",
     "system/caps_lock_notification_controller_unittest.cc",
     "system/gesture_education/gesture_education_notification_controller_unittest.cc",
-    "system/holding_space/holding_space_tray_unittest.cc",
     "system/ime/ime_feature_pod_controller_unittest.cc",
     "system/ime_menu/ime_menu_tray_unittest.cc",
     "system/locale/locale_feature_pod_controller_unittest.cc",

ash/public/cpp/holding_space/holding_space_item.cc

@@ -63,8 +63,6 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
     const base::FilePath& file_path,
     const GURL& file_system_url,
     std::unique_ptr<HoldingSpaceImage> image) {
-  DCHECK(!file_system_url.is_empty());
-
   // Note: std::make_unique does not work with private constructors.
   return base::WrapUnique(new HoldingSpaceItem(
       type, GetFileBackedItemId(type, file_path), file_path, file_system_url,
@@ -76,6 +74,7 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
 // serialization versions are supported, care must be taken to handle each.
 std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::Deserialize(
     const base::DictionaryValue& dict,
+    FileSystemUrlResolver file_system_url_resolver,
     ImageResolver image_resolver) {
   const base::Optional<int> version = dict.FindIntPath(kVersionPath);
   DCHECK(version.has_value() && version.value() == kVersion);
@@ -84,10 +83,11 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::Deserialize(
   const base::FilePath file_path = DeserializeFilePath(dict);
 
   // NOTE: `std::make_unique` does not work with private constructors.
-  return base::WrapUnique(new HoldingSpaceItem(
-      type, DeserializeId(dict), file_path,
-      /*file_system_url=*/GURL(), file_path.BaseName().LossyDisplayName(),
-      std::move(image_resolver).Run(type, file_path)));
+  return base::WrapUnique(
+      new HoldingSpaceItem(type, DeserializeId(dict), file_path,
+                           std::move(file_system_url_resolver).Run(file_path),
+                           file_path.BaseName().LossyDisplayName(),
+                           std::move(image_resolver).Run(type, file_path)));
 }
 
 // static
@@ -144,14 +144,4 @@ HoldingSpaceItem::HoldingSpaceItem(Type type,
       text_(text),
       image_(std::move(image)) {}
 
-bool HoldingSpaceItem::IsFinalized() const {
-  return !file_system_url_.is_empty();
-}
-
-void HoldingSpaceItem::Finalize(const GURL& file_system_url) {
-  DCHECK(!IsFinalized());
-  DCHECK(!file_system_url.is_empty());
-  file_system_url_ = file_system_url;
-}
-
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_item.h

@@ -50,7 +50,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
                                          const base::FilePath& file_path);
 
   // Creates a HoldingSpaceItem that's backed by a file system URL.
-  // NOTE: `file_system_url` is expected to be non-empty.
   static std::unique_ptr<HoldingSpaceItem> CreateFileBackedItem(
       Type type,
       const base::FilePath& file_path,
@@ -65,10 +64,9 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
       std::unique_ptr<HoldingSpaceImage>(Type, const base::FilePath&)>;
 
   // Deserializes from `base::DictionaryValue` to `HoldingSpaceItem`.
-  // This creates a partially initialized item with an empty file system URL.
-  // The item should be finalized using `Finalize()`.
   static std::unique_ptr<HoldingSpaceItem> Deserialize(
       const base::DictionaryValue& dict,
+      FileSystemUrlResolver file_system_url_resolver,
       ImageResolver image_resolver);
 
   // Deserializes `id_` from a serialized `HoldingSpaceItem`.
@@ -80,15 +78,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // Serializes from `HoldingSpaceItem` to `base::DictionaryValue`.
   base::DictionaryValue Serialize() const;
 
-  // Indicates whether the item has been finalized. This will be false for items
-  // created using `Deserialize()` for which `Finalize()` has not yet been
-  // called.
-  // Non-finalized items should not be shown in the holding space UI.
-  bool IsFinalized() const;
-
-  // Used to finalize partially initialized items created by `Deserialize()`.
-  void Finalize(const GURL& file_system_url);
-
   const std::string& id() const { return id_; }
 
   Type type() const { return type_; }

ash/public/cpp/holding_space/holding_space_item_unittest.cc

@@ -32,7 +32,7 @@ using HoldingSpaceItemTest = testing::TestWithParam<HoldingSpaceItem::Type>;
 // Tests round-trip serialization for each holding space item type.
 TEST_P(HoldingSpaceItemTest, Serialization) {
   const base::FilePath file_path("file_path");
-  const GURL file_system_url("filesystem:file_system_url");
+  const GURL file_system_url("file_system_url");
   const gfx::ImageSkia placeholder(gfx::test::CreateImageSkia(10, 10));
 
   const auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
@@ -45,6 +45,9 @@ TEST_P(HoldingSpaceItemTest, Serialization) {
 
   const auto deserialized_holding_space_item = HoldingSpaceItem::Deserialize(
       serialized_holding_space_item,
+      /*file_system_url_resolver=*/
+      base::BindLambdaForTesting(
+          [&](const base::FilePath& file_path) { return file_system_url; }),
       /*image_resolver=*/
       base::BindLambdaForTesting([&](HoldingSpaceItem::Type type,
                                      const base::FilePath& file_path) {
@@ -52,19 +55,13 @@ TEST_P(HoldingSpaceItemTest, Serialization) {
             placeholder, /*async_bitmap_resolver=*/base::DoNothing());
       }));
 
-  EXPECT_FALSE(deserialized_holding_space_item->IsFinalized());
-  EXPECT_TRUE(deserialized_holding_space_item->file_system_url().is_empty());
-
-  deserialized_holding_space_item->Finalize(file_system_url);
-  EXPECT_TRUE(deserialized_holding_space_item->IsFinalized());
   EXPECT_EQ(*deserialized_holding_space_item, *holding_space_item);
 }
 
 // Tests deserialization of id for each holding space item type.
 TEST_P(HoldingSpaceItemTest, DeserializeId) {
   const auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
-      /*type=*/GetParam(), base::FilePath("file_path"),
-      GURL("filesystem:file_system_url"),
+      /*type=*/GetParam(), base::FilePath("file_path"), GURL("file_system_url"),
       std::make_unique<HoldingSpaceImage>(
           /*placeholder=*/gfx::test::CreateImageSkia(10, 10),
           /*async_bitmap_resolver=*/base::DoNothing()));

ash/public/cpp/holding_space/holding_space_model.cc

@@ -43,28 +43,6 @@ void HoldingSpaceModel::RemoveItem(const std::string& id) {
     observer.OnHoldingSpaceItemRemoved(item.get());
 }
 
-void HoldingSpaceModel::FinalizeOrRemoveItem(const std::string& id,
-                                             const GURL& file_system_url) {
-  if (file_system_url.is_empty()) {
-    RemoveItem(id);
-    return;
-  }
-
-  auto item_it = std::find_if(
-      items_.begin(), items_.end(),
-      [&id](const std::unique_ptr<HoldingSpaceItem>& item) -> bool {
-        return id == item->id();
-      });
-  DCHECK(item_it != items_.end());
-
-  HoldingSpaceItem* item = item_it->get();
-  DCHECK(!item->IsFinalized());
-
-  item->Finalize(file_system_url);
-  for (auto& observer : observers_)
-    observer.OnHoldingSpaceItemFinalized(item);
-}
-
 void HoldingSpaceModel::RemoveIf(Predicate predicate) {
   for (int i = items_.size() - 1; i >= 0; --i) {
     const HoldingSpaceItem* item = items_.at(i).get();

ash/public/cpp/holding_space/holding_space_model.h

@@ -12,7 +12,6 @@
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/callback.h"
 #include "base/observer_list.h"
-#include "url/gurl.h"
 
 namespace ash {
 
@@ -41,10 +40,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModel {
   // Removes a single holding space item from the model.
   void RemoveItem(const std::string& id);
 
-  // Finalizes a partially initialized holding space item using the provided
-  // file system URL. The item will be removed if the file system url is empty.
-  void FinalizeOrRemoveItem(const std::string& id, const GURL& file_system_url);
-
   // Removes all holding space items from the model for which the specified
   // `predicate` returns true.
   using Predicate = base::RepeatingCallback<bool(const HoldingSpaceItem*)>;

ash/public/cpp/holding_space/holding_space_model_observer.h

@@ -20,9 +20,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModelObserver
 
   // Called when an item gets removed from the holding space model.
   virtual void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) = 0;
-
-  // Called when a partially initialized holding space `item` gets finalized.
-  virtual void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) = 0;
 };
 
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_test_api.h

@@ -58,12 +58,6 @@ class ASH_EXPORT HoldingSpaceTestApi {
   // If holding space UI is not visible, an empty collection is returned.
   std::vector<views::View*> GetScreenCaptureViews();
 
-  // Returns whether the pinned files container is shown.
-  bool PinnedFilesContainerShown() const;
-
-  // Returns whether the recent files container is shown.
-  bool RecentFilesContainerShown() const;
-
  private:
   HoldingSpaceTray* holding_space_tray_ = nullptr;
 };

ash/system/holding_space/holding_space_item_views_container.cc

@@ -27,10 +27,8 @@ void HoldingSpaceItemViewsContainer::ChildVisibilityChanged(
 void HoldingSpaceItemViewsContainer::OnHoldingSpaceModelAttached(
     HoldingSpaceModel* model) {
   model_observer_.Add(model);
-  for (const auto& item : model->items()) {
-    if (item->IsFinalized())
-      AddHoldingSpaceItemView(item.get());
-  }
+  for (const auto& item : model->items())
+    AddHoldingSpaceItemView(item.get());
 }
 
 void HoldingSpaceItemViewsContainer::OnHoldingSpaceModelDetached(
@@ -41,9 +39,6 @@ void HoldingSpaceItemViewsContainer::OnHoldingSpaceModelDetached(
 
 void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
-  if (!item->IsFinalized())
-    return;
-
   AddHoldingSpaceItemView(item);
 }
 
@@ -52,9 +47,4 @@ void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemRemoved(
   RemoveHoldingSpaceItemView(item);
 }
 
-void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemFinalized(
-    const HoldingSpaceItem* item) {
-  AddHoldingSpaceItemView(item);
-}
-
-}  // namespace ash
+}  // namespace ash
\ No newline at end of file

ash/system/holding_space/holding_space_item_views_container.h

@@ -43,7 +43,6 @@ class HoldingSpaceItemViewsContainer : public views::View,
   // HoldingSpaceModelObserver:
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
  private:
   ScopedObserver<HoldingSpaceController, HoldingSpaceControllerObserver>

ash/system/holding_space/holding_space_test_api.cc

@@ -126,22 +126,4 @@ std::vector<views::View*> HoldingSpaceTestApi::GetScreenCaptureViews() {
   return screen_capture_views;
 }
 
-bool HoldingSpaceTestApi::PinnedFilesContainerShown() const {
-  if (!holding_space_tray_->GetBubbleView())
-    return false;
-
-  views::View* container = holding_space_tray_->GetBubbleView()->GetViewByID(
-      kHoldingSpacePinnedFilesContainerId);
-  return container && container->GetVisible();
-}
-
-bool HoldingSpaceTestApi::RecentFilesContainerShown() const {
-  if (!holding_space_tray_->GetBubbleView())
-    return false;
-
-  views::View* container = holding_space_tray_->GetBubbleView()->GetViewByID(
-      kHoldingSpaceRecentFilesContainerId);
-  return container && container->GetVisible();
-}
-
 }  // namespace ash

ash/system/holding_space/holding_space_tray.cc

@@ -7,7 +7,6 @@
 
 #include "ash/accessibility/accessibility_controller_impl.h"
 #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_metrics.h"
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
 #include "ash/public/cpp/shelf_config.h"
@@ -28,19 +27,6 @@
 
 namespace ash {
 
-namespace {
-
-// Returns whether the holding space model contains any finalized items.
-bool ModelContainsFinalizedItems(HoldingSpaceModel* model) {
-  for (const auto& item : model->items()) {
-    if (item->IsFinalized())
-      return true;
-  }
-  return false;
-}
-
-}  // namespace
-
 HoldingSpaceTray::HoldingSpaceTray(Shelf* shelf) : TrayBackgroundView(shelf) {
   controller_observer_.Add(HoldingSpaceController::Get());
   SetVisible(false);
@@ -161,8 +147,7 @@ void HoldingSpaceTray::UpdateVisibility() {
                 .has_value()
           : false;
 
-  SetVisiblePreferred(!has_ever_pinned_item ||
-                      ModelContainsFinalizedItems(model));
+  SetVisiblePreferred(!model->items().empty() || !has_ever_pinned_item);
 }
 
 base::string16 HoldingSpaceTray::GetAccessibleNameForBubble() {
@@ -195,11 +180,6 @@ void HoldingSpaceTray::OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) {
   UpdateVisibility();
 }
 
-void HoldingSpaceTray::OnHoldingSpaceItemFinalized(
-    const HoldingSpaceItem* item) {
-  UpdateVisibility();
-}
-
 void HoldingSpaceTray::OnWidgetDragWillStart(views::Widget* widget) {
   // The holding space bubble should be closed while dragging holding space
   // items so as not to obstruct drop targets. Post the task to close the bubble

ash/system/holding_space/holding_space_tray.h

@@ -64,7 +64,6 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   // HoldingSpaceModelObserver:
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
   // views::WidgetObserver:
   void OnWidgetDragWillStart(views::Widget* widget) override;

ash/system/holding_space/pinned_files_container.cc

@@ -18,7 +18,6 @@
 #include "ash/system/holding_space/holding_space_item_chips_container.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_item_style.h"
-#include "base/containers/adapters.h"
 #include "base/optional.h"
 #include "components/prefs/pref_service.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -150,25 +149,12 @@ void PinnedFilesContainer::ViewHierarchyChanged(
 void PinnedFilesContainer::AddHoldingSpaceItemView(
     const HoldingSpaceItem* item) {
   DCHECK(!base::Contains(views_by_item_id_, item->id()));
-  DCHECK(item->IsFinalized());
 
-  if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
-    return;
-
-  // Find the position to which the view should be added.
-  size_t index = 0;
-  for (const auto& candidate :
-       base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->id() == item->id())
-      break;
-    if (candidate->IsFinalized() &&
-        candidate->type() == HoldingSpaceItem::Type::kPinnedFile) {
-      ++index;
-    }
+  if (item->type() == HoldingSpaceItem::Type::kPinnedFile) {
+    views_by_item_id_[item->id()] = item_chips_container_->AddChildViewAt(
+        std::make_unique<HoldingSpaceItemChipView>(delegate_, item),
+        /*index=*/0);
   }
-
-  views_by_item_id_[item->id()] = item_chips_container_->AddChildViewAt(
-      std::make_unique<HoldingSpaceItemChipView>(delegate_, item), index);
 }
 
 void PinnedFilesContainer::RemoveAllHoldingSpaceItemViews() {

ash/system/holding_space/recent_files_container.cc

@@ -148,26 +148,10 @@ void RecentFilesContainer::ViewHierarchyChanged(
 
 void RecentFilesContainer::AddHoldingSpaceItemView(
     const HoldingSpaceItem* item) {
-  DCHECK(item->IsFinalized());
-
-  if (item->type() != HoldingSpaceItem::Type::kScreenshot &&
-      item->type() != HoldingSpaceItem::Type::kDownload) {
-    return;
-  }
-
-  size_t index = 0;
-  for (const auto& candidate :
-       base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->id() == item->id())
-      break;
-    if (candidate->IsFinalized() && candidate->type() == item->type())
-      ++index;
-  }
-
   if (item->type() == HoldingSpaceItem::Type::kScreenshot)
-    AddHoldingSpaceScreenCaptureView(item, index);
+    AddHoldingSpaceScreenCaptureView(item);
   else if (item->type() == HoldingSpaceItem::Type::kDownload)
-    AddHoldingSpaceDownloadView(item, index);
+    AddHoldingSpaceDownloadView(item);
 }
 
 void RecentFilesContainer::RemoveAllHoldingSpaceItemViews() {
@@ -185,14 +169,10 @@ void RecentFilesContainer::RemoveHoldingSpaceItemView(
 }
 
 void RecentFilesContainer::AddHoldingSpaceScreenCaptureView(
-    const HoldingSpaceItem* item,
-    size_t index) {
+    const HoldingSpaceItem* item) {
   DCHECK_EQ(item->type(), HoldingSpaceItem::Type::kScreenshot);
   DCHECK(!base::Contains(views_by_item_id_, item->id()));
 
-  if (index >= kMaxScreenCaptures)
-    return;
-
   // Remove the last screen capture view if we are already at max capacity.
   if (screen_captures_container_->children().size() == kMaxScreenCaptures) {
     std::unique_ptr<views::View> view =
@@ -205,7 +185,7 @@ void RecentFilesContainer::AddHoldingSpaceScreenCaptureView(
   // Add the screen capture view to the front in order to sort by recency.
   views_by_item_id_[item->id()] = screen_captures_container_->AddChildViewAt(
       std::make_unique<HoldingSpaceItemScreenCaptureView>(delegate_, item),
-      index);
+      /*index=*/0);
 }
 
 void RecentFilesContainer::RemoveHoldingSpaceScreenCaptureView(
@@ -228,8 +208,7 @@ void RecentFilesContainer::RemoveHoldingSpaceScreenCaptureView(
   // the back in order to maintain sort by recency.
   for (const auto& candidate :
        base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->IsFinalized() &&
-        candidate->type() == HoldingSpaceItem::Type::kScreenshot &&
+    if (candidate->type() == HoldingSpaceItem::Type::kScreenshot &&
         !base::Contains(views_by_item_id_, candidate->id())) {
       views_by_item_id_[candidate->id()] =
           screen_captures_container_->AddChildView(
@@ -241,14 +220,10 @@ void RecentFilesContainer::RemoveHoldingSpaceScreenCaptureView(
 }
 
 void RecentFilesContainer::AddHoldingSpaceDownloadView(
-    const HoldingSpaceItem* item,
-    size_t index) {
+    const HoldingSpaceItem* item) {
   DCHECK_EQ(item->type(), HoldingSpaceItem::Type::kDownload);
   DCHECK(!base::Contains(views_by_item_id_, item->id()));
 
-  if (index >= kMaxDownloads)
-    return;
-
   // Remove the last download view if we are already at max capacity.
   if (downloads_container_->children().size() == kMaxDownloads) {
     std::unique_ptr<views::View> view = downloads_container_->RemoveChildViewT(
@@ -259,7 +234,7 @@ void RecentFilesContainer::AddHoldingSpaceDownloadView(
 
   // Add the download view to the front in order to sort by recency.
   views_by_item_id_[item->id()] = downloads_container_->AddChildViewAt(
-      std::make_unique<HoldingSpaceItemChipView>(delegate_, item), index);
+      std::make_unique<HoldingSpaceItemChipView>(delegate_, item), /*index=*/0);
 }
 
 void RecentFilesContainer::RemoveHoldingSpaceDownloadView(
@@ -282,8 +257,7 @@ void RecentFilesContainer::RemoveHoldingSpaceDownloadView(
   // back in order to maintain sort by recency.
   for (const auto& candidate :
        base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->IsFinalized() &&
-        candidate->type() == HoldingSpaceItem::Type::kDownload &&
+    if (candidate->type() == HoldingSpaceItem::Type::kDownload &&
         !base::Contains(views_by_item_id_, candidate->id())) {
       views_by_item_id_[candidate->id()] = downloads_container_->AddChildView(
           std::make_unique<HoldingSpaceItemChipView>(delegate_,

ash/system/holding_space/recent_files_container.h

@@ -34,10 +34,9 @@ class RecentFilesContainer : public HoldingSpaceItemViewsContainer {
   void RemoveHoldingSpaceItemView(const HoldingSpaceItem* item) override;
 
  private:
-  void AddHoldingSpaceScreenCaptureView(const HoldingSpaceItem* item,
-                                        size_t index);
+  void AddHoldingSpaceScreenCaptureView(const HoldingSpaceItem* item);
   void RemoveHoldingSpaceScreenCaptureView(const HoldingSpaceItem* item);
-  void AddHoldingSpaceDownloadView(const HoldingSpaceItem* item, size_t index);
+  void AddHoldingSpaceDownloadView(const HoldingSpaceItem* item);
   void RemoveHoldingSpaceDownloadView(const HoldingSpaceItem* item);
   void OnScreenCapturesContainerViewHierarchyChanged(
       const views::ViewHierarchyChangedDetails& details);

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

@@ -32,6 +32,24 @@ namespace {
 
 // Helpers ---------------------------------------------------------------------
 
+// Adds and returns a holding space item of the specified `type` backed by the
+// file at the specified `file_path`.
+HoldingSpaceItem* AddItem(Profile* profile,
+                          HoldingSpaceItem::Type type,
+                          const base::FilePath& file_path) {
+  auto item = HoldingSpaceItem::CreateFileBackedItem(
+      type, file_path,
+      holding_space_util::ResolveFileSystemUrl(profile, file_path),
+      /*image=*/
+      std::make_unique<HoldingSpaceImage>(
+          /*placeholder=*/gfx::ImageSkia(),
+          /*async_bitmap_resolver=*/base::DoNothing()));
+
+  auto* item_ptr = item.get();
+  HoldingSpaceController::Get()->model()->AddItem(std::move(item));
+  return item_ptr;
+}
+
 // Returns the path of the downloads mount point for the given `profile`.
 base::FilePath GetDownloadsPath(Profile* profile) {
   base::FilePath result;
@@ -170,23 +188,6 @@ HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddScreenshotFile() {
                  /*file_path=*/CreateImageFile(GetProfile()));
 }
 
-HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddItem(
-    Profile* profile,
-    HoldingSpaceItem::Type type,
-    const base::FilePath& file_path) {
-  auto item = HoldingSpaceItem::CreateFileBackedItem(
-      type, file_path,
-      holding_space_util::ResolveFileSystemUrl(profile, file_path),
-      /*image=*/
-      std::make_unique<HoldingSpaceImage>(
-          /*placeholder=*/gfx::ImageSkia(),
-          /*async_bitmap_resolver=*/base::DoNothing()));
-
-  auto* item_ptr = item.get();
-  HoldingSpaceController::Get()->model()->AddItem(std::move(item));
-  return item_ptr;
-}
-
 std::vector<views::View*> HoldingSpaceBrowserTestBase::GetDownloadChips() {
   return test_api_->GetDownloadChips();
 }

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

@@ -8,7 +8,6 @@
 #include <memory>
 #include <vector>
 
-#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/test/base/in_process_browser_test.h"
 
@@ -64,12 +63,6 @@ class HoldingSpaceBrowserTestBase : public InProcessBrowserTest {
   // Adds and returns an arbitrary screenshot file to the holding space.
   HoldingSpaceItem* AddScreenshotFile();
 
-  // Adds and returns a holding space item of the specified `type` backed by the
-  // file at the specified `file_path`.
-  HoldingSpaceItem* AddItem(Profile* profile,
-                            HoldingSpaceItem::Type type,
-                            const base::FilePath& file_path);
-
   // Returns the collection of download chips in holding space UI.
   // If holding space UI is not visible, an empty collection is returned.
   std::vector<views::View*> GetDownloadChips();

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

@@ -17,14 +17,12 @@
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/unguessable_token.h"
-#include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
-#include "storage/browser/file_system/external_mount_points.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace ash {
@@ -38,30 +36,11 @@ constexpr char kTextFilePath[] = "text.txt";
 
 // Helpers ---------------------------------------------------------------------
 
-// Copies the file for the `relative_path` in the test data directory to
-// downloads directory, and returns the path to the copy.
-base::FilePath TestFile(Profile* profile, const std::string& relative_path) {
+// Returns the file for the `relative_path` in the test data directory.
+base::FilePath TestFile(const std::string& relative_path) {
   base::FilePath source_root;
   EXPECT_TRUE(base::PathService::Get(base::DIR_SOURCE_ROOT, &source_root));
-  const base::FilePath source_file =
-      source_root.AppendASCII(kTestDataDir).AppendASCII(relative_path);
-
-  base::FilePath target_dir;
-  if (!storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
-          file_manager::util::GetDownloadsMountPointName(profile),
-          &target_dir)) {
-    ADD_FAILURE() << "Failed to get downloads mount point";
-    return base::FilePath();
-  }
-
-  const base::FilePath target_path = target_dir.Append(source_file.BaseName());
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  if (!base::CopyFile(source_file, target_path)) {
-    ADD_FAILURE() << "Failed to create file.";
-    return base::FilePath();
-  }
-
-  return target_path;
+  return source_root.AppendASCII(kTestDataDir).AppendASCII(relative_path);
 }
 
 }  // namespace
@@ -81,9 +60,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, CopyImageToClipboard) {
 
   {
     // Create a holding space item backed by a non-image file.
-    auto* holding_space_item =
-        AddItem(GetProfile(), HoldingSpaceItem::Type::kDownload,
-                TestFile(GetProfile(), kTextFilePath));
+    auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+        HoldingSpaceItem::Type::kDownload, TestFile(kTextFilePath), GURL(),
+        std::make_unique<HoldingSpaceImage>(
+            /*placeholder=*/gfx::ImageSkia(),
+            /*async_bitmap_resolver=*/base::DoNothing()));
 
     // We expect `HoldingSpaceClient::CopyImageToClipboard()` to fail when the
     // backing file for `holding_space_item` is not an image file.
@@ -99,9 +80,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, CopyImageToClipboard) {
 
   {
     // Create a holding space item backed by an image file.
-    auto* holding_space_item =
-        AddItem(GetProfile(), HoldingSpaceItem::Type::kDownload,
-                TestFile(GetProfile(), kImageFilePath));
+    auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+        HoldingSpaceItem::Type::kDownload, TestFile(kImageFilePath), GURL(),
+        std::make_unique<HoldingSpaceImage>(
+            /*placeholder=*/gfx::ImageSkia(),
+            /*async_bitmap_resolver=*/base::DoNothing()));
 
     // We expect `HoldingSpaceClient::CopyImageToClipboard()` to succeed when
     // the backing file for `holding_space_item` is an image file.
@@ -146,8 +129,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItems) {
   {
     // Create a holding space item backed by a non-existing file.
     auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
-        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"),
-        GURL("filesystem:fake"),
+        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"), GURL(),
         std::make_unique<HoldingSpaceImage>(
             /*placeholder=*/gfx::ImageSkia(),
             /*async_bitmap_resolver=*/base::DoNothing()));
@@ -199,8 +181,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, MAYBE_ShowItemInFolder) {
   {
     // Create a holding space item backed by a non-existing file.
     auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
-        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"),
-        GURL("filesystem:fake"),
+        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"), GURL(),
         std::make_unique<HoldingSpaceImage>(
             /*placeholder=*/gfx::ImageSkia(),
             /*async_bitmap_resolver=*/base::DoNothing()));

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

@@ -95,9 +95,6 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (!item->IsFinalized())
-    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());
@@ -110,22 +107,17 @@ void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemRemoved(
   // Since we were watching the directory containing `item`'s backing file and
   // not the backing file itself, we only need to remove the associated watch if
   // there are no other holding space items backed by the same directory.
-  const bool remove_watch = std::none_of(
-      model()->items().begin(), model()->items().end(),
-      [removed_item = item](const auto& item) {
-        return item->IsFinalized() && item->file_path().DirName() ==
-                                          removed_item->file_path().DirName();
-      });
+  const bool remove_watch =
+      std::none_of(model()->items().begin(), model()->items().end(),
+                   [removed_item = item](const auto& item) {
+                     return item->file_path().DirName() ==
+                            removed_item->file_path().DirName();
+                   });
 
   if (remove_watch)
     RemoveWatch(item->file_path().DirName());
 }
 
-void HoldingSpaceFileSystemDelegate::OnHoldingSpaceItemFinalized(
-    const HoldingSpaceItem* item) {
-  AddWatch(item->file_path().DirName());
-}
-
 void HoldingSpaceFileSystemDelegate::OnFilePathChanged(
     const base::FilePath& file_path,
     bool error) {

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

@@ -41,7 +41,6 @@ class HoldingSpaceFileSystemDelegate : public HoldingSpaceKeyedServiceDelegate {
   void Init() override;
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
   // Invoked when the specified `file_path` has changed.
   void OnFilePathChanged(const base::FilePath& file_path, bool error);

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

@@ -18,7 +18,6 @@
 #include "base/unguessable_token.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/extensions/component_loader.h"
-#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -43,10 +42,6 @@ class MockHoldingSpaceModelObserver : public HoldingSpaceModelObserver {
               OnHoldingSpaceItemRemoved,
               (const HoldingSpaceItem* item),
               (override));
-  MOCK_METHOD(void,
-              OnHoldingSpaceItemFinalized,
-              (const HoldingSpaceItem* item),
-              (override));
 };
 
 // Helpers ---------------------------------------------------------------------
@@ -108,10 +103,8 @@ const HoldingSpaceItem* AddHoldingSpaceItem(Profile* profile) {
         run_loop.Quit();
       });
 
-  base::FilePath item_path = CreateTextFile(profile);
   holding_space_model->AddItem(HoldingSpaceItem::CreateFileBackedItem(
-      HoldingSpaceItem::Type::kDownload, item_path,
-      holding_space_util::ResolveFileSystemUrl(profile, item_path),
+      HoldingSpaceItem::Type::kDownload, CreateTextFile(profile), GURL(),
       std::make_unique<HoldingSpaceImage>(
           /*placeholder=*/gfx::ImageSkia(),
           /*async_bitmap_resolver=*/base::DoNothing())));

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

@@ -42,9 +42,6 @@ void HoldingSpaceKeyedServiceDelegate::OnHoldingSpaceItemAdded(
 void HoldingSpaceKeyedServiceDelegate::OnHoldingSpaceItemRemoved(
     const HoldingSpaceItem* item) {}
 
-void HoldingSpaceKeyedServiceDelegate::OnHoldingSpaceItemFinalized(
-    const HoldingSpaceItem* item) {}
-
 void HoldingSpaceKeyedServiceDelegate::OnPersistenceRestored() {}
 
 }  // namespace ash

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

@@ -49,7 +49,6 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
   // HoldingSpaceModelObserver:
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
   // Invoked when holding space persistence has been restored.
   virtual void OnPersistenceRestored();

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

@@ -548,9 +548,8 @@ TEST_F(HoldingSpaceKeyedServiceTest, RestorePersistentStorage) {
           auto stale_holding_space_item =
               HoldingSpaceItem::CreateFileBackedItem(
                   type,
-                  downloads_mount.GetRootPath().AppendASCII(
-                      base::UnguessableToken::Create().ToString()),
-                  GURL("filesystem:fake_file_system_url"),
+                  base::FilePath(base::UnguessableToken::Create().ToString()),
+                  GURL(),
                   std::make_unique<HoldingSpaceImage>(
                       /*placeholder=*/gfx::ImageSkia(),
                       /*async_bitmap_resolver=*/base::DoNothing()));
@@ -644,13 +643,11 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderFilesFromPersistance) {
                 std::move(fresh_holding_space_item));
           }
 
-          const base::FilePath stale_item_file =
-              downloads_mount.GetRootPath().AppendASCII(
-                  base::UnguessableToken::Create().ToString());
           auto stale_holding_space_item =
               HoldingSpaceItem::CreateFileBackedItem(
-                  type, stale_item_file,
-                  GetFileSystemUrl(GetProfile(), stale_item_file),
+                  type,
+                  base::FilePath(base::UnguessableToken::Create().ToString()),
+                  GURL(),
                   std::make_unique<HoldingSpaceImage>(
                       /*placeholder=*/gfx::ImageSkia(),
                       /*async_bitmap_resolver=*/base::DoNothing()));

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

@@ -86,26 +86,18 @@ void HoldingSpacePersistenceDelegate::RestoreModelFromPersistence() {
 
   for (const auto& persisted_holding_space_item :
        persisted_holding_space_items->GetList()) {
-    std::unique_ptr<HoldingSpaceItem> holding_space_item =
-        HoldingSpaceItem::Deserialize(
-            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_items.push_back(HoldingSpaceItem::Deserialize(
+        base::Value::AsDictionaryValue(persisted_holding_space_item),
+        base::BindOnce(&holding_space_util::ResolveFileSystemUrl,
+                       base::Unretained(profile())),
+        base::BindOnce(&holding_space_util::ResolveImage,
+                       base::Unretained(thumbnail_loader_))));
     holding_space_util::ValidityRequirement requirements;
+    HoldingSpaceItem* holding_space_item = holding_space_items.back().get();
     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));
   }
 
   holding_space_util::PartitionFilePathsByValidity(
@@ -123,10 +115,8 @@ void HoldingSpacePersistenceDelegate::RestoreModelByValidity(
 
   // 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())) {
+    if (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.

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

@@ -84,10 +84,6 @@ class MockHoldingSpaceModelObserver : public HoldingSpaceModelObserver {
               OnHoldingSpaceItemRemoved,
               (const HoldingSpaceItem* item),
               (override));
-  MOCK_METHOD(void,
-              OnHoldingSpaceItemFinalized,
-              (const HoldingSpaceItem* item),
-              (override));
 };
 
 // DropTargetView --------------------------------------------------------------