Add more HoldingSpaceClient APIs.

New HoldingSpaceClient APIs will be accessed from //ash to:
- Copy an item to clipboard (stubbed)
- Open an item in its folder (implemented)
- Pin an item (stubbed)
- Unpin an item (stubbed)

Stubbed APIs will be implemented in a follow up CL.

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

ash/public/cpp/holding_space/holding_space_client.h

@@ -15,11 +15,28 @@ class HoldingSpaceItem;
 // Interface for the holding space browser client.
 class ASH_PUBLIC_EXPORT HoldingSpaceClient {
  public:
-  // Attempts to open the specified holding space `item`. Success is returned
-  // via the supplied `callback`.
-  using OpenItemCallback = base::OnceCallback<void(bool)>;
+  using SuccessCallback = base::OnceCallback<void(bool)>;
+
+  // Attempts to copy the specified holding space `item` to the clipboard.
+  // Success is returned via the supplied `callback`.
+  virtual void CopyToClipboard(const HoldingSpaceItem& item,
+                               SuccessCallback callback) = 0;
+
+  // Attempts to open the specified holding space `item`.
+  // Success is returned via the supplied `callback`.
   virtual void OpenItem(const HoldingSpaceItem& item,
-                        OpenItemCallback callback) = 0;
+                        SuccessCallback callback) = 0;
+
+  // Attempts to open the specified holding space `item` in its folder.
+  // Success is returned via the supplied `callback`.
+  virtual void OpenItemInFolder(const HoldingSpaceItem& item,
+                                SuccessCallback callback) = 0;
+
+  // Pins the specified `item`.
+  virtual void PinItem(const HoldingSpaceItem& item) = 0;
+
+  // Unpins the specified `item`.
+  virtual void UnpinItem(const HoldingSpaceItem& item) = 0;
 
  protected:
   HoldingSpaceClient() = default;

chrome/browser/ui/ash/holding_space/BUILD.gn

@@ -43,6 +43,8 @@ source_set("test_support") {
     "//ash:test_support",
     "//ash/public/cpp",
     "//base/test:test_support",
+    "//chrome/browser",
+    "//chrome/browser/chromeos",
     "//chrome/browser/extensions",
     "//chrome/test:test_support_ui",
     "//ui/views",

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

@@ -14,7 +14,10 @@
 #include "ash/public/cpp/holding_space/holding_space_test_api.h"
 #include "base/bind_helpers.h"
 #include "base/unguessable_token.h"
+#include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/extensions/component_loader.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "storage/browser/file_system/external_mount_points.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/view.h"
 
@@ -24,13 +27,22 @@ namespace {
 
 // Helpers ---------------------------------------------------------------------
 
-// Creates a file at `root_path` with the specified `extension`, returning the
-// path of the created file.
-base::FilePath CreateFile(const base::FilePath& root_path,
-                          const std::string& extension) {
-  const base::FilePath file_path = root_path.Append(base::StringPrintf(
-      "%s.%s", base::UnguessableToken::Create().ToString().c_str(),
-      extension.c_str()));
+// Returns the path of the downloads mount point for the given `profile`.
+base::FilePath GetDownloadsPath(Profile* profile) {
+  base::FilePath result;
+  EXPECT_TRUE(
+      storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
+          file_manager::util::GetDownloadsMountPointName(profile), &result));
+  return result;
+}
+
+// Creates a file at the root of the downloads mount point with the specified
+// `extension`, returning the path of the created file.
+base::FilePath CreateFile(Profile* profile, const std::string& extension) {
+  const base::FilePath file_path =
+      GetDownloadsPath(profile).Append(base::StringPrintf(
+          "%s.%s", base::UnguessableToken::Create().ToString().c_str(),
+          extension.c_str()));
 
   {
     base::ScopedAllowBlockingForTesting allow_blocking;
@@ -47,14 +59,16 @@ base::FilePath CreateFile(const base::FilePath& root_path,
   return file_path;
 }
 
-// Creates a .txt file at `root_path` and returns the path of the created file.
-base::FilePath CreateTextFile(const base::FilePath& root_path) {
-  return CreateFile(root_path, "txt");
+// Creates a .txt file at the root of the downloads mount point and returns the
+// path of the created file.
+base::FilePath CreateTextFile(Profile* profile) {
+  return CreateFile(profile, "txt");
 }
 
-// Creates a .png file at `root_path` and returns the path of the created file.
-base::FilePath CreateImageFile(const base::FilePath& root_path) {
-  return CreateFile(root_path, "png");
+// Creates a .png file at the root of the downloads mount point and returns the
+// path of the created file.
+base::FilePath CreateImageFile(Profile* profile) {
+  return CreateFile(profile, "png");
 }
 
 }  // namespace
@@ -72,6 +86,10 @@ aura::Window* HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows() {
   return HoldingSpaceTestApi::GetRootWindowForNewWindows();
 }
 
+Profile* HoldingSpaceBrowserTestBase::GetProfile() {
+  return ProfileManager::GetActiveUserProfile();
+}
+
 void HoldingSpaceBrowserTestBase::Show() {
   test_api_->Show();
 }
@@ -87,7 +105,7 @@ bool HoldingSpaceBrowserTestBase::IsShowing() {
 HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddPinnedFile() {
   auto item = HoldingSpaceItem::CreateFileBackedItem(
       HoldingSpaceItem::Type::kPinnedFile,
-      /*file_path=*/CreateTextFile(scoped_temp_dir_.GetPath()),
+      /*file_path=*/CreateTextFile(GetProfile()),
       /*file_system_url=*/GURL(),
       /*image=*/
       std::make_unique<HoldingSpaceImage>(
@@ -102,7 +120,7 @@ HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddPinnedFile() {
 HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddScreenshotFile() {
   auto item = HoldingSpaceItem::CreateFileBackedItem(
       HoldingSpaceItem::Type::kScreenshot,
-      /*file_path=*/CreateImageFile(scoped_temp_dir_.GetPath()),
+      /*file_path=*/CreateImageFile(GetProfile()),
       /*file_system_url=*/GURL(),
       /*image=*/
       std::make_unique<HoldingSpaceImage>(
@@ -129,7 +147,6 @@ void HoldingSpaceBrowserTestBase::SetUpInProcessBrowserTestFixture() {
 
 void HoldingSpaceBrowserTestBase::SetUpOnMainThread() {
   InProcessBrowserTest::SetUpOnMainThread();
-  ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir());
   test_api_ = std::make_unique<HoldingSpaceTestApi>();
 }
 

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

@@ -8,10 +8,11 @@
 #include <memory>
 #include <vector>
 
-#include "base/files/scoped_temp_dir.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/test/base/in_process_browser_test.h"
 
+class Profile;
+
 namespace aura {
 class Window;
 }  // namespace aura
@@ -34,6 +35,9 @@ class HoldingSpaceBrowserTestBase : public InProcessBrowserTest {
   // Returns the root window that newly created windows should be added to.
   static aura::Window* GetRootWindowForNewWindows();
 
+  // Returns the currently active profile.
+  Profile* GetProfile();
+
   // Shows holding space UI. This is a no-op if it's already showing.
   void Show();
 
@@ -63,7 +67,6 @@ class HoldingSpaceBrowserTestBase : public InProcessBrowserTest {
   void SetUpOnMainThread() override;
 
   base::test::ScopedFeatureList scoped_feature_list_;
-  base::ScopedTempDir scoped_temp_dir_;
   std::unique_ptr<HoldingSpaceTestApi> test_api_;
 };
 

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

@@ -6,25 +6,31 @@
 
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/bind.h"
+#include "base/notreached.h"
 #include "chrome/browser/chromeos/file_manager/open_util.h"
 
 namespace ash {
 
-HoldingSpaceClientImpl::HoldingSpaceClientImpl(Profile* profile)
-    : profile_(profile) {}
+namespace {
 
-HoldingSpaceClientImpl::~HoldingSpaceClientImpl() = default;
+using SuccessCallback = HoldingSpaceClient::SuccessCallback;
 
-void HoldingSpaceClientImpl::OpenItem(const HoldingSpaceItem& item,
-                                      OpenItemCallback callback) {
-  if (item.file_path().empty()) {
+// Helpers ---------------------------------------------------------------------
+
+// Attempts to open the file or folder at the specified `file_path`.
+// Success is returned via the supplied `callback`.
+void OpenFileOrFolder(Profile* profile,
+                      const base::FilePath& file_path,
+                      platform_util::OpenItemType type,
+                      SuccessCallback callback) {
+  if (file_path.empty()) {
     std::move(callback).Run(/*success=*/false);
     return;
   }
   file_manager::util::OpenItem(
-      profile_, item.file_path(), platform_util::OPEN_FILE,
+      profile, file_path, type,
       base::BindOnce(
-          [](OpenItemCallback callback,
+          [](SuccessCallback callback,
              platform_util::OpenOperationResult result) {
             const bool success = result == platform_util::OPEN_SUCCEEDED;
             std::move(callback).Run(success);
@@ -32,4 +38,42 @@ void HoldingSpaceClientImpl::OpenItem(const HoldingSpaceItem& item,
           std::move(callback)));
 }
 
+}  // namespace
+
+// HoldingSpaceClientImpl ------------------------------------------------------
+
+HoldingSpaceClientImpl::HoldingSpaceClientImpl(Profile* profile)
+    : profile_(profile) {}
+
+HoldingSpaceClientImpl::~HoldingSpaceClientImpl() = default;
+
+// TODO(crbug/1126274): Implement.
+void HoldingSpaceClientImpl::CopyToClipboard(const HoldingSpaceItem& item,
+                                             SuccessCallback callback) {
+  NOTIMPLEMENTED();
+  std::move(callback).Run(/*success=*/false);
+}
+
+void HoldingSpaceClientImpl::OpenItem(const HoldingSpaceItem& item,
+                                      SuccessCallback callback) {
+  OpenFileOrFolder(profile_, item.file_path(), platform_util::OPEN_FILE,
+                   std::move(callback));
+}
+
+void HoldingSpaceClientImpl::OpenItemInFolder(const HoldingSpaceItem& item,
+                                              SuccessCallback callback) {
+  OpenFileOrFolder(profile_, item.file_path().DirName(),
+                   platform_util::OPEN_FOLDER, std::move(callback));
+}
+
+// TODO(crbug/1126274): Implement.
+void HoldingSpaceClientImpl::PinItem(const HoldingSpaceItem& item) {
+  NOTIMPLEMENTED();
+}
+
+// TODO(crbug/1126274): Implement.
+void HoldingSpaceClientImpl::UnpinItem(const HoldingSpaceItem& item) {
+  NOTIMPLEMENTED();
+}
+
 }  // namespace ash

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

@@ -22,8 +22,11 @@ class HoldingSpaceClientImpl : public HoldingSpaceClient {
   ~HoldingSpaceClientImpl() override;
 
   // HoldingSpaceClient:
-  void OpenItem(const HoldingSpaceItem& item,
-                OpenItemCallback callback) override;
+  void CopyToClipboard(const HoldingSpaceItem&, SuccessCallback) override;
+  void OpenItem(const HoldingSpaceItem&, SuccessCallback) override;
+  void OpenItemInFolder(const HoldingSpaceItem&, SuccessCallback) override;
+  void PinItem(const HoldingSpaceItem& item) override;
+  void UnpinItem(const HoldingSpaceItem& item) override;
 
  private:
   Profile* const profile_;

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

@@ -14,64 +14,17 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/unguessable_token.h"
-#include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
-#include "storage/browser/file_system/external_mount_points.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace ash {
-namespace {
-
-// Returns the path of the downloads mount point for the given `profile`.
-base::FilePath GetDownloadsPath(Profile* profile) {
-  base::FilePath result;
-  EXPECT_TRUE(
-      storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
-          file_manager::util::GetDownloadsMountPointName(profile), &result));
-  return result;
-}
-
-// Creates a txt file at the path of the downloads mount point for `profile`.
-base::FilePath CreateTextFile(Profile* profile) {
-  const std::string relative_path = base::StringPrintf(
-      "%s.txt", base::UnguessableToken::Create().ToString().c_str());
-  const base::FilePath path = GetDownloadsPath(profile).Append(relative_path);
-
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  if (!base::CreateDirectory(path.DirName()))
-    return base::FilePath();
-  if (!base::WriteFile(path, /*content=*/std::string()))
-    return base::FilePath();
-
-  return path;
-}
-
-}  // namespace
-
-class HoldingSpaceClientImplTest : public InProcessBrowserTest {
- public:
-  HoldingSpaceClientImplTest() {
-    scoped_feature_list_.InitAndEnableFeature(features::kTemporaryHoldingSpace);
-  }
-
-  HoldingSpaceClientImplTest(const HoldingSpaceClientImplTest& other) = delete;
-  HoldingSpaceClientImplTest& operator=(
-      const HoldingSpaceClientImplTest& other) = delete;
-  ~HoldingSpaceClientImplTest() override = default;
-
-  // InProcessBrowserTest:
-  void SetUpInProcessBrowserTestFixture() override {
-    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
-    extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
-  }
 
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
+using HoldingSpaceClientImplTest = HoldingSpaceBrowserTestBase;
 
 // Verifies that `HoldingSpaceClient::OpenItem()` works as intended when
 // attempting to open holding space items backed by both non-existing and
@@ -82,14 +35,14 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
   auto* holding_space_client = HoldingSpaceController::Get()->client();
   ASSERT_TRUE(holding_space_client);
 
-  // Create a holding space item backed by a non-existing file.
-  auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
-      HoldingSpaceItem::Type::kDownload, base::FilePath("foo"), GURL(),
-      std::make_unique<HoldingSpaceImage>(
-          /*placeholder=*/gfx::ImageSkia(),
-          /*async_bitmap_resolver=*/base::DoNothing()));
-
   {
+    // Create a holding space item backed by a non-existing file.
+    auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"), GURL(),
+        std::make_unique<HoldingSpaceImage>(
+            /*placeholder=*/gfx::ImageSkia(),
+            /*async_bitmap_resolver=*/base::DoNothing()));
+
     // We expect `HoldingSpaceClient::OpenItem()` to fail when the backing file
     // for `holding_space_item` does not exist.
     base::RunLoop run_loop;
@@ -102,15 +55,10 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
     run_loop.Run();
   }
 
-  // Create a holding space item backed by a newly created txt file.
-  holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
-      HoldingSpaceItem::Type::kDownload, CreateTextFile(browser()->profile()),
-      GURL(),
-      std::make_unique<HoldingSpaceImage>(
-          /*placeholder=*/gfx::ImageSkia(),
-          /*async_bitmap_resolver=*/base::DoNothing()));
-
   {
+    // 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
     // file for `holding_space_item` exists.
     base::RunLoop run_loop;
@@ -124,4 +72,50 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItem) {
   }
 }
 
+// Verifies that `HoldingSpaceClient::OpenItemInFolder()` works as intended when
+// attempting to open holding space items backed by both non-existing and
+// existing files.
+IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenItemInFolder) {
+  ASSERT_TRUE(HoldingSpaceController::Get());
+
+  auto* holding_space_client = HoldingSpaceController::Get()->client();
+  ASSERT_TRUE(holding_space_client);
+
+  {
+    // Create a holding space item backed by a non-existing file.
+    auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
+        HoldingSpaceItem::Type::kDownload, base::FilePath("foo"), GURL(),
+        std::make_unique<HoldingSpaceImage>(
+            /*placeholder=*/gfx::ImageSkia(),
+            /*async_bitmap_resolver=*/base::DoNothing()));
+
+    // We expect `HoldingSpaceClient::OpenItemInFolder()` to fail when the
+    // backing file for `holding_space_item` does not exist.
+    base::RunLoop run_loop;
+    holding_space_client->OpenItemInFolder(
+        *holding_space_item,
+        base::BindLambdaForTesting([&run_loop](bool success) {
+          EXPECT_FALSE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
+  {
+    // Create a holding space item backed by a newly created txt file.
+    HoldingSpaceItem* holding_space_item = AddPinnedFile();
+
+    // We expect `HoldingSpaceClient::OpenItemInFolder()` to succeed when the
+    // backing file for `holding_space_item` exists.
+    base::RunLoop run_loop;
+    holding_space_client->OpenItemInFolder(
+        *holding_space_item,
+        base::BindLambdaForTesting([&run_loop](bool success) {
+          EXPECT_TRUE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+}
+
 }  // namespace ash