Move in-progress command label_id, icon, and handler to the model.

This allows in-progress commands to be fully defined and handled at
the same point in code as is providing the item update. Logic specific
to cancel/pause/resume can now be removed from the views delegate.

In the future, it could be possible to remove hard-coded logic about
primary/secondary actions but that is out of scope for this CL.

Also in the future we could move all commands to the model, but that
is similarly out of scope for this CL.

Bug: 1314548
Change-Id: I28bd20341b5e3f698028b675ebaa030ecfa78a9d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3806784
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/main@{#1031633}

ash/public/cpp/holding_space/holding_space_client.h

@@ -34,12 +34,6 @@ class ASH_PUBLIC_EXPORT HoldingSpaceClient {
   // Adds a screen recording item backed by the provided `file_path`.
   virtual void AddScreenRecording(const base::FilePath& file_path) = 0;
 
-  // Attempts to cancel the specified in-progress holding space `items`. In the
-  // case of in-progress holding space downloads, this would attempt to cancel
-  // the underlying download which would subsequently result in item removal.
-  virtual void CancelItems(
-      const std::vector<const HoldingSpaceItem*>& items) = 0;
-
   // Attempts to copy the contents of the image file backing the specified
   // holding space `item` to the clipboard. If the backing file is not suspected
   // to contain image data, this method will abort early. Success is returned
@@ -64,24 +58,12 @@ class ASH_PUBLIC_EXPORT HoldingSpaceClient {
   // Success is returned via the supplied `callback`.
   virtual void OpenMyFiles(SuccessCallback callback) = 0;
 
-  // Attempts to pause progress of the specified in-progress holding space
-  // `items`. In the case of in-progress holding space downloads, this would
-  // attempt to pause the underlying download.
-  virtual void PauseItems(
-      const std::vector<const HoldingSpaceItem*>& items) = 0;
-
   // Pins the specified `file_paths`.
   virtual void PinFiles(const std::vector<base::FilePath>& file_paths) = 0;
 
   // Pins the specified holding space `items`.
   virtual void PinItems(const std::vector<const HoldingSpaceItem*>& items) = 0;
 
-  // Attempts to resume progress of the specified in-progress holding space
-  // `items`. In the case of in-progress holding space downloads, this would
-  // attempt to resume the underlying download.
-  virtual void ResumeItems(
-      const std::vector<const HoldingSpaceItem*>& items) = 0;
-
   // Attempts to show the specified holding space `item` in its folder.
   // Success is returned via the supplied `callback`.
   virtual void ShowItemInFolder(const HoldingSpaceItem& item,

ash/public/cpp/holding_space/holding_space_item.cc

@@ -33,6 +33,41 @@ constexpr char kVersionPath[] = "version";
 
 }  // namespace
 
+// HoldingSpaceItem::InProgressCommand -----------------------------------------
+
+HoldingSpaceItem::InProgressCommand::InProgressCommand(
+    HoldingSpaceCommandId command_id,
+    int label_id,
+    const gfx::VectorIcon* icon,
+    Handler handler)
+    : command_id(command_id),
+      label_id(label_id),
+      icon(icon),
+      handler(std::move(handler)) {
+  DCHECK(holding_space_util::IsInProgressCommand(command_id));
+}
+
+HoldingSpaceItem::InProgressCommand::InProgressCommand(
+    const InProgressCommand& other)
+    : command_id(other.command_id),
+      label_id(other.label_id),
+      icon(other.icon),
+      handler(other.handler) {}
+
+HoldingSpaceItem::InProgressCommand::~InProgressCommand() = default;
+
+HoldingSpaceItem::InProgressCommand&
+HoldingSpaceItem::InProgressCommand::operator=(const InProgressCommand& other) =
+    default;
+
+bool HoldingSpaceItem::InProgressCommand::operator==(
+    const InProgressCommand& other) const {
+  return command_id == other.command_id && label_id == other.label_id &&
+         icon == other.icon && handler == other.handler;
+}
+
+// HoldingSpaceItem ------------------------------------------------------------
+
 HoldingSpaceItem::~HoldingSpaceItem() {
   deletion_callback_list_.Notify();
 }
@@ -246,9 +281,12 @@ bool HoldingSpaceItem::SetProgress(const HoldingSpaceProgress& progress) {
 }
 
 bool HoldingSpaceItem::SetInProgressCommands(
-    std::set<HoldingSpaceCommandId> in_progress_commands) {
+    std::vector<InProgressCommand> in_progress_commands) {
   DCHECK(std::all_of(in_progress_commands.begin(), in_progress_commands.end(),
-                     &holding_space_util::IsInProgressCommand));
+                     [](const InProgressCommand& in_progress_command) {
+                       return holding_space_util::IsInProgressCommand(
+                           in_progress_command.command_id);
+                     }));
 
   if (progress_.IsComplete() || in_progress_commands_ == in_progress_commands)
     return false;

ash/public/cpp/holding_space/holding_space_item.h

@@ -6,8 +6,8 @@
 #define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_ITEM_H_
 
 #include <memory>
-#include <set>
 #include <string>
+#include <vector>
 
 #include "ash/public/cpp/ash_public_export.h"
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
@@ -16,6 +16,7 @@
 #include "base/callback_list.h"
 #include "base/files/file_path.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
+#include "ui/gfx/vector_icon_types.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -33,6 +34,41 @@ class HoldingSpaceImage;
 // Contains data needed to display a single item in the holding space UI.
 class ASH_PUBLIC_EXPORT HoldingSpaceItem {
  public:
+  // Models a command for an in-progress item which is shown in the item's
+  // context menu and possibly, in the case of cancel/pause/resume, as primary/
+  // secondary actions on the item's view itself.
+  struct InProgressCommand {
+   public:
+    using Handler =
+        base::RepeatingCallback<void(const HoldingSpaceItem* item,
+                                     HoldingSpaceCommandId command_id)>;
+
+    InProgressCommand(HoldingSpaceCommandId command_id,
+                      int label_id,
+                      const gfx::VectorIcon* icon,
+                      Handler handler);
+
+    InProgressCommand(const InProgressCommand& other);
+
+    InProgressCommand& operator=(const InProgressCommand& other);
+
+    ~InProgressCommand();
+
+    bool operator==(const InProgressCommand& other) const;
+
+    // The identifier for the command.
+    HoldingSpaceCommandId command_id;
+
+    // The identifier for the label to be displayed for the command.
+    int label_id;
+
+    // The icon to be displayed for the command.
+    const gfx::VectorIcon* icon;
+
+    // The handler to be invoked to perform command execution.
+    Handler handler;
+  };
+
   // Items types supported by the holding space.
   // NOTE: These values are recorded in histograms and persisted in preferences
   // so append new values to the end and do not change the meaning of existing
@@ -149,7 +185,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // context menu and possibly, in the case of cancel/pause/resume, as primary/
   // secondary actions on the item view itself.
   bool SetInProgressCommands(
-      std::set<HoldingSpaceCommandId> in_progress_commands);
+      std::vector<InProgressCommand> in_progress_commands);
 
   // Sets the `progress_` of the item, returning `true` if a change occurred or
   // `false` to indicate no-op.
@@ -183,7 +219,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
 
   const HoldingSpaceProgress& progress() const { return progress_; }
 
-  const std::set<HoldingSpaceCommandId>& in_progress_commands() const {
+  const std::vector<InProgressCommand>& in_progress_commands() const {
     return in_progress_commands_;
   }
 
@@ -230,7 +266,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // The commands for an in-progress item which are shown in the item's context
   // menu and possibly, in the case of cancel/pause/resume, as primary/secondary
   // actions on the item's view itself.
-  std::set<HoldingSpaceCommandId> in_progress_commands_;
+  std::vector<InProgressCommand> in_progress_commands_;
 
   // Mutable to allow const access from `AddDeletionCallback()`.
   mutable base::RepeatingClosureList deletion_callback_list_;

ash/public/cpp/holding_space/holding_space_item_unittest.cc

@@ -5,7 +5,6 @@
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 
 #include <memory>
-#include <set>
 #include <vector>
 
 #include "ash/public/cpp/holding_space/holding_space_image.h"
@@ -18,6 +17,7 @@
 #include "base/values.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/chromeos/styles/cros_styles.h"
+#include "ui/gfx/paint_vector_icon.h"
 
 namespace ash {
 
@@ -132,8 +132,10 @@ TEST_P(HoldingSpaceItemTest, InProgressCommands) {
   EXPECT_TRUE(holding_space_item->in_progress_commands().empty());
 
   // It should be possible to update commands to a new value.
-  std::set<HoldingSpaceCommandId> in_progress_commands;
-  in_progress_commands.insert(HoldingSpaceCommandId::kCancelItem);
+  std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands;
+  in_progress_commands.push_back(HoldingSpaceItem::InProgressCommand(
+      HoldingSpaceCommandId::kCancelItem, /*label_id=*/-1, &gfx::kNoneIcon,
+      /*handler=*/base::DoNothing()));
   EXPECT_TRUE(holding_space_item->SetInProgressCommands(in_progress_commands));
   EXPECT_EQ(holding_space_item->in_progress_commands(), in_progress_commands);
 

ash/public/cpp/holding_space/holding_space_model.cc

@@ -22,7 +22,7 @@ HoldingSpaceModel::ScopedItemUpdate::~ScopedItemUpdate() {
 
   // Cache computed fields.
   const std::u16string accessible_name = item_->GetAccessibleName();
-  const std::set<HoldingSpaceCommandId> in_progress_commands =
+  const std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands =
       item_->in_progress_commands();
 
   // Update accessible name.
@@ -107,9 +107,13 @@ HoldingSpaceModel::ScopedItemUpdate::SetBackingFile(
 
 HoldingSpaceModel::ScopedItemUpdate&
 HoldingSpaceModel::ScopedItemUpdate::SetInProgressCommands(
-    std::set<HoldingSpaceCommandId> in_progress_commands) {
-  DCHECK(std::all_of(in_progress_commands.begin(), in_progress_commands.end(),
-                     &holding_space_util::IsInProgressCommand));
+    std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands) {
+  DCHECK(std::all_of(
+      in_progress_commands.begin(), in_progress_commands.end(),
+      [](const HoldingSpaceItem::InProgressCommand& in_progress_command) {
+        return holding_space_util::IsInProgressCommand(
+            in_progress_command.command_id);
+      }));
   in_progress_commands_ = std::move(in_progress_commands);
   return *this;
 }

ash/public/cpp/holding_space/holding_space_model.h

@@ -65,7 +65,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModel {
     // context menu and possibly, in the case of cancel/pause/resume, as
     // primary/secondary actions on the item view itself.
     ScopedItemUpdate& SetInProgressCommands(
-        std::set<HoldingSpaceCommandId> in_progress_commands);
+        std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands);
 
     // Sets whether the image for the item should be forcibly invalidated and
     // returns a reference to `this`.
@@ -100,7 +100,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceModel {
     absl::optional<absl::optional<std::u16string>> accessible_name_;
     absl::optional<base::FilePath> file_path_;
     absl::optional<GURL> file_system_url_;
-    absl::optional<std::set<HoldingSpaceCommandId>> in_progress_commands_;
+    absl::optional<std::vector<HoldingSpaceItem::InProgressCommand>>
+        in_progress_commands_;
     absl::optional<HoldingSpaceProgress> progress_;
     absl::optional<absl::optional<std::u16string>> secondary_text_;
     absl::optional<absl::optional<cros_styles::ColorName>>

ash/public/cpp/holding_space/holding_space_model_unittest.cc

@@ -5,7 +5,6 @@
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 
 #include <memory>
-#include <set>
 #include <vector>
 
 #include "ash/public/cpp/holding_space/holding_space_image.h"
@@ -17,6 +16,7 @@
 #include "base/scoped_observation.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/chromeos/styles/cros_styles.h"
+#include "ui/gfx/paint_vector_icon.h"
 
 namespace ash {
 namespace {
@@ -25,6 +25,13 @@ using UpdatedField = HoldingSpaceModelObserver::UpdatedField;
 
 // Helpers ---------------------------------------------------------------------
 
+HoldingSpaceItem::InProgressCommand CreateInProgressCommand(
+    HoldingSpaceCommandId command_id) {
+  return HoldingSpaceItem::InProgressCommand(command_id, /*label_id=*/-1,
+                                             &gfx::kNoneIcon,
+                                             /*handler=*/base::DoNothing());
+}
+
 std::vector<HoldingSpaceItem::Type> GetHoldingSpaceItemTypes() {
   std::vector<HoldingSpaceItem::Type> types;
   for (int i = 0; i <= static_cast<int>(HoldingSpaceItem::Type::kMaxValue); ++i)
@@ -244,8 +251,9 @@ TEST_P(HoldingSpaceModelTest, UpdateItem_Atomic) {
   EXPECT_EQ(item_ptr->file_system_url(), updated_file_system_url);
 
   // Update in-progress commands.
-  std::set<HoldingSpaceCommandId> in_progress_commands;
-  in_progress_commands.insert(HoldingSpaceCommandId::kCancelItem);
+  std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands;
+  in_progress_commands.push_back(
+      CreateInProgressCommand(HoldingSpaceCommandId::kCancelItem));
   model()
       .UpdateItem(item_ptr->id())
       ->SetInProgressCommands(in_progress_commands);
@@ -291,7 +299,8 @@ TEST_P(HoldingSpaceModelTest, UpdateItem_Atomic) {
             cros_styles::ColorName::kTextColorAlert);
 
   // Update all attributes.
-  in_progress_commands.insert(HoldingSpaceCommandId::kPauseItem);
+  in_progress_commands.push_back(
+      CreateInProgressCommand(HoldingSpaceCommandId::kPauseItem));
   updated_file_path = base::FilePath("again_updated_file_path");
   updated_file_system_url = GURL("filesystem::again_updated_file_system_url");
   model()
@@ -388,8 +397,9 @@ TEST_P(HoldingSpaceModelTest, UpdateItem_InProgressCommands) {
   EXPECT_TRUE(item_ptr->in_progress_commands().empty());
 
   // Update in-progress commands.
-  std::set<HoldingSpaceCommandId> in_progress_commands;
-  in_progress_commands.insert(HoldingSpaceCommandId::kCancelItem);
+  std::vector<HoldingSpaceItem::InProgressCommand> in_progress_commands;
+  in_progress_commands.push_back(
+      CreateInProgressCommand(HoldingSpaceCommandId::kCancelItem));
   model()
       .UpdateItem(item_ptr->id())
       ->SetInProgressCommands(in_progress_commands);
@@ -399,7 +409,8 @@ TEST_P(HoldingSpaceModelTest, UpdateItem_InProgressCommands) {
   EXPECT_EQ(item_ptr->in_progress_commands(), in_progress_commands);
 
   // Update in-progress commands again.
-  in_progress_commands.insert(HoldingSpaceCommandId::kPauseItem);
+  in_progress_commands.push_back(
+      CreateInProgressCommand(HoldingSpaceCommandId::kPauseItem));
   model()
       .UpdateItem(item_ptr->id())
       ->SetInProgressCommands(in_progress_commands);
@@ -410,7 +421,8 @@ TEST_P(HoldingSpaceModelTest, UpdateItem_InProgressCommands) {
 
   // Update in-progress commands and progress to completion. Because the item is
   // no longer in progress, in-progress commands should be empty.
-  in_progress_commands.insert(HoldingSpaceCommandId::kResumeItem);
+  in_progress_commands.push_back(
+      CreateInProgressCommand(HoldingSpaceCommandId::kResumeItem));
   model()
       .UpdateItem(item_ptr->id())
       ->SetInProgressCommands(in_progress_commands)

ash/public/cpp/holding_space/holding_space_util.cc

@@ -48,5 +48,27 @@ bool IsInProgressCommand(HoldingSpaceCommandId command_id) {
   }
 }
 
+bool SupportsInProgressCommand(const HoldingSpaceItem* item,
+                               HoldingSpaceCommandId command_id) {
+  DCHECK(IsInProgressCommand(command_id));
+  return std::any_of(
+      item->in_progress_commands().begin(), item->in_progress_commands().end(),
+      [&](const HoldingSpaceItem::InProgressCommand& in_progress_command) {
+        return in_progress_command.command_id == command_id;
+      });
+}
+
+bool ExecuteInProgressCommand(const HoldingSpaceItem* item,
+                              HoldingSpaceCommandId command_id) {
+  DCHECK(IsInProgressCommand(command_id));
+  for (const auto& in_progress_command : item->in_progress_commands()) {
+    if (in_progress_command.command_id == command_id) {
+      in_progress_command.handler.Run(item, command_id);
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace holding_space_util
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_util.h

@@ -21,6 +21,19 @@ ASH_PUBLIC_EXPORT gfx::Size GetMaxImageSizeForType(HoldingSpaceItem::Type type);
 // view itself.
 ASH_PUBLIC_EXPORT bool IsInProgressCommand(HoldingSpaceCommandId command_id);
 
+// Returns whether the specified `item` supports a given in-progress command
+// which is shown in the `item`'s context menu and possibly, in the case of
+// cancel/pause/resume, as primary/secondary actions on the `item` view itself.
+ASH_PUBLIC_EXPORT bool SupportsInProgressCommand(
+    const HoldingSpaceItem* item,
+    HoldingSpaceCommandId command_id);
+
+// Attempts to execute the in-progress command specified by `command_id` on
+// `item`, returning whether the attempt was successful.
+ASH_PUBLIC_EXPORT bool ExecuteInProgressCommand(
+    const HoldingSpaceItem* item,
+    HoldingSpaceCommandId command_id);
+
 }  // namespace holding_space_util
 }  // namespace ash