Persist/restore holding space items.

Holding space items will be persisted to profile prefs. Note that we
don't persist Downloads as those have a separate persistence storage.
Also note that we don't yet persist images. Ideally, we shouldn't need
to.

Known deficiences:
- Doesn't yet restore downloads
- Doesn't yet restore images

Bug: 1119496
Change-Id: I74f929c8bb802a15e6771dc58ea68ec20380fe26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2367701Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#801136}

ash/public/cpp/BUILD.gn

@@ -308,6 +308,7 @@ component("cpp") {
 
   deps = [
     "//ash/public/cpp/vector_icons",
+    "//base/util/values:values_util",
     "//chromeos/constants",
     "//chromeos/dbus/power:power_manager_proto",
     "//chromeos/services/assistant/public/cpp",
@@ -355,6 +356,7 @@ source_set("unit_tests") {
     "android_intent_helper_unittest.cc",
     "app_list/app_list_config_provider_unittest.cc",
     "default_scale_factor_retriever_unittest.cc",
+    "holding_space/holding_space_item_unittest.cc",
     "metrics_util_unittest.cc",
     "pagination/pagination_model_unittest.cc",
     "power_utils_unittest.cc",

ash/public/cpp/holding_space/holding_space_item.cc

@@ -7,11 +7,26 @@
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/strcat.h"
+#include "base/util/values/values_util.h"
 
 namespace ash {
 
 namespace {
 
+// Used to indicate which version of serialization is being used. When
+// intentionally breaking backwards compatibility, increment this value and
+// perform any necessary conversions in `Deserialize()`.
+constexpr int kVersion = 1;
+
+// Preference paths.
+// NOTE: As these paths are written to preferences, changes must ensure
+// backwards compatibility. When intentionally breaking backwards compatibility,
+// increment `kVersion` and perform any needed conversions in `Deserialize()`.
+constexpr char kFilePathPath[] = "filePath";
+constexpr char kIdPath[] = "id";
+constexpr char kTypePath[] = "type";
+constexpr char kVersionPath[] = "version";
+
 std::string TypeToString(HoldingSpaceItem::Type type) {
   switch (type) {
     case HoldingSpaceItem::Type::kPinnedFile:
@@ -27,6 +42,12 @@ std::string TypeToString(HoldingSpaceItem::Type type) {
 
 HoldingSpaceItem::~HoldingSpaceItem() = default;
 
+bool HoldingSpaceItem::operator==(const HoldingSpaceItem& rhs) const {
+  return type_ == rhs.type_ && id_ == rhs.id_ && file_path_ == rhs.file_path_ &&
+         file_system_url_ == rhs.file_system_url_ && text_ == rhs.text_ &&
+         image_.BackedBySameObjectAs(rhs.image_);
+}
+
 // static
 std::string HoldingSpaceItem::GetFileBackedItemId(
     Type type,
@@ -46,6 +67,57 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
       file_path.BaseName().LossyDisplayName(), image));
 }
 
+// static
+// NOTE: This method must remain in sync with `Serialize()`. If multiple
+// 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);
+
+  const base::Optional<int> type = dict.FindIntPath(kTypePath);
+  DCHECK(type.has_value());
+
+  const base::Optional<base::FilePath> file_path =
+      util::ValueToFilePath(dict.FindPath(kFilePathPath));
+  DCHECK(file_path.has_value());
+
+  // NOTE: `std::make_unique` does not work with private constructors.
+  return base::WrapUnique(new HoldingSpaceItem(
+      static_cast<Type>(type.value()), DeserializeId(dict), file_path.value(),
+      std::move(file_system_url_resolver).Run(file_path.value()),
+      file_path->BaseName().LossyDisplayName(),
+      std::move(image_resolver).Run(file_path.value())));
+}
+
+// static
+// NOTE: This method must remain in sync with `Serialize()`. If multiple
+// serialization versions are supported, care must be taken to handle each.
+const std::string& HoldingSpaceItem::DeserializeId(
+    const base::DictionaryValue& dict) {
+  const base::Optional<int> version = dict.FindIntPath(kVersionPath);
+  DCHECK(version.has_value() && version.value() == kVersion);
+
+  const std::string* id = dict.FindStringPath(kIdPath);
+  DCHECK(id);
+
+  return *id;
+}
+
+// NOTE: This method must remain in sync with `Deserialize()`. The
+// return value will be written to preferences so this implementation must
+// maintain backwards compatibility so long as `kVersion` remains unchanged.
+base::DictionaryValue HoldingSpaceItem::Serialize() const {
+  base::DictionaryValue dict;
+  dict.SetIntPath(kVersionPath, kVersion);
+  dict.SetIntPath(kTypePath, static_cast<int>(type_));
+  dict.SetStringPath(kIdPath, id_);
+  dict.SetPath(kFilePathPath, util::FilePathToValue(file_path_));
+  return dict;
+}
+
 HoldingSpaceItem::HoldingSpaceItem(Type type,
                                    const std::string& id,
                                    const base::FilePath& file_path,

ash/public/cpp/holding_space/holding_space_item.h

@@ -5,13 +5,19 @@
 #ifndef ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_ITEM_H_
 #define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_ITEM_H_
 
+#include <memory>
 #include <string>
 
 #include "ash/public/cpp/ash_public_export.h"
+#include "base/callback_forward.h"
 #include "base/strings/string16.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
 
+namespace base {
+class DictionaryValue;
+}  // namespace base
+
 namespace ash {
 
 // Contains data needed to display a single item in the temporary holding space
@@ -19,12 +25,21 @@ namespace ash {
 class ASH_PUBLIC_EXPORT HoldingSpaceItem {
  public:
   // Items types supported by the holding space.
-  enum class Type { kPinnedFile, kScreenshot, kDownload };
+  // NOTE: These values are persisted in preferences so append new values to the
+  // end and do not change the meaning of existing values.
+  enum class Type {
+    kPinnedFile = 0,
+    kScreenshot = 1,
+    kDownload = 2,
+    kMaxValue = kDownload,
+  };
 
   HoldingSpaceItem(const HoldingSpaceItem&) = delete;
   HoldingSpaceItem operator=(const HoldingSpaceItem&) = delete;
   ~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,
@@ -37,6 +52,25 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
       const GURL& file_system_url,
       const gfx::ImageSkia& 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 file path.
+  using ImageResolver =
+      base::OnceCallback<gfx::ImageSkia(const base::FilePath&)>;
+
+  // Deserializes from `base::DictionaryValue` to `HoldingSpaceItem`.
+  static std::unique_ptr<HoldingSpaceItem> Deserialize(
+      const base::DictionaryValue& dict,
+      FileSystemUrlResolver file_system_url_resolver,
+      ImageResolver image_resolver);
+
+  // Deserializes id from a serialized `HoldingSpaceItem`.
+  static const std::string& DeserializeId(const base::DictionaryValue& dict);
+
+  // Serializes from `HoldingSpaceItem` to `base::DictionaryValue`.
+  base::DictionaryValue Serialize() const;
+
   const std::string& id() const { return id_; }
 
   Type type() const { return type_; }
@@ -50,7 +84,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   const GURL& file_system_url() const { return file_system_url_; }
 
  private:
-  // Contructor for file backed items.
+  // Constructor for file backed items.
   HoldingSpaceItem(Type type,
                    const std::string& id,
                    const base::FilePath& file_path,

ash/public/cpp/holding_space/holding_space_item_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/public/cpp/holding_space/holding_space_item.h"
+
+#include <memory>
+#include <vector>
+
+#include "base/test/bind_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/image/image_unittest_util.h"
+
+namespace ash {
+
+namespace {
+
+std::vector<HoldingSpaceItem::Type> GetHoldingSpaceItemTypes() {
+  std::vector<HoldingSpaceItem::Type> types;
+  for (int i = 0; i <= static_cast<int>(HoldingSpaceItem::Type::kMaxValue); ++i)
+    types.push_back(static_cast<HoldingSpaceItem::Type>(i));
+  return types;
+}
+
+}  // namespace
+
+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("file_system_url");
+  const gfx::ImageSkia image(gfx::test::CreateImageSkia(10, 10));
+
+  const auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+      /*type=*/GetParam(), file_path, file_system_url, image);
+
+  const base::DictionaryValue serialized_holding_space_item =
+      holding_space_item->Serialize();
+
+  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(
+          [&](const base::FilePath& file_path) { return image; }));
+
+  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("file_system_url"),
+      gfx::test::CreateImageSkia(10, 10));
+
+  const base::DictionaryValue serialized_holding_space_item =
+      holding_space_item->Serialize();
+
+  const std::string& deserialized_holding_space_id =
+      HoldingSpaceItem::DeserializeId(serialized_holding_space_item);
+
+  EXPECT_EQ(deserialized_holding_space_id, holding_space_item->id());
+}
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         HoldingSpaceItemTest,
+                         testing::ValuesIn(GetHoldingSpaceItemTypes()));
+
+}  // namespace ash

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

@@ -12,29 +12,48 @@
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/account_id/account_id.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/scoped_user_pref_update.h"
 
 namespace ash {
 
+namespace {
+
+PrefService* GetPrefService(content::BrowserContext* context) {
+  Profile* profile = Profile::FromBrowserContext(context);
+  DCHECK(profile);
+  return profile->GetPrefs();
+}
+
+}  // namespace
+
+// static
+constexpr char HoldingSpaceKeyedService::kPersistencePath[];
+
 HoldingSpaceKeyedService::HoldingSpaceKeyedService(
     content::BrowserContext* context,
     const AccountId& account_id)
     : browser_context_(context) {
+  RestoreModel();
+  holding_space_model_observer_.Add(&holding_space_model_);
   HoldingSpaceController::Get()->RegisterModelForUser(account_id,
                                                       &holding_space_model_);
 }
 
 HoldingSpaceKeyedService::~HoldingSpaceKeyedService() = default;
 
+// static
+void HoldingSpaceKeyedService::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterListPref(kPersistencePath);
+}
+
 void HoldingSpaceKeyedService::AddScreenshot(
     const base::FilePath& screenshot_file,
     const gfx::ImageSkia& image) {
-  GURL file_system_url;
-  if (!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
-          Profile::FromBrowserContext(browser_context_), screenshot_file,
-          file_manager::kFileManagerAppId, &file_system_url)) {
-    VLOG(2) << "Unable to convert screenshot file path to File System URL";
+  GURL file_system_url = ResolveFileSystemUrl(screenshot_file);
+  if (file_system_url.is_empty())
     return;
-  }
 
   auto item = HoldingSpaceItem::CreateFileBackedItem(
       HoldingSpaceItem::Type::kScreenshot, screenshot_file, file_system_url,
@@ -42,4 +61,61 @@ void HoldingSpaceKeyedService::AddScreenshot(
   holding_space_model_.AddItem(std::move(item));
 }
 
+void HoldingSpaceKeyedService::OnHoldingSpaceItemAdded(
+    const HoldingSpaceItem* item) {
+  // `kDownload` type holding space items have their own persistence mechanism.
+  if (item->type() == HoldingSpaceItem::Type::kDownload)
+    return;
+
+  // Write the new |item| to persistent storage.
+  ListPrefUpdate update(GetPrefService(browser_context_), kPersistencePath);
+  update->Append(item->Serialize());
+}
+
+void HoldingSpaceKeyedService::OnHoldingSpaceItemRemoved(
+    const HoldingSpaceItem* item) {
+  // `kDownload` type holding space items have their own persistence mechanism.
+  if (item->type() == HoldingSpaceItem::Type::kDownload)
+    return;
+
+  // Remove the |item| from persistent storage.
+  ListPrefUpdate update(GetPrefService(browser_context_), kPersistencePath);
+  update->EraseListValueIf([&item](const base::Value& existing_item) {
+    return HoldingSpaceItem::DeserializeId(
+               base::Value::AsDictionaryValue(existing_item)) == item->id();
+  });
+}
+
+// TODO(dmblack): Restore download holding space items.
+void HoldingSpaceKeyedService::RestoreModel() {
+  DCHECK(holding_space_model_.items().empty());
+  const auto* holding_space_items =
+      GetPrefService(browser_context_)->GetList(kPersistencePath);
+  for (const auto& holding_space_item : holding_space_items->GetList()) {
+    holding_space_model_.AddItem(HoldingSpaceItem::Deserialize(
+        base::Value::AsDictionaryValue(holding_space_item),
+        base::BindOnce(&HoldingSpaceKeyedService::ResolveFileSystemUrl,
+                       base::Unretained(this)),
+        base::BindOnce(&HoldingSpaceKeyedService::ResolveImage,
+                       base::Unretained(this))));
+  }
+}
+
+GURL HoldingSpaceKeyedService::ResolveFileSystemUrl(
+    const base::FilePath& file_path) const {
+  GURL file_system_url;
+  if (!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
+          Profile::FromBrowserContext(browser_context_), file_path,
+          file_manager::kFileManagerAppId, &file_system_url)) {
+    VLOG(2) << "Unable to convert file path to File System URL.";
+  }
+  return file_system_url;
+}
+
+// TODO(dmblack): Implement.
+gfx::ImageSkia HoldingSpaceKeyedService::ResolveImage(
+    const base::FilePath& file_path) const {
+  return gfx::ImageSkia();
+}
+
 }  // namespace ash

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

@@ -6,29 +6,42 @@
 #define CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_KEYED_SERVICE_H_
 
 #include "ash/public/cpp/holding_space/holding_space_model.h"
+#include "ash/public/cpp/holding_space/holding_space_model_observer.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "components/account_id/account_id.h"
 #include "components/keyed_service/core/keyed_service.h"
 
+class GURL;
+
 namespace base {
 class FilePath;
-}
+}  // namespace base
 
 namespace content {
 class BrowserContext;
-}
+}  // namespace content
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}  // namespace user_prefs
 
 namespace gfx {
 class ImageSkia;
-}
+}  // namespace gfx
 
 namespace ash {
 
 // Browser context keyed service that:
 // *   Manages the temporary holding space per-profile data model.
 // *   Serves as an entry point to add holding space items from Chrome.
-class HoldingSpaceKeyedService : public KeyedService {
+class HoldingSpaceKeyedService : public KeyedService,
+                                 public HoldingSpaceModelObserver {
  public:
+  // Preference path at which holding space items are persisted.
+  // NOTE: Any changes to persistence must be backwards compatible.
+  static constexpr char kPersistencePath[] = "ash.holding_space.items";
+
   HoldingSpaceKeyedService(content::BrowserContext* context,
                            const AccountId& account_id);
   HoldingSpaceKeyedService(const HoldingSpaceKeyedService& other) = delete;
@@ -36,6 +49,9 @@ class HoldingSpaceKeyedService : public KeyedService {
       delete;
   ~HoldingSpaceKeyedService() override;
 
+  // Registers profile preferences for holding space.
+  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
   // Adds a screenshot item backed by the provided absolute file path.
   // The path is expected to be under a mount point path recognized by the file
   // manager app (otherwise, the item will be dropped silently).
@@ -47,8 +63,22 @@ class HoldingSpaceKeyedService : public KeyedService {
   }
 
  private:
+  // HoldingSpaceModelObserver:
+  void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
+  void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
+
+  // Restores |holding_space_model_| from persistent storage.
+  void RestoreModel();
+
+  // Resolves file attributes from a file path;
+  GURL ResolveFileSystemUrl(const base::FilePath& file_path) const;
+  gfx::ImageSkia ResolveImage(const base::FilePath& file_path) const;
+
   content::BrowserContext* const browser_context_;
   HoldingSpaceModel holding_space_model_;
+
+  ScopedObserver<HoldingSpaceModel, HoldingSpaceModelObserver>
+      holding_space_model_observer_{this};
 };
 
 }  // namespace ash

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

@@ -55,4 +55,9 @@ bool HoldingSpaceKeyedServiceFactory::ServiceIsCreatedWithBrowserContext()
   return true;
 }
 
+void HoldingSpaceKeyedServiceFactory::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  HoldingSpaceKeyedService::RegisterProfilePrefs(registry);
+}
+
 }  // namespace ash

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

@@ -10,7 +10,7 @@
 
 namespace content {
 class BrowserContext;
-}
+}  // namespace content
 
 namespace ash {
 
@@ -29,6 +29,8 @@ class HoldingSpaceKeyedServiceFactory
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
   bool ServiceIsCreatedWithBrowserContext() const override;
+  void RegisterProfilePrefs(
+      user_prefs::PrefRegistrySyncable* registry) override;
 
  private:
   friend base::NoDestructor<HoldingSpaceKeyedServiceFactory>;