Use correct icon for folder in holding space

The thubmail loader is already checking whether the holding space item
is a folder, but it used to return an empty bitmap in this case (relying
on the placeholder icon creation to generate the icon; which relies
purely on the file path, and may thus generate an incorrect icon -
currently it generates a generic file icon; with this change this will
be WAI).
Instead, create the correct folder icon in the holding space thumbnail
loader.

BUG=1141034

Change-Id: I7d619adc892d2595284df210cefcab44f9279f43
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552370Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829966}

ash/public/cpp/holding_space/holding_space_image.cc

@@ -37,7 +37,7 @@ class HoldingSpaceImage::ImageSkiaSource : public gfx::ImageSkiaSource {
 
     // When missing the cache, asynchronously resolve the bitmap for `scale`.
     async_bitmap_resolver_.Run(
-        gfx::ScaleToCeiledSize(placeholder_.size(), scale),
+        gfx::ScaleToCeiledSize(placeholder_.size(), scale), scale,
         base::BindOnce(&ImageSkiaSource::CacheImageForScale,
                        weak_factory_.GetWeakPtr(), scale));
 

ash/public/cpp/holding_space/holding_space_image.h

@@ -24,8 +24,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
   using BitmapCallback = base::OnceCallback<void(const SkBitmap*)>;
 
   // Returns a bitmap asynchronously for a given size.
-  using AsyncBitmapResolver =
-      base::RepeatingCallback<void(const gfx::Size&, BitmapCallback)>;
+  using AsyncBitmapResolver = base::RepeatingCallback<
+      void(const gfx::Size&, float scale_factor, BitmapCallback)>;
 
   HoldingSpaceImage(const gfx::ImageSkia& placeholder,
                     AsyncBitmapResolver async_bitmap_resolver);

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

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/ash/holding_space/holding_space_thumbnail_loader.h"
 
+#include "ash/public/cpp/holding_space/holding_space_color_provider.h"
 #include "ash/public/cpp/image_downloader.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ -20,6 +21,8 @@
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/extensions/api/messaging/native_message_port.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
+#include "chromeos/ui/vector_icons/vector_icons.h"
 #include "extensions/browser/api/messaging/channel_endpoint.h"
 #include "extensions/browser/api/messaging/message_service.h"
 #include "extensions/browser/api/messaging/native_message_host.h"
@@ -32,6 +35,7 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/skia_util.h"
 #include "url/gurl.h"
 
@@ -44,6 +48,9 @@ namespace {
 constexpr char kNativeMessageHostName[] =
     "com.google.holding_space_thumbnail_loader";
 
+// The DIP size for the folder icon.
+const size_t kFolderIconDipSize = 20;
+
 using ThumbnailDataCallback = base::OnceCallback<void(const std::string& data)>;
 
 // Handles a parsed message sent from image loader extension in response to a
@@ -189,8 +196,9 @@ HoldingSpaceThumbnailLoader::~HoldingSpaceThumbnailLoader() = default;
 
 HoldingSpaceThumbnailLoader::ThumbnailRequest::ThumbnailRequest(
     const base::FilePath& item_path,
-    const gfx::Size& size)
-    : item_path(item_path), size(size) {}
+    const gfx::Size& size,
+    float scale_factor)
+    : item_path(item_path), size(size), scale_factor(scale_factor) {}
 
 HoldingSpaceThumbnailLoader::ThumbnailRequest::~ThumbnailRequest() = default;
 
@@ -223,8 +231,16 @@ void HoldingSpaceThumbnailLoader::LoadForFileWithMetadata(
     return;
   }
 
+  // Short-circuit icons for folders.
   if (file_info.is_directory) {
-    std::move(callback).Run(nullptr);
+    std::move(callback).Run(
+        holding_space_util::CreatePlaceholderImage(
+            gfx::CreateVectorIcon(
+                chromeos::kFiletypeFolderIcon,
+                request.scale_factor * kFolderIconDipSize,
+                HoldingSpaceColorProvider::Get()->GetFileIconColor()),
+            request.size)
+            .bitmap());
     return;
   }
 

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

@@ -38,7 +38,9 @@ class HoldingSpaceThumbnailLoader {
 
   // Thumbnail request data that will be forwarded to the image loader.
   struct ThumbnailRequest {
-    ThumbnailRequest(const base::FilePath& item_path, const gfx::Size& size);
+    ThumbnailRequest(const base::FilePath& item_path,
+                     const gfx::Size& size,
+                     float scale_factor);
     ~ThumbnailRequest();
 
     // The absolute item file path.
@@ -46,6 +48,9 @@ class HoldingSpaceThumbnailLoader {
 
     // The desired bitmap size.
     const gfx::Size size;
+
+    // The scale factor for which the bitmap is being generated.
+    float scale_factor;
   };
 
   // Returns a weak pointer to this instance.

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

@@ -166,7 +166,8 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadNonExistentItem) {
 
   base::RunLoop run_loop;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kNonExistent), gfx::Size(48, 48));
+      GetTestPath(TestPath::kNonExistent), gfx::Size(48, 48),
+      /*scale_factor=*/1.0f);
   loader->Load(request,
                base::BindLambdaForTesting([&run_loop](const SkBitmap* bitmap) {
                  EXPECT_FALSE(bitmap);
@@ -180,14 +181,33 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadFolder) {
   ASSERT_TRUE(loader);
 
   base::RunLoop run_loop;
+  SkBitmap bitmap;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kEmptyDir), gfx::Size(48, 48));
-  loader->Load(request,
-               base::BindLambdaForTesting([&run_loop](const SkBitmap* bitmap) {
-                 EXPECT_FALSE(bitmap);
-                 run_loop.Quit();
-               }));
+      GetTestPath(TestPath::kEmptyDir), gfx::Size(48, 48),
+      /*scale_factor=*/1.0f);
+  loader->Load(request, base::BindOnce(&CopyBitmapAndRunClosure,
+                                       run_loop.QuitClosure(), &bitmap));
   run_loop.Run();
+  EXPECT_FALSE(bitmap.isNull());
+  EXPECT_EQ(48, bitmap.width());
+  EXPECT_EQ(48, bitmap.height());
+}
+
+IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadFolderScale2) {
+  ash::HoldingSpaceThumbnailLoader* loader = GetThumbnailLoader();
+  ASSERT_TRUE(loader);
+
+  base::RunLoop run_loop;
+  SkBitmap bitmap;
+  ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
+      GetTestPath(TestPath::kEmptyDir), gfx::Size(48, 48),
+      /*scale_factor=*/2.0f);
+  loader->Load(request, base::BindOnce(&CopyBitmapAndRunClosure,
+                                       run_loop.QuitClosure(), &bitmap));
+  run_loop.Run();
+  EXPECT_FALSE(bitmap.isNull());
+  EXPECT_EQ(48, bitmap.width());
+  EXPECT_EQ(48, bitmap.height());
 }
 
 IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadJpg) {
@@ -197,7 +217,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadJpg) {
   SkBitmap bitmap;
   base::RunLoop run_loop;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kJpg), gfx::Size(48, 48));
+      GetTestPath(TestPath::kJpg), gfx::Size(48, 48), /*scale_factor=*/1.0f);
   loader->Load(request, base::BindOnce(&CopyBitmapAndRunClosure,
                                        run_loop.QuitClosure(), &bitmap));
   run_loop.Run();
@@ -213,7 +233,8 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadBrokenJpg) {
 
   base::RunLoop run_loop;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kBrokenJpg), gfx::Size(48, 48));
+      GetTestPath(TestPath::kBrokenJpg), gfx::Size(48, 48),
+      /*scale_factor=*/1.0f);
   loader->Load(request,
                base::BindLambdaForTesting([&run_loop](const SkBitmap* bitmap) {
                  EXPECT_FALSE(bitmap);
@@ -229,7 +250,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, LoadPng) {
   SkBitmap bitmap;
   base::RunLoop run_loop;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kPng), gfx::Size(48, 48));
+      GetTestPath(TestPath::kPng), gfx::Size(48, 48), /*scale_factor=*/1.0f);
   loader->Load(request, base::BindOnce(&CopyBitmapAndRunClosure,
                                        run_loop.QuitClosure(), &bitmap));
   run_loop.Run();
@@ -243,7 +264,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, RepeatedLoads) {
   ASSERT_TRUE(loader);
 
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request(
-      GetTestPath(TestPath::kPng), gfx::Size(48, 48));
+      GetTestPath(TestPath::kPng), gfx::Size(48, 48), /*scale_factor=*/1.0f);
 
   SkBitmap bitmap1;
   base::RunLoop run_loop1;
@@ -284,14 +305,14 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, ConcurentLoads) {
 
   SkBitmap bitmap1;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request1(
-      GetTestPath(TestPath::kPng), gfx::Size(48, 48));
+      GetTestPath(TestPath::kPng), gfx::Size(48, 48), /*scale_factor=*/1.0f);
   base::RunLoop run_loop1;
   loader->Load(request1, base::BindOnce(&CopyBitmapAndRunClosure,
                                         run_loop1.QuitClosure(), &bitmap1));
 
   SkBitmap bitmap2;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request2(
-      GetTestPath(TestPath::kPng), gfx::Size(96, 96));
+      GetTestPath(TestPath::kPng), gfx::Size(96, 96), /*scale_factor=*/2.0f);
   base::RunLoop run_loop2;
   loader->Load(request2, base::BindOnce(&CopyBitmapAndRunClosure,
                                         run_loop2.QuitClosure(), &bitmap2));
@@ -299,7 +320,7 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceThumbnailLoaderTest, ConcurentLoads) {
   SkBitmap bitmap3;
   base::RunLoop run_loop3;
   ash::HoldingSpaceThumbnailLoader::ThumbnailRequest request3(
-      GetTestPath(TestPath::kJpg), gfx::Size(48, 48));
+      GetTestPath(TestPath::kJpg), gfx::Size(48, 48), /*scale_factor=*/1.0f);
   loader->Load(request3, base::BindOnce(&CopyBitmapAndRunClosure,
                                         run_loop3.QuitClosure(), &bitmap3));
 

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

@@ -43,14 +43,7 @@ gfx::ImageSkia GetPlaceholderImage(HoldingSpaceItem::Type type,
   }
 
   const SkColor color = HoldingSpaceColorProvider::Get()->GetFileIconColor();
-
-  // 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, color));
+  return CreatePlaceholderImage(GetIconForPath(file_path, color), size);
 }
 
 }  // namespace
@@ -190,13 +183,24 @@ std::unique_ptr<HoldingSpaceImage> ResolveImage(
       base::BindRepeating(
           [](const base::WeakPtr<HoldingSpaceThumbnailLoader>& thumbnail_loader,
              const base::FilePath& file_path, const gfx::Size& size,
-             HoldingSpaceImage::BitmapCallback callback) {
+             float scale_factor, HoldingSpaceImage::BitmapCallback callback) {
             if (thumbnail_loader)
-              thumbnail_loader->Load({file_path, size}, std::move(callback));
+              thumbnail_loader->Load({file_path, size, scale_factor},
+                                     std::move(callback));
           },
           thumbnail_loader->GetWeakPtr(), file_path));
 }
 
+gfx::ImageSkia CreatePlaceholderImage(const gfx::ImageSkia& file_type_image,
+                                      const gfx::Size& size) {
+  // 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), file_type_image);
+}
+
 void SetNowForTesting(base::Optional<base::Time> now) {
   now_for_testing = now;
 }

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

@@ -19,6 +19,11 @@ namespace base {
 class FilePath;
 }  // namespace base
 
+namespace gfx {
+class ImageSkia;
+class Size;
+}  // namespace gfx
+
 namespace ash {
 
 class HoldingSpaceImage;
@@ -74,6 +79,11 @@ std::unique_ptr<HoldingSpaceImage> ResolveImage(
     HoldingSpaceItem::Type type,
     const base::FilePath& file_path);
 
+// Given a base image for a file type, returns a placeholder image to be used in
+// the holding space UI.
+gfx::ImageSkia CreatePlaceholderImage(const gfx::ImageSkia& file_type_image,
+                                      const gfx::Size& size);
+
 void SetNowForTesting(base::Optional<base::Time> now);
 
 }  // namespace holding_space_util