Add concept of in-progress holding space items.

This CL adds (but does not yet utilize) in-progress holding space items,
those being holding space items w/ progress < `1.f`. In a near future
CL, Chrome downloads will be added in-progress instead of on completion.

Note that this CL also removes some abstraction between the holding
space keyed service and its delegates. Given how the code base has
evolved, it seems permissible to allow delegates to directly
communicate w/ the service.

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

ash/public/cpp/BUILD.gn

@@ -382,6 +382,7 @@ source_set("unit_tests") {
     "file_icon_util_unittest.cc",
     "holding_space/holding_space_image_unittest.cc",
     "holding_space/holding_space_item_unittest.cc",
+    "holding_space/holding_space_model_unittest.cc",
     "metrics_util_unittest.cc",
     "pagination/pagination_model_unittest.cc",
     "power_utils_unittest.cc",

ash/public/cpp/holding_space/holding_space_item.cc

@@ -37,7 +37,7 @@ HoldingSpaceItem::~HoldingSpaceItem() {
 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_ == *rhs.image_;
+         *image_ == *rhs.image_ && progress_ == rhs.progress_;
 }
 
 // static
@@ -46,13 +46,24 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
     const base::FilePath& file_path,
     const GURL& file_system_url,
     ImageResolver image_resolver) {
+  return CreateFileBackedItem(type, file_path, file_system_url,
+                              /*progress=*/1.f, std::move(image_resolver));
+}
+
+// static
+std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
+    Type type,
+    const base::FilePath& file_path,
+    const GURL& file_system_url,
+    const base::Optional<float>& progress,
+    ImageResolver image_resolver) {
   DCHECK(!file_system_url.is_empty());
 
   // Note: std::make_unique does not work with private constructors.
   return base::WrapUnique(new HoldingSpaceItem(
       type, /*id=*/base::UnguessableToken::Create().ToString(), file_path,
       file_system_url, file_path.BaseName().LossyDisplayName(),
-      std::move(image_resolver).Run(type, file_path)));
+      std::move(image_resolver).Run(type, file_path), progress));
 }
 
 // static
@@ -86,7 +97,7 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::Deserialize(
   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)));
+      std::move(image_resolver).Run(type, file_path), /*progress=*/1.f));
 }
 
 // static
@@ -145,12 +156,31 @@ void HoldingSpaceItem::Initialize(const GURL& file_system_url) {
   file_system_url_ = file_system_url;
 }
 
-void HoldingSpaceItem::UpdateBackingFile(const base::FilePath& file_path,
+bool HoldingSpaceItem::UpdateBackingFile(const base::FilePath& file_path,
                                          const GURL& file_system_url) {
+  if (file_path_ == file_path && file_system_url_ == file_system_url)
+    return false;
+
   file_path_ = file_path;
   file_system_url_ = file_system_url;
   text_ = file_path.BaseName().LossyDisplayName();
   image_->UpdateBackingFilePath(file_path);
+
+  return true;
+}
+
+bool HoldingSpaceItem::UpdateProgress(const base::Optional<float>& progress) {
+  // NOTE: Once set to `1.f`, `progress_` becomes read-only.
+  if (progress_ == progress || progress_ == 1.f)
+    return false;
+
+  if (progress.has_value()) {
+    DCHECK_GE(progress.value(), 0.f);
+    DCHECK_LE(progress.value(), 1.f);
+  }
+
+  progress_ = progress;
+  return true;
 }
 
 void HoldingSpaceItem::InvalidateImage() {
@@ -177,12 +207,19 @@ HoldingSpaceItem::HoldingSpaceItem(Type type,
                                    const base::FilePath& file_path,
                                    const GURL& file_system_url,
                                    const std::u16string& text,
-                                   std::unique_ptr<HoldingSpaceImage> image)
+                                   std::unique_ptr<HoldingSpaceImage> image,
+                                   const base::Optional<float>& progress)
     : type_(type),
       id_(id),
       file_path_(file_path),
       file_system_url_(file_system_url),
       text_(text),
-      image_(std::move(image)) {}
+      image_(std::move(image)),
+      progress_(progress) {
+  if (progress_.has_value()) {
+    DCHECK_GE(progress_.value(), 0.f);
+    DCHECK_LE(progress_.value(), 1.f);
+  }
+}
 
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_item.h

@@ -12,6 +12,7 @@
 #include "base/callback_forward.h"
 #include "base/callback_list.h"
 #include "base/files/file_path.h"
+#include "base/optional.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -22,8 +23,7 @@ namespace ash {
 
 class HoldingSpaceImage;
 
-// Contains data needed to display a single item in the temporary holding space
-// UI.
+// Contains data needed to display a single item in the holding space UI.
 class ASH_PUBLIC_EXPORT HoldingSpaceItem {
  public:
   // Items types supported by the holding space.
@@ -59,6 +59,16 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
       const GURL& file_system_url,
       ImageResolver image_resolver);
 
+  // Creates a HoldingSpaceItem that's backed by a file system URL.
+  // NOTE: `file_system_url` is expected to be non-empty.
+  // NOTE: If present, `progress` must be >= `0.f` and <= `1.f`.
+  static std::unique_ptr<HoldingSpaceItem> CreateFileBackedItem(
+      Type type,
+      const base::FilePath& file_path,
+      const GURL& file_system_url,
+      const base::Optional<float>& progress,
+      ImageResolver image_resolver);
+
   // Returns `true` if `type` is a download type, `false` otherwise.
   static bool IsDownload(HoldingSpaceItem::Type type);
 
@@ -91,10 +101,16 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // `Deserialize()`.
   void Initialize(const GURL& file_system_url);
 
-  // Updates the file backing the item to `file_path` and `file_system_url`.
-  void UpdateBackingFile(const base::FilePath& file_path,
+  // Updates the file backing the item to `file_path` and `file_system_url`,
+  // returning `false` to indicate no-op.
+  bool UpdateBackingFile(const base::FilePath& file_path,
                          const GURL& file_system_url);
 
+  // Updates the `progress_` of the item, returning `false` to indicate no-op.
+  // NOTE: If present, `progress` must be >= `0.f` and <= `1.f`.
+  // NOTE: Once set to `1.f`, `progress_` becomes read-only.
+  bool UpdateProgress(const base::Optional<float>& progress);
+
   // Invalidates the current holding space image, so fresh image representations
   // are loaded when the image is next needed.
   void InvalidateImage();
@@ -114,6 +130,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
 
   const GURL& file_system_url() const { return file_system_url_; }
 
+  const base::Optional<float>& progress() const { return progress_; }
+
   HoldingSpaceImage& image_for_testing() { return *image_; }
 
  private:
@@ -123,7 +141,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
                    const base::FilePath& file_path,
                    const GURL& file_system_url,
                    const std::u16string& text,
-                   std::unique_ptr<HoldingSpaceImage> image);
+                   std::unique_ptr<HoldingSpaceImage> image,
+                   const base::Optional<float>& progress);
 
   const Type type_;
 
@@ -142,6 +161,11 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // The image representation of the item.
   std::unique_ptr<HoldingSpaceImage> image_;
 
+  // The progress of the item.
+  // If present, the value is >= `0.f` and <= `1.f`.
+  // If absent, `progress_` is indeterminate.
+  base::Optional<float> progress_;
+
   // Mutable to allow const access from `AddDeletionCallback()`.
   mutable base::RepeatingClosureList deletion_callback_list_;
 };

ash/public/cpp/holding_space/holding_space_item_unittest.cc

@@ -76,6 +76,51 @@ TEST_P(HoldingSpaceItemTest, DeserializeId) {
   EXPECT_EQ(deserialized_holding_space_id, holding_space_item->id());
 }
 
+// Tests progress for each holding space item type.
+TEST_P(HoldingSpaceItemTest, Progress) {
+  // Create a `holding_space_item` w/ explicitly specified progress.
+  auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+      /*type=*/GetParam(), base::FilePath("file_path"),
+      GURL("filesystem::file_system_url"), /*progress=*/0.5f,
+      /*image_resolver=*/base::BindOnce(&CreateFakeHoldingSpaceImage));
+
+  // Since explicitly specified during construction, progress should be `0.5`.
+  EXPECT_EQ(holding_space_item->progress(), 0.5f);
+
+  // It should be possible to update progress to a new value.
+  EXPECT_TRUE(holding_space_item->UpdateProgress(0.75f));
+  EXPECT_EQ(holding_space_item->progress(), 0.75f);
+
+  // It should no-op to try to update progress to its existing value.
+  EXPECT_FALSE(holding_space_item->UpdateProgress(0.75f));
+  EXPECT_EQ(holding_space_item->progress(), 0.75f);
+
+  // It should be possible to set indeterminate progress.
+  EXPECT_TRUE(holding_space_item->UpdateProgress(base::nullopt));
+  EXPECT_EQ(holding_space_item->progress(), base::nullopt);
+
+  // It should be possible to set progress complete.
+  EXPECT_TRUE(holding_space_item->UpdateProgress(1.f));
+  EXPECT_EQ(holding_space_item->progress(), 1.f);
+
+  // Once progress has been marked completed, it should become read-only.
+  EXPECT_FALSE(holding_space_item->UpdateProgress(0.75f));
+  EXPECT_EQ(holding_space_item->progress(), 1.f);
+
+  // Create a `holding_space_item` w/ default progress.
+  holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+      /*type=*/GetParam(), base::FilePath("file_path"),
+      GURL("filesystem::file_system_url"),
+      /*image_resolver=*/base::BindOnce(&CreateFakeHoldingSpaceImage));
+
+  // Since not specified during construction, progress should be `1.f`.
+  EXPECT_EQ(holding_space_item->progress(), 1.f);
+
+  // Since progress is marked completed, it should be read-only.
+  EXPECT_FALSE(holding_space_item->UpdateProgress(0.75f));
+  EXPECT_EQ(holding_space_item->progress(), 1.f);
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          HoldingSpaceItemTest,
                          testing::ValuesIn(GetHoldingSpaceItemTypes()));

ash/public/cpp/holding_space/holding_space_model.cc

@@ -88,7 +88,28 @@ void HoldingSpaceModel::UpdateBackingFileForItem(
   HoldingSpaceItem* item = item_it->get();
   DCHECK(item->IsInitialized());
 
-  item->UpdateBackingFile(file_path, file_system_url);
+  if (!item->UpdateBackingFile(file_path, file_system_url))
+    return;
+
+  for (auto& observer : observers_)
+    observer.OnHoldingSpaceItemUpdated(item);
+}
+
+void HoldingSpaceModel::UpdateProgressForItem(
+    const std::string& id,
+    const base::Optional<float>& progress) {
+  auto item_it = std::find_if(
+      items_.begin(), items_.end(),
+      [&id](const std::unique_ptr<HoldingSpaceItem>& item) -> bool {
+        return item->id() == id;
+      });
+  DCHECK(item_it != items_.end());
+
+  HoldingSpaceItem* item = item_it->get();
+  DCHECK(item->IsInitialized());
+
+  if (!item->UpdateProgress(progress))
+    return;
 
   for (auto& observer : observers_)
     observer.OnHoldingSpaceItemUpdated(item);

ash/public/cpp/holding_space/holding_space_model.h

@@ -15,6 +15,7 @@
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/callback.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -65,6 +66,12 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModel {
                                 const base::FilePath& file_path,
                                 const GURL& file_system_url);
 
+  // Updates the progress for a single holding space item.
+  // NOTE: If present, `progress` must be >= `0.f` and <= `1.f`.
+  // NOTE: Once set to `1.f`, holding space item progress becomes read-only.
+  void UpdateProgressForItem(const std::string& id,
+                             const base::Optional<float>& progress);
+
   // Removes all holding space items from the model for which the specified
   // `predicate` returns true.
   using Predicate = base::RepeatingCallback<bool(const HoldingSpaceItem*)>;