WebApp: Download icons for Web App installation.

- Use WebAppIconDownloader in WebAppDataRetriever.
- Extract InstallFinalizer interface:
  WebAppInstallFinalizer is a BMO implementation.
  BookmarkAppInstallFinalizer will use extensions and CrxInstaller.

Bug: 901226
Change-Id: Ie865bfd4facfc6f890ab92d5d6451a37a34979ea
Reviewed-on: https://chromium-review.googlesource.com/c/1343410Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609958}

chrome/browser/web_applications/BUILD.gn

@@ -23,6 +23,8 @@ source_set("web_applications") {
     "web_app_database.h",
     "web_app_database_factory.cc",
     "web_app_database_factory.h",
+    "web_app_install_finalizer.cc",
+    "web_app_install_finalizer.h",
     "web_app_install_manager.cc",
     "web_app_install_manager.h",
     "web_app_registrar.cc",
@@ -52,6 +54,8 @@ source_set("web_applications_test_support") {
   sources = [
     "test/test_data_retriever.cc",
     "test/test_data_retriever.h",
+    "test/test_install_finalizer.cc",
+    "test/test_install_finalizer.h",
     "test/test_system_web_app_manager.cc",
     "test/test_system_web_app_manager.h",
     "test/test_web_app_database.cc",

chrome/browser/web_applications/components/BUILD.gn

@@ -4,6 +4,7 @@
 
 source_set("components") {
   sources = [
+    "install_finalizer.h",
     "install_manager.h",
     "pending_app_manager.cc",
     "pending_app_manager.h",

chrome/browser/web_applications/components/install_finalizer.h

+// Copyright 2018 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 CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_INSTALL_FINALIZER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_INSTALL_FINALIZER_H_
+
+#include <memory>
+
+#include "base/callback_forward.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+
+struct WebApplicationInfo;
+
+namespace web_app {
+
+enum class InstallResultCode;
+
+// An abstract finalizer for the installation process, represents the last step.
+// Takes WebApplicationInfo as input, writes data to disk (e.g icons, shortcuts)
+// and registers an app.
+class InstallFinalizer {
+ public:
+  using InstallFinalizedCallback =
+      base::OnceCallback<void(const AppId& app_id, InstallResultCode code)>;
+
+  virtual void FinalizeInstall(std::unique_ptr<WebApplicationInfo> web_app_info,
+                               InstallFinalizedCallback callback) = 0;
+
+  virtual ~InstallFinalizer() = default;
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_INSTALL_FINALIZER_H_

chrome/browser/web_applications/components/web_app_data_retriever.cc

@@ -15,8 +15,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/installable/installable_data.h"
 #include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/web_applications/components/web_app_icon_downloader.h"
 #include "chrome/browser/web_applications/components/web_app_icon_generator.h"
-#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "chrome/common/chrome_render_frame.mojom.h"
 #include "chrome/common/web_application_info.h"
 #include "content/public/browser/navigation_entry.h"
@@ -28,29 +28,6 @@
 
 namespace web_app {
 
-namespace {
-
-char GetLetterForIcon(const GURL& app_url) {
-  char icon_letter = ' ';
-  std::string domain_and_registry(
-      net::registry_controlled_domains::GetDomainAndRegistry(
-          app_url,
-          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
-
-  // TODO(crbug.com/867311): Decode the app URL or the domain before retrieving
-  // the first character, otherwise we generate an icon with "x" if the domain
-  // or app URL starts with a UTF-8 character.
-  if (!domain_and_registry.empty()) {
-    icon_letter = domain_and_registry[0];
-  } else if (app_url.has_host()) {
-    icon_letter = app_url.host_piece()[0];
-  }
-  DCHECK(icon_letter >= '!' && icon_letter <= '~');
-  return icon_letter;
-}
-
-}  // namespace
-
 WebAppDataRetriever::WebAppDataRetriever() = default;
 
 WebAppDataRetriever::~WebAppDataRetriever() = default;
@@ -110,25 +87,23 @@ void WebAppDataRetriever::CheckInstallabilityAndRetrieveManifest(
                   weak_ptr_factory_.GetWeakPtr(), base::Passed(&callback)));
 }
 
-void WebAppDataRetriever::GetIcons(const GURL& app_url,
+void WebAppDataRetriever::GetIcons(content::WebContents* web_contents,
                                    const std::vector<GURL>& icon_urls,
+                                   bool skip_page_fav_icons,
                                    GetIconsCallback callback) {
-  // TODO(crbug.com/864904): Download icons using |icon_urls|.
-
-  const char icon_letter = GetLetterForIcon(app_url);
-  const std::set<int> sizes_to_generate = SizesToGenerate();
-
-  std::vector<WebApplicationInfo::IconInfo> icons;
-  for (int size : sizes_to_generate) {
-    WebApplicationInfo::IconInfo icon_info;
-    icon_info.width = size;
-    icon_info.height = size;
-    icon_info.data = GenerateBitmap(size, SK_ColorDKGRAY, icon_letter);
-    icons.push_back(icon_info);
-  }
+  DCHECK(!icon_urls.empty());
 
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(icons)));
+  // TODO(loyso): Refactor WebAppIconDownloader: crbug.com/907296.
+  icon_downloader_ = std::make_unique<WebAppIconDownloader>(
+      web_contents, icon_urls,
+      "Extensions.BookmarkApp.Icon.HttpStatusCodeClassOnCreate",
+      base::BindOnce(&WebAppDataRetriever::OnIconsDownloaded,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+
+  if (skip_page_fav_icons)
+    icon_downloader_->SkipPageFavicons();
+
+  icon_downloader_->Start();
 }
 
 void WebAppDataRetriever::OnGetWebApplicationInfo(
@@ -169,4 +144,18 @@ void WebAppDataRetriever::OnDidPerformInstallableCheck(
   std::move(callback).Run(*data.manifest, is_installable);
 }
 
+void WebAppDataRetriever::OnIconsDownloaded(GetIconsCallback callback,
+                                            bool success,
+                                            const IconsMap& icons_map) {
+  // |icons_map| is owned by |icon_downloader_|. Take a copy before destroying
+  // the downloader. Return empty |result_map| if the tab has navigated away
+  // during the icon download.
+  IconsMap result_map;
+  if (success)
+    result_map = icons_map;
+  icon_downloader_.reset();
+
+  std::move(callback).Run(std::move(result_map));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/web_app_data_retriever.h

@@ -5,13 +5,17 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_WEB_APP_DATA_RETRIEVER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_WEB_APP_DATA_RETRIEVER_H_
 
+#include <map>
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "chrome/common/chrome_render_frame.mojom.h"
 
+class GURL;
 struct InstallableData;
 struct WebApplicationInfo;
 
@@ -25,6 +29,8 @@ class WebContents;
 
 namespace web_app {
 
+class WebAppIconDownloader;
+
 // Class used by BookmarkAppInstallationTask to retrieve the necessary
 // information to install an app. Should only be called from the UI thread.
 class WebAppDataRetriever {
@@ -35,9 +41,8 @@ class WebAppDataRetriever {
   // |is_installable| is false if installability check failed.
   using CheckInstallabilityCallback =
       base::OnceCallback<void(const blink::Manifest&, bool is_installable)>;
-  // Returns empty vector if error.
-  using GetIconsCallback =
-      base::OnceCallback<void(std::vector<WebApplicationInfo::IconInfo>)>;
+  // Returns empty map if error.
+  using GetIconsCallback = base::OnceCallback<void(IconsMap)>;
 
   WebAppDataRetriever();
   virtual ~WebAppDataRetriever();
@@ -52,11 +57,11 @@ class WebAppDataRetriever {
       content::WebContents* web_contents,
       CheckInstallabilityCallback callback);
 
-  // Downloads icons from |icon_urls|. If icons are missing for certain required
-  // sizes, generates them based on |app_url|. Runs |callback| with a vector of
-  // the retrieved and generated icons.
-  virtual void GetIcons(const GURL& app_url,
+  // Downloads icons from |icon_urls|. Runs |callback| with a map of
+  // the retrieved icons.
+  virtual void GetIcons(content::WebContents* web_contents,
                         const std::vector<GURL>& icon_urls,
+                        bool skip_page_fav_icons,
                         GetIconsCallback callback);
 
  private:
@@ -70,9 +75,15 @@ class WebAppDataRetriever {
   void OnDidPerformInstallableCheck(CheckInstallabilityCallback callback,
                                     const InstallableData& data);
 
+  void OnIconsDownloaded(GetIconsCallback callback,
+                         bool success,
+                         const IconsMap& icons_map);
+
   // Saved callback from GetWebApplicationInfo().
   GetWebApplicationInfoCallback get_web_app_info_callback_;
 
+  std::unique_ptr<WebAppIconDownloader> icon_downloader_;
+
   base::WeakPtrFactory<WebAppDataRetriever> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(WebAppDataRetriever);

chrome/browser/web_applications/components/web_app_data_retriever_unittest.cc

@@ -15,7 +15,6 @@
 #include "chrome/browser/installable/fake_installable_manager.h"
 #include "chrome/browser/installable/installable_data.h"
 #include "chrome/browser/installable/installable_manager.h"
-#include "chrome/browser/web_applications/components/web_app_icon_generator.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/navigation_entry.h"
@@ -36,11 +35,6 @@ const char kFooUrl2[] = "https://foo.example/bar";
 const char kFooTitle[] = "Foo Title";
 const char kBarUrl[] = "https://bar.example";
 
-constexpr int kIconSizesToGenerate[] = {
-    icon_size::k32, icon_size::k64,  icon_size::k48,
-    icon_size::k96, icon_size::k128, icon_size::k256,
-};
-
 }  // namespace
 
 class FakeChromeRenderFrame
@@ -279,32 +273,6 @@ TEST_F(WebAppDataRetrieverTest, GetWebApplicationInfo_FrameNavigated) {
   EXPECT_EQ(nullptr, web_app_info());
 }
 
-TEST_F(WebAppDataRetrieverTest, GetIcons_NoIconsProvided) {
-  base::RunLoop run_loop;
-  WebAppDataRetriever retriever;
-  retriever.GetIcons(
-      GURL(kFooUrl), std::vector<GURL>(),
-      base::BindOnce(&WebAppDataRetrieverTest::GetIconsCallback,
-                     base::Unretained(this), run_loop.QuitClosure()));
-  run_loop.Run();
-
-  // Make sure that icons have been generated for all sizes.
-  for (int size : kIconSizesToGenerate) {
-    int generated_icons_for_size =
-        std::count_if(icons().begin(), icons().end(),
-                      [&size](const WebApplicationInfo::IconInfo& icon) {
-                        return icon.width == size && icon.height == size;
-                      });
-    EXPECT_EQ(1, generated_icons_for_size);
-  }
-
-  for (const auto& icon : icons()) {
-    EXPECT_FALSE(icon.data.drawsNothing());
-    // Since all icons are generated, they should have an empty url.
-    EXPECT_TRUE(icon.url.is_empty());
-  }
-}
-
 TEST_F(WebAppDataRetrieverTest, CheckInstallabilityAndRetrieveManifest) {
   const GURL manifest_start_url = GURL("https://example.com/start");
   const std::string manifest_short_name = "Short Name from Manifest";

chrome/browser/web_applications/components/web_app_icon_downloader.cc

@@ -95,7 +95,7 @@ void WebAppIconDownloader::FetchIcons(const std::vector<GURL>& urls) {
   // callback.
   if (in_progress_requests_.empty() && !need_favicon_urls_) {
     base::MessageLoopCurrent::Get()->task_runner()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback_), true, favicon_map_));
+        FROM_HERE, base::BindOnce(std::move(callback_), true, icons_map_));
   }
 }
 
@@ -116,11 +116,11 @@ void WebAppIconDownloader::DidDownloadFavicon(
                                   http_status_code / 100, 5);
   }
 
-  favicon_map_[image_url] = bitmaps;
+  icons_map_[image_url] = bitmaps;
 
   // Once all requests have been resolved, perform post-download tasks.
   if (in_progress_requests_.empty() && !need_favicon_urls_)
-    std::move(callback_).Run(true, favicon_map_);
+    std::move(callback_).Run(true, icons_map_);
 }
 
 // content::WebContentsObserver overrides:
@@ -132,8 +132,8 @@ void WebAppIconDownloader::DidFinishNavigation(
 
   // Clear all pending requests.
   in_progress_requests_.clear();
-  favicon_map_.clear();
-  std::move(callback_).Run(false, favicon_map_);
+  icons_map_.clear();
+  std::move(callback_).Run(false, icons_map_);
 }
 
 void WebAppIconDownloader::DidUpdateFaviconURL(

chrome/browser/web_applications/components/web_app_icon_downloader.h

@@ -13,6 +13,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "content/public/browser/web_contents_observer.h"
 
 class SkBitmap;
@@ -36,12 +37,9 @@ namespace web_app {
 // icons) for a tab.
 class WebAppIconDownloader : public content::WebContentsObserver {
  public:
-  typedef std::map<GURL, std::vector<SkBitmap> > FaviconMap;
-  typedef base::OnceCallback<void(
-      bool, /* success */
-      /* A map of icon urls to the bitmaps provided by that url. */
-      const FaviconMap&)>
-      WebAppIconDownloaderCallback;
+  using WebAppIconDownloaderCallback =
+      base::OnceCallback<void(bool success, const IconsMap& icons_map)>;
+
   // |extra_favicon_urls| allows callers to provide icon urls that aren't
   // provided by the renderer (e.g touch icons on non-android environments).
   // |https_status_code_class_histogram_name| optionally specifies a histogram
@@ -100,7 +98,7 @@ class WebAppIconDownloader : public content::WebContentsObserver {
   std::vector<GURL> extra_favicon_urls_;
 
   // The icons which were downloaded. Populated by FetchIcons().
-  FaviconMap favicon_map_;
+  IconsMap icons_map_;
 
   // Request ids of in-progress requests.
   std::set<int> in_progress_requests_;

chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc

@@ -73,13 +73,12 @@ class TestWebAppIconDownloader : public WebAppIconDownloader {
 
   size_t pending_requests() const { return in_progress_requests_.size(); }
 
-  void DownloadsComplete(bool success,
-                         const WebAppIconDownloader::FaviconMap& map) {
+  void DownloadsComplete(bool success, const IconsMap& map) {
     downloads_succeeded_ = success;
     favicon_map_ = map;
   }
 
-  WebAppIconDownloader::FaviconMap favicon_map() const { return favicon_map_; }
+  IconsMap favicon_map() const { return favicon_map_; }
 
   void CompleteImageDownload(
       int id,
@@ -102,7 +101,7 @@ class TestWebAppIconDownloader : public WebAppIconDownloader {
 
  private:
   std::vector<content::FaviconURL> initial_favicon_urls_;
-  WebAppIconDownloader::FaviconMap favicon_map_;
+  IconsMap favicon_map_;
   int id_counter_;
   base::Optional<bool> downloads_succeeded_;
 

chrome/browser/web_applications/components/web_app_icon_generator.cc

@@ -108,6 +108,10 @@ void GenerateIcons(std::set<int> generate_sizes,
       net::registry_controlled_domains::GetDomainAndRegistry(
           app_url,
           net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
+
+  // TODO(crbug.com/867311): Decode the app URL or the domain before retrieving
+  // the first character, otherwise we generate an icon with "x" if the domain
+  // or app URL starts with a UTF-8 character.
   if (!domain_and_registry.empty()) {
     icon_letter = domain_and_registry[0];
   } else if (app_url.has_host()) {

chrome/browser/web_applications/components/web_app_install_utils.cc

@@ -14,7 +14,7 @@ namespace web_app {
 
 namespace {
 
-void ReplaceWebAppIcons(std::map<int, web_app::BitmapAndSource> bitmap_map,
+void ReplaceWebAppIcons(std::map<int, BitmapAndSource> bitmap_map,
                         WebApplicationInfo* web_app_info) {
   web_app_info->icons.clear();
 
@@ -113,10 +113,10 @@ void MergeInstallableDataIcon(const InstallableData& data,
 }
 
 std::vector<BitmapAndSource> FilterSquareIcons(
-    const std::map<GURL, std::vector<SkBitmap>>& bitmaps,
+    const IconsMap& icons_map,
     const WebApplicationInfo& web_app_info) {
   std::vector<BitmapAndSource> downloaded_icons;
-  for (const std::pair<GURL, std::vector<SkBitmap>>& url_bitmap : bitmaps) {
+  for (const std::pair<GURL, std::vector<SkBitmap>>& url_bitmap : icons_map) {
     for (const SkBitmap& bitmap : url_bitmap.second) {
       if (bitmap.empty() || bitmap.width() != bitmap.height())
         continue;

chrome/browser/web_applications/components/web_app_install_utils.h

@@ -29,6 +29,9 @@ enum class ForInstallableSite {
   kUnknown,
 };
 
+// A map of icon urls to the bitmaps provided by that url.
+using IconsMap = std::map<GURL, std::vector<SkBitmap>>;
+
 // Update the given WebApplicationInfo with information from the manifest.
 void UpdateWebAppInfoFromManifest(const blink::Manifest& manifest,
                                   WebApplicationInfo* web_app_info,
@@ -49,10 +52,10 @@ std::vector<GURL> GetValidIconUrlsToDownload(
 void MergeInstallableDataIcon(const InstallableData& data,
                               WebApplicationInfo* web_app_info);
 
-// Get a list of non-empty square icons from downloaded |bitmaps| and
+// Get a list of non-empty square icons from downloaded |icons_map| and
 // |web_app_info| (merged together).
 std::vector<BitmapAndSource> FilterSquareIcons(
-    const std::map<GURL, std::vector<SkBitmap>>& bitmaps,
+    const IconsMap& icons_map,
     const WebApplicationInfo& web_app_info);
 
 // Ensure that the necessary-sized icons are available by resizing larger

chrome/browser/web_applications/test/test_data_retriever.cc

@@ -23,12 +23,17 @@ void TestDataRetriever::GetWebApplicationInfo(
       FROM_HERE, base::BindOnce(std::move(callback), std::move(web_app_info_)));
 }
 
-void TestDataRetriever::GetIcons(const GURL& app_url,
+void TestDataRetriever::GetIcons(content::WebContents* web_contents,
                                  const std::vector<GURL>& icon_urls,
+                                 bool skip_page_fav_icons,
                                  GetIconsCallback callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback),
-                                std::vector<WebApplicationInfo::IconInfo>()));
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(icons_map_)));
+  icons_map_.clear();
+}
+
+void TestDataRetriever::SetIcons(IconsMap icons_map) {
+  icons_map_ = std::move(icons_map);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/test/test_data_retriever.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/web_applications/components/web_app_data_retriever.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 
 struct WebApplicationInfo;
 
@@ -21,15 +22,20 @@ class TestDataRetriever : public WebAppDataRetriever {
   explicit TestDataRetriever(std::unique_ptr<WebApplicationInfo> web_app_info);
   ~TestDataRetriever() override;
 
+  // WebAppDataRetriever:
   void GetWebApplicationInfo(content::WebContents* web_contents,
                              GetWebApplicationInfoCallback callback) override;
-
-  void GetIcons(const GURL& app_url,
+  void GetIcons(content::WebContents* web_contents,
                 const std::vector<GURL>& icon_urls,
+                bool skip_page_fav_icons,
                 GetIconsCallback callback) override;
 
+  // Set icons to respond on |GetIcons|.
+  void SetIcons(IconsMap icons_map);
+
  private:
   std::unique_ptr<WebApplicationInfo> web_app_info_;
+  IconsMap icons_map_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDataRetriever);
 };

chrome/browser/web_applications/test/test_install_finalizer.cc

+// Copyright 2018 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.
+
+#include <utility>
+
+#include "chrome/browser/web_applications/test/test_install_finalizer.h"
+
+#include "base/callback.h"
+#include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/common/web_application_info.h"
+
+namespace web_app {
+
+TestInstallFinalizer::TestInstallFinalizer() {}
+
+TestInstallFinalizer::~TestInstallFinalizer() = default;
+
+void TestInstallFinalizer::FinalizeInstall(
+    std::unique_ptr<WebApplicationInfo> web_app_info,
+    InstallFinalizedCallback callback) {
+  const AppId app_id = GenerateAppIdFromURL(web_app_info->app_url);
+
+  web_app_info_ = std::move(web_app_info);
+  std::move(callback).Run(app_id, InstallResultCode::kSuccess);
+}
+
+}  // namespace web_app

chrome/browser/web_applications/test/test_install_finalizer.h

+// Copyright 2018 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 CHROME_BROWSER_WEB_APPLICATIONS_TEST_TEST_INSTALL_FINALIZER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_TEST_TEST_INSTALL_FINALIZER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
+
+struct WebApplicationInfo;
+
+namespace web_app {
+
+class TestInstallFinalizer final : public InstallFinalizer {
+ public:
+  TestInstallFinalizer();
+  ~TestInstallFinalizer() override;
+
+  // InstallFinalizer:
+  void FinalizeInstall(std::unique_ptr<WebApplicationInfo> web_app_info,
+                       InstallFinalizedCallback callback) override;
+
+  std::unique_ptr<WebApplicationInfo> web_app_info() {
+    return std::move(web_app_info_);
+  }
+
+ private:
+  std::unique_ptr<WebApplicationInfo> web_app_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestInstallFinalizer);
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_TEST_TEST_INSTALL_FINALIZER_H_

chrome/browser/web_applications/web_app_install_finalizer.cc

+// Copyright 2018 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.
+
+#include <utility>
+
+#include "chrome/browser/web_applications/web_app_install_finalizer.h"
+
+#include "base/callback.h"
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_registrar.h"
+#include "chrome/common/web_application_info.h"
+
+namespace web_app {
+
+WebAppInstallFinalizer::WebAppInstallFinalizer(WebAppRegistrar* registrar)
+    : registrar_(registrar) {}
+
+WebAppInstallFinalizer::~WebAppInstallFinalizer() = default;
+
+void WebAppInstallFinalizer::FinalizeInstall(
+    std::unique_ptr<WebApplicationInfo> web_app_info,
+    InstallFinalizedCallback callback) {
+  const AppId app_id = GenerateAppIdFromURL(web_app_info->app_url);
+  auto web_app = std::make_unique<WebApp>(app_id);
+
+  web_app->SetName(base::UTF16ToUTF8(web_app_info->title));
+  web_app->SetDescription(base::UTF16ToUTF8(web_app_info->description));
+  web_app->SetLaunchUrl(web_app_info->app_url);
+  web_app->SetScope(web_app_info->scope);
+  web_app->SetThemeColor(web_app_info->theme_color);
+
+  // TODO(loyso): Add web_app_info->icons into web_app. Save them on disk.
+
+  registrar_->RegisterApp(std::move(web_app));
+
+  std::move(callback).Run(app_id, InstallResultCode::kSuccess);
+}
+
+}  // namespace web_app

chrome/browser/web_applications/web_app_install_finalizer.h

+// Copyright 2018 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 CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_FINALIZER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_FINALIZER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
+
+struct WebApplicationInfo;
+
+namespace web_app {
+
+class WebAppRegistrar;
+
+class WebAppInstallFinalizer final : public InstallFinalizer {
+ public:
+  explicit WebAppInstallFinalizer(WebAppRegistrar* registrar);
+  ~WebAppInstallFinalizer() override;
+
+  // InstallFinalizer:
+  void FinalizeInstall(std::unique_ptr<WebApplicationInfo> web_app_info,
+                       InstallFinalizedCallback callback) override;
+
+ private:
+  WebAppRegistrar* registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebAppInstallFinalizer);
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_FINALIZER_H_

chrome/browser/web_applications/web_app_install_manager.cc

@@ -8,26 +8,24 @@
 
 #include "base/callback.h"
 #include "base/logging.h"
-#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_data_retriever.h"
-#include "chrome/browser/web_applications/components/web_app_helpers.h"
-#include "chrome/browser/web_applications/components/web_app_install_utils.h"
-#include "chrome/browser/web_applications/web_app.h"
-#include "chrome/browser/web_applications/web_app_registrar.h"
+#include "chrome/browser/web_applications/components/web_app_icon_generator.h"
 #include "chrome/browser/web_applications/web_app_utils.h"
 #include "chrome/common/web_application_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
+#include "third_party/blink/public/common/manifest/manifest.h"
 
 namespace web_app {
 
-WebAppInstallManager::WebAppInstallManager(Profile* profile,
-                                           WebAppRegistrar* registrar)
-    : profile_(profile),
-      registrar_(registrar),
-      data_retriever_(std::make_unique<WebAppDataRetriever>()) {
-  DCHECK(AllowWebAppInstallation(profile_));
+WebAppInstallManager::WebAppInstallManager(
+    Profile* profile,
+    std::unique_ptr<InstallFinalizer> install_finalizer)
+    : data_retriever_(std::make_unique<WebAppDataRetriever>()),
+      install_finalizer_(std::move(install_finalizer)) {
+  DCHECK(AllowWebAppInstallation(profile));
 }
 
 WebAppInstallManager::~WebAppInstallManager() = default;
@@ -64,6 +62,11 @@ void WebAppInstallManager::SetDataRetrieverForTesting(
   data_retriever_ = std::move(data_retriever);
 }
 
+void WebAppInstallManager::SetInstallFinalizerForTesting(
+    std::unique_ptr<InstallFinalizer> install_finalizer) {
+  install_finalizer_ = std::move(install_finalizer);
+}
+
 void WebAppInstallManager::CallInstallCallback(const AppId& app_id,
                                                InstallResultCode code) {
   Observe(nullptr);
@@ -119,26 +122,49 @@ void WebAppInstallManager::OnDidPerformInstallableCheck(
       is_installable && !force_shortcut_app ? ForInstallableSite::kYes
                                             : ForInstallableSite::kNo;
 
-  // TODO(loyso): Implement installation logic from BookmarkAppHelper:
-  // - UpdateShareTargetInPrefs.
-  // - WebAppIconDownloader.
-  // etc
+  // TODO(loyso): Implement UpdateShareTargetInPrefs installation logic.
 
   UpdateWebAppInfoFromManifest(manifest, web_app_info.get(),
                                for_installable_site);
 
-  const AppId app_id = GenerateAppIdFromURL(web_app_info->app_url);
-  auto web_app = std::make_unique<WebApp>(app_id);
+  std::vector<GURL> icon_urls;
+  for (auto& icon_info : web_app_info->icons) {
+    if (icon_info.url.is_valid())
+      icon_urls.push_back(icon_info.url);
+  }
+
+  // If the manifest specified icons, don't use the page icons.
+  const bool skip_page_fav_icons = !manifest.icons.empty();
+
+  data_retriever_->GetIcons(
+      web_contents(), icon_urls, skip_page_fav_icons,
+      base::BindOnce(&WebAppInstallManager::OnIconsRetrieved,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(web_app_info)));
+}
+
+void WebAppInstallManager::OnIconsRetrieved(
+    std::unique_ptr<WebApplicationInfo> web_app_info,
+    IconsMap icons_map) {
+  // If interrupted, install_callback_ is already invoked or may invoke later.
+  if (InstallInterrupted())
+    return;
+
+  DCHECK(web_app_info);
 
-  web_app->SetName(base::UTF16ToUTF8(web_app_info->title));
-  web_app->SetDescription(base::UTF16ToUTF8(web_app_info->description));
-  web_app->SetLaunchUrl(web_app_info->app_url);
-  web_app->SetScope(web_app_info->scope);
-  web_app->SetThemeColor(web_app_info->theme_color);
+  std::vector<BitmapAndSource> downloaded_icons =
+      FilterSquareIcons(icons_map, *web_app_info);
+  ResizeDownloadedIconsGenerateMissing(std::move(downloaded_icons),
+                                       web_app_info.get());
 
-  registrar_->RegisterApp(std::move(web_app));
+  install_finalizer_->FinalizeInstall(
+      std::move(web_app_info),
+      base::BindOnce(&WebAppInstallManager::OnInstallFinalized,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
 
-  CallInstallCallback(app_id, InstallResultCode::kSuccess);
+void WebAppInstallManager::OnInstallFinalized(const AppId& app_id,
+                                              InstallResultCode code) {
+  CallInstallCallback(app_id, code);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_manager.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "content/public/browser/web_contents_observer.h"
 
 class Profile;
@@ -25,13 +26,14 @@ class WebContents;
 
 namespace web_app {
 
+class InstallFinalizer;
 class WebAppDataRetriever;
-class WebAppRegistrar;
 
 class WebAppInstallManager final : public InstallManager,
                                    content::WebContentsObserver {
  public:
-  WebAppInstallManager(Profile* profile, WebAppRegistrar* registrar);
+  WebAppInstallManager(Profile* profile,
+                       std::unique_ptr<InstallFinalizer> install_finalizer);
   ~WebAppInstallManager() override;
 
   // InstallManager:
@@ -45,6 +47,8 @@ class WebAppInstallManager final : public InstallManager,
 
   void SetDataRetrieverForTesting(
       std::unique_ptr<WebAppDataRetriever> data_retriever);
+  void SetInstallFinalizerForTesting(
+      std::unique_ptr<InstallFinalizer> install_finalizer);
 
  private:
   void CallInstallCallback(const AppId& app_id, InstallResultCode code);
@@ -61,13 +65,15 @@ class WebAppInstallManager final : public InstallManager,
       bool force_shortcut_app,
       const blink::Manifest& manifest,
       bool is_installable);
+  void OnIconsRetrieved(std::unique_ptr<WebApplicationInfo> web_app_info,
+                        IconsMap icons_map);
+  void OnInstallFinalized(const AppId& app_id, InstallResultCode code);
 
   // Saved callback:
   OnceInstallCallback install_callback_;
 
-  Profile* profile_;
-  WebAppRegistrar* registrar_;
   std::unique_ptr<WebAppDataRetriever> data_retriever_;
+  std::unique_ptr<InstallFinalizer> install_finalizer_;
 
   base::WeakPtrFactory<WebAppInstallManager> weak_ptr_factory_{this};
 

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -16,10 +16,13 @@
 #include "chrome/browser/installable/installable_manager.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/components/web_app_icon_generator.h"
 #include "chrome/browser/web_applications/test/test_data_retriever.h"
+#include "chrome/browser/web_applications/test/test_install_finalizer.h"
 #include "chrome/browser/web_applications/test/test_web_app_database.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "chrome/browser/web_applications/web_app_utils.h"
 #include "chrome/test/base/testing_profile.h"
@@ -36,8 +39,11 @@ class WebAppInstallManagerTest : public WebAppTest {
 
     database_ = std::make_unique<TestWebAppDatabase>();
     registrar_ = std::make_unique<WebAppRegistrar>(database_.get());
-    install_manager_ =
-        std::make_unique<WebAppInstallManager>(profile(), registrar_.get());
+
+    auto install_finalizer =
+        std::make_unique<WebAppInstallFinalizer>(registrar_.get());
+    install_manager_ = std::make_unique<WebAppInstallManager>(
+        profile(), std::move(install_finalizer));
   }
 
   void CreateRendererAppInfo(const GURL& url,
@@ -53,8 +59,10 @@ class WebAppInstallManagerTest : public WebAppTest {
     web_app_info->scope = scope;
     web_app_info->theme_color = theme_color;
 
-    install_manager_->SetDataRetrieverForTesting(
-        std::make_unique<TestDataRetriever>(std::move(web_app_info)));
+    auto data_retriever =
+        std::make_unique<TestDataRetriever>(std::move(web_app_info));
+    data_retriever_ = data_retriever.get();
+    install_manager_->SetDataRetrieverForTesting(std::move(data_retriever));
   }
 
   void CreateRendererAppInfo(const GURL& url,
@@ -74,10 +82,42 @@ class WebAppInstallManagerTest : public WebAppTest {
     return base::NullableString16(base::UTF8ToUTF16(str), false);
   }
 
+  void SetInstallFinalizerForTesting() {
+    auto install_finalizer = std::make_unique<TestInstallFinalizer>();
+    install_finalizer_ = install_finalizer.get();
+    install_manager_->SetInstallFinalizerForTesting(
+        std::move(install_finalizer));
+  }
+
+  void SetIconsMapToRetrieve(IconsMap icons_map) {
+    CHECK(data_retriever_);
+    data_retriever_->SetIcons(std::move(icons_map));
+  }
+
+  AppId InstallWebApp() {
+    AppId app_id;
+    base::RunLoop run_loop;
+    const bool force_shortcut_app = false;
+    install_manager_->InstallWebApp(
+        web_contents(), force_shortcut_app,
+        base::BindLambdaForTesting(
+            [&](const AppId& installed_app_id, InstallResultCode code) {
+              EXPECT_EQ(InstallResultCode::kSuccess, code);
+              app_id = installed_app_id;
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+    return app_id;
+  }
+
  protected:
   std::unique_ptr<TestWebAppDatabase> database_;
   std::unique_ptr<WebAppRegistrar> registrar_;
   std::unique_ptr<WebAppInstallManager> install_manager_;
+
+  // Owned by install_manager_:
+  TestDataRetriever* data_retriever_ = nullptr;
+  TestInstallFinalizer* install_finalizer_ = nullptr;
 };
 
 TEST_F(WebAppInstallManagerTest, InstallFromWebContents) {
@@ -175,7 +215,6 @@ TEST_F(WebAppInstallManagerTest, WebContentsDestroyed) {
   EXPECT_TRUE(callback_called);
 }
 
-// TODO(loyso): Convert more tests from bookmark_app_helper_unittest.cc
 TEST_F(WebAppInstallManagerTest, InstallableCheck) {
   const std::string renderer_description = "RendererDescription";
   CreateRendererAppInfo(GURL("https://renderer.com/path"), "RendererName",
@@ -188,6 +227,7 @@ TEST_F(WebAppInstallManagerTest, InstallableCheck) {
   const GURL manifest_scope = GURL("https://example.com/scope");
   const base::Optional<SkColor> manifest_theme_color = 0xAABBCCDD;
 
+  {
     auto manifest = std::make_unique<blink::Manifest>();
     manifest->short_name = ToNullableUTF16("Short Name from Manifest");
     manifest->name = ToNullableUTF16(manifest_name);
@@ -198,6 +238,7 @@ TEST_F(WebAppInstallManagerTest, InstallableCheck) {
     FakeInstallableManager::CreateForWebContentsWithManifest(
         web_contents(), NO_ERROR_DETECTED, GURL("https://example.com/manifest"),
         std::move(manifest));
+  }
 
   base::RunLoop run_loop;
   bool callback_called = false;
@@ -228,4 +269,42 @@ TEST_F(WebAppInstallManagerTest, InstallableCheck) {
   EXPECT_EQ(manifest_theme_color, web_app->theme_color());
 }
 
+TEST_F(WebAppInstallManagerTest, GetIcons_NoIconsProvided) {
+  CreateRendererAppInfo(GURL("https://example.com/path"), "Name",
+                        "Description");
+  CreateDefaultInstallableManager();
+
+  SetInstallFinalizerForTesting();
+
+  IconsMap icons_map;
+  SetIconsMapToRetrieve(std::move(icons_map));
+
+  InstallWebApp();
+
+  std::unique_ptr<WebApplicationInfo> info = install_finalizer_->web_app_info();
+
+  constexpr int kIconSizesToGenerate[] = {
+      icon_size::k32, icon_size::k64,  icon_size::k48,
+      icon_size::k96, icon_size::k128, icon_size::k256,
+  };
+
+  // Make sure that icons have been generated for all sizes.
+  for (int size : kIconSizesToGenerate) {
+    int generated_icons_for_size =
+        std::count_if(info->icons.begin(), info->icons.end(),
+                      [&size](const WebApplicationInfo::IconInfo& icon) {
+                        return icon.width == size && icon.height == size;
+                      });
+    EXPECT_EQ(1, generated_icons_for_size);
+  }
+
+  for (const auto& icon : info->icons) {
+    EXPECT_FALSE(icon.data.drawsNothing());
+    // Since all icons are generated, they should have an empty url.
+    EXPECT_TRUE(icon.url.is_empty());
+  }
+}
+
+// TODO(loyso): Convert more tests from bookmark_app_helper_unittest.cc
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_provider.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_database.h"
 #include "chrome/browser/web_applications/web_app_database_factory.h"
+#include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_install_manager.h"
 #include "chrome/browser/web_applications/web_app_provider_factory.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
@@ -61,8 +62,11 @@ void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
   database_factory_ = std::make_unique<WebAppDatabaseFactory>(profile);
   database_ = std::make_unique<WebAppDatabase>(database_factory_.get());
   registrar_ = std::make_unique<WebAppRegistrar>(database_.get());
-  install_manager_ =
-      std::make_unique<WebAppInstallManager>(profile, registrar_.get());
+
+  auto install_finalizer =
+      std::make_unique<WebAppInstallFinalizer>(registrar_.get());
+  install_manager_ = std::make_unique<WebAppInstallManager>(
+      profile, std::move(install_finalizer));
 
   registrar_->Init(base::DoNothing());
 }