WebApp: Redesign WebAppIconManager read icon methods API.

All async WebAppIconManager read icon methods must return void.

A call site should check if an app has appropriate icon
via corresponding Has*Icon methods.

These changes are required for integration with AppService.

This code is disabled by default behind kDesktopPWAsWithoutExtensions
base feature.

Bug: 1029221
Change-Id: Iad46ea383f062d0b3787e11d20b5f89a08eb61f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1943650Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720123}

chrome/browser/ui/web_applications/web_app_browser_controller.cc

@@ -49,10 +49,12 @@ gfx::ImageSkia WebAppBrowserController::GetWindowAppIcon() const {
     return *app_icon_;
   app_icon_ = GetFallbackAppIcon();
 
-  provider_.icon_manager().ReadSmallestIcon(
-      GetAppId(), gfx::kFaviconSize,
-      base::BindOnce(&WebAppBrowserController::OnReadIcon,
-                     weak_ptr_factory_.GetWeakPtr()));
+  if (provider_.icon_manager().HasSmallestIcon(GetAppId(), gfx::kFaviconSize)) {
+    provider_.icon_manager().ReadSmallestIcon(
+        GetAppId(), gfx::kFaviconSize,
+        base::BindOnce(&WebAppBrowserController::OnReadIcon,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
 
   return *app_icon_;
 }

chrome/browser/web_applications/components/app_icon_manager.h

@@ -22,26 +22,30 @@ class AppIconManager {
   AppIconManager();
   virtual ~AppIconManager();
 
-  // Reads icon's bitmap for an app. Returns false if no IconInfo for
-  // |icon_size_in_px|. Returns empty SkBitmap in |callback| if IO error.
+  // Returns false if no downloaded icon with precise |icon_size_in_px|.
+  virtual bool HasIcon(const AppId& app_id, int icon_size_in_px) const = 0;
+  // Returns false if no downloaded icon matching |icon_size_in_px|.
+  virtual bool HasSmallestIcon(const AppId& app_id,
+                               int icon_size_in_px) const = 0;
+
+  // Reads icon's bitmap for an app. Returns empty SkBitmap in |callback| if IO
+  // error.
   using ReadIconCallback = base::OnceCallback<void(const SkBitmap&)>;
-  virtual bool ReadIcon(const AppId& app_id,
+  virtual void ReadIcon(const AppId& app_id,
                         int icon_size_in_px,
                         ReadIconCallback callback) const = 0;
 
-  // Reads smallest icon with size at least |icon_size_in_px|.
-  // Returns false if there is no such icon.
-  // Returns empty SkBitmap in |callback| if IO error.
-  virtual bool ReadSmallestIcon(const AppId& app_id,
+  // Reads smallest icon with size at least |icon_size_in_px|. Returns empty
+  // SkBitmap in |callback| if IO error.
+  virtual void ReadSmallestIcon(const AppId& app_id,
                                 int icon_size_in_px,
                                 ReadIconCallback callback) const = 0;
 
   // Reads smallest icon, compressed as PNG with size at least
-  // |icon_size_in_px|. Returns false if there is no such icon. Returns empty
-  // |data| in |callback| if IO error.
+  // |icon_size_in_px|. Returns empty |data| in |callback| if IO error.
   using ReadCompressedIconCallback =
       base::OnceCallback<void(std::vector<uint8_t> data)>;
-  virtual bool ReadSmallestCompressedIcon(
+  virtual void ReadSmallestCompressedIcon(
       const AppId& app_id,
       int icon_size_in_px,
       ReadCompressedIconCallback callback) const = 0;

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.cc

@@ -23,17 +23,21 @@ void OnExtensionIconLoaded(BookmarkAppIconManager::ReadIconCallback callback,
   std::move(callback).Run(image.IsEmpty() ? SkBitmap() : *image.ToSkBitmap());
 }
 
-bool ReadExtensionIcon(Profile* profile,
+const Extension* GetBookmarkApp(Profile* profile,
+                                const web_app::AppId& app_id) {
+  const Extension* extension =
+      ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(app_id);
+  return (extension && extension->from_bookmark()) ? extension : nullptr;
+}
+
+void ReadExtensionIcon(Profile* profile,
                        const web_app::AppId& app_id,
                        int icon_size_in_px,
                        ExtensionIconSet::MatchType match_type,
                        BookmarkAppIconManager::ReadIconCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  const Extension* extension =
-      ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(app_id);
-  if (!extension)
-    return false;
-  DCHECK(extension->from_bookmark());
+  const Extension* extension = GetBookmarkApp(profile, app_id);
+  DCHECK(extension);
 
   ImageLoader* loader = ImageLoader::Get(profile);
   loader->LoadImageAsync(
@@ -41,7 +45,6 @@ bool ReadExtensionIcon(Profile* profile,
       IconsInfo::GetIconResource(extension, icon_size_in_px, match_type),
       gfx::Size(icon_size_in_px, icon_size_in_px),
       base::BindOnce(&OnExtensionIconLoaded, std::move(callback)));
-  return true;
 }
 
 }  // anonymous namespace
@@ -51,27 +54,39 @@ BookmarkAppIconManager::BookmarkAppIconManager(Profile* profile)
 
 BookmarkAppIconManager::~BookmarkAppIconManager() = default;
 
-bool BookmarkAppIconManager::ReadIcon(const web_app::AppId& app_id,
+bool BookmarkAppIconManager::HasIcon(const web_app::AppId& app_id,
+                                     int icon_size_in_px) const {
+  return GetBookmarkApp(profile_, app_id);
+}
+
+bool BookmarkAppIconManager::HasSmallestIcon(const web_app::AppId& app_id,
+                                             int icon_size_in_px) const {
+  return GetBookmarkApp(profile_, app_id);
+}
+
+void BookmarkAppIconManager::ReadIcon(const web_app::AppId& app_id,
                                       int icon_size_in_px,
                                       ReadIconCallback callback) const {
-  return ReadExtensionIcon(profile_, app_id, icon_size_in_px,
-                           ExtensionIconSet::MATCH_EXACTLY,
-                           std::move(callback));
+  DCHECK(HasIcon(app_id, icon_size_in_px));
+  ReadExtensionIcon(profile_, app_id, icon_size_in_px,
+                    ExtensionIconSet::MATCH_EXACTLY, std::move(callback));
 }
 
-bool BookmarkAppIconManager::ReadSmallestIcon(const web_app::AppId& app_id,
+void BookmarkAppIconManager::ReadSmallestIcon(const web_app::AppId& app_id,
                                               int icon_size_in_px,
                                               ReadIconCallback callback) const {
-  return ReadExtensionIcon(profile_, app_id, icon_size_in_px,
-                           ExtensionIconSet::MATCH_BIGGER, std::move(callback));
+  DCHECK(HasSmallestIcon(app_id, icon_size_in_px));
+  ReadExtensionIcon(profile_, app_id, icon_size_in_px,
+                    ExtensionIconSet::MATCH_BIGGER, std::move(callback));
 }
 
-bool BookmarkAppIconManager::ReadSmallestCompressedIcon(
+void BookmarkAppIconManager::ReadSmallestCompressedIcon(
     const web_app::AppId& app_id,
     int icon_size_in_px,
     ReadCompressedIconCallback callback) const {
   NOTIMPLEMENTED();
-  return false;
+  DCHECK(HasSmallestIcon(app_id, icon_size_in_px));
+  std::move(callback).Run(std::vector<uint8_t>());
 }
 
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.h

@@ -20,13 +20,17 @@ class BookmarkAppIconManager : public web_app::AppIconManager {
   ~BookmarkAppIconManager() override;
 
   // AppIconManager:
-  bool ReadIcon(const web_app::AppId& app_id,
+  bool HasIcon(const web_app::AppId& app_id,
+               int icon_size_in_px) const override;
+  bool HasSmallestIcon(const web_app::AppId& app_id,
+                       int icon_size_in_px) const override;
+  void ReadIcon(const web_app::AppId& app_id,
                 int icon_size_in_px,
                 ReadIconCallback callback) const override;
-  bool ReadSmallestIcon(const web_app::AppId& app_id,
+  void ReadSmallestIcon(const web_app::AppId& app_id,
                         int icon_size_in_px,
                         ReadIconCallback callback) const override;
-  bool ReadSmallestCompressedIcon(
+  void ReadSmallestCompressedIcon(
       const web_app::AppId& app_id,
       int icon_size_in_px,
       ReadCompressedIconCallback callback) const override;

chrome/browser/web_applications/web_app_icon_manager.cc

@@ -258,55 +258,64 @@ void WebAppIconManager::DeleteData(AppId app_id, WriteDataCallback callback) {
       std::move(callback));
 }
 
-bool WebAppIconManager::ReadIcon(const AppId& app_id,
-                                 int icon_size_in_px,
-                                 ReadIconCallback callback) const {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
+bool WebAppIconManager::HasIcon(const AppId& app_id,
+                                int icon_size_in_px) const {
   const WebApp* web_app = registrar_.GetAppById(app_id);
   if (!web_app)
     return false;
 
   for (const WebApp::IconInfo& icon_info : web_app->icons()) {
-    if (icon_info.size_in_px == icon_size_in_px) {
-      ReadIconInternal(app_id, icon_size_in_px, std::move(callback));
+    if (icon_info.size_in_px == icon_size_in_px)
       return true;
-    }
   }
 
   return false;
 }
 
-bool WebAppIconManager::ReadSmallestIcon(const AppId& app_id,
+bool WebAppIconManager::HasSmallestIcon(const AppId& app_id,
+                                        int icon_size_in_px) const {
+  int best_size_in_px = 0;
+  return FindBestSizeInPx(app_id, icon_size_in_px, &best_size_in_px);
+}
+
+void WebAppIconManager::ReadIcon(const AppId& app_id,
+                                 int icon_size_in_px,
+                                 ReadIconCallback callback) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(HasIcon(app_id, icon_size_in_px));
+
+  ReadIconInternal(app_id, icon_size_in_px, std::move(callback));
+}
+
+void WebAppIconManager::ReadSmallestIcon(const AppId& app_id,
                                          int icon_size_in_px,
                                          ReadIconCallback callback) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   int best_size_in_px = 0;
-  if (!FindBestSizeInPx(app_id, icon_size_in_px, &best_size_in_px))
-    return false;
+  bool has_smallest_icon =
+      FindBestSizeInPx(app_id, icon_size_in_px, &best_size_in_px);
+  DCHECK(has_smallest_icon);
 
   ReadIconInternal(app_id, best_size_in_px, std::move(callback));
-  return true;
 }
 
-bool WebAppIconManager::ReadSmallestCompressedIcon(
+void WebAppIconManager::ReadSmallestCompressedIcon(
     const AppId& app_id,
     int icon_size_in_px,
     ReadCompressedIconCallback callback) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   int best_size_in_px = 0;
-  if (!FindBestSizeInPx(app_id, icon_size_in_px, &best_size_in_px))
-    return false;
+  bool has_smallest_icon =
+      FindBestSizeInPx(app_id, icon_size_in_px, &best_size_in_px);
+  DCHECK(has_smallest_icon);
 
   base::PostTaskAndReplyWithResult(
       FROM_HERE, kTaskTraits,
       base::BindOnce(ReadCompressedIconBlocking, utils_->Clone(),
                      web_apps_directory_, app_id, best_size_in_px),
       std::move(callback));
-
-  return true;
 }
 
 bool WebAppIconManager::FindBestSizeInPx(const AppId& app_id,

chrome/browser/web_applications/web_app_icon_manager.h

@@ -36,13 +36,15 @@ class WebAppIconManager : public AppIconManager {
   void DeleteData(AppId app_id, WriteDataCallback callback);
 
   // AppIconManager:
-  bool ReadIcon(const AppId& app_id,
+  bool HasIcon(const AppId& app_id, int icon_size_in_px) const override;
+  bool HasSmallestIcon(const AppId& app_id, int icon_size_in_px) const override;
+  void ReadIcon(const AppId& app_id,
                 int icon_size_in_px,
                 ReadIconCallback callback) const override;
-  bool ReadSmallestIcon(const AppId& app_id,
+  void ReadSmallestIcon(const AppId& app_id,
                         int icon_size_in_px,
                         ReadIconCallback callback) const override;
-  bool ReadSmallestCompressedIcon(
+  void ReadSmallestCompressedIcon(
       const AppId& app_id,
       int icon_size_in_px,
       ReadCompressedIconCallback callback) const override;

chrome/browser/web_applications/web_app_icon_manager_unittest.cc

@@ -85,17 +85,18 @@ class WebAppIconManagerTest : public WebAppTest {
 
   std::vector<uint8_t> ReadSmallestCompressedIcon(const AppId& app_id,
                                                   int icon_size_in_px) {
+    EXPECT_TRUE(icon_manager().HasSmallestIcon(app_id, icon_size_in_px));
+
     std::vector<uint8_t> result;
 
     base::RunLoop run_loop;
-    bool icon_requested = icon_manager().ReadSmallestCompressedIcon(
+    icon_manager().ReadSmallestCompressedIcon(
         app_id, icon_size_in_px,
         base::BindLambdaForTesting([&](std::vector<uint8_t> data) {
           result = std::move(data);
           run_loop.Quit();
         }));
 
-    EXPECT_TRUE(icon_requested);
     run_loop.Run();
     return result;
   }
@@ -146,17 +147,17 @@ TEST_F(WebAppIconManagerTest, WriteAndReadIcon) {
 
   controller().RegisterApp(std::move(web_app));
 
+  EXPECT_TRUE(icon_manager().HasIcon(app_id, sizes_px[0]));
   {
     base::RunLoop run_loop;
 
-    const bool icon_requested = icon_manager().ReadIcon(
+    icon_manager().ReadIcon(
         app_id, sizes_px[0],
         base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
           EXPECT_FALSE(bitmap.empty());
           EXPECT_EQ(colors[0], bitmap.getColor(0, 0));
           run_loop.Quit();
         }));
-    EXPECT_TRUE(icon_requested);
 
     run_loop.Run();
   }
@@ -176,20 +177,20 @@ TEST_F(WebAppIconManagerTest, ReadIconFailed) {
 
   controller().RegisterApp(std::move(web_app));
 
-  // Request non-existing icon size.
-  EXPECT_FALSE(
-      icon_manager().ReadIcon(app_id, icon_size::k96, base::DoNothing()));
+  // Check non-existing icon size.
+  EXPECT_FALSE(icon_manager().HasIcon(app_id, icon_size::k96));
+
+  EXPECT_TRUE(icon_manager().HasIcon(app_id, icon_size_px));
 
   // Request existing icon size which doesn't exist on disk.
   base::RunLoop run_loop;
 
-  const bool icon_requested = icon_manager().ReadIcon(
+  icon_manager().ReadIcon(
       app_id, icon_size_px,
       base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
         EXPECT_TRUE(bitmap.empty());
         run_loop.Quit();
       }));
-  EXPECT_TRUE(icon_requested);
 
   run_loop.Run();
 }
@@ -207,24 +208,19 @@ TEST_F(WebAppIconManagerTest, FindExact) {
 
   controller().RegisterApp(std::move(web_app));
 
-  {
-    const bool icon_requested = icon_manager().ReadIcon(
-        app_id, 40, base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
-          NOTREACHED();
-        }));
-    EXPECT_FALSE(icon_requested);
-  }
+  EXPECT_FALSE(icon_manager().HasIcon(app_id, 40));
 
   {
     base::RunLoop run_loop;
 
-    const bool icon_requested = icon_manager().ReadIcon(
+    EXPECT_TRUE(icon_manager().HasIcon(app_id, 20));
+
+    icon_manager().ReadIcon(
         app_id, 20, base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
           EXPECT_FALSE(bitmap.empty());
           EXPECT_EQ(SK_ColorBLUE, bitmap.getColor(0, 0));
           run_loop.Quit();
         }));
-    EXPECT_TRUE(icon_requested);
 
     run_loop.Run();
   }
@@ -243,24 +239,18 @@ TEST_F(WebAppIconManagerTest, FindSmallest) {
 
   controller().RegisterApp(std::move(web_app));
 
-  {
-    const bool icon_requested = icon_manager().ReadSmallestIcon(
-        app_id, 70, base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
-          NOTREACHED();
-        }));
-    EXPECT_FALSE(icon_requested);
-  }
+  EXPECT_FALSE(icon_manager().HasIcon(app_id, 70));
 
   {
     base::RunLoop run_loop;
 
-    const bool icon_requested = icon_manager().ReadSmallestIcon(
+    EXPECT_TRUE(icon_manager().HasSmallestIcon(app_id, 40));
+    icon_manager().ReadSmallestIcon(
         app_id, 40, base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
           EXPECT_FALSE(bitmap.empty());
           EXPECT_EQ(SK_ColorGREEN, bitmap.getColor(0, 0));
           run_loop.Quit();
         }));
-    EXPECT_TRUE(icon_requested);
 
     run_loop.Run();
   }
@@ -268,13 +258,13 @@ TEST_F(WebAppIconManagerTest, FindSmallest) {
   {
     base::RunLoop run_loop;
 
-    const bool icon_requested = icon_manager().ReadSmallestIcon(
+    EXPECT_TRUE(icon_manager().HasSmallestIcon(app_id, 20));
+    icon_manager().ReadSmallestIcon(
         app_id, 20, base::BindLambdaForTesting([&](const SkBitmap& bitmap) {
           EXPECT_FALSE(bitmap.empty());
           EXPECT_EQ(SK_ColorBLUE, bitmap.getColor(0, 0));
           run_loop.Quit();
         }));
-    EXPECT_TRUE(icon_requested);
 
     run_loop.Run();
   }