Add screenshots to the holding space model

Introduces HoldingSpaceItem::Type enumeration with types that will be
supported in the holding space UI - pinned files, downloads and
screenshots. Also adds data that will needed to represent the items in
the UI (in addition to the existing text item representation)
*   the item's absolute file path
*   the file system URL of the file backing the item
*   the image representation of the item

Changes how the item ID is calculated - instead of a randomly generated
GUID, the ID is now deffered from the item type and file system URL.

Changes HoldingSpacePresenter to track items per type (the class is
still only used by a test, but should be eventually be hooked up with
the holding space tray bubble code).

Updates screenshot grabber to add the screenshot to the holding space
after creating the screenshot file and loading the screenshot bitmap.

BUG=1112014

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

ash/holding_space/holding_space_presenter.cc

@@ -31,18 +31,29 @@ void HoldingSpacePresenter::OnHoldingSpaceModelDetached(
 
 void HoldingSpacePresenter::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
-  item_ids_.push_back(item->id());
+  item_ids_[item->type()].push_back(item->id());
 }
 
 void HoldingSpacePresenter::OnHoldingSpaceItemRemoved(
     const HoldingSpaceItem* item) {
-  base::Erase(item_ids_, item->id());
+  base::Erase(item_ids_[item->type()], item->id());
+}
+
+const std::vector<std::string>& HoldingSpacePresenter::GetItemIds(
+    HoldingSpaceItem::Type type) const {
+  const auto& item_list_it = item_ids_.find(type);
+  if (item_list_it == item_ids_.end()) {
+    static std::vector<std::string> empty_list;
+    return empty_list;
+  }
+  return item_list_it->second;
 }
 
 void HoldingSpacePresenter::HandleNewModel(HoldingSpaceModel* model) {
   model_observer_.Add(model);
 
   for (auto& item : model->items())
-    item_ids_.push_back(item->id());
+    item_ids_[item->type()].push_back(item->id());
 }
+
 }  // namespace ash

ash/holding_space/holding_space_presenter.h

@@ -11,8 +11,10 @@
 #include "ash/ash_export.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_controller_observer.h"
+#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/public/cpp/holding_space/holding_space_model_observer.h"
+#include "base/containers/flat_map.h"
 #include "base/scoped_observer.h"
 
 namespace ash {
@@ -43,7 +45,7 @@ class ASH_EXPORT HoldingSpacePresenter : public HoldingSpaceControllerObserver,
   void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
 
-  const std::vector<std::string>& item_ids() const { return item_ids_; }
+  const std::vector<std::string>& GetItemIds(HoldingSpaceItem::Type type) const;
 
  private:
   // Handles the items from the model, and starts observing the model.
@@ -51,7 +53,7 @@ class ASH_EXPORT HoldingSpacePresenter : public HoldingSpaceControllerObserver,
 
   // IDs of items in the active holding space model, as observed by the holding
   // space presenter.
-  std::vector<std::string> item_ids_;
+  base::flat_map<HoldingSpaceItem::Type, std::vector<std::string>> item_ids_;
 
   ScopedObserver<HoldingSpaceController, HoldingSpaceControllerObserver>
       controller_observer_{this};

ash/public/cpp/holding_space/holding_space_item.cc

@@ -4,10 +4,59 @@
 
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 
+#include "base/files/file_path.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/strcat.h"
+
 namespace ash {
 
-HoldingSpaceItem::HoldingSpaceItem(const std::string& id) : id_(id) {}
+namespace {
+
+std::string TypeToString(HoldingSpaceItem::Type type) {
+  switch (type) {
+    case HoldingSpaceItem::Type::kPinnedFile:
+      return "pinned_file";
+    case HoldingSpaceItem::Type::kDownload:
+      return "download";
+    case HoldingSpaceItem::Type::kScreenshot:
+      return "screenshot";
+  }
+}
+
+}  // namespace
 
 HoldingSpaceItem::~HoldingSpaceItem() = default;
 
+// static
+std::string HoldingSpaceItem::GetFileBackedItemId(
+    Type type,
+    const base::FilePath& file_path) {
+  return base::StrCat({TypeToString(type), ":", file_path.value()});
+}
+
+// static
+std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
+    Type type,
+    const base::FilePath& file_path,
+    const GURL& file_system_url,
+    const gfx::ImageSkia& image) {
+  // Note: std::make_unique does not work with private constructors.
+  return base::WrapUnique(new HoldingSpaceItem(
+      type, GetFileBackedItemId(type, file_path), file_path, file_system_url,
+      file_path.BaseName().LossyDisplayName(), image));
+}
+
+HoldingSpaceItem::HoldingSpaceItem(Type type,
+                                   const std::string& id,
+                                   const base::FilePath& file_path,
+                                   const GURL& file_system_url,
+                                   const base::string16& text,
+                                   const gfx::ImageSkia& image)
+    : type_(type),
+      id_(id),
+      file_path_(file_path),
+      file_system_url_(file_system_url),
+      text_(text),
+      image_(image) {}
+
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_item.h

@@ -8,8 +8,9 @@
 #include <string>
 
 #include "ash/public/cpp/ash_public_export.h"
-#include "base/optional.h"
 #include "base/strings/string16.h"
+#include "ui/gfx/image/image_skia.h"
+#include "url/gurl.h"
 
 namespace ash {
 
@@ -17,22 +18,62 @@ namespace ash {
 // UI.
 class ASH_PUBLIC_EXPORT HoldingSpaceItem {
  public:
-  explicit HoldingSpaceItem(const std::string& id);
-  HoldingSpaceItem(const HoldingSpaceItem& other) = delete;
-  HoldingSpaceItem operator=(const HoldingSpaceItem& other) = delete;
+  // Items types supported by the holding space.
+  enum class Type { kPinnedFile, kScreenshot, kDownload };
+
+  HoldingSpaceItem(const HoldingSpaceItem&) = delete;
+  HoldingSpaceItem operator=(const HoldingSpaceItem&) = delete;
   ~HoldingSpaceItem();
 
+  // 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,
+                                         const base::FilePath& file_path);
+
+  // Creates a HoldingSpaceItem that's backed by a file system URL.
+  static std::unique_ptr<HoldingSpaceItem> CreateFileBackedItem(
+      Type type,
+      const base::FilePath& file_path,
+      const GURL& file_system_url,
+      const gfx::ImageSkia& image);
+
   const std::string& id() const { return id_; }
 
-  void set_text(const base::string16& text) { text_ = text; }
-  const base::Optional<base::string16>& text() const { return text_; }
+  Type type() const { return type_; }
+
+  const base::string16& text() const { return text_; }
+
+  const gfx::ImageSkia& image() const { return image_; }
+
+  const base::FilePath& file_path() const { return file_path_; }
+
+  const GURL& file_system_url() const { return file_system_url_; }
 
  private:
+  // Contructor for file backed items.
+  HoldingSpaceItem(Type type,
+                   const std::string& id,
+                   const base::FilePath& file_path,
+                   const GURL& file_system_url,
+                   const base::string16& text,
+                   const gfx::ImageSkia& image);
+
+  const Type type_;
+
   // The holding space item ID assigned to the item.
   std::string id_;
 
-  // If set, the text data associated with the item.
-  base::Optional<base::string16> text_;
+  // The file path by which the item is backed.
+  base::FilePath file_path_;
+
+  // The file system URL of the file that backs the item.
+  GURL file_system_url_;
+
+  // If set, the text that should be shown for the item.
+  base::string16 text_;
+
+  // The image representation of the item.
+  gfx::ImageSkia image_;
 };
 
 }  // namespace ash

chrome/browser/ui/ash/chrome_screenshot_grabber.cc

@@ -10,6 +10,7 @@
 #include <utility>
 #include <vector>
 
+#include "ash/public/cpp/ash_features.h"
 #include "ash/public/cpp/notification_utils.h"
 #include "ash/shell.h"
 #include "base/base64.h"
@@ -38,6 +39,8 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/chrome_screenshot_grabber_test_observer.h"
 #include "chrome/browser/ui/ash/clipboard_util.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
@@ -559,6 +562,12 @@ void ChromeScreenshotGrabber::OnReadScreenshotFileForPreviewCompleted(
   NotificationDisplayService::GetForProfile(GetProfile())
       ->Display(NotificationHandler::Type::TRANSIENT, *notification,
                 /*metadata=*/nullptr);
+
+  if (success && ash::features::IsTemporaryHoldingSpaceEnabled()) {
+    ash::HoldingSpaceKeyedServiceFactory::GetInstance()
+        ->GetService(GetProfile())
+        ->AddScreenshot(screenshot_path, image.AsImageSkia());
+  }
 }
 
 Profile* ChromeScreenshotGrabber::GetProfile() {

chrome/browser/ui/ash/chrome_screenshot_grabber_browsertest.cc

@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ash/public/cpp/ash_features.h"
+#include "ash/public/cpp/holding_space/holding_space_controller.h"
+#include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/threading/thread_restrictions.h"
@@ -28,12 +32,17 @@
 using testing::_;
 using testing::Return;
 
+// Parameterized by TemporaryHoldingSpace feature state.
 class ChromeScreenshotGrabberBrowserTest
     : public InProcessBrowserTest,
+      public testing::WithParamInterface<bool>,
       public ChromeScreenshotGrabberTestObserver,
       public ui::ClipboardObserver {
  public:
-  ChromeScreenshotGrabberBrowserTest() = default;
+  ChromeScreenshotGrabberBrowserTest() {
+    scoped_feature_list_.InitWithFeatureState(
+        ash::features::kTemporaryHoldingSpace, GetParam());
+  }
   ~ChromeScreenshotGrabberBrowserTest() override = default;
 
   void SetUpOnMainThread() override {
@@ -81,6 +90,14 @@ class ChromeScreenshotGrabberBrowserTest
         ui::ClipboardBuffer::kCopyPaste, /* data_dst = */ nullptr);
   }
 
+  bool TemporaryHoldingSpaceEnabled() const { return GetParam(); }
+
+  ash::HoldingSpaceModel* GetHoldingSpaceModel() const {
+    ash::HoldingSpaceController* const controller =
+        ash::HoldingSpaceController::Get();
+    return controller ? controller->model() : nullptr;
+  }
+
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
   std::unique_ptr<NotificationDisplayServiceTester> display_service_;
@@ -91,10 +108,16 @@ class ChromeScreenshotGrabberBrowserTest
   policy::MockDlpContentManager mock_dlp_content_manager_;
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeScreenshotGrabberBrowserTest);
 };
 
-IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest, TakeScreenshot) {
+INSTANTIATE_TEST_SUITE_P(All,
+                         ChromeScreenshotGrabberBrowserTest,
+                         testing::Bool());
+
+IN_PROC_BROWSER_TEST_P(ChromeScreenshotGrabberBrowserTest, TakeScreenshot) {
   ChromeScreenshotGrabber* chrome_screenshot_grabber =
       ChromeScreenshotGrabber::Get();
   SetTestObserver(chrome_screenshot_grabber, this);
@@ -124,6 +147,20 @@ IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest, TakeScreenshot) {
     EXPECT_TRUE(base::PathExists(screenshot_path_));
   }
 
+  if (TemporaryHoldingSpaceEnabled()) {
+    ash::HoldingSpaceModel* holding_space_model = GetHoldingSpaceModel();
+    ASSERT_TRUE(holding_space_model);
+    ASSERT_EQ(1u, holding_space_model->items().size());
+
+    ash::HoldingSpaceItem* holding_space_item =
+        holding_space_model->items()[0].get();
+    EXPECT_EQ(ash::HoldingSpaceItem::Type::kScreenshot,
+              holding_space_item->type());
+    EXPECT_EQ(screenshot_path_, holding_space_item->file_path());
+  } else {
+    EXPECT_FALSE(GetHoldingSpaceModel());
+  }
+
   EXPECT_FALSE(IsImageClipboardAvailable());
   ui::ClipboardMonitor::GetInstance()->AddObserver(this);
 
@@ -138,7 +175,7 @@ IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest, TakeScreenshot) {
   EXPECT_TRUE(IsImageClipboardAvailable());
 }
 
-IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest,
+IN_PROC_BROWSER_TEST_P(ChromeScreenshotGrabberBrowserTest,
                        ScreenshotsDisallowed) {
   ChromeScreenshotGrabber* chrome_screenshot_grabber =
       ChromeScreenshotGrabber::Get();
@@ -152,9 +189,17 @@ IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest,
   EXPECT_TRUE(notification_added_);
   EXPECT_TRUE(display_service_->GetNotification(std::string("screenshot")));
   EXPECT_EQ(ui::ScreenshotResult::DISABLED, screenshot_result_);
+
+  if (TemporaryHoldingSpaceEnabled()) {
+    ash::HoldingSpaceModel* holding_space_model = GetHoldingSpaceModel();
+    ASSERT_TRUE(holding_space_model);
+    EXPECT_TRUE(holding_space_model->items().empty());
+  } else {
+    EXPECT_FALSE(GetHoldingSpaceModel());
+  }
 }
 
-IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest,
+IN_PROC_BROWSER_TEST_P(ChromeScreenshotGrabberBrowserTest,
                        ScreenshotsRestricted) {
   ChromeScreenshotGrabber* chrome_screenshot_grabber =
       ChromeScreenshotGrabber::Get();
@@ -172,4 +217,12 @@ IN_PROC_BROWSER_TEST_F(ChromeScreenshotGrabberBrowserTest,
   EXPECT_TRUE(notification_added_);
   EXPECT_TRUE(display_service_->GetNotification(std::string("screenshot")));
   EXPECT_EQ(ui::ScreenshotResult::DISABLED, screenshot_result_);
+
+  if (TemporaryHoldingSpaceEnabled()) {
+    ash::HoldingSpaceModel* holding_space_model = GetHoldingSpaceModel();
+    ASSERT_TRUE(holding_space_model);
+    EXPECT_TRUE(holding_space_model->items().empty());
+  } else {
+    EXPECT_FALSE(GetHoldingSpaceModel());
+  }
 }

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

@@ -6,23 +6,39 @@
 
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "base/files/file_path.h"
 #include "base/guid.h"
+#include "chrome/browser/chromeos/file_manager/app_id.h"
+#include "chrome/browser/chromeos/file_manager/fileapi_util.h"
+#include "chrome/browser/profiles/profile.h"
 #include "components/account_id/account_id.h"
 
 namespace ash {
 
 HoldingSpaceKeyedService::HoldingSpaceKeyedService(
     content::BrowserContext* context,
-    const AccountId& account_id) {
+    const AccountId& account_id)
+    : browser_context_(context) {
   HoldingSpaceController::Get()->RegisterModelForUser(account_id,
                                                       &holding_space_model_);
 }
 
 HoldingSpaceKeyedService::~HoldingSpaceKeyedService() = default;
 
-void HoldingSpaceKeyedService::AddTextItem(const base::string16& text) {
-  auto item = std::make_unique<HoldingSpaceItem>(base::GenerateGUID());
-  item->set_text(text);
+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";
+    return;
+  }
+
+  auto item = HoldingSpaceItem::CreateFileBackedItem(
+      HoldingSpaceItem::Type::kScreenshot, screenshot_file, file_system_url,
+      image);
   holding_space_model_.AddItem(std::move(item));
 }
 

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

@@ -10,10 +10,18 @@
 #include "components/account_id/account_id.h"
 #include "components/keyed_service/core/keyed_service.h"
 
+namespace base {
+class FilePath;
+}
+
 namespace content {
 class BrowserContext;
 }
 
+namespace gfx {
+class ImageSkia;
+}
+
 namespace ash {
 
 // Browser context keyed service that:
@@ -28,15 +36,18 @@ class HoldingSpaceKeyedService : public KeyedService {
       delete;
   ~HoldingSpaceKeyedService() override;
 
-  // Adds a text item to the service's holding space model.
-  // |text|: The item's text value.
-  void AddTextItem(const base::string16& text);
+  // 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).
+  void AddScreenshot(const base::FilePath& screenshot_path,
+                     const gfx::ImageSkia& image);
 
   const HoldingSpaceModel* model_for_testing() const {
     return &holding_space_model_;
   }
 
  private:
+  content::BrowserContext* const browser_context_;
   HoldingSpaceModel holding_space_model_;
 };
 

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

@@ -8,8 +8,14 @@
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/chromeos/file_manager/app_id.h"
+#include "chrome/browser/chromeos/file_manager/fileapi_util.h"
+#include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
@@ -18,9 +24,62 @@
 #include "components/account_id/account_id.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "components/user_manager/scoped_user_manager.h"
+#include "storage/browser/file_system/external_mount_points.h"
+#include "storage/browser/file_system/file_system_context.h"
+#include "storage/browser/file_system/file_system_url.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_unittest_util.h"
 
 namespace ash {
 
+namespace {
+
+const gfx::ImageSkia CreateSolidColorImage(int width,
+                                           int height,
+                                           SkColor color) {
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(width, height);
+  bitmap.eraseColor(color);
+  return gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
+}
+
+// Utility class that registers an external file system mount point, and grants
+// file manager app access permission for the mount point.
+class ScopedExternalMountPoint {
+ public:
+  ScopedExternalMountPoint(Profile* profile, const std::string& name)
+      : name_(name) {
+    if (!temp_dir_.CreateUniqueTempDir())
+      return;
+
+    storage::ExternalMountPoints::GetSystemInstance()->RegisterFileSystem(
+        name_, storage::kFileSystemTypeNativeLocal,
+        storage::FileSystemMountOption(), temp_dir_.GetPath());
+    file_manager::util::GetFileSystemContextForExtensionId(
+        profile, file_manager::kFileManagerAppId)
+        ->external_backend()
+        ->GrantFileAccessToExtension(file_manager::kFileManagerAppId,
+                                     base::FilePath(name_));
+  }
+
+  ~ScopedExternalMountPoint() {
+    storage::ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(name_);
+  }
+
+  bool IsValid() const { return temp_dir_.IsValid(); }
+
+  const base::FilePath& GetRootPath() const { return temp_dir_.GetPath(); }
+
+  const std::string& name() const { return name_; }
+
+ private:
+  base::ScopedTempDir temp_dir_;
+  std::string name_;
+};
+
+}  // namespace
+
 class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
  public:
   HoldingSpaceKeyedServiceTest()
@@ -71,6 +130,57 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
     return ash_test_helper()->test_session_controller_client();
   }
 
+  // Creates a file under path |mount_point_path|/|relative_path| with the
+  // provided content.
+  // Returns the created file's file path, or an empty path on failure.
+  base::FilePath CreateFile(const base::FilePath& mount_point_path,
+                            const base::FilePath& relative_path,
+                            const std::string& content) {
+    const base::FilePath path = mount_point_path.Append(relative_path);
+    if (!base::CreateDirectory(path.DirName()))
+      return base::FilePath();
+    if (!base::WriteFile(path, content))
+      return base::FilePath();
+    return path;
+  }
+
+  // Resolves a file system URL in the file manager's file system context, and
+  // returns the file's virtual path relative to the mount point root.
+  // Returns an empty file if the URL cannot be resolved to a file. For example,
+  // if it's not well formed, or the file manager app cannot access it.
+  base::FilePath GetVirtualPathFromUrl(
+      const GURL& url,
+      const std::string& expected_mount_point) {
+    storage::FileSystemContext* fs_context =
+        file_manager::util::GetFileSystemContextForExtensionId(
+            GetProfile(), file_manager::kFileManagerAppId);
+    storage::FileSystemURL fs_url = fs_context->CrackURL(url);
+
+    base::RunLoop run_loop;
+    base::FilePath result;
+    base::FilePath* result_ptr = &result;
+    fs_context->ResolveURL(
+        fs_url,
+        base::BindLambdaForTesting(
+            [&run_loop, &expected_mount_point, &result_ptr](
+                base::File::Error result, const storage::FileSystemInfo& info,
+                const base::FilePath& file_path,
+                storage::FileSystemContext::ResolvedEntryType type) {
+              EXPECT_EQ(base::File::Error::FILE_OK, result);
+              EXPECT_EQ(storage::FileSystemContext::RESOLVED_ENTRY_FILE, type);
+              if (expected_mount_point == info.name) {
+                *result_ptr = file_path;
+              } else {
+                ADD_FAILURE() << "Mount point name '" << info.name
+                              << "' does not match expected '"
+                              << expected_mount_point << "'";
+              }
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+    return result;
+  }
+
  private:
   chromeos::FakeChromeUserManager* fake_user_manager_;
   user_manager::ScopedUserManager user_manager_enabler_;
@@ -78,7 +188,16 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-TEST_F(HoldingSpaceKeyedServiceTest, AddTextItem) {
+// Tests adding a screenshot item. Verifies that adding a screenshot creates a
+// holding space item with a file system URL that can be accessed by the file
+// manager app.
+TEST_F(HoldingSpaceKeyedServiceTest, AddScreenshotItem) {
+  // Create a test downloads mount point.
+  ScopedExternalMountPoint downloads_mount(
+      GetProfile(),
+      file_manager::util::GetDownloadsMountPointName(GetProfile()));
+  ASSERT_TRUE(downloads_mount.IsValid());
+
   // Verify that the holding space model gets set even if the holding space
   // keyed service is not explicitly created.
   HoldingSpaceModel* const initial_model =
@@ -87,20 +206,58 @@ TEST_F(HoldingSpaceKeyedServiceTest, AddTextItem) {
 
   HoldingSpaceKeyedService* const holding_space_service =
       HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(GetProfile());
-  const base::string16 item_1 = base::ASCIIToUTF16("Test text item");
-  holding_space_service->AddTextItem(item_1);
-  const base::string16 item_2 = base::ASCIIToUTF16("Second test text item");
-  holding_space_service->AddTextItem(item_2);
+  const base::FilePath item_1_virtual_path("Screenshot 1.png");
+  // Create a fake screenshot file on the local file system - later parts of the
+  // test will try to resolve the file's file system URL, which fails if the
+  // file does not exist.
+  const base::FilePath item_1_full_path =
+      CreateFile(downloads_mount.GetRootPath(), item_1_virtual_path, "red");
+  ASSERT_FALSE(item_1_full_path.empty());
+
+  holding_space_service->AddScreenshot(
+      item_1_full_path, CreateSolidColorImage(64, 64, SK_ColorRED));
+
+  const base::FilePath item_2_virtual_path =
+      base::FilePath("Alt/Screenshot 2.png");
+  // Create a fake screenshot file on the local file system - later parts of the
+  // test will try to resolve the file's file system URL, which fails if the
+  // file does not exist.
+  const base::FilePath item_2_full_path =
+      CreateFile(downloads_mount.GetRootPath(), item_2_virtual_path, "blue");
+  ASSERT_FALSE(item_2_full_path.empty());
+  holding_space_service->AddScreenshot(
+      item_2_full_path, CreateSolidColorImage(64, 64, SK_ColorBLUE));
 
   EXPECT_EQ(initial_model, HoldingSpaceController::Get()->model());
   EXPECT_EQ(HoldingSpaceController::Get()->model(),
             holding_space_service->model_for_testing());
 
-  std::vector<base::string16> text_items;
-  for (auto& item : HoldingSpaceController::Get()->model()->items()) {
-    text_items.push_back(item->text().value_or(base::ASCIIToUTF16("null")));
-  }
-  EXPECT_EQ(std::vector<base::string16>({item_1, item_2}), text_items);
+  HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
+  ASSERT_EQ(2u, model->items().size());
+
+  const HoldingSpaceItem* item_1 = model->items()[0].get();
+  EXPECT_EQ(item_1_full_path, item_1->file_path());
+  EXPECT_TRUE(
+      gfx::BitmapsAreEqual(*CreateSolidColorImage(64, 64, SK_ColorRED).bitmap(),
+                           *item_1->image().bitmap()));
+  // Verify the item file system URL resolves to the correct file in the file
+  // manager's context.
+  EXPECT_EQ(
+      item_1_virtual_path,
+      GetVirtualPathFromUrl(item_1->file_system_url(), downloads_mount.name()));
+  EXPECT_EQ(base::ASCIIToUTF16("Screenshot 1.png"), item_1->text());
+
+  const HoldingSpaceItem* item_2 = model->items()[1].get();
+  EXPECT_EQ(item_2_full_path, item_2->file_path());
+  EXPECT_TRUE(gfx::BitmapsAreEqual(
+      *CreateSolidColorImage(64, 64, SK_ColorBLUE).bitmap(),
+      *item_2->image().bitmap()));
+  // Verify the item file system URL resolves to the correct file in the file
+  // manager's context.
+  EXPECT_EQ(
+      item_2_virtual_path,
+      GetVirtualPathFromUrl(item_2->file_system_url(), downloads_mount.name()));
+  EXPECT_EQ(base::ASCIIToUTF16("Screenshot 2.png"), item_2->text());
 }
 
 TEST_F(HoldingSpaceKeyedServiceTest, SecondaryUserProfile) {