Fetch appropriate sized thumbnails for holding space.

Previously thumbnails were fetched at a constant size and exhibited
heavy pixelation when rendering in holding space UI.

Now, we'll request the appropriate thumbnail size for each holding space
type which will ensure consistency with what we intend to render.

Known issue: The image loader we use to fetch thumbnails only supports
cropping of square images. To prevent image distortion for non-square
image requests, we'll ensure that we always request square images and
perform an additional crop on our end.


Bug: 1113772
Change-Id: I43e86b37e31d1dc94fd737bc29021dc68658c8b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2399438
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805423}

ash/public/cpp/BUILD.gn

@@ -135,6 +135,7 @@ component("cpp") {
     "frame_utils.h",
     "gesture_action_type.h",
     "holding_space/holding_space_client.h",
+    "holding_space/holding_space_constants.h",
     "holding_space/holding_space_controller.cc",
     "holding_space/holding_space_controller.h",
     "holding_space/holding_space_controller_observer.h",

ash/public/cpp/holding_space/holding_space_constants.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_CONSTANTS_H_
+#define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_CONSTANTS_H_
+
+#include "ui/gfx/geometry/insets.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace ash {
+
+// UI constants.
+constexpr gfx::Insets kHoldingSpaceContainerPadding(16);
+constexpr int kHoldingSpaceContainerSeparation = 8;
+constexpr gfx::Insets kHoldingSpaceChipPadding(8);
+constexpr int kHoldingSpaceChipChildSpacing = 8;
+constexpr int kHoldingSpaceChipCornerRadius = 8;
+constexpr int kHoldingSpaceChipIconSize = 24;
+constexpr int kHoldingSpaceColumnPadding = 8;
+constexpr int kHoldingSpaceColumnWidth = 160;
+constexpr int kHoldingSpaceRowPadding = 8;
+constexpr gfx::Size kHoldingSpaceScreenshotSize(104, 80);
+
+}  // namespace ash
+
+#endif  // ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_CONSTANTS_H_

ash/public/cpp/holding_space/holding_space_item.cc

@@ -77,8 +77,7 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::Deserialize(
   const base::Optional<int> version = dict.FindIntPath(kVersionPath);
   DCHECK(version.has_value() && version.value() == kVersion);
 
-  const base::Optional<int> type = dict.FindIntPath(kTypePath);
-  DCHECK(type.has_value());
+  const Type type = static_cast<Type>(dict.FindIntPath(kTypePath).value());
 
   const base::Optional<base::FilePath> file_path =
       util::ValueToFilePath(dict.FindPath(kFilePathPath));
@@ -86,10 +85,10 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::Deserialize(
 
   // NOTE: `std::make_unique` does not work with private constructors.
   return base::WrapUnique(new HoldingSpaceItem(
-      static_cast<Type>(type.value()), DeserializeId(dict), file_path.value(),
+      type, DeserializeId(dict), file_path.value(),
       std::move(file_system_url_resolver).Run(file_path.value()),
       file_path->BaseName().LossyDisplayName(),
-      std::move(image_resolver).Run(file_path.value())));
+      std::move(image_resolver).Run(type, file_path.value())));
 }
 
 // static

ash/public/cpp/holding_space/holding_space_item.h

@@ -57,9 +57,9 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
   // Returns a file system URL for a given file path.
   using FileSystemUrlResolver = base::OnceCallback<GURL(const base::FilePath&)>;
 
-  // Returns an image for a given file path.
-  using ImageResolver = base::OnceCallback<std::unique_ptr<HoldingSpaceImage>(
-      const base::FilePath&)>;
+  // Returns an image for a given type and file path.
+  using ImageResolver = base::OnceCallback<
+      std::unique_ptr<HoldingSpaceImage>(Type, const base::FilePath&)>;
 
   // Deserializes from `base::DictionaryValue` to `HoldingSpaceItem`.
   static std::unique_ptr<HoldingSpaceItem> Deserialize(

ash/public/cpp/holding_space/holding_space_item_unittest.cc

@@ -49,7 +49,8 @@ TEST_P(HoldingSpaceItemTest, Serialization) {
       base::BindLambdaForTesting(
           [&](const base::FilePath& file_path) { return file_system_url; }),
       /*image_resolver=*/
-      base::BindLambdaForTesting([&](const base::FilePath& file_path) {
+      base::BindLambdaForTesting([&](HoldingSpaceItem::Type type,
+                                     const base::FilePath& file_path) {
         return std::make_unique<HoldingSpaceImage>(
             placeholder, /*async_bitmap_resolver=*/base::DoNothing());
       }));

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/holding_space_item_chip_view.h"
 
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/shelf_config.h"
@@ -122,7 +123,9 @@ void HoldingSpaceItemChipView::AddPinButton() {
 }
 
 void HoldingSpaceItemChipView::Update() {
-  image_->SetImage(item_->image().image_skia(), {kTrayItemSize, kTrayItemSize});
+  image_->SetImage(
+      item_->image().image_skia(),
+      gfx::Size(kHoldingSpaceChipIconSize, kHoldingSpaceChipIconSize));
 }
 
 BEGIN_METADATA(HoldingSpaceItemChipView, views::InkDropHostView)

ash/system/holding_space/holding_space_item_chips_container.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/holding_space_item_chips_container.h"
 
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/system/holding_space/holding_space_item_chip_view.h"
 #include "ash/system/tray/tray_constants.h"
@@ -31,9 +32,8 @@ const char* HoldingSpaceItemChipsContainer::GetClassName() const {
 }
 
 void HoldingSpaceItemChipsContainer::AddItemChip(const HoldingSpaceItem* item) {
-  if ((children().size() % 2) == 0) {
+  if ((children().size() % 2) == 0)
     layout_->StartRowWithPadding(0, 0, 0, kHoldingSpaceRowPadding);
-  }
   layout_->AddView(std::make_unique<HoldingSpaceItemChipView>(item));
 }
 

ash/system/holding_space/holding_space_screenshot_view.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/holding_space_screenshot_view.h"
 
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/system/tray/tray_constants.h"

ash/system/holding_space/holding_space_tray.cc

@@ -6,6 +6,7 @@
 #include <memory>
 
 #include "ash/accessibility/accessibility_controller_impl.h"
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/public/cpp/system_tray_client.h"
 #include "ash/resources/vector_icons/vector_icons.h"

ash/system/holding_space/pinned_files_container.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/pinned_files_container.h"
 
+#include "ash/public/cpp/holding_space/holding_space_constants.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"

ash/system/holding_space/recent_files_container.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/recent_files_container.h"
 
+#include "ash/public/cpp/holding_space/holding_space_constants.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"
@@ -56,6 +57,7 @@ RecentFilesContainer::RecentFilesContainer() {
 
   recent_downloads_container_ =
       AddChildView(std::make_unique<HoldingSpaceItemChipsContainer>());
+
   // TODO(crbug.com/1125254): Populate containers if and when holding space
   // model is attached, below is a temporary solution.
   for (const auto& item : HoldingSpaceController::Get()->model()->items()) {

ash/system/tray/tray_constants.h

@@ -235,17 +235,6 @@ constexpr int kPrivacyScreenToastSubLabelFontSize = 13;
 constexpr gfx::Insets kPrivacyScreenToastInsets(10, 16);
 constexpr int kPrivacyScreenToastSpacing = 16;
 
-// constants used for holding space tray.
-constexpr gfx::Insets kHoldingSpaceContainerPadding = gfx::Insets(16);
-constexpr gfx::Insets kHoldingSpaceChipPadding = gfx::Insets(8);
-constexpr int kHoldingSpaceChipChildSpacing = 8;
-constexpr int kHoldingSpaceChipCornerRadius = 8;
-constexpr int kHoldingSpaceContainerSeparation = 8;
-constexpr int kHoldingSpaceColumnWidth = 160;
-constexpr int kHoldingSpaceColumnPadding = 8;
-constexpr gfx::Size kHoldingSpaceScreenshotSize(104, 80);
-constexpr int kHoldingSpaceRowPadding = 8;
-
 // Constants used for media tray.
 constexpr int kMediaTrayPadding = 8;
 

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

@@ -61,6 +61,7 @@ void HoldingSpaceKeyedService::AddPinnedFile(
       HoldingSpaceItem::Type::kPinnedFile, file_system_url.path(),
       file_system_url.ToGURL(),
       holding_space_util::ResolveImage(&thumbnail_loader_,
+                                       HoldingSpaceItem::Type::kPinnedFile,
                                        file_system_url.path())));
 }
 
@@ -94,7 +95,9 @@ void HoldingSpaceKeyedService::AddScreenshot(
 
   AddItem(HoldingSpaceItem::CreateFileBackedItem(
       HoldingSpaceItem::Type::kScreenshot, screenshot_file, file_system_url,
-      holding_space_util::ResolveImage(&thumbnail_loader_, screenshot_file)));
+      holding_space_util::ResolveImage(&thumbnail_loader_,
+                                       HoldingSpaceItem::Type::kScreenshot,
+                                       screenshot_file)));
 }
 
 void HoldingSpaceKeyedService::AddDownload(
@@ -106,7 +109,9 @@ void HoldingSpaceKeyedService::AddDownload(
 
   AddItem(HoldingSpaceItem::CreateFileBackedItem(
       HoldingSpaceItem::Type::kDownload, download_file, file_system_url,
-      holding_space_util::ResolveImage(&thumbnail_loader_, download_file)));
+      holding_space_util::ResolveImage(&thumbnail_loader_,
+                                       HoldingSpaceItem::Type::kDownload,
+                                       download_file)));
 }
 
 void HoldingSpaceKeyedService::AddItem(std::unique_ptr<HoldingSpaceItem> item) {

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

@@ -350,7 +350,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, AddScreenshotItem) {
   EXPECT_TRUE(gfx::BitmapsAreEqual(
       *holding_space_util::ResolveImage(
            holding_space_service->thumbnail_loader_for_testing(),
-           item_1_full_path)
+           HoldingSpaceItem::Type::kScreenshot, item_1_full_path)
            ->image_skia()
            .bitmap(),
       *item_1->image().image_skia().bitmap()));
@@ -366,7 +366,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, AddScreenshotItem) {
   EXPECT_TRUE(gfx::BitmapsAreEqual(
       *holding_space_util::ResolveImage(
            holding_space_service->thumbnail_loader_for_testing(),
-           item_2_full_path)
+           HoldingSpaceItem::Type::kScreenshot, item_2_full_path)
            ->image_skia()
            .bitmap(),
       *item_2->image().image_skia().bitmap()));
@@ -426,7 +426,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, UpdatePersistentStorage) {
     auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
         type, file_path, file_system_url,
         holding_space_util::ResolveImage(
-            primary_holding_space_service->thumbnail_loader_for_testing(),
+            primary_holding_space_service->thumbnail_loader_for_testing(), type,
             file_path));
 
     // We do not persist `kDownload` type items.
@@ -492,7 +492,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, RestorePersistentStorage) {
                   holding_space_util::ResolveImage(
                       primary_holding_space_service
                           ->thumbnail_loader_for_testing(),
-                      file));
+                      type, file));
 
           persisted_holding_space_items_before_restoration->Append(
               fresh_holding_space_item->Serialize());

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

@@ -29,7 +29,10 @@
 #include "services/data_decoder/public/cpp/data_decoder.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "storage/browser/file_system/file_system_context.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/skia_util.h"
 #include "url/gurl.h"
 
 namespace ash {
@@ -236,6 +239,12 @@ void HoldingSpaceThumbnailLoader::LoadForFileWithMetadata(
   base::UnguessableToken request_id = base::UnguessableToken::Create();
   requests_[request_id] = std::move(callback);
 
+  // Unfortunately the image loader only supports cropping to square dimensions
+  // but a request for a non-cropped, non-square image would result in image
+  // distortion. To work around this we always request square images and then
+  // crop to requested dimensions on our end if necessary after bitmap decoding.
+  const int size = std::max(request.size.width(), request.size.height());
+
   // Generate an image loader request. The request type is defined in
   // ui/file_manager/image_loader/load_image_request.js.
   base::Value request_value(base::Value::Type::DICTIONARY);
@@ -245,8 +254,8 @@ void HoldingSpaceThumbnailLoader::LoadForFileWithMetadata(
   request_value.SetBoolKey("cache", true);
   request_value.SetBoolKey("crop", true);
   request_value.SetKey("priority", base::Value(1));
-  request_value.SetKey("width", base::Value(request.size.width()));
-  request_value.SetKey("height", base::Value(request.size.height()));
+  request_value.SetKey("width", base::Value(size));
+  request_value.SetKey("height", base::Value(size));
 
   std::string request_message;
   base::JSONWriter::Write(request_value, &request_message);
@@ -256,7 +265,7 @@ void HoldingSpaceThumbnailLoader::LoadForFileWithMetadata(
   auto native_message_host = std::make_unique<ThumbnailLoaderNativeMessageHost>(
       request_id.ToString(), request_message,
       base::BindOnce(&HoldingSpaceThumbnailLoader::OnThumbnailLoaded,
-                     weak_factory_.GetWeakPtr(), request_id));
+                     weak_factory_.GetWeakPtr(), request_id, request.size));
   const extensions::PortId port_id(base::UnguessableToken::Create(),
                                    1 /* port_number */, true /* is_opener */);
   extensions::MessageService* const message_service =
@@ -273,12 +282,13 @@ void HoldingSpaceThumbnailLoader::LoadForFileWithMetadata(
 
 void HoldingSpaceThumbnailLoader::OnThumbnailLoaded(
     const base::UnguessableToken& request_id,
+    const gfx::Size& requested_size,
     const std::string& data) {
   if (!requests_.count(request_id))
     return;
 
   if (data.empty()) {
-    RespondToRequest(request_id, nullptr);
+    RespondToRequest(request_id, requested_size, nullptr);
     return;
   }
 
@@ -286,21 +296,36 @@ void HoldingSpaceThumbnailLoader::OnThumbnailLoaded(
   ThumbnailDecoder* thumbnail_decoder_ptr = thumbnail_decoder.get();
   thumbnail_decoders_.emplace(request_id, std::move(thumbnail_decoder));
   thumbnail_decoder_ptr->Start(
-      data, base::BindOnce(&HoldingSpaceThumbnailLoader::RespondToRequest,
-                           weak_factory_.GetWeakPtr(), request_id));
+      data,
+      base::BindOnce(&HoldingSpaceThumbnailLoader::RespondToRequest,
+                     weak_factory_.GetWeakPtr(), request_id, requested_size));
 }
 
 void HoldingSpaceThumbnailLoader::RespondToRequest(
     const base::UnguessableToken& request_id,
+    const gfx::Size& requested_size,
     const SkBitmap* bitmap) {
   thumbnail_decoders_.erase(request_id);
   auto request_it = requests_.find(request_id);
   if (request_it == requests_.end())
     return;
 
+  // To work around cropping limitations of the image loader, we requested a
+  // square image. If requested dimensions were non-square, we need to perform
+  // additional cropping on our end.
+  SkBitmap cropped_bitmap;
+  if (bitmap) {
+    gfx::Rect cropped_rect(0, 0, bitmap->width(), bitmap->height());
+    if (cropped_rect.size() != requested_size) {
+      cropped_bitmap = *bitmap;
+      cropped_rect.ClampToCenteredSize(requested_size);
+      bitmap->extractSubset(&cropped_bitmap, gfx::RectToSkIRect(cropped_rect));
+    }
+  }
+
   ImageCallback callback = std::move(request_it->second);
   requests_.erase(request_it);
-  std::move(callback).Run(bitmap);
+  std::move(callback).Run(cropped_bitmap.isNull() ? bitmap : &cropped_bitmap);
 }
 
 }  // namespace ash

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

@@ -69,14 +69,18 @@ class HoldingSpaceThumbnailLoader {
 
   // Callback to the image loader request.
   // `request_id` identifies the thumbnail request.
-  // `data` - Image data returned by the image loader. Expected to be in a data
+  // `requested_size` - size of the thumbnail that was requested.
+  // `data` - image data returned by the image loader. Expected to be in a data
   // URL form. It will attempt to decode the received data.
   void OnThumbnailLoaded(const base::UnguessableToken& request_id,
+                         const gfx::Size& requested_size,
                          const std::string& data);
 
   // Finalizes the thumbnail request identified by `request_id`. It invokes the
-  // request callback with `bitmap`.
+  // request callback with `bitmap`. If `bitmap` size is larger than the
+  // originally `requested_size`, the bitmap will be cropped.
   void RespondToRequest(const base::UnguessableToken& request_id,
+                        const gfx::Size& requested_size,
                         const SkBitmap* bitmap);
 
   Profile* const profile_;

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

@@ -7,19 +7,50 @@
 #include <map>
 
 #include "ash/public/cpp/file_icon_util.h"
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
-#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/barrier_closure.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_thumbnail_loader.h"
 #include "storage/browser/file_system/file_system_context.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_skia_operations.h"
 #include "url/gurl.h"
 
 namespace ash {
 namespace holding_space_util {
 
+namespace {
+
+// Helpers ---------------------------------------------------------------------
+
+gfx::ImageSkia GetPlaceholderImage(HoldingSpaceItem::Type type,
+                                   const base::FilePath& file_path) {
+  gfx::Size size;
+  switch (type) {
+    case HoldingSpaceItem::Type::kDownload:
+    case HoldingSpaceItem::Type::kPinnedFile:
+      size = gfx::Size(kHoldingSpaceChipIconSize, kHoldingSpaceChipIconSize);
+      break;
+    case HoldingSpaceItem::Type::kScreenshot:
+      size = kHoldingSpaceScreenshotSize;
+      break;
+  }
+
+  // NOTE: We superimpose the file type icon for `file_path` over a transparent
+  // bitmap in order to center it within the placeholder image at a fixed size.
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(size.width(), size.height());
+  return gfx::ImageSkiaOperations::CreateSuperimposedImage(
+      gfx::ImageSkia::CreateFrom1xBitmap(bitmap), GetIconForPath(file_path));
+}
+
+}  // namespace
+
+// Utilities -------------------------------------------------------------------
+
 void ItemExists(Profile* profile,
                 const HoldingSpaceItem* item,
                 ItemExistsCallback callback) {
@@ -124,12 +155,12 @@ GURL ResolveFileSystemUrl(Profile* profile, const base::FilePath& file_path) {
   return file_system_url;
 }
 
-// TODO(dmblack): Handle different sizes for different holding space item types.
 std::unique_ptr<HoldingSpaceImage> ResolveImage(
     HoldingSpaceThumbnailLoader* thumbnail_loader,
+    HoldingSpaceItem::Type type,
     const base::FilePath& file_path) {
   return std::make_unique<HoldingSpaceImage>(
-      /*placeholder=*/GetIconForPath(file_path),
+      GetPlaceholderImage(type, file_path),
       base::BindRepeating(
           [](const base::WeakPtr<HoldingSpaceThumbnailLoader>& thumbnail_loader,
              const base::FilePath& file_path, const gfx::Size& size,

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

@@ -8,6 +8,7 @@
 #include <memory>
 #include <vector>
 
+#include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/callback_forward.h"
 
 class GURL;
@@ -20,7 +21,6 @@ class FilePath;
 namespace ash {
 
 class HoldingSpaceImage;
-class HoldingSpaceItem;
 class HoldingSpaceThumbnailLoader;
 
 using HoldingSpaceItemPtr = std::unique_ptr<HoldingSpaceItem>;
@@ -50,6 +50,7 @@ GURL ResolveFileSystemUrl(Profile* profile, const base::FilePath& file_path);
 // Resolves the image associated with the specified `file_path`.
 std::unique_ptr<HoldingSpaceImage> ResolveImage(
     HoldingSpaceThumbnailLoader* thumbnail_loader,
+    HoldingSpaceItem::Type type,
     const base::FilePath& file_path);
 
 }  // namespace holding_space_util