Decouple type and file path from holding space item id.

Previously holding space item id was a concatenation of type and file
path. Moving forward, holding space items will be retained across moves
and renames of their backing files, so a decoupled unique ID is desired
which can be stable throughout the lifetime of the holding space item.

This change is backwards compatible w.r.t persistence.

Bug: 1136173
Change-Id: If1d3f218fe61dcc96f9336f8be6a0b197ab3c0dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583155Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#835496}

ash/public/cpp/holding_space/holding_space_item.cc

@@ -7,6 +7,7 @@
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/strcat.h"
+#include "base/unguessable_token.h"
 #include "base/util/values/values_util.h"
 
 namespace ash {
@@ -27,21 +28,6 @@ constexpr char kIdPath[] = "id";
 constexpr char kTypePath[] = "type";
 constexpr char kVersionPath[] = "version";
 
-std::string TypeToString(HoldingSpaceItem::Type type) {
-  switch (type) {
-    case HoldingSpaceItem::Type::kPinnedFile:
-      return "pinned_file";
-    case HoldingSpaceItem::Type::kDownload:
-      return "download";
-    case HoldingSpaceItem::Type::kScreenshot:
-      return "screenshot";
-    case HoldingSpaceItem::Type::kNearbyShare:
-      return "nearby_share";
-    case HoldingSpaceItem::Type::kScreenRecording:
-      return "screen_recording";
-  }
-}
-
 }  // namespace
 
 HoldingSpaceItem::~HoldingSpaceItem() = default;
@@ -52,13 +38,6 @@ bool HoldingSpaceItem::operator==(const HoldingSpaceItem& rhs) const {
          *image_ == *rhs.image_;
 }
 
-// static
-std::string HoldingSpaceItem::GetFileBackedItemId(
-    Type type,
-    const base::FilePath& file_path) {
-  return base::StrCat({TypeToString(type), ":", file_path.value()});
-}
-
 // static
 std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
     Type type,
@@ -69,8 +48,9 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
 
   // 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,
-      file_path.BaseName().LossyDisplayName(), std::move(image)));
+      type, /*id=*/base::UnguessableToken::Create().ToString(), file_path,
+      file_system_url, file_path.BaseName().LossyDisplayName(),
+      std::move(image)));
 }
 
 // static
@@ -133,6 +113,16 @@ base::DictionaryValue HoldingSpaceItem::Serialize() const {
   return dict;
 }
 
+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;
+}
+
 HoldingSpaceItem::HoldingSpaceItem(Type type,
                                    const std::string& id,
                                    const base::FilePath& file_path,
@@ -146,14 +136,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

@@ -45,11 +45,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
 
   bool operator==(const HoldingSpaceItem& rhs) const;
 
-  // Generates an item ID for a holding space item backed by a file, based on
-  // the file's file system URL.
-  static std::string GetFileBackedItemId(Type type,
-                                         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(
@@ -58,9 +53,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
       const GURL& file_system_url,
       std::unique_ptr<HoldingSpaceImage> image);
 
-  // Returns a file system URL for a given file path.
-  using FileSystemUrlResolver = base::OnceCallback<GURL(const base::FilePath&)>;
-
   // Returns an image for a given type and file path.
   using ImageResolver = base::OnceCallback<
       std::unique_ptr<HoldingSpaceImage>(Type, const base::FilePath&)>;

ash/public/cpp/holding_space/holding_space_model.cc

@@ -6,13 +6,13 @@
 
 #include <algorithm>
 
-#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model_observer.h"
 #include "base/check.h"
 
 namespace ash {
 
 HoldingSpaceModel::HoldingSpaceModel() = default;
+
 HoldingSpaceModel::~HoldingSpaceModel() = default;
 
 void HoldingSpaceModel::AddItem(std::unique_ptr<HoldingSpaceItem> item) {
@@ -98,6 +98,25 @@ const HoldingSpaceItem* HoldingSpaceModel::GetItem(
   return item_it->get();
 }
 
+const HoldingSpaceItem* HoldingSpaceModel::GetItem(
+    HoldingSpaceItem::Type type,
+    const base::FilePath& file_path) const {
+  auto item_it = std::find_if(
+      items_.begin(), items_.end(),
+      [&type, &file_path](const std::unique_ptr<HoldingSpaceItem>& item) {
+        return item->type() == type && item->file_path() == file_path;
+      });
+
+  if (item_it == items_.end())
+    return nullptr;
+  return item_it->get();
+}
+
+bool HoldingSpaceModel::ContainsItem(HoldingSpaceItem::Type type,
+                                     const base::FilePath& file_path) const {
+  return GetItem(type, file_path) != nullptr;
+}
+
 void HoldingSpaceModel::AddObserver(HoldingSpaceModelObserver* observer) {
   observers_.AddObserver(observer);
 }

ash/public/cpp/holding_space/holding_space_model.h

@@ -10,13 +10,17 @@
 #include <vector>
 
 #include "ash/public/cpp/ash_public_export.h"
+#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/callback.h"
 #include "base/observer_list.h"
 #include "url/gurl.h"
 
+namespace base {
+class FilePath;
+}  // namespace base
+
 namespace ash {
 
-class HoldingSpaceItem;
 class HoldingSpaceModelObserver;
 
 // The data model for the temporary holding space UI. It contains the list of
@@ -57,6 +61,17 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModel {
   // Returns nullptr if the item does not exist in the model.
   const HoldingSpaceItem* GetItem(const std::string& id) const;
 
+  // Gets a single holding space item with the specified `type` backed by the
+  // specified `file_path`. Returns `nullptr` if the item does not exist in the
+  // model.
+  const HoldingSpaceItem* GetItem(HoldingSpaceItem::Type type,
+                                  const base::FilePath& file_path) const;
+
+  // Returns whether or not there exists a holding space item of the specified
+  // `type` backed by the specified `file_path`.
+  bool ContainsItem(HoldingSpaceItem::Type type,
+                    const base::FilePath& file_path) const;
+
   const ItemList& items() const { return items_; }
 
   void AddObserver(HoldingSpaceModelObserver* observer);

ash/system/holding_space/holding_space_item_view.cc

@@ -348,9 +348,9 @@ void HoldingSpaceItemView::OnPaintSelect(gfx::Canvas* canvas, gfx::Size size) {
 }
 
 void HoldingSpaceItemView::OnPinPressed() {
-  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
-      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
-                                            item()->file_path()));
+  const bool is_item_pinned =
+      HoldingSpaceController::Get()->model()->ContainsItem(
+          HoldingSpaceItem::Type::kPinnedFile, item()->file_path());
 
   // Unpinning `item()` may result in the destruction of this view.
   auto weak_ptr = weak_factory_.GetWeakPtr();
@@ -370,9 +370,9 @@ void HoldingSpaceItemView::UpdatePin() {
     return;
   }
 
-  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
-      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
-                                            item()->file_path()));
+  const bool is_item_pinned =
+      HoldingSpaceController::Get()->model()->ContainsItem(
+          HoldingSpaceItem::Type::kPinnedFile, item()->file_path());
 
   pin_->SetToggled(!is_item_pinned);
   pin_->SetVisible(true);

ash/system/holding_space/holding_space_item_view_delegate.cc

@@ -364,10 +364,8 @@ ui::SimpleMenuModel* HoldingSpaceItemViewDelegate::BuildMenuModel() {
 
   const bool is_any_unpinned = std::any_of(
       selection.begin(), selection.end(), [](const HoldingSpaceItemView* view) {
-        return !HoldingSpaceController::Get()->model()->GetItem(
-            HoldingSpaceItem::GetFileBackedItemId(
-                HoldingSpaceItem::Type::kPinnedFile,
-                view->item()->file_path()));
+        return !HoldingSpaceController::Get()->model()->ContainsItem(
+            HoldingSpaceItem::Type::kPinnedFile, view->item()->file_path());
       });
 
   if (is_any_unpinned) {

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

@@ -117,8 +117,8 @@ void HoldingSpaceKeyedService::RegisterProfilePrefs(
 
 void HoldingSpaceKeyedService::AddPinnedFile(
     const storage::FileSystemURL& file_system_url) {
-  if (holding_space_model_.GetItem(HoldingSpaceItem::GetFileBackedItemId(
-          HoldingSpaceItem::Type::kPinnedFile, file_system_url.path()))) {
+  if (holding_space_model_.ContainsItem(HoldingSpaceItem::Type::kPinnedFile,
+                                        file_system_url.path())) {
     return;
   }
 
@@ -153,9 +153,8 @@ void HoldingSpaceKeyedService::AddPinnedFile(
 
 void HoldingSpaceKeyedService::RemovePinnedFile(
     const storage::FileSystemURL& file_system_url) {
-  const HoldingSpaceItem* holding_space_item =
-      holding_space_model_.GetItem(HoldingSpaceItem::GetFileBackedItemId(
-          HoldingSpaceItem::Type::kPinnedFile, file_system_url.path()));
+  const HoldingSpaceItem* holding_space_item = holding_space_model_.GetItem(
+      HoldingSpaceItem::Type::kPinnedFile, file_system_url.path());
   if (!holding_space_item)
     return;
 
@@ -176,8 +175,8 @@ void HoldingSpaceKeyedService::RemovePinnedFile(
 
 bool HoldingSpaceKeyedService::ContainsPinnedFile(
     const storage::FileSystemURL& file_system_url) const {
-  return holding_space_model_.GetItem(HoldingSpaceItem::GetFileBackedItemId(
-      HoldingSpaceItem::Type::kPinnedFile, file_system_url.path()));
+  return holding_space_model_.ContainsItem(HoldingSpaceItem::Type::kPinnedFile,
+                                           file_system_url.path());
 }
 
 std::vector<GURL> HoldingSpaceKeyedService::GetPinnedFiles() const {
@@ -205,9 +204,8 @@ void HoldingSpaceKeyedService::AddScreenshot(
 
 void HoldingSpaceKeyedService::AddDownload(
     const base::FilePath& download_file) {
-  const bool already_exists =
-      holding_space_model_.GetItem(HoldingSpaceItem::GetFileBackedItemId(
-          HoldingSpaceItem::Type::kDownload, download_file));
+  const bool already_exists = holding_space_model_.ContainsItem(
+      HoldingSpaceItem::Type::kDownload, download_file);
   if (already_exists)
     return;
 
@@ -225,9 +223,8 @@ void HoldingSpaceKeyedService::AddDownload(
 
 void HoldingSpaceKeyedService::AddNearbyShare(
     const base::FilePath& nearby_share_path) {
-  const bool already_exists =
-      holding_space_model_.GetItem(HoldingSpaceItem::GetFileBackedItemId(
-          HoldingSpaceItem::Type::kNearbyShare, nearby_share_path));
+  const bool already_exists = holding_space_model_.ContainsItem(
+      HoldingSpaceItem::Type::kNearbyShare, nearby_share_path);
   if (already_exists)
     return;
 

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

@@ -93,83 +93,125 @@ base::FilePath CreateTextFile(
   return path;
 }
 
-// Waits for a holding space item with the provided id to be added to the
-// holding space model.
-// Returns immediately if the item already exists.
-void WaitForItemAddition(const std::string& item_id) {
-  auto* holding_space_model = ash::HoldingSpaceController::Get()->model();
-  if (holding_space_model->GetItem(item_id))
+// Waits for a holding space item matching the provided `predicate` to be added
+// to the holding space model. Returns immediately if the item already exists.
+void WaitForItemAddition(
+    base::RepeatingCallback<bool(const HoldingSpaceItem*)> predicate) {
+  auto* model = ash::HoldingSpaceController::Get()->model();
+  if (std::any_of(model->items().begin(), model->items().end(),
+                  [&predicate](const auto& item) {
+                    return predicate.Run(item.get());
+                  })) {
     return;
+  }
 
   testing::NiceMock<MockHoldingSpaceModelObserver> mock;
   base::ScopedObservation<HoldingSpaceModel, HoldingSpaceModelObserver>
       observer{&mock};
-  observer.Observe(holding_space_model);
+  observer.Observe(model);
 
   base::RunLoop run_loop;
   ON_CALL(mock, OnHoldingSpaceItemAdded)
       .WillByDefault([&](const HoldingSpaceItem* item) {
-        if (item->id() == item_id)
+        if (predicate.Run(item))
           run_loop.Quit();
       });
   run_loop.Run();
 }
 
-// Waits for a holding space item with the provided id to be removed from the
-// holding space model.
-// Returns immediately if the model does not contain such item.
-void WaitForItemRemoval(const std::string& item_id) {
-  auto* holding_space_model = ash::HoldingSpaceController::Get()->model();
-  if (!holding_space_model->GetItem(item_id))
+// Waits for a holding space item with the provided `item_id` to be added to the
+// holding space model. Returns immediately if the item already exists.
+void WaitForItemAddition(const std::string& item_id) {
+  WaitForItemAddition(
+      base::BindLambdaForTesting([&item_id](const HoldingSpaceItem* item) {
+        return item->id() == item_id;
+      }));
+}
+
+// Waits for a holding space item matching the provided `predicate` to be
+// removed from the holding space model. Returns immediately if the model does
+// not contain such an item.
+void WaitForItemRemoval(
+    base::RepeatingCallback<bool(const HoldingSpaceItem*)> predicate) {
+  auto* model = ash::HoldingSpaceController::Get()->model();
+  if (std::none_of(model->items().begin(), model->items().end(),
+                   [&predicate](const auto& item) {
+                     return predicate.Run(item.get());
+                   })) {
     return;
+  }
 
   testing::NiceMock<MockHoldingSpaceModelObserver> mock;
   base::ScopedObservation<HoldingSpaceModel, HoldingSpaceModelObserver>
       observer{&mock};
-  observer.Observe(holding_space_model);
+  observer.Observe(model);
 
   base::RunLoop run_loop;
   ON_CALL(mock, OnHoldingSpaceItemRemoved)
       .WillByDefault([&](const HoldingSpaceItem* item) {
-        if (item->id() == item_id)
+        if (predicate.Run(item))
           run_loop.Quit();
       });
   run_loop.Run();
 }
 
-// Waits for a holding space item with the provided id to be added to the
-// holding space model and finalized.
-// Returns immediately if the item already exists and is finalized.
-void WaitForItemFinalization(const std::string& item_id) {
-  auto* holding_space_model = ash::HoldingSpaceController::Get()->model();
-  if (!holding_space_model->GetItem(item_id))
-    WaitForItemAddition(item_id);
+// Waits for a holding space item with the provided `item_id` to be removed from
+// the holding space model. Returns immediately if the model does not contain
+// such an item.
+void WaitForItemRemoval(const std::string& item_id) {
+  WaitForItemRemoval(
+      base::BindLambdaForTesting([&item_id](const HoldingSpaceItem* item) {
+        return item->id() == item_id;
+      }));
+}
+
+// Waits for a holding space item matching the provided `predicate` to be added
+// to the holding space model and finalized. Returns immediately if the item
+// already exists and is finalized.
+void WaitForItemFinalization(
+    base::RepeatingCallback<bool(const HoldingSpaceItem*)> predicate) {
+  WaitForItemAddition(predicate);
+
+  auto* model = ash::HoldingSpaceController::Get()->model();
+  auto item_it = std::find_if(
+      model->items().begin(), model->items().end(),
+      [&predicate](const auto& item) { return predicate.Run(item.get()); });
 
-  const HoldingSpaceItem* item = holding_space_model->GetItem(item_id);
-  if (item->IsFinalized())
+  DCHECK(item_it != model->items().end());
+  if (item_it->get()->IsFinalized())
     return;
 
   testing::NiceMock<MockHoldingSpaceModelObserver> mock;
   base::ScopedObservation<HoldingSpaceModel, HoldingSpaceModelObserver>
       observer{&mock};
-  observer.Observe(holding_space_model);
+  observer.Observe(model);
 
   base::RunLoop run_loop;
   ON_CALL(mock, OnHoldingSpaceItemFinalized)
       .WillByDefault([&](const HoldingSpaceItem* item) {
-        if (item->id() == item_id)
+        if (item == item_it->get())
           run_loop.Quit();
       });
   ON_CALL(mock, OnHoldingSpaceItemRemoved)
       .WillByDefault([&](const HoldingSpaceItem* item) {
-        if (item->id() != item_id)
+        if (item != item_it->get())
           return;
-        ADD_FAILURE() << "Item unexpectedly removed " << item_id;
+        ADD_FAILURE() << "Item unexpectedly removed: " << item->id();
         run_loop.Quit();
       });
   run_loop.Run();
 }
 
+// Waits for a holding space item with the provided `item_id` to be added to the
+// holding space model and finalized. Returns immediately if the item already
+// exists and is finalized.
+void WaitForItemFinalization(const std::string& item_id) {
+  WaitForItemFinalization(
+      base::BindLambdaForTesting([&item_id](const HoldingSpaceItem* item) {
+        return item->id() == item_id;
+      }));
+}
+
 // Adds a holding space item backed by a txt file at `item_path`.
 // Returns a pointer to the added item.
 const HoldingSpaceItem* AddHoldingSpaceItem(Profile* profile,
@@ -426,8 +468,11 @@ IN_PROC_BROWSER_TEST_P(HoldingSpaceKeyedServiceBrowserTest,
 
 IN_PROC_BROWSER_TEST_P(HoldingSpaceKeyedServiceBrowserTest,
                        RestoreItemsOnRestart) {
-  WaitForItemFinalization(HoldingSpaceItem::GetFileBackedItemId(
-      HoldingSpaceItem::Type::kDownload, GetPredefinedTestFile(0)));
+  WaitForItemFinalization(
+      base::BindLambdaForTesting([this](const HoldingSpaceItem* item) {
+        return item->type() == HoldingSpaceItem::Type::kDownload &&
+               item->file_path() == GetPredefinedTestFile(0);
+      }));
 }
 
 }  // namespace ash