WebApp: Backward compatibility install for bookmark apps from sync.

Design doc: go/chrome-bmo-migration.

This CL unifies sync-initiated install for server-side bookmark
apps to BMO-enabled clients and BMO-disabled clients.

We try to install the web app as if the user had requested to
install it (in background mode, no dialog).
If online install fails, we fallback to WebApplicationInfo data
received form the sync server.
Note that user_display_mode field from server (aka open_as_window bool)
always overwrites a default value.

- Extract GetIconSizes() / ContainsOneIconOfEachSize test utils.
- Extract SetAboutBlankResultLoaded TestUrlLoader method.

Bug: 1020037
Change-Id: I109fa884af1c3b373b2b5922527c98ff6b06a921
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2126339Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759544}

chrome/browser/web_applications/components/install_manager.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/callback_forward.h"
+#include "base/optional.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/components/web_app_install_utils.h"
@@ -97,7 +98,8 @@ class InstallManager {
       WebappInstallSource install_source,
       OnceInstallCallback callback) = 0;
 
-  // These params are a subset of ExternalInstallOptions.
+  // See related ExternalInstallOptions struct and
+  // ConvertExternalInstallOptionsToParams function.
   struct InstallParams {
     InstallParams();
     ~InstallParams();
@@ -108,6 +110,8 @@ class InstallManager {
     // URL to be used as start_url if manifest is unavailable.
     GURL fallback_start_url;
 
+    bool locally_installed = true;
+    // These OS shortcut fields can't be true if |locally_installed| is false.
     bool add_to_applications_menu = true;
     bool add_to_desktop = true;
     bool add_to_quick_launch_bar = true;

chrome/browser/web_applications/test/test_web_app_url_loader.cc

@@ -56,4 +56,9 @@ void TestWebAppUrlLoader::LoadUrl(const GURL& url,
       FROM_HERE, base::BindOnce(std::move(callback), result));
 }
 
+void TestWebAppUrlLoader::SetAboutBlankResultLoaded() {
+  SetNextLoadUrlResult(GURL("about:blank"),
+                       WebAppUrlLoader::Result::kUrlLoaded);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/test/test_web_app_url_loader.h

@@ -35,6 +35,9 @@ class TestWebAppUrlLoader : public WebAppUrlLoader {
                UrlComparison url_comparison,
                ResultCallback callback) override;
 
+  // Sets the result for the next about:blank load to be ok.
+  void SetAboutBlankResultLoaded();
+
  private:
   bool should_save_requests_ = false;
 

chrome/browser/web_applications/test/web_app_icon_test_utils.cc

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/components/web_app_icon_generator.h"
 #include "chrome/browser/web_applications/components/web_app_utils.h"
 #include "chrome/browser/web_applications/file_utils_wrapper.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -15,6 +16,15 @@
 
 namespace web_app {
 
+namespace {
+
+constexpr int kIconSizes[] = {
+    icon_size::k32, icon_size::k64,  icon_size::k48,
+    icon_size::k96, icon_size::k128, icon_size::k256,
+};
+
+}  // namespace
+
 SkBitmap CreateSquareIcon(int size_px, SkColor solid_color) {
   SkBitmap bitmap;
   bitmap.allocN32Pixels(size_px, size_px);
@@ -78,4 +88,23 @@ bool ReadBitmap(FileUtilsWrapper* utils,
       icon_data.size(), bitmap);
 }
 
+base::span<const int> GetIconSizes() {
+  return base::span<const int>(kIconSizes, base::size(kIconSizes));
+}
+
+bool ContainsOneIconOfEachSize(
+    const std::map<SquareSizePx, SkBitmap>& icon_bitmaps) {
+  for (int size_px : kIconSizes) {
+    int num_icons_for_size = std::count_if(
+        icon_bitmaps.begin(), icon_bitmaps.end(),
+        [&size_px](const std::pair<SquareSizePx, SkBitmap>& icon) {
+          return icon.first == size_px;
+        });
+    if (num_icons_for_size != 1)
+      return false;
+  }
+
+  return true;
+}
+
 }  // namespace web_app

chrome/browser/web_applications/test/web_app_icon_test_utils.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/containers/span.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/components/web_app_install_utils.h"
@@ -42,6 +43,11 @@ bool ReadBitmap(FileUtilsWrapper* utils,
                 const base::FilePath& file_path,
                 SkBitmap* bitmap);
 
+base::span<const int> GetIconSizes();
+
+bool ContainsOneIconOfEachSize(
+    const std::map<SquareSizePx, SkBitmap>& icon_bitmaps);
+
 }  // namespace web_app
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_TEST_WEB_APP_ICON_TEST_UTILS_H_

chrome/browser/web_applications/web_app_install_manager.cc

@@ -115,44 +115,59 @@ void WebAppInstallManager::InstallWebAppWithParams(
 }
 
 void WebAppInstallManager::InstallBookmarkAppFromSync(
-    const AppId& app_id,
+    const AppId& bookmark_app_id,
     std::unique_ptr<WebApplicationInfo> web_application_info,
     OnceInstallCallback callback) {
-  if (registrar()->AsWebAppRegistrar()) {
-    // If new Web Applications system is enabled, any legacy sync-initiated
-    // installation requests are ignored for now.
-    // TODO(crbug.com/1020037): If app_id is not installed, migrate bookmark app
-    // to new web app: Enqueue installation as if the user had requested to
-    // install it.
-    return;
-  }
-
   // Skip sync update if app exists.
   // All manifest fields will be set locally via update (see crbug.com/926083)
   // so we must not sync them in order to avoid a device-to-device sync war.
-  if (registrar()->IsInstalled(app_id)) {
-    std::move(callback).Run(app_id,
+  if (registrar()->IsInstalled(bookmark_app_id)) {
+    std::move(callback).Run(bookmark_app_id,
                             InstallResultCode::kSuccessAlreadyInstalled);
     return;
   }
 
-  bool is_locally_installed = registrar()->IsLocallyInstalled(app_id);
+  bool is_locally_installed = false;
 #if defined(OS_CHROMEOS)
   // On Chrome OS, sync always locally installs an app.
   is_locally_installed = true;
 #endif
 
+  // If bookmark_app_id is not installed enqueue full background installation
+  // flow. This install may produce a web app or an extension-based bookmark
+  // app, depending on the BMO flag.
+  GURL launch_url = web_application_info->app_url;
+
   auto task = std::make_unique<WebAppInstallTask>(
       profile(), registrar(), shortcut_manager(), file_handler_manager(),
       finalizer(), data_retriever_factory_.Run());
 
+  task->ExpectAppId(bookmark_app_id);
+
+  InstallParams params;
+  params.user_display_mode = web_application_info->open_as_window
+                                 ? DisplayMode::kStandalone
+                                 : DisplayMode::kBrowser;
+  params.fallback_start_url = launch_url;
+  // If app is not locally installed then no OS integration like OS shortcuts.
+  params.locally_installed = is_locally_installed;
+  params.add_to_applications_menu = is_locally_installed;
+  params.add_to_desktop = is_locally_installed;
+  params.add_to_quick_launch_bar = is_locally_installed;
+  task->SetInstallParams(params);
+
+  OnceInstallCallback task_completed_callback = base::BindOnce(
+      &WebAppInstallManager::OnBookmarkAppInstalledAfterSync,
+      base::Unretained(this), bookmark_app_id, std::move(web_application_info),
+      is_locally_installed, std::move(callback));
+
   base::OnceClosure start_task = base::BindOnce(
-      &WebAppInstallTask::LoadAndInstallWebAppFromSync,
-      base::Unretained(task.get()), EnsureWebContentsCreated(),
-      base::Unretained(url_loader_.get()), std::move(web_application_info),
-      is_locally_installed, WebappInstallSource::SYNC,
+      &WebAppInstallTask::LoadAndInstallWebAppFromManifestWithFallback,
+      base::Unretained(task.get()), launch_url, EnsureWebContentsCreated(),
+      base::Unretained(url_loader_.get()), WebappInstallSource::SYNC,
       base::BindOnce(&WebAppInstallManager::OnQueuedTaskCompleted,
-                     base::Unretained(this), task.get(), std::move(callback)));
+                     base::Unretained(this), task.get(),
+                     std::move(task_completed_callback)));
 
   EnqueueTask(std::move(task), std::move(start_task));
 }
@@ -231,6 +246,32 @@ void WebAppInstallManager::SetUrlLoaderForTesting(
   url_loader_ = std::move(url_loader);
 }
 
+void WebAppInstallManager::OnBookmarkAppInstalledAfterSync(
+    const AppId& bookmark_app_id,
+    std::unique_ptr<WebApplicationInfo> web_application_info,
+    bool is_locally_installed,
+    OnceInstallCallback callback,
+    const AppId& web_app_id,
+    InstallResultCode code) {
+  // TODO(loyso): Record |code| for this specific case in
+  // Webapp.BookmarkAppInstalledAfterSyncResult UMA.
+  if (IsSuccess(code)) {
+    DCHECK_EQ(bookmark_app_id, web_app_id);
+    std::move(callback).Run(web_app_id, code);
+  } else {
+    // Install failed. Do the fallback install from info.
+    InstallFinalizer::FinalizeOptions options;
+    options.install_source = WebappInstallSource::SYNC;
+    options.locally_installed = is_locally_installed;
+
+    FilterAndResizeIconsGenerateMissing(web_application_info.get(),
+                                        /*icons_map=*/nullptr);
+
+    finalizer()->FinalizeInstall(*web_application_info, options,
+                                 std::move(callback));
+  }
+}
+
 void WebAppInstallManager::EnqueueTask(std::unique_ptr<WebAppInstallTask> task,
                                        base::OnceClosure start_task) {
   DCHECK(web_contents_);

chrome/browser/web_applications/web_app_install_manager.h

@@ -61,7 +61,7 @@ class WebAppInstallManager final : public InstallManager,
                                WebappInstallSource install_source,
                                OnceInstallCallback callback) override;
   void InstallBookmarkAppFromSync(
-      const AppId& app_id,
+      const AppId& bookmark_app_id,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       OnceInstallCallback callback) override;
   void UpdateWebAppFromInfo(
@@ -85,6 +85,14 @@ class WebAppInstallManager final : public InstallManager,
   bool has_web_contents_for_testing() const { return web_contents_ != nullptr; }
 
  private:
+  void OnBookmarkAppInstalledAfterSync(
+      const AppId& bookmark_app_id,
+      std::unique_ptr<WebApplicationInfo> web_application_info,
+      bool is_locally_installed,
+      OnceInstallCallback callback,
+      const AppId& web_app_id,
+      InstallResultCode code);
+
   void EnqueueTask(std::unique_ptr<WebAppInstallTask> task,
                    base::OnceClosure start_task);
   void MaybeStartQueuedTask();

chrome/browser/web_applications/web_app_install_task.cc

@@ -90,6 +90,16 @@ void WebAppInstallTask::ExpectAppId(const AppId& expected_app_id) {
   expected_app_id_ = expected_app_id;
 }
 
+void WebAppInstallTask::SetInstallParams(
+    const InstallManager::InstallParams& install_params) {
+  if (!install_params.locally_installed) {
+    DCHECK(!install_params.add_to_applications_menu);
+    DCHECK(!install_params.add_to_desktop);
+    DCHECK(!install_params.add_to_quick_launch_bar);
+  }
+  install_params_ = install_params;
+}
+
 void WebAppInstallTask::LoadWebAppAndCheckInstallability(
     const GURL& url,
     WebappInstallSource install_source,
@@ -217,9 +227,9 @@ void WebAppInstallTask::InstallWebAppWithParams(
   CheckInstallPreconditions();
 
   Observe(contents);
+  SetInstallParams(install_params);
   install_callback_ = std::move(install_callback);
   install_source_ = install_source;
-  install_params_ = install_params;
   background_installation_ = true;
 
   data_retriever_->GetWebApplicationInfo(
@@ -228,33 +238,6 @@ void WebAppInstallTask::InstallWebAppWithParams(
                      base::Unretained(this), /*force_shortcut_app=*/false));
 }
 
-void WebAppInstallTask::LoadAndInstallWebAppFromSync(
-    content::WebContents* web_contents,
-    WebAppUrlLoader* url_loader,
-    std::unique_ptr<WebApplicationInfo> web_application_info,
-    bool is_locally_installed,
-    WebappInstallSource install_source,
-    InstallManager::OnceInstallCallback callback) {
-  CheckInstallPreconditions();
-
-  Observe(web_contents);
-  install_callback_ = std::move(callback);
-  install_source_ = install_source;
-  background_installation_ = true;
-
-  // Default display mode, used if we fail to load the manifest.
-  web_application_info->display_mode = DisplayMode::kBrowser;
-
-  const GURL launch_url = web_application_info->app_url;
-  url_loader->LoadUrl(
-      launch_url, web_contents,
-      WebAppUrlLoader::UrlComparison::kIgnoreQueryParamsAndRef,
-      base::BindOnce(
-          &WebAppInstallTask::OnWebAppFromSyncUrlLoadedRetrieveManifest,
-          weak_ptr_factory_.GetWeakPtr(), std::move(web_application_info),
-          is_locally_installed));
-}
-
 void WebAppInstallTask::UpdateWebAppFromInfo(
     content::WebContents* web_contents,
     const AppId& app_id,
@@ -616,59 +599,6 @@ void WebAppInstallTask::OnDidCheckForIntentToPlayStore(
                      for_installable_site));
 }
 
-void WebAppInstallTask::OnWebAppFromSyncUrlLoadedRetrieveManifest(
-    std::unique_ptr<WebApplicationInfo> web_application_info,
-    bool is_locally_installed,
-    WebAppUrlLoader::Result result) {
-  if (ShouldStopInstall())
-    return;
-
-  if (result != WebAppUrlLoader::Result::kUrlLoaded) {
-    WebAppFromSyncLoadIcons(std::move(web_application_info),
-                            is_locally_installed);
-    return;
-  }
-
-  web_contents()->GetManifest(
-      base::BindOnce(&WebAppInstallTask::OnWebAppFromSyncManifestRetrieved,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     std::move(web_application_info), is_locally_installed));
-}
-
-void WebAppInstallTask::OnWebAppFromSyncManifestRetrieved(
-    std::unique_ptr<WebApplicationInfo> web_application_info,
-    bool is_locally_installed,
-    const GURL& manifest_url,
-    const blink::Manifest& manifest) {
-  if (ShouldStopInstall())
-    return;
-
-  if (!manifest.IsEmpty()) {
-    // TODO(crbug.com/1067519): Copy more fields from the manifest.
-    web_application_info->display_mode = manifest.display;
-  }
-
-  WebAppFromSyncLoadIcons(std::move(web_application_info),
-                          is_locally_installed);
-}
-
-void WebAppInstallTask::WebAppFromSyncLoadIcons(
-    std::unique_ptr<WebApplicationInfo> web_application_info,
-    bool is_locally_installed) {
-  std::vector<GURL> icon_urls =
-      GetValidIconUrlsToDownload(*web_application_info);
-
-  // Skip downloading the page favicons as everything in is the URL list.
-  data_retriever_->GetIcons(
-      web_contents(), icon_urls, /*skip_page_fav_icons*/ true,
-      install_source_ == WebappInstallSource::SYNC
-          ? WebAppIconDownloader::Histogram::kForSync
-          : WebAppIconDownloader::Histogram::kForCreate,
-      base::BindOnce(&WebAppInstallTask::OnIconsRetrieved,
-                     base::Unretained(this), std::move(web_application_info),
-                     is_locally_installed));
-}
-
 void WebAppInstallTask::OnIconsRetrieved(
     std::unique_ptr<WebApplicationInfo> web_app_info,
     bool is_locally_installed,
@@ -761,10 +691,13 @@ void WebAppInstallTask::OnDialogCompleted(
   InstallFinalizer::FinalizeOptions finalize_options;
   finalize_options.install_source = install_source_;
   finalize_options.locally_installed = true;
-  if (install_params_ &&
-      install_params_->user_display_mode != DisplayMode::kUndefined) {
-    web_app_info_copy.open_as_window =
-        install_params_->user_display_mode != DisplayMode::kBrowser;
+  if (install_params_) {
+    finalize_options.locally_installed = install_params_->locally_installed;
+
+    if (install_params_->user_display_mode != DisplayMode::kUndefined) {
+      web_app_info_copy.open_as_window =
+          install_params_->user_display_mode != DisplayMode::kBrowser;
+    }
   }
 
   install_finalizer_->FinalizeInstall(

chrome/browser/web_applications/web_app_install_task.h

@@ -58,6 +58,8 @@ class WebAppInstallTask : content::WebContentsObserver {
   // The actual resulting app_id is reported as a part of OnceInstallCallback.
   void ExpectAppId(const AppId& expected_app_id);
 
+  void SetInstallParams(const InstallManager::InstallParams& install_params);
+
   using LoadWebAppAndCheckInstallabilityCallback = base::OnceCallback<void(
       std::unique_ptr<content::WebContents> web_contents,
       const AppId& app_id,
@@ -117,19 +119,6 @@ class WebAppInstallTask : content::WebContentsObserver {
       WebappInstallSource install_source,
       InstallManager::OnceInstallCallback callback);
 
-  // Starts background installation of a web app: does not show UI dialog.
-  // |web_application_info| contains most of the data needed for installation.
-  // The launch URL and its manifest are loaded to determine the display mode.
-  // Icons will be downloaded from the icon URLs provided in
-  // |web_application_info|. Doesn't memorize |url_loader| pointer.
-  void LoadAndInstallWebAppFromSync(
-      content::WebContents* web_contents,
-      WebAppUrlLoader* url_loader,
-      std::unique_ptr<WebApplicationInfo> web_application_info,
-      bool is_locally_installed,
-      WebappInstallSource install_source,
-      InstallManager::OnceInstallCallback callback);
-
   void UpdateWebAppFromInfo(
       content::WebContents* web_contents,
       const AppId& app_id,
@@ -207,19 +196,6 @@ class WebAppInstallTask : content::WebContentsObserver {
       const std::string& intent,
       bool should_intent_to_store);
 
-  void OnWebAppFromSyncUrlLoadedRetrieveManifest(
-      std::unique_ptr<WebApplicationInfo> web_application_info,
-      bool is_locally_installed,
-      WebAppUrlLoader::Result result);
-  void OnWebAppFromSyncManifestRetrieved(
-      std::unique_ptr<WebApplicationInfo> web_application_info,
-      bool is_locally_installed,
-      const GURL& manifest_url,
-      const blink::Manifest& manifest);
-  void WebAppFromSyncLoadIcons(
-      std::unique_ptr<WebApplicationInfo> web_application_info,
-      bool is_locally_installed);
-
   void OnIconsRetrieved(std::unique_ptr<WebApplicationInfo> web_app_info,
                         bool is_locally_installed,
                         IconsMap icons_map);