Update HoldingSpaceClient APIs for multiselect.

With the ability to select multiple holding space items at a time, we
want to be able to open, pin, and unpin items in bulk.

Bug: 1129981
Change-Id: Ifa0f80de5f3a30c87678858d66c697b8852381a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426085
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810083}

ash/public/cpp/holding_space/holding_space_client.h

@@ -5,6 +5,8 @@
 #ifndef ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_CLIENT_H_
 #define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_CLIENT_H_
 
+#include <vector>
+
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/callback_forward.h"
 
@@ -35,21 +37,22 @@ class ASH_PUBLIC_EXPORT HoldingSpaceClient {
   // Success is returned via the supplied `callback`.
   virtual void OpenDownloads(SuccessCallback callback) = 0;
 
-  // Attempts to open the specified holding space `item`.
+  // Attempts to open the specified holding space `items`.
   // Success is returned via the supplied `callback`.
-  virtual void OpenItem(const HoldingSpaceItem& item,
-                        SuccessCallback callback) = 0;
+  virtual void OpenItems(const std::vector<const HoldingSpaceItem*>& items,
+                         SuccessCallback callback) = 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,
                                 SuccessCallback callback) = 0;
 
-  // Pins the specified `item`.
-  virtual void PinItem(const HoldingSpaceItem& item) = 0;
+  // Pins the specified holding space `items`.
+  virtual void PinItems(const std::vector<const HoldingSpaceItem*>& items) = 0;
 
-  // Unpins the specified `item`.
-  virtual void UnpinItem(const HoldingSpaceItem& item) = 0;
+  // Unpins the specified holding space `items`.
+  virtual void UnpinItems(
+      const std::vector<const HoldingSpaceItem*>& items) = 0;
 
  protected:
   HoldingSpaceClient() = default;

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -94,9 +94,9 @@ void HoldingSpaceItemChipView::ButtonPressed(views::Button* sender,
   // Unpinning `item()` may result in the destruction of this view.
   auto weak_ptr = weak_factory_.GetWeakPtr();
   if (is_item_pinned)
-    HoldingSpaceController::Get()->client()->UnpinItem(*item());
+    HoldingSpaceController::Get()->client()->UnpinItems({item()});
   else
-    HoldingSpaceController::Get()->client()->PinItem(*item());
+    HoldingSpaceController::Get()->client()->PinItems({item()});
 
   if (weak_ptr)
     UpdatePin();

ash/system/holding_space/holding_space_item_view_delegate.cc

@@ -33,8 +33,8 @@ HoldingSpaceItemViewDelegate* instance = nullptr;
 // Helpers ---------------------------------------------------------------------
 
 // Attempts to open the specified holding space `item`.
-void OpenItem(const HoldingSpaceItem& item) {
-  HoldingSpaceController::Get()->client()->OpenItem(item, base::DoNothing());
+void OpenItem(const HoldingSpaceItem* item) {
+  HoldingSpaceController::Get()->client()->OpenItems({item}, base::DoNothing());
 }
 
 }  // namespace
@@ -56,7 +56,7 @@ void HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewGestureEvent(
     HoldingSpaceItemView* view,
     const ui::GestureEvent& event) {
   if (event.type() == ui::ET_GESTURE_TAP)
-    OpenItem(*view->item());
+    OpenItem(view->item());
 }
 
 // TODO(dmblack): Handle multiple selection.
@@ -64,7 +64,7 @@ bool HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewKeyPressed(
     HoldingSpaceItemView* view,
     const ui::KeyEvent& event) {
   if (event.key_code() == ui::KeyboardCode::VKEY_RETURN) {
-    OpenItem(*view->item());
+    OpenItem(view->item());
     return true;
   }
   return false;
@@ -75,7 +75,7 @@ bool HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewMousePressed(
     HoldingSpaceItemView* view,
     const ui::MouseEvent& event) {
   if (event.flags() & ui::EF_IS_DOUBLE_CLICK) {
-    OpenItem(*view->item());
+    OpenItem(view->item());
     return true;
   }
   return false;
@@ -150,15 +150,16 @@ void HoldingSpaceItemViewDelegate::ExecuteCommand(int command_id,
           *selected_view->item(), base::DoNothing());
       break;
     case HoldingSpaceCommandId::kPinItem:
-      HoldingSpaceController::Get()->client()->PinItem(*selected_view->item());
+      HoldingSpaceController::Get()->client()->PinItems(
+          {selected_view->item()});
       break;
     case HoldingSpaceCommandId::kShowInFolder:
       HoldingSpaceController::Get()->client()->ShowItemInFolder(
           *selected_view->item(), base::DoNothing());
       break;
     case HoldingSpaceCommandId::kUnpinItem:
-      HoldingSpaceController::Get()->client()->UnpinItem(
-          *selected_view->item());
+      HoldingSpaceController::Get()->client()->UnpinItems(
+          {selected_view->item()});
       break;
   }
 }

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

@@ -4,8 +4,11 @@
 
 #include "chrome/browser/ui/ash/holding_space/holding_space_client_impl.h"
 
+#include <memory>
+
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/notreached.h"
 #include "base/task/task_traits.h"
@@ -87,21 +90,42 @@ void HoldingSpaceClientImpl::OpenDownloads(SuccessCallback callback) {
           std::move(callback)));
 }
 
-void HoldingSpaceClientImpl::OpenItem(const HoldingSpaceItem& item,
-                                      SuccessCallback callback) {
-  if (item.file_path().empty()) {
+void HoldingSpaceClientImpl::OpenItems(
+    const std::vector<const HoldingSpaceItem*>& items,
+    SuccessCallback callback) {
+  if (items.empty()) {
     std::move(callback).Run(/*success=*/false);
     return;
   }
-  file_manager::util::OpenItem(
-      profile_, item.file_path(), platform_util::OPEN_FILE,
+
+  auto complete_success = std::make_unique<bool>(true);
+  auto* complete_success_ptr = complete_success.get();
+
+  base::RepeatingClosure barrier_closure = base::BarrierClosure(
+      items.size(),
       base::BindOnce(
-          [](SuccessCallback callback,
-             platform_util::OpenOperationResult result) {
-            const bool success = result == platform_util::OPEN_SUCCEEDED;
-            std::move(callback).Run(success);
+          [](std::unique_ptr<bool> complete_success, SuccessCallback callback) {
+            std::move(callback).Run(*complete_success);
           },
-          std::move(callback)));
+          std::move(complete_success), std::move(callback)));
+
+  for (const HoldingSpaceItem* item : items) {
+    if (item->file_path().empty()) {
+      *complete_success_ptr = false;
+      barrier_closure.Run();
+      return;
+    }
+    file_manager::util::OpenItem(
+        profile_, item->file_path(), platform_util::OPEN_FILE,
+        base::BindOnce(
+            [](base::RepeatingClosure barrier_closure, bool* complete_success,
+               platform_util::OpenOperationResult result) {
+              const bool success = result == platform_util::OPEN_SUCCEEDED;
+              *complete_success &= success;
+              barrier_closure.Run();
+            },
+            barrier_closure, complete_success_ptr));
+  }
 }
 
 void HoldingSpaceClientImpl::ShowItemInFolder(const HoldingSpaceItem& item,
@@ -121,23 +145,30 @@ void HoldingSpaceClientImpl::ShowItemInFolder(const HoldingSpaceItem& item,
           std::move(callback)));
 }
 
-void HoldingSpaceClientImpl::PinItem(const HoldingSpaceItem& item) {
-  DCHECK_NE(item.type(), HoldingSpaceItem::Type::kPinnedFile);
-  const storage::FileSystemURL& file_system_url =
-      file_manager::util::GetFileSystemContextForExtensionId(
-          profile_, file_manager::kFileManagerAppId)
-          ->CrackURL(item.file_system_url());
-  GetHoldingSpaceKeyedService(profile_)->AddPinnedFile(file_system_url);
+void HoldingSpaceClientImpl::PinItems(
+    const std::vector<const HoldingSpaceItem*>& items) {
+  HoldingSpaceKeyedService* service = GetHoldingSpaceKeyedService(profile_);
+  for (const HoldingSpaceItem* item : items) {
+    const storage::FileSystemURL& file_system_url =
+        file_manager::util::GetFileSystemContextForExtensionId(
+            profile_, file_manager::kFileManagerAppId)
+            ->CrackURL(item->file_system_url());
+    if (!service->ContainsPinnedFile(file_system_url))
+      service->AddPinnedFile(file_system_url);
+  }
 }
 
-void HoldingSpaceClientImpl::UnpinItem(const HoldingSpaceItem& item) {
-  const storage::FileSystemURL& file_system_url =
-      file_manager::util::GetFileSystemContextForExtensionId(
-          profile_, file_manager::kFileManagerAppId)
-          ->CrackURL(item.file_system_url());
-  DCHECK(GetHoldingSpaceKeyedService(profile_)->ContainsPinnedFile(
-      file_system_url));
-  GetHoldingSpaceKeyedService(profile_)->RemovePinnedFile(file_system_url);
+void HoldingSpaceClientImpl::UnpinItems(
+    const std::vector<const HoldingSpaceItem*>& items) {
+  HoldingSpaceKeyedService* service = GetHoldingSpaceKeyedService(profile_);
+  for (const HoldingSpaceItem* item : items) {
+    const storage::FileSystemURL& file_system_url =
+        file_manager::util::GetFileSystemContextForExtensionId(
+            profile_, file_manager::kFileManagerAppId)
+            ->CrackURL(item->file_system_url());
+    if (service->ContainsPinnedFile(file_system_url))
+      service->RemovePinnedFile(file_system_url);
+  }
 }
 
 }  // namespace ash

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

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_CLIENT_IMPL_H_
 #define CHROME_BROWSER_UI_ASH_HOLDING_SPACE_HOLDING_SPACE_CLIENT_IMPL_H_
 
+#include <vector>
+
 #include "ash/public/cpp/holding_space/holding_space_client.h"
 #include "base/callback.h"
 
@@ -25,10 +27,11 @@ class HoldingSpaceClientImpl : public HoldingSpaceClient {
   void AddScreenshot(const base::FilePath& file_path) override;
   void CopyImageToClipboard(const HoldingSpaceItem&, SuccessCallback) override;
   void OpenDownloads(SuccessCallback callback) override;
-  void OpenItem(const HoldingSpaceItem&, SuccessCallback) override;
+  void OpenItems(const std::vector<const HoldingSpaceItem*>& items,
+                 SuccessCallback callback) override;
   void ShowItemInFolder(const HoldingSpaceItem&, SuccessCallback) override;
-  void PinItem(const HoldingSpaceItem& item) override;
-  void UnpinItem(const HoldingSpaceItem& item) override;
+  void PinItems(const std::vector<const HoldingSpaceItem*>& items) override;
+  void UnpinItems(const std::vector<const HoldingSpaceItem*>& items) override;
 
  private:
   Profile* const profile_;

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

@@ -117,10 +117,10 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenDownloads) {
   run_loop.Run();
 }
 
-// Verifies that `HoldingSpaceClient::OpenItem()` works as intended when
+// Verifies that `HoldingSpaceClient::OpenItems()` works as intended when
 // attempting to open holding space items backed by both non-existing and
 // existing files.
-IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
+IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItems) {
   ASSERT_TRUE(HoldingSpaceController::Get());
 
   auto* holding_space_client = HoldingSpaceController::Get()->client();
@@ -134,11 +134,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
             /*placeholder=*/gfx::ImageSkia(),
             /*async_bitmap_resolver=*/base::DoNothing()));
 
-    // We expect `HoldingSpaceClient::OpenItem()` to fail when the backing file
+    // We expect `HoldingSpaceClient::OpenItems()` to fail when the backing file
     // for `holding_space_item` does not exist.
     base::RunLoop run_loop;
-    holding_space_client->OpenItem(
-        *holding_space_item,
+    holding_space_client->OpenItems(
+        {holding_space_item.get()},
         base::BindLambdaForTesting([&run_loop](bool success) {
           EXPECT_FALSE(success);
           run_loop.Quit();
@@ -150,11 +150,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
     // Create a holding space item backed by a newly created txt file.
     HoldingSpaceItem* holding_space_item = AddPinnedFile();
 
-    // We expect `HoldingSpaceClient::OpenItem()` to succeed when the backing
+    // We expect `HoldingSpaceClient::OpenItems()` to succeed when the backing
     // file for `holding_space_item` exists.
     base::RunLoop run_loop;
-    holding_space_client->OpenItem(
-        *holding_space_item,
+    holding_space_client->OpenItems(
+        {holding_space_item},
         base::BindLambdaForTesting([&run_loop](bool success) {
           EXPECT_TRUE(success);
           run_loop.Quit();
@@ -215,8 +215,8 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, MAYBE_ShowItemInFolder) {
   }
 }
 
-// Verifies that `HoldingSpaceClient::PinItem()` works as intended.
-IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, PinItem) {
+// Verifies that `HoldingSpaceClient::PinItems()` works as intended.
+IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, PinItems) {
   ASSERT_TRUE(HoldingSpaceController::Get());
 
   auto* holding_space_client = HoldingSpaceController::Get()->client();
@@ -228,7 +228,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, PinItem) {
   ASSERT_EQ(1u, holding_space_model->items().size());
 
   // Attempt to pin the download holding space item.
-  holding_space_client->PinItem(*download_item);
+  holding_space_client->PinItems({download_item});
   ASSERT_EQ(2u, holding_space_model->items().size());
 
   // The pinned holding space item should have type `kPinnedFile` but share the
@@ -239,8 +239,8 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, PinItem) {
   EXPECT_EQ(download_item->file_path(), pinned_file_item->file_path());
 }
 
-// Verifies that `HoldingSpaceClient::UnpinItem()` works as intended.
-IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, UnpinItem) {
+// Verifies that `HoldingSpaceClient::UnpinItems()` works as intended.
+IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, UnpinItems) {
   ASSERT_TRUE(HoldingSpaceController::Get());
 
   auto* holding_space_client = HoldingSpaceController::Get()->client();
@@ -252,7 +252,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, UnpinItem) {
   ASSERT_EQ(1u, holding_space_model->items().size());
 
   // Attempt to unpin the pinned file holding space item.
-  holding_space_client->UnpinItem(*pinned_file_item);
+  holding_space_client->UnpinItems({pinned_file_item});
   ASSERT_EQ(0u, holding_space_model->items().size());
 }