Desktop PWAs: Cache favicons for use in context menus

Existing menu code such as RenderViewContextMenu assumes the icons are
available synchronously.

We cache the favicon for each web app. If no icon is cached, the menu
item is shown with no visible icon.

References to Bookmark Apps in RenderViewContextMenu are replaced
with references to web apps.

Bug: 1052707
Change-Id: Ib6dd23c448e01dfb36575008532fcce76b339ddd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216834
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772556}

chrome/browser/apps/intent_helper/apps_navigation_throttle.cc

@@ -15,7 +15,6 @@
 #include "chrome/browser/apps/app_service/browser_app_launcher.h"
 #include "chrome/browser/apps/intent_helper/intent_picker_auto_display_service.h"
 #include "chrome/browser/apps/intent_helper/page_transition_util.h"
-#include "chrome/browser/extensions/menu_manager.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -23,6 +22,7 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/intent_picker_tab_helper.h"
 #include "chrome/browser/ui/web_applications/web_app_launch_utils.h"
+#include "chrome/browser/web_applications/components/app_icon_manager.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
@@ -35,6 +35,7 @@
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/constants.h"
 #include "third_party/blink/public/mojom/referrer.mojom.h"
+#include "ui/gfx/image/image.h"
 #include "url/origin.h"
 
 namespace {
@@ -356,17 +357,14 @@ std::vector<IntentPickerAppInfo> AppsNavigationThrottle::FindPwaForUrl(
   if (!app_id)
     return apps;
 
-  // TODO(crbug.com/1052707): Use AppIconManager to read PWA icons.
-  auto* menu_manager =
-      extensions::MenuManager::Get(web_contents->GetBrowserContext());
+  auto* const provider = web_app::WebAppProviderBase::GetProviderBase(profile);
+  gfx::Image icon = gfx::Image::CreateFrom1xBitmap(
+      provider->icon_manager().GetFavicon(*app_id));
 
   // Prefer the web and place apps of type PWA before apps of type ARC.
   // TODO(crbug.com/824598): deterministically sort this list.
-  apps.emplace(apps.begin(), PickerEntryType::kWeb,
-               menu_manager->GetIconForExtension(*app_id), *app_id,
-               web_app::WebAppProviderBase::GetProviderBase(profile)
-                   ->registrar()
-                   .GetAppShortName(*app_id));
+  apps.emplace(apps.begin(), PickerEntryType::kWeb, icon, *app_id,
+               provider->registrar().GetAppShortName(*app_id));
 
   return apps;
 }

chrome/browser/renderer_context_menu/render_view_context_menu.cc

@@ -81,6 +81,7 @@
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/history/foreign_session_handler.h"
+#include "chrome/browser/web_applications/components/app_icon_manager.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
@@ -159,6 +160,7 @@
 #include "ui/base/models/image_model.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/strings/grit/ui_strings.h"
 
@@ -1174,7 +1176,7 @@ void RenderViewContextMenu::AppendLinkItems() {
         in_app ? IDS_CONTENT_CONTEXT_OPENLINKOFFTHERECORD_INAPP
                : IDS_CONTENT_CONTEXT_OPENLINKOFFTHERECORD);
 
-    AppendOpenInBookmarkAppLinkItems();
+    AppendOpenInWebAppLinkItems();
     AppendOpenWithLinkItems();
 
     // While ChromeOS supports multiple profiles, only one can be open at a
@@ -1356,7 +1358,7 @@ void RenderViewContextMenu::AppendSmartSelectionActionItems() {
 #endif
 }
 
-void RenderViewContextMenu::AppendOpenInBookmarkAppLinkItems() {
+void RenderViewContextMenu::AppendOpenInWebAppLinkItems() {
   Profile* const profile = Profile::FromBrowserContext(browser_context_);
 
   base::Optional<web_app::AppId> app_id =
@@ -1373,16 +1375,15 @@ void RenderViewContextMenu::AppendOpenInBookmarkAppLinkItems() {
     open_in_app_string_id = IDS_CONTENT_CONTEXT_OPENLINKBOOKMARKAPP;
   }
 
-  auto* provider = web_app::WebAppProviderBase::GetProviderBase(profile);
+  auto* const provider = web_app::WebAppProviderBase::GetProviderBase(profile);
   menu_model_.AddItem(
       IDC_CONTENT_CONTEXT_OPENLINKBOOKMARKAPP,
       l10n_util::GetStringFUTF16(
           open_in_app_string_id,
           base::UTF8ToUTF16(provider->registrar().GetAppShortName(*app_id))));
 
-  MenuManager* menu_manager = MenuManager::Get(browser_context_);
-  // TODO(crbug.com/1052707): Use AppIconManager to read PWA icons.
-  gfx::Image icon = menu_manager->GetIconForExtension(*app_id);
+  gfx::Image icon = gfx::Image::CreateFrom1xBitmap(
+      provider->icon_manager().GetFavicon(*app_id));
   menu_model_.SetIcon(menu_model_.GetItemCount() - 1,
                       ui::ImageModel::FromImage(icon));
 }
@@ -2207,7 +2208,7 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
       break;
 
     case IDC_CONTENT_CONTEXT_OPENLINKBOOKMARKAPP:
-      ExecOpenBookmarkApp();
+      ExecOpenWebApp();
       break;
 
     case IDC_CONTENT_CONTEXT_SAVELINKAS:
@@ -2725,7 +2726,7 @@ bool RenderViewContextMenu::IsOpenLinkOTREnabled() const {
   return incognito_avail != IncognitoModePrefs::DISABLED;
 }
 
-void RenderViewContextMenu::ExecOpenBookmarkApp() {
+void RenderViewContextMenu::ExecOpenWebApp() {
   base::Optional<web_app::AppId> app_id =
       web_app::FindInstalledAppWithUrlInScope(
           Profile::FromBrowserContext(browser_context_), params_.link_url);

chrome/browser/renderer_context_menu/render_view_context_menu.h

@@ -160,7 +160,7 @@ class RenderViewContextMenu : public RenderViewContextMenuBase {
   void AppendLinkItems();
   void AppendOpenWithLinkItems();
   void AppendSmartSelectionActionItems();
-  void AppendOpenInBookmarkAppLinkItems();
+  void AppendOpenInWebAppLinkItems();
   void AppendQuickAnswersItems();
   void AppendImageItems();
   void AppendAudioItems();
@@ -211,7 +211,7 @@ class RenderViewContextMenu : public RenderViewContextMenuBase {
   bool IsOpenLinkOTREnabled() const;
 
   // Command execution functions.
-  void ExecOpenBookmarkApp();
+  void ExecOpenWebApp();
   void ExecProtocolHandler(int event_flags, int handler_index);
   void ExecOpenLinkInProfile(int profile_index);
   void ExecInspectElement();

chrome/browser/web_applications/components/app_icon_manager.h

@@ -23,6 +23,9 @@ class AppIconManager {
   AppIconManager() = default;
   virtual ~AppIconManager() = default;
 
+  virtual void Start() = 0;
+  virtual void Shutdown() = 0;
+
   // Returns false if any icon in |icon_sizes_in_px| is missing from downloaded
   // icons for a given app. |icon_sizes_in_px| must be sorted in ascending
   // order.
@@ -75,6 +78,8 @@ class AppIconManager {
       SquareSizePx icon_size_in_px,
       ReadCompressedIconCallback callback) const = 0;
 
+  virtual SkBitmap GetFavicon(const AppId& app_id) const = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AppIconManager);
 };

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.cc

@@ -10,6 +10,7 @@
 
 #include "base/check_op.h"
 #include "base/notreached.h"
+#include "chrome/browser/extensions/menu_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_registrar.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
@@ -96,6 +97,10 @@ BookmarkAppIconManager::BookmarkAppIconManager(Profile* profile)
 
 BookmarkAppIconManager::~BookmarkAppIconManager() = default;
 
+void BookmarkAppIconManager::Start() {}
+
+void BookmarkAppIconManager::Shutdown() {}
+
 bool BookmarkAppIconManager::HasIcons(
     const web_app::AppId& app_id,
     const std::vector<SquareSizePx>& icon_sizes_in_px) const {
@@ -172,4 +177,10 @@ void BookmarkAppIconManager::ReadSmallestCompressedIcon(
   std::move(callback).Run(std::vector<uint8_t>());
 }
 
+SkBitmap BookmarkAppIconManager::GetFavicon(
+    const web_app::AppId& app_id) const {
+  auto* menu_manager = extensions::MenuManager::Get(profile_);
+  return menu_manager->GetIconForExtension(app_id).AsBitmap();
+}
+
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.h

@@ -22,6 +22,8 @@ class BookmarkAppIconManager : public web_app::AppIconManager {
   ~BookmarkAppIconManager() override;
 
   // AppIconManager:
+  void Start() override;
+  void Shutdown() override;
   bool HasIcons(
       const web_app::AppId& app_id,
       const std::vector<SquareSizePx>& icon_sizes_in_px) const override;
@@ -42,6 +44,7 @@ class BookmarkAppIconManager : public web_app::AppIconManager {
       const web_app::AppId& app_id,
       SquareSizePx icon_size_in_px,
       ReadCompressedIconCallback callback) const override;
+  SkBitmap GetFavicon(const web_app::AppId& app_id) const override;
 
  private:
   Profile* const profile_;

chrome/browser/web_applications/web_app_icon_manager.cc

@@ -27,6 +27,7 @@
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/favicon_size.h"
 
 namespace web_app {
 
@@ -494,6 +495,15 @@ void WebAppIconManager::DeleteData(AppId app_id, WriteDataCallback callback) {
       std::move(callback));
 }
 
+void WebAppIconManager::Start() {
+  for (const AppId& app_id : registrar_.GetAppIds()) {
+    ReadFavicon(app_id);
+  }
+  registrar_observer_.Add(&registrar_);
+}
+
+void WebAppIconManager::Shutdown() {}
+
 bool WebAppIconManager::HasIcons(
     const AppId& app_id,
     const std::vector<SquareSizePx>& icon_sizes_in_px) const {
@@ -590,6 +600,21 @@ void WebAppIconManager::ReadSmallestCompressedIcon(
       std::move(callback));
 }
 
+SkBitmap WebAppIconManager::GetFavicon(const web_app::AppId& app_id) const {
+  auto iter = favicon_cache_.find(app_id);
+  if (iter == favicon_cache_.end())
+    return SkBitmap();
+  return iter->second;
+}
+
+void WebAppIconManager::OnWebAppInstalled(const AppId& app_id) {
+  ReadFavicon(app_id);
+}
+
+void WebAppIconManager::OnAppRegistrarDestroyed() {
+  registrar_observer_.RemoveAll();
+}
+
 bool WebAppIconManager::HasIconToResize(const AppId& app_id,
                                         SquareSizePx desired_icon_size) const {
   return FindDownloadedSizeInPxMatchBigger(app_id, desired_icon_size)
@@ -620,6 +645,11 @@ void WebAppIconManager::ReadIconAndResize(const AppId& app_id,
       std::move(callback));
 }
 
+void WebAppIconManager::SetFaviconReadCallbackForTesting(
+    FaviconReadCallback callback) {
+  favicon_read_callback_ = callback;
+}
+
 base::Optional<SquareSizePx>
 WebAppIconManager::FindDownloadedSizeInPxMatchBigger(
     const AppId& app_id,
@@ -656,4 +686,20 @@ WebAppIconManager::FindDownloadedSizeInPxMatchSmaller(
   return base::nullopt;
 }
 
+void WebAppIconManager::ReadFavicon(const AppId& app_id) {
+  if (!HasSmallestIcon(app_id, gfx::kFaviconSize))
+    return;
+
+  ReadSmallestIcon(app_id, gfx::kFaviconSize,
+                   base::BindOnce(&WebAppIconManager::OnReadFavicon,
+                                  weak_ptr_factory_.GetWeakPtr(), app_id));
+}
+
+void WebAppIconManager::OnReadFavicon(const AppId& app_id,
+                                      const SkBitmap& bitmap) {
+  favicon_cache_[app_id] = bitmap;
+  if (favicon_read_callback_)
+    favicon_read_callback_.Run(app_id);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_icon_manager.h

@@ -9,10 +9,15 @@
 #include <memory>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/web_applications/components/app_icon_manager.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
+#include "chrome/browser/web_applications/components/app_registrar_observer.h"
 #include "chrome/common/web_application_info.h"
 
 class Profile;
@@ -23,8 +28,11 @@ class FileUtilsWrapper;
 class WebAppRegistrar;
 
 // Exclusively used from the UI thread.
-class WebAppIconManager : public AppIconManager {
+class WebAppIconManager : public AppIconManager, public AppRegistrarObserver {
  public:
+  using FaviconReadCallback =
+      base::RepeatingCallback<void(const AppId& app_id)>;
+
   WebAppIconManager(Profile* profile,
                     WebAppRegistrar& registrar,
                     std::unique_ptr<FileUtilsWrapper> utils);
@@ -43,6 +51,8 @@ class WebAppIconManager : public AppIconManager {
   void DeleteData(AppId app_id, WriteDataCallback callback);
 
   // AppIconManager:
+  void Start() override;
+  void Shutdown() override;
   bool HasIcons(
       const AppId& app_id,
       const std::vector<SquareSizePx>& icon_sizes_in_px) const override;
@@ -63,6 +73,11 @@ class WebAppIconManager : public AppIconManager {
       const AppId& app_id,
       SquareSizePx icon_size_in_px,
       ReadCompressedIconCallback callback) const override;
+  SkBitmap GetFavicon(const web_app::AppId& app_id) const override;
+
+  // AppRegistrarObserver:
+  void OnWebAppInstalled(const AppId& app_id) override;
+  void OnAppRegistrarDestroyed() override;
 
   // If there is no icon at the downloaded sizes, we may resize what we can get.
   bool HasIconToResize(const AppId& app_id,
@@ -73,6 +88,8 @@ class WebAppIconManager : public AppIconManager {
                          SquareSizePx desired_icon_size,
                          ReadIconsCallback callback) const;
 
+  void SetFaviconReadCallbackForTesting(FaviconReadCallback callback);
+
  private:
   base::Optional<SquareSizePx> FindDownloadedSizeInPxMatchBigger(
       const AppId& app_id,
@@ -81,10 +98,22 @@ class WebAppIconManager : public AppIconManager {
       const AppId& app_id,
       SquareSizePx desired_size) const;
 
-  const WebAppRegistrar& registrar_;
+  void ReadFavicon(const AppId& app_id);
+  void OnReadFavicon(const AppId& app_id, const SkBitmap&);
+
+  WebAppRegistrar& registrar_;
   base::FilePath web_apps_directory_;
   std::unique_ptr<FileUtilsWrapper> utils_;
 
+  ScopedObserver<AppRegistrar, AppRegistrarObserver> registrar_observer_{this};
+
+  // We cache a single low-resolution icon for each app.
+  std::map<AppId, SkBitmap> favicon_cache_;
+
+  FaviconReadCallback favicon_read_callback_;
+
+  base::WeakPtrFactory<WebAppIconManager> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(WebAppIconManager);
 };
 

chrome/browser/web_applications/web_app_icon_manager_unittest.cc

@@ -32,6 +32,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/favicon_size.h"
 
 namespace web_app {
 
@@ -653,4 +654,64 @@ TEST_F(WebAppIconManagerTest, MatchSizes) {
   EXPECT_EQ(kWebAppIconSmall, extension_misc::EXTENSION_ICON_SMALL);
 }
 
+TEST_F(WebAppIconManagerTest, CacheExistingAppFavicon) {
+  auto web_app = CreateWebApp();
+  const AppId app_id = web_app->app_id();
+
+  const std::vector<int> sizes_px{gfx::kFaviconSize, icon_size::k48};
+  const std::vector<SkColor> colors{SK_ColorGREEN, SK_ColorRED};
+  WriteIcons(app_id, sizes_px, colors);
+
+  web_app->SetDownloadedIconSizes(sizes_px);
+
+  controller().RegisterApp(std::move(web_app));
+
+  base::RunLoop run_loop;
+  icon_manager().SetFaviconReadCallbackForTesting(
+      base::BindLambdaForTesting([&](const AppId& cached_app_id) {
+        EXPECT_EQ(cached_app_id, app_id);
+        run_loop.Quit();
+      }));
+
+  icon_manager().Start();
+  run_loop.Run();
+
+  SkBitmap bitmap = icon_manager().GetFavicon(app_id);
+  EXPECT_FALSE(bitmap.empty());
+  EXPECT_EQ(gfx::kFaviconSize, bitmap.width());
+  EXPECT_EQ(gfx::kFaviconSize, bitmap.height());
+  EXPECT_EQ(SK_ColorGREEN, bitmap.getColor(0, 0));
+}
+
+TEST_F(WebAppIconManagerTest, CacheNewAppFavicon) {
+  icon_manager().Start();
+
+  auto web_app = CreateWebApp();
+  const AppId app_id = web_app->app_id();
+
+  const std::vector<int> sizes_px{gfx::kFaviconSize, icon_size::k48};
+  const std::vector<SkColor> colors{SK_ColorBLUE, SK_ColorRED};
+  WriteIcons(app_id, sizes_px, colors);
+
+  web_app->SetDownloadedIconSizes(sizes_px);
+
+  base::RunLoop run_loop;
+  icon_manager().SetFaviconReadCallbackForTesting(
+      base::BindLambdaForTesting([&](const AppId& cached_app_id) {
+        EXPECT_EQ(cached_app_id, app_id);
+        run_loop.Quit();
+      }));
+
+  controller().RegisterApp(std::move(web_app));
+  registrar().NotifyWebAppInstalled(app_id);
+
+  run_loop.Run();
+
+  SkBitmap bitmap = icon_manager().GetFavicon(app_id);
+  EXPECT_FALSE(bitmap.empty());
+  EXPECT_EQ(gfx::kFaviconSize, bitmap.width());
+  EXPECT_EQ(gfx::kFaviconSize, bitmap.height());
+  EXPECT_EQ(SK_ColorBLUE, bitmap.getColor(0, 0));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_provider.cc

@@ -155,6 +155,7 @@ void WebAppProvider::Shutdown() {
   install_manager_->Shutdown();
   manifest_update_manager_->Shutdown();
   system_web_app_manager_->Shutdown();
+  icon_manager_->Shutdown();
   install_finalizer_->Shutdown();
   registrar_->Shutdown();
 }
@@ -274,6 +275,7 @@ void WebAppProvider::OnRegistryControllerReady() {
 
   registrar_->Start();
   install_finalizer_->Start();
+  icon_manager_->Start();
   external_web_app_manager_->Start();
   web_app_policy_manager_->Start();
   system_web_app_manager_->Start();