desktop-pwas: Implement WebAppIconManager::WriteShortcutIconsData()

In the initial CL for Shortcuts Menu (https://crrev.com/c/2146212), we
used the WebAppIconManager::WriteData method to write both app icon
and shortcut icon data to disk.

App icons are for the web app as a whole. Shortcut icons on the other
hand, are specified for a shortcut in the web app manifest's shortcuts
member array. So, in the interest of code readability and debuggability
we should write these to disk separately.

With this in mind, I left a TODO in the initial CL to create separate
methods for writing these 2 different kinds of icon data to disk.

This CL addresses that TODO. It adds a separate
WebAppIconManager::WriteShortcutIconsData method. It also reverts
WebAppIconManager::WriteData to only write app icon specific data.

Additionally, it updates existing unittests to work with the refactored
code and adds new tests for reading and writing shortcut icon data.

Finally, it renames  occurrences of *ShortcutIcon*/*shortcut_icon* to
*ShortcutsMenuIcon*/*shortcuts_menu_icon*.

Bug: 1069308
Change-Id: I530b3af75277f1d6c5f2b0ed6a67ce469bba7fed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204972
Commit-Queue: Rahul Singh <rahsin@microsoft.com>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772442}

chrome/browser/web_applications/components/app_icon_manager.h

@@ -42,16 +42,17 @@ class AppIconManager {
                          const std::vector<SquareSizePx>& icon_sizes_in_px,
                          ReadIconsCallback callback) const = 0;
 
-  using ReadShortcutIconsCallback = base::OnceCallback<void(
-      std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons_bitmaps)>;
+  using ReadShortcutsMenuIconsCallback =
+      base::OnceCallback<void(std::vector<std::map<SquareSizePx, SkBitmap>>
+                                  shortcuts_menu_icons_bitmaps)>;
 
-  // Reads bitmaps for all shortcut icons for an app. Returns a vector of
+  // Reads bitmaps for all shortcuts menu icons for an app. Returns a vector of
   // map<SquareSizePx, SkBitmap>. The index of a map in the vector is the same
   // as that of its corresponding shortcut in the manifest's shortcuts vector.
   // Returns empty vector in |callback| if we hit any error.
-  virtual void ReadAllShortcutIcons(
+  virtual void ReadAllShortcutsMenuIcons(
       const AppId& app_id,
-      ReadShortcutIconsCallback callback) const = 0;
+      ReadShortcutsMenuIconsCallback callback) const = 0;
 
   // Reads all icon bitmaps for an app. Returns empty |icon_bitmaps| in
   // |callback| if IO error.

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/web_applications/extensions/bookmark_app_icon_manager.h"
 
+#include <map>
+#include <string>
+#include <utility>
+
 #include "base/check_op.h"
 #include "base/notreached.h"
 #include "chrome/browser/profiles/profile.h"
@@ -142,9 +146,9 @@ void BookmarkAppIconManager::ReadAllIcons(const web_app::AppId& app_id,
                      std::move(callback));
 }
 
-void BookmarkAppIconManager::ReadAllShortcutIcons(
+void BookmarkAppIconManager::ReadAllShortcutsMenuIcons(
     const web_app::AppId& app_id,
-    ReadShortcutIconsCallback callback) const {
+    ReadShortcutsMenuIconsCallback callback) const {
   // TODO(https://crbug.com/926083): This needs to be implemented to support
   // Manifest update and local installs.
   NOTIMPLEMENTED();

chrome/browser/web_applications/extensions/bookmark_app_icon_manager.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_ICON_MANAGER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_ICON_MANAGER_H_
 
+#include <vector>
+
 #include "chrome/browser/web_applications/components/app_icon_manager.h"
 
 class Profile;
@@ -30,8 +32,9 @@ class BookmarkAppIconManager : public web_app::AppIconManager {
                  ReadIconsCallback callback) const override;
   void ReadAllIcons(const web_app::AppId& app_id,
                     ReadIconsCallback callback) const override;
-  void ReadAllShortcutIcons(const web_app::AppId& app_id,
-                            ReadShortcutIconsCallback callback) const override;
+  void ReadAllShortcutsMenuIcons(
+      const web_app::AppId& app_id,
+      ReadShortcutsMenuIconsCallback callback) const override;
   void ReadSmallestIcon(const web_app::AppId& app_id,
                         SquareSizePx icon_size_in_px,
                         ReadIconCallback callback) const override;

chrome/browser/web_applications/web_app.cc

@@ -179,32 +179,32 @@ void WebApp::SetAdditionalSearchTerms(
 }
 
 void WebApp::SetShortcutInfos(
-    std::vector<WebAppShortcutMenuItemInfo> shortcut_infos) {
+    std::vector<WebAppShortcutsMenuItemInfo> shortcut_infos) {
   shortcut_infos_ = std::move(shortcut_infos);
 }
 
-void WebApp::SetDownloadedShortcutIconsSizes(
+void WebApp::SetDownloadedShortcutsMenuIconsSizes(
     std::vector<std::vector<SquareSizePx>> sizes) {
-  downloaded_shortcut_icons_sizes_ = std::move(sizes);
+  downloaded_shortcuts_menu_icons_sizes_ = std::move(sizes);
 }
 
-WebApp::WebAppShortcutMenuItemInfo::WebAppShortcutMenuItemInfo() = default;
+WebApp::WebAppShortcutsMenuItemInfo::WebAppShortcutsMenuItemInfo() = default;
 
-WebApp::WebAppShortcutMenuItemInfo::WebAppShortcutMenuItemInfo(
-    const WebAppShortcutMenuItemInfo& other) = default;
+WebApp::WebAppShortcutsMenuItemInfo::WebAppShortcutsMenuItemInfo(
+    const WebAppShortcutsMenuItemInfo& other) = default;
 
-WebApp::WebAppShortcutMenuItemInfo::WebAppShortcutMenuItemInfo(
-    WebAppShortcutMenuItemInfo&&) noexcept = default;
+WebApp::WebAppShortcutsMenuItemInfo::WebAppShortcutsMenuItemInfo(
+    WebAppShortcutsMenuItemInfo&&) noexcept = default;
 
-WebApp::WebAppShortcutMenuItemInfo::~WebAppShortcutMenuItemInfo() = default;
+WebApp::WebAppShortcutsMenuItemInfo::~WebAppShortcutsMenuItemInfo() = default;
 
-WebApp::WebAppShortcutMenuItemInfo&
-WebApp::WebAppShortcutMenuItemInfo::operator=(
-    const WebAppShortcutMenuItemInfo&) = default;
+WebApp::WebAppShortcutsMenuItemInfo&
+WebApp::WebAppShortcutsMenuItemInfo::operator=(
+    const WebAppShortcutsMenuItemInfo&) = default;
 
-WebApp::WebAppShortcutMenuItemInfo&
-WebApp::WebAppShortcutMenuItemInfo::operator=(
-    WebAppShortcutMenuItemInfo&&) noexcept = default;
+WebApp::WebAppShortcutsMenuItemInfo&
+WebApp::WebAppShortcutsMenuItemInfo::operator=(
+    WebAppShortcutsMenuItemInfo&&) noexcept = default;
 
 void WebApp::SetSyncData(SyncData sync_data) {
   sync_data_ = std::move(sync_data);

chrome/browser/web_applications/web_app.h

@@ -100,36 +100,37 @@ class WebApp {
   const SyncData& sync_data() const { return sync_data_; }
 
   // Stores info needed to create app icon shortcuts menu and for downloading
-  // associated shortcut icons when supported by OS platform (eg. Windows).
-  struct WebAppShortcutMenuItemInfo {
-    WebAppShortcutMenuItemInfo();
-    WebAppShortcutMenuItemInfo(const WebAppShortcutMenuItemInfo&);
-    WebAppShortcutMenuItemInfo(WebAppShortcutMenuItemInfo&&) noexcept;
-    ~WebAppShortcutMenuItemInfo();
-    WebAppShortcutMenuItemInfo& operator=(const WebAppShortcutMenuItemInfo&);
-    WebAppShortcutMenuItemInfo& operator=(
-        WebAppShortcutMenuItemInfo&&) noexcept;
-
-    // Title of shortcut item in App Icon Shortcut Menu.
+  // associated shortcuts menu icons when supported by OS platform (eg.
+  // Windows).
+  struct WebAppShortcutsMenuItemInfo {
+    WebAppShortcutsMenuItemInfo();
+    WebAppShortcutsMenuItemInfo(const WebAppShortcutsMenuItemInfo&);
+    WebAppShortcutsMenuItemInfo(WebAppShortcutsMenuItemInfo&&) noexcept;
+    ~WebAppShortcutsMenuItemInfo();
+    WebAppShortcutsMenuItemInfo& operator=(const WebAppShortcutsMenuItemInfo&);
+    WebAppShortcutsMenuItemInfo& operator=(
+        WebAppShortcutsMenuItemInfo&&) noexcept;
+
+    // Title of shortcuts menu item in App Icon Shortcut Menu.
     base::string16 name;
 
-    // URL launched when shortcut item is selected.
+    // URL launched when shortcuts menu item is selected.
     GURL url;
 
-    // List of shortcut icon URLs with associated square size.
-    std::vector<WebApplicationIconInfo> shortcut_icon_infos;
+    // List of shortcuts menu icon URLs with associated square size.
+    std::vector<WebApplicationIconInfo> shortcuts_menu_icon_infos;
   };
 
   // Represents the "shortcuts" field in the manifest.
-  const std::vector<WebAppShortcutMenuItemInfo>& shortcut_infos() const {
+  const std::vector<WebAppShortcutsMenuItemInfo>& shortcut_infos() const {
     return shortcut_infos_;
   }
 
-  // Represents which shortcut icon sizes we successfully downloaded from the
-  // shortcut_infos.
+  // Represents which shortcuts menu icon sizes we successfully downloaded for
+  // each WebAppShortcutsMenuItemInfo.shortcuts_menu_icon_infos.
   const std::vector<std::vector<SquareSizePx>>&
-  downloaded_shortcut_icons_sizes() const {
-    return downloaded_shortcut_icons_sizes_;
+  downloaded_shortcuts_menu_icons_sizes() const {
+    return downloaded_shortcuts_menu_icons_sizes_;
   }
 
   // A Web App can be installed from multiple sources simultaneously. Installs
@@ -161,8 +162,9 @@ class WebApp {
   void SetIconInfos(std::vector<WebApplicationIconInfo> icon_infos);
   // Performs sorting of |sizes| vector. Must be called rarely.
   void SetDownloadedIconSizes(std::vector<SquareSizePx> sizes);
-  void SetShortcutInfos(std::vector<WebAppShortcutMenuItemInfo> shortcut_infos);
-  void SetDownloadedShortcutIconsSizes(
+  void SetShortcutInfos(
+      std::vector<WebAppShortcutsMenuItemInfo> shortcut_infos);
+  void SetDownloadedShortcutsMenuIconsSizes(
       std::vector<std::vector<SquareSizePx>> icon_sizes);
   void SetFileHandlers(apps::FileHandlers file_handlers);
   void SetAdditionalSearchTerms(
@@ -198,9 +200,9 @@ class WebApp {
   std::vector<WebApplicationIconInfo> icon_infos_;
   std::vector<SquareSizePx> downloaded_icon_sizes_;
   // TODO(https://crbug.com/1069312): Serialize shortcut_infos_ and
-  // downloaded_shortcut_icons_sizes_ fields in WebAppDatabase.
-  std::vector<WebAppShortcutMenuItemInfo> shortcut_infos_;
-  std::vector<std::vector<SquareSizePx>> downloaded_shortcut_icons_sizes_;
+  // downloaded_shortcuts_menu_icons_sizes_ fields in WebAppDatabase.
+  std::vector<WebAppShortcutsMenuItemInfo> shortcut_infos_;
+  std::vector<std::vector<SquareSizePx>> downloaded_shortcuts_menu_icons_sizes_;
   apps::FileHandlers file_handlers_;
   std::vector<std::string> additional_search_terms_;
 

chrome/browser/web_applications/web_app_icon_manager.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/web_applications/web_app_icon_manager.h"
 
+#include <string>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/scoped_temp_dir.h"
@@ -63,12 +66,13 @@ base::FilePath GetAppIconsDirectory(
 }
 
 // This is a private implementation detail of WebAppIconManager, where and how
-// to store shortcut icons files.
-base::FilePath GetAppShortcutIconsDirectory(
+// to store shortcuts menu icons files.
+base::FilePath GetAppShortcutsMenuIconsDirectory(
     const base::FilePath& app_manifest_resources_directory) {
-  static constexpr base::FilePath::CharType kShortcutIconsDirectoryName[] =
-      FILE_PATH_LITERAL("Shortcut Icons");
-  return app_manifest_resources_directory.Append(kShortcutIconsDirectoryName);
+  static constexpr base::FilePath::CharType kShortcutsMenuIconsDirectoryName[] =
+      FILE_PATH_LITERAL("Shortcuts Menu Icons");
+  return app_manifest_resources_directory.Append(
+      kShortcutsMenuIconsDirectoryName);
 }
 
 bool WriteIcon(FileUtilsWrapper* utils,
@@ -115,33 +119,31 @@ bool WriteIcons(FileUtilsWrapper* utils,
   return true;
 }
 
-// Writes shortcut icon files to the Shortcut Icons directory. Creates a new
-// directory per shortcut item using its index in the vector.
-bool WriteShortcutIcons(FileUtilsWrapper* utils,
-                        const base::FilePath& app_dir,
-                        const std::vector<std::map<SquareSizePx, SkBitmap>>&
-                            shortcut_icons_bitmaps) {
-  const base::FilePath shortcut_icons_dir =
-      GetAppShortcutIconsDirectory(app_dir);
-  if (!utils->CreateDirectory(shortcut_icons_dir)) {
-    return false;
-  }
+// Writes shortcuts menu icons files to the Shortcut Icons directory. Creates a
+// new directory per shortcut item using its index in the vector.
+bool WriteShortcutsMenuIcons(
+    FileUtilsWrapper* utils,
+    const base::FilePath& shortcuts_menu_icons_dir,
+    const std::vector<std::map<SquareSizePx, SkBitmap>>&
+        shortcuts_menu_icons_bitmaps) {
+  DCHECK(utils->DirectoryExists(shortcuts_menu_icons_dir));
 
   int shortcut_index = -1;
   for (const std::map<SquareSizePx, SkBitmap>& icon_bitmaps :
-       shortcut_icons_bitmaps) {
+       shortcuts_menu_icons_bitmaps) {
     ++shortcut_index;
     if (icon_bitmaps.empty())
       continue;
 
-    const base::FilePath shortcut_icon_dir =
-        shortcut_icons_dir.AppendASCII(base::NumberToString(shortcut_index));
-    if (!utils->CreateDirectory(shortcut_icon_dir)) {
+    const base::FilePath shortcuts_menu_icon_dir =
+        shortcuts_menu_icons_dir.AppendASCII(
+            base::NumberToString(shortcut_index));
+    if (!utils->CreateDirectory(shortcuts_menu_icon_dir))
       return false;
-    }
+
     for (const std::pair<const SquareSizePx, SkBitmap>& icon_bitmap :
          icon_bitmaps) {
-      if (!WriteIcon(utils, shortcut_icon_dir, icon_bitmap.second))
+      if (!WriteIcon(utils, shortcuts_menu_icon_dir, icon_bitmap.second))
         return false;
     }
   }
@@ -150,12 +152,10 @@ bool WriteShortcutIcons(FileUtilsWrapper* utils,
 
 // Performs blocking I/O. May be called on another thread.
 // Returns true if no errors occurred.
-bool WriteDataBlocking(
-    const std::unique_ptr<FileUtilsWrapper>& utils,
-    const base::FilePath& web_apps_directory,
-    const AppId& app_id,
-    const std::map<SquareSizePx, SkBitmap>& icons,
-    const std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons) {
+bool WriteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
+                       const base::FilePath& web_apps_directory,
+                       const AppId& app_id,
+                       const std::map<SquareSizePx, SkBitmap>& icons) {
   // Create the temp directory under the web apps root.
   // This guarantees it is on the same file system as the WebApp's eventual
   // install target.
@@ -174,11 +174,6 @@ bool WriteDataBlocking(
 
   if (!WriteIcons(utils.get(), app_temp_dir.GetPath(), icons))
     return false;
-  if (!shortcut_icons.empty() &&
-      !WriteShortcutIcons(utils.get(), app_temp_dir.GetPath(),
-                          shortcut_icons)) {
-    return false;
-  }
 
   base::FilePath manifest_resources_directory =
       GetManifestResourcesDirectory(web_apps_directory);
@@ -203,6 +198,65 @@ bool WriteDataBlocking(
   return true;
 }
 
+// Performs blocking I/O. May be called on another thread.
+// Returns true if no errors occurred.
+bool WriteShortcutsMenuIconsDataBlocking(
+    const std::unique_ptr<FileUtilsWrapper>& utils,
+    const base::FilePath& web_apps_directory,
+    const AppId& app_id,
+    const std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons) {
+  if (shortcuts_menu_icons.empty())
+    return false;
+
+  // Create the temp directory under the web apps root.
+  // This guarantees it is on the same file system as the WebApp's eventual
+  // install target.
+  base::FilePath temp_dir = GetWebAppsTempDirectory(web_apps_directory);
+  if (!CreateDirectoryIfNotExists(utils.get(), temp_dir))
+    return false;
+
+  base::ScopedTempDir app_temp_dir;
+  if (!app_temp_dir.CreateUniqueTempDirUnderPath(temp_dir))
+    return false;
+
+  const base::FilePath shortcuts_menu_icons_temp_dir =
+      GetAppShortcutsMenuIconsDirectory(app_temp_dir.GetPath());
+  if (!utils->CreateDirectory(shortcuts_menu_icons_temp_dir))
+    return false;
+
+  if (!WriteShortcutsMenuIcons(utils.get(), shortcuts_menu_icons_temp_dir,
+                               shortcuts_menu_icons))
+    return false;
+
+  base::FilePath manifest_resources_directory =
+      GetManifestResourcesDirectory(web_apps_directory);
+  if (!CreateDirectoryIfNotExists(utils.get(), manifest_resources_directory))
+    return false;
+
+  base::FilePath app_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_directory, app_id);
+
+  // Create app_dir if it doesn't already exist. We'll need this for
+  // WriteShortcutsMenuIconsData unittests.
+  if (!CreateDirectoryIfNotExists(utils.get(), app_dir))
+    return false;
+
+  base::FilePath shortcuts_menu_icons_dir =
+      GetAppShortcutsMenuIconsDirectory(app_dir);
+
+  // Delete the destination. Needed for update. Return if destination isn't
+  // clear.
+  if (!utils->DeleteFileRecursively(shortcuts_menu_icons_dir))
+    return false;
+
+  // Commit: move whole shortcuts menu icons data dir to final destination in
+  // one mv operation.
+  if (!utils->Move(shortcuts_menu_icons_temp_dir, shortcuts_menu_icons_dir))
+    return false;
+
+  return true;
+}
+
 // Performs blocking I/O. May be called on another thread.
 // Returns true if no errors occurred.
 bool DeleteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
@@ -224,19 +278,20 @@ base::FilePath GetIconFileName(const base::FilePath& web_apps_directory,
   return icons_dir.AppendASCII(base::StringPrintf("%i.png", icon_size_px));
 }
 
-base::FilePath GetManifestResourcesShortcutIconFileName(
+base::FilePath GetManifestResourcesShortcutsMenuIconFileName(
     const base::FilePath& web_apps_directory,
     const AppId& app_id,
     int index,
     int icon_size_px) {
   const base::FilePath manifest_app_dir =
       GetManifestResourcesDirectoryForApp(web_apps_directory, app_id);
-  const base::FilePath manifest_shortcut_icons_dir =
-      GetAppShortcutIconsDirectory(manifest_app_dir);
-  const base::FilePath manifest_shortcut_icon_dir =
-      manifest_shortcut_icons_dir.AppendASCII(base::NumberToString(index));
+  const base::FilePath manifest_shortcuts_menu_icons_dir =
+      GetAppShortcutsMenuIconsDirectory(manifest_app_dir);
+  const base::FilePath manifest_shortcuts_menu_icon_dir =
+      manifest_shortcuts_menu_icons_dir.AppendASCII(
+          base::NumberToString(index));
 
-  return manifest_shortcut_icon_dir.AppendASCII(
+  return manifest_shortcuts_menu_icon_dir.AppendASCII(
       base::NumberToString(icon_size_px) + ".png");
 }
 
@@ -268,18 +323,18 @@ SkBitmap ReadIconBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
 
 // Performs blocking I/O. May be called on another thread.
 // Returns empty SkBitmap if any errors occurred.
-SkBitmap ReadShortcutIconBlocking(FileUtilsWrapper* utils,
-                                  const base::FilePath& web_apps_directory,
-                                  const AppId& app_id,
-                                  int index,
-                                  int icon_size_px) {
-  base::FilePath manifest_shortcut_icon_file =
-      GetManifestResourcesShortcutIconFileName(web_apps_directory, app_id,
-                                               index, icon_size_px);
+SkBitmap ReadShortcutsMenuIconBlocking(FileUtilsWrapper* utils,
+                                       const base::FilePath& web_apps_directory,
+                                       const AppId& app_id,
+                                       int index,
+                                       int icon_size_px) {
+  base::FilePath manifest_shortcuts_menu_icon_file =
+      GetManifestResourcesShortcutsMenuIconFileName(web_apps_directory, app_id,
+                                                    index, icon_size_px);
 
   std::string icon_data;
 
-  if (!utils->ReadFileToString(manifest_shortcut_icon_file, &icon_data)) {
+  if (!utils->ReadFileToString(manifest_shortcuts_menu_icon_file, &icon_data)) {
     return SkBitmap();
   }
 
@@ -342,22 +397,25 @@ std::map<SquareSizePx, SkBitmap> ReadIconsBlocking(
 }
 
 // Performs blocking I/O. May be called on another thread.
-std::vector<std::map<SquareSizePx, SkBitmap>> ReadShortcutIconsBlocking(
+std::vector<std::map<SquareSizePx, SkBitmap>> ReadShortcutsMenuIconsBlocking(
     FileUtilsWrapper* utils,
     const base::FilePath& web_apps_directory,
     const AppId& app_id,
-    const std::vector<std::vector<SquareSizePx>>& shortcut_icons_sizes) {
+    const std::vector<std::vector<SquareSizePx>>& shortcuts_menu_icons_sizes) {
   std::vector<std::map<SquareSizePx, SkBitmap>> results;
   int curr_index = 0;
-  for (const auto& icon_sizes : shortcut_icons_sizes) {
+  for (const auto& icon_sizes : shortcuts_menu_icons_sizes) {
     std::map<SquareSizePx, SkBitmap> result;
     for (SquareSizePx icon_size_px : icon_sizes) {
-      SkBitmap bitmap = ReadShortcutIconBlocking(
+      SkBitmap bitmap = ReadShortcutsMenuIconBlocking(
           utils, web_apps_directory, app_id, curr_index, icon_size_px);
       if (!bitmap.empty())
         result[icon_size_px] = bitmap;
     }
     ++curr_index;
+    // We always push_back (even when result is empty) to keep a given
+    // std::map's index in sync with that of its corresponding shortcuts menu
+    // item.
     results.push_back(result);
   }
   return results;
@@ -402,15 +460,27 @@ WebAppIconManager::~WebAppIconManager() = default;
 void WebAppIconManager::WriteData(
     AppId app_id,
     std::map<SquareSizePx, SkBitmap> icons,
-    std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons,
     WriteDataCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, kTaskTraits,
       base::BindOnce(WriteDataBlocking, utils_->Clone(), web_apps_directory_,
-                     std::move(app_id), std::move(icons),
-                     std::move(shortcut_icons)),
+                     std::move(app_id), std::move(icons)),
+      std::move(callback));
+}
+
+void WebAppIconManager::WriteShortcutsMenuIconsData(
+    AppId app_id,
+    std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons,
+    WriteDataCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, kTaskTraits,
+      base::BindOnce(WriteShortcutsMenuIconsDataBlocking, utils_->Clone(),
+                     web_apps_directory_, std::move(app_id),
+                     std::move(shortcuts_menu_icons)),
       std::move(callback));
 }
 
@@ -469,9 +539,9 @@ void WebAppIconManager::ReadAllIcons(const AppId& app_id,
       std::move(callback));
 }
 
-void WebAppIconManager::ReadAllShortcutIcons(
+void WebAppIconManager::ReadAllShortcutsMenuIcons(
     const AppId& app_id,
-    ReadShortcutIconsCallback callback) const {
+    ReadShortcutsMenuIconsCallback callback) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   const WebApp* web_app = registrar_.GetAppById(app_id);
   if (!web_app) {
@@ -481,9 +551,9 @@ void WebAppIconManager::ReadAllShortcutIcons(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, kTaskTraits,
-      base::BindOnce(ReadShortcutIconsBlocking, utils_.get(),
+      base::BindOnce(ReadShortcutsMenuIconsBlocking, utils_.get(),
                      web_apps_directory_, app_id,
-                     web_app->downloaded_shortcut_icons_sizes()),
+                     web_app->downloaded_shortcuts_menu_icons_sizes()),
       std::move(callback));
 }
 

chrome/browser/web_applications/web_app_icon_manager.h

@@ -7,6 +7,7 @@
 
 #include <map>
 #include <memory>
+#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -31,12 +32,14 @@ class WebAppIconManager : public AppIconManager {
 
   // Writes all data (icons) for an app.
   using WriteDataCallback = base::OnceCallback<void(bool success)>;
-  // TODO(https://crbug.com/1069308): Create a dedicated WriteShortcutIconsData
-  // method here, so we can write shortcuts_icons_bitmaps separately.
+
   void WriteData(AppId app_id,
                  std::map<SquareSizePx, SkBitmap> icons,
-                 std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons,
                  WriteDataCallback callback);
+  void WriteShortcutsMenuIconsData(
+      AppId app_id,
+      std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons,
+      WriteDataCallback callback);
   void DeleteData(AppId app_id, WriteDataCallback callback);
 
   // AppIconManager:
@@ -50,8 +53,9 @@ class WebAppIconManager : public AppIconManager {
                  ReadIconsCallback callback) const override;
   void ReadAllIcons(const AppId& app_id,
                     ReadIconsCallback callback) const override;
-  void ReadAllShortcutIcons(const AppId& app_id,
-                            ReadShortcutIconsCallback callback) const override;
+  void ReadAllShortcutsMenuIcons(
+      const AppId& app_id,
+      ReadShortcutsMenuIconsCallback callback) const override;
   void ReadSmallestIcon(const AppId& app_id,
                         SquareSizePx icon_size_in_px,
                         ReadIconCallback callback) const override;

chrome/browser/web_applications/web_app_icon_manager_unittest.cc

@@ -6,6 +6,8 @@
 
 #include <algorithm>
 #include <memory>
+#include <string>
+#include <utility>
 
 #include "base/bind_helpers.h"
 #include "base/files/file_enumerator.h"
@@ -65,7 +67,6 @@ class WebAppIconManagerTest : public WebAppTest {
 
     base::RunLoop run_loop;
     icon_manager_->WriteData(app_id, std::move(web_app_info->icon_bitmaps),
-                             std::vector<std::map<SquareSizePx, SkBitmap>>(),
                              base::BindLambdaForTesting([&](bool success) {
                                EXPECT_TRUE(success);
                                run_loop.Quit();
@@ -73,38 +74,53 @@ class WebAppIconManagerTest : public WebAppTest {
     run_loop.Run();
   }
 
-  void WriteShortcutIcons(const AppId& app_id,
-                          const std::vector<int>& sizes_px,
-                          const std::vector<SkColor>& colors) {
+  void WriteShortcutsMenuIcons(const AppId& app_id,
+                               const std::vector<int>& sizes_px,
+                               const std::vector<SkColor>& colors) {
     DCHECK_EQ(sizes_px.size(), colors.size());
-    std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons;
+    std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons;
     for (size_t i = 0; i < sizes_px.size(); i++) {
-      std::map<SquareSizePx, SkBitmap> shortcut_icon_map;
+      std::map<SquareSizePx, SkBitmap> shortcuts_menu_icon_map;
       std::vector<SquareSizePx> icon_sizes;
-      shortcut_icon_map.emplace(sizes_px[i],
-                                CreateSquareIcon(sizes_px[i], colors[i]));
-      shortcut_icons.push_back(std::move(shortcut_icon_map));
+      shortcuts_menu_icon_map.emplace(sizes_px[i],
+                                      CreateSquareIcon(sizes_px[i], colors[i]));
+      shortcuts_menu_icons.push_back(std::move(shortcuts_menu_icon_map));
     }
+    base::RunLoop run_loop;
+    icon_manager_->WriteShortcutsMenuIconsData(
+        app_id, std::move(shortcuts_menu_icons),
+        base::BindLambdaForTesting([&](bool success) {
+          EXPECT_TRUE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
 
+  std::vector<std::map<SquareSizePx, SkBitmap>> ReadAllShortcutsMenuIcons(
+      const AppId& app_id) {
+    std::vector<std::map<SquareSizePx, SkBitmap>> result;
     base::RunLoop run_loop;
-    icon_manager_->WriteData(app_id, std::map<SquareSizePx, SkBitmap>(),
-                             shortcut_icons,
-                             base::BindLambdaForTesting([&](bool success) {
-                               EXPECT_TRUE(success);
-                               run_loop.Quit();
-                             }));
+    icon_manager().ReadAllShortcutsMenuIcons(
+        app_id, base::BindLambdaForTesting(
+                    [&](std::vector<std::map<SquareSizePx, SkBitmap>>
+                            shortcuts_menu_icons_map) {
+                      result = std::move(shortcuts_menu_icons_map);
+                      run_loop.Quit();
+                    }));
     run_loop.Run();
+    return result;
   }
 
-  std::vector<std::vector<SquareSizePx>> CreateDownloadedShortcutIconsSizes(
-      std::vector<SquareSizePx> sizes_px) {
-    std::vector<std::vector<SquareSizePx>> downloaded_shortcut_icons_sizes;
+  std::vector<std::vector<SquareSizePx>>
+  CreateDownloadedShortcutsMenuIconsSizes(std::vector<SquareSizePx> sizes_px) {
+    std::vector<std::vector<SquareSizePx>>
+        downloaded_shortcuts_menu_icons_sizes;
     for (const auto& size : sizes_px) {
       std::vector<SquareSizePx> icon_sizes;
       icon_sizes.push_back(size);
-      downloaded_shortcut_icons_sizes.push_back(std::move(icon_sizes));
+      downloaded_shortcuts_menu_icons_sizes.push_back(std::move(icon_sizes));
     }
-    return downloaded_shortcut_icons_sizes;
+    return downloaded_shortcuts_menu_icons_sizes;
   }
 
   std::vector<uint8_t> ReadSmallestCompressedIcon(const AppId& app_id,
@@ -245,7 +261,6 @@ TEST_F(WebAppIconManagerTest, OverwriteIcons) {
 
     // Overwrite red icons with green ones.
     icon_manager().WriteData(app_id, std::move(icon_bitmaps),
-                             std::vector<std::map<SquareSizePx, SkBitmap>>(),
                              base::BindLambdaForTesting([&](bool success) {
                                EXPECT_TRUE(success);
                                run_loop.Quit();
@@ -309,7 +324,28 @@ TEST_F(WebAppIconManagerTest, ReadAllIcons) {
   }
 }
 
-TEST_F(WebAppIconManagerTest, WriteAndReadAllShortcutIcons) {
+TEST_F(WebAppIconManagerTest, ReadShortcutsMenuIconsFailed) {
+  auto web_app = CreateWebApp();
+  const AppId app_id = web_app->app_id();
+
+  const std::vector<SquareSizePx> sizes_px{icon_size::k96, icon_size::k256};
+
+  // Set shortcuts menu icons meta-info but don't write bitmaps to disk.
+  web_app->SetDownloadedShortcutsMenuIconsSizes(
+      CreateDownloadedShortcutsMenuIconsSizes(sizes_px));
+
+  controller().RegisterApp(std::move(web_app));
+
+  // Request shortcuts menu icons which don't exist on disk.
+  std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons_map =
+      ReadAllShortcutsMenuIcons(app_id);
+  EXPECT_EQ(sizes_px.size(), shortcuts_menu_icons_map.size());
+  for (const auto& icon_map : shortcuts_menu_icons_map) {
+    EXPECT_EQ(0u, icon_map.size());
+  }
+}
+
+TEST_F(WebAppIconManagerTest, WriteAndReadAllShortcutsMenuIcons) {
   auto web_app = CreateWebApp();
   const AppId app_id = web_app->app_id();
 
@@ -318,35 +354,50 @@ TEST_F(WebAppIconManagerTest, WriteAndReadAllShortcutIcons) {
   const std::vector<SkColor> colors = {SK_ColorRED, SK_ColorWHITE,
                                        SK_ColorBLUE};
 
-  WriteShortcutIcons(app_id, sizes_px, colors);
+  WriteShortcutsMenuIcons(app_id, sizes_px, colors);
 
-  web_app->SetDownloadedShortcutIconsSizes(
-      CreateDownloadedShortcutIconsSizes(sizes_px));
+  web_app->SetDownloadedShortcutsMenuIconsSizes(
+      CreateDownloadedShortcutsMenuIconsSizes(sizes_px));
 
   controller().RegisterApp(std::move(web_app));
-  {
-    base::RunLoop run_loop;
 
-    icon_manager().ReadAllShortcutIcons(
-        app_id,
-        base::BindLambdaForTesting(
-            [&](std::vector<std::map<SquareSizePx, SkBitmap>>
-                    shortcut_icons_map) {
-              EXPECT_EQ(3u, shortcut_icons_map.size());
-              EXPECT_EQ(sizes_px[0], shortcut_icons_map[0].begin()->first);
-              EXPECT_EQ(colors[0],
-                        shortcut_icons_map[0].begin()->second.getColor(0, 0));
-              EXPECT_EQ(sizes_px[1], shortcut_icons_map[1].begin()->first);
-              EXPECT_EQ(colors[1],
-                        shortcut_icons_map[1].begin()->second.getColor(0, 0));
-              EXPECT_EQ(sizes_px[2], shortcut_icons_map[2].begin()->first);
-              EXPECT_EQ(colors[2],
-                        shortcut_icons_map[2].begin()->second.getColor(0, 0));
-              run_loop.Quit();
-            }));
+  std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons_map =
+      ReadAllShortcutsMenuIcons(app_id);
+  EXPECT_EQ(3u, shortcuts_menu_icons_map.size());
+  EXPECT_EQ(sizes_px[0], shortcuts_menu_icons_map[0].begin()->first);
+  EXPECT_EQ(colors[0],
+            shortcuts_menu_icons_map[0].begin()->second.getColor(0, 0));
+  EXPECT_EQ(sizes_px[1], shortcuts_menu_icons_map[1].begin()->first);
+  EXPECT_EQ(colors[1],
+            shortcuts_menu_icons_map[1].begin()->second.getColor(0, 0));
+  EXPECT_EQ(sizes_px[2], shortcuts_menu_icons_map[2].begin()->first);
+  EXPECT_EQ(colors[2],
+            shortcuts_menu_icons_map[2].begin()->second.getColor(0, 0));
+}
 
-    run_loop.Run();
-  }
+TEST_F(WebAppIconManagerTest, WriteShortcutsMenuIconsEmptyMap) {
+  auto web_app = CreateWebApp();
+  const AppId app_id = web_app->app_id();
+
+  web_app->SetDownloadedShortcutsMenuIconsSizes(
+      CreateDownloadedShortcutsMenuIconsSizes(std::vector<SquareSizePx>{}));
+
+  controller().RegisterApp(std::move(web_app));
+
+  std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons;
+  base::RunLoop run_loop;
+  icon_manager().WriteShortcutsMenuIconsData(
+      app_id, std::move(shortcuts_menu_icons),
+      base::BindLambdaForTesting([&](bool success) {
+        EXPECT_FALSE(success);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+
+  // Make sure that nothing was written to disk.
+  std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons_map =
+      ReadAllShortcutsMenuIcons(app_id);
+  EXPECT_EQ(0u, shortcuts_menu_icons_map.size());
 }
 
 TEST_F(WebAppIconManagerTest, ReadIconsFailed) {

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -242,10 +242,10 @@ void WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync(
 
   icon_manager_->WriteData(
       std::move(app_id), std::move(icon_bitmaps),
-      std::vector<std::map<SquareSizePx, SkBitmap>>(),
       base::BindOnce(&WebAppInstallFinalizer::OnIconsDataWritten,
                      weak_ptr_factory_.GetWeakPtr(),
                      std::move(fallback_install_callback), std::move(web_app),
+                     std::vector<std::map<SquareSizePx, SkBitmap>>(),
                      /*is_new_install=*/true));
 }
 
@@ -424,38 +424,63 @@ void WebAppInstallFinalizer::SetWebAppManifestFieldsAndWriteData(
   web_app->SetIconInfos(web_app_info.icon_infos);
   web_app->SetDownloadedIconSizes(GetSquareSizePxs(web_app_info.icon_bitmaps));
 
-  std::vector<WebApp::WebAppShortcutMenuItemInfo> web_app_shortcut_infos;
-  std::vector<std::vector<SquareSizePx>> downloaded_shortcut_icons_sizes;
+  std::vector<WebApp::WebAppShortcutsMenuItemInfo> web_app_shortcut_infos;
+  std::vector<std::vector<SquareSizePx>> downloaded_shortcuts_menu_icons_sizes;
   for (const auto& shortcut : web_app_info.shortcut_infos) {
-    WebApp::WebAppShortcutMenuItemInfo shortcut_info;
+    WebApp::WebAppShortcutsMenuItemInfo shortcut_info;
     shortcut_info.name = shortcut.name;
     shortcut_info.url = shortcut.url;
-    shortcut_info.shortcut_icon_infos = shortcut.shortcut_icon_infos;
+    shortcut_info.shortcuts_menu_icon_infos = shortcut.shortcut_icon_infos;
     web_app_shortcut_infos.push_back(shortcut_info);
-    downloaded_shortcut_icons_sizes.push_back(
+    downloaded_shortcuts_menu_icons_sizes.push_back(
         GetSquareSizePxs(shortcut.shortcut_icon_bitmaps));
   }
   web_app->SetShortcutInfos(std::move(web_app_shortcut_infos));
-  web_app->SetDownloadedShortcutIconsSizes(
-      std::move(downloaded_shortcut_icons_sizes));
+  web_app->SetDownloadedShortcutsMenuIconsSizes(
+      std::move(downloaded_shortcuts_menu_icons_sizes));
 
   SetWebAppFileHandlers(web_app_info.file_handlers, web_app.get());
 
   AppId app_id = web_app->app_id();
 
-  std::vector<std::map<SquareSizePx, SkBitmap>> shortcut_icons_bitmaps;
+  std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons_bitmaps;
   for (const auto& shortcut : web_app_info.shortcut_infos)
-    shortcut_icons_bitmaps.push_back(shortcut.shortcut_icon_bitmaps);
+    shortcuts_menu_icons_bitmaps.push_back(shortcut.shortcut_icon_bitmaps);
 
   icon_manager_->WriteData(
       std::move(app_id), web_app_info.icon_bitmaps,
-      std::move(shortcut_icons_bitmaps),
       base::BindOnce(&WebAppInstallFinalizer::OnIconsDataWritten,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     std::move(web_app), is_new_install));
+                     std::move(web_app),
+                     std::move(shortcuts_menu_icons_bitmaps), is_new_install));
 }
 
 void WebAppInstallFinalizer::OnIconsDataWritten(
+    InstallFinalizedCallback callback,
+    std::unique_ptr<WebApp> web_app,
+    std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons,
+    bool is_new_install,
+    bool success) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!success) {
+    std::move(callback).Run(AppId(), InstallResultCode::kWriteDataFailed);
+    return;
+  }
+
+  if (shortcuts_menu_icons.empty()) {
+    OnShortcutsMenuIconsDataWritten(std::move(callback), std::move(web_app),
+                                    is_new_install, success);
+  } else {
+    AppId app_id = web_app->app_id();
+    icon_manager_->WriteShortcutsMenuIconsData(
+        app_id, shortcuts_menu_icons,
+        base::BindOnce(&WebAppInstallFinalizer::OnShortcutsMenuIconsDataWritten,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                       std::move(web_app), is_new_install));
+  }
+}
+
+void WebAppInstallFinalizer::OnShortcutsMenuIconsDataWritten(
     InstallFinalizedCallback callback,
     std::unique_ptr<WebApp> web_app,
     bool is_new_install,

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -5,15 +5,18 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_FINALIZER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_FINALIZER_H_
 
+#include <map>
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/common/web_application_info.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 class Profile;
-struct WebApplicationInfo;
 
 namespace web_app {
 
@@ -63,10 +66,18 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
       bool is_new_install,
       InstallFinalizedCallback callback);
 
-  void OnIconsDataWritten(InstallFinalizedCallback callback,
-                          std::unique_ptr<WebApp> web_app,
-                          bool is_new_install,
-                          bool success);
+  void OnIconsDataWritten(
+      InstallFinalizedCallback callback,
+      std::unique_ptr<WebApp> web_app,
+      std::vector<std::map<SquareSizePx, SkBitmap>> shortcuts_menu_icons,
+      bool is_new_install,
+      bool success);
+
+  void OnShortcutsMenuIconsDataWritten(InstallFinalizedCallback callback,
+                                       std::unique_ptr<WebApp> web_app,
+                                       bool is_new_install,
+                                       bool success);
+
   void OnIconsDataDeleted(const AppId& app_id,
                           UninstallWebAppCallback callback,
                           bool success);

chrome/browser/web_applications/web_app_migration_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/web_applications/web_app_migration_manager.h"
 
+#include <map>
 #include <utility>
 
 #include "base/bind.h"
@@ -121,7 +122,6 @@ void WebAppMigrationManager::OnBookmarkAppIconsRead(
   // old extension's representation to new web app representation (project BMO).
   web_app_icon_manager_->WriteData(
       app_id, std::move(icon_bitmaps),
-      std::vector<std::map<SquareSizePx, SkBitmap>>(),
       base::BindOnce(&WebAppMigrationManager::OnWebAppIconsWritten,
                      weak_ptr_factory_.GetWeakPtr()));
 }