desktop-pwas: Move adding apps to ExtensionIdsMap into BookmarkAppInstallationTask

Adding an newly installed extension to the ExtensionIdsMap is considered
part of the installation process and should therefore be in
BookmarkAppInstallationTask rather than in PendingAppManager.

This is the first in a series of patches to remove usage of ExtensionIdsMap
from PendingBookmarkAppManager and make PendingBookmarkAppManager a simple queue
of install/uninstall tasks. Performing pre-installation checks should be either
moved into BookmarkAppInstallationTask or into a separate class.

Bug: 877898
Change-Id: I30469f2682a3028e71390afc116d2785bb1ae2e9
Reviewed-on: https://chromium-review.googlesource.com/c/1484879Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636177}

chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/bookmark_app_helper.h"
 #include "chrome/browser/favicon/favicon_utils.h"
 #include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/profiles/profile.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_data_retriever.h"
@@ -57,6 +58,7 @@ BookmarkAppInstallationTask::BookmarkAppInstallationTask(
     Profile* profile,
     web_app::PendingAppManager::AppInfo app_info)
     : profile_(profile),
+      extension_ids_map_(profile_->GetPrefs()),
       app_info_(std::move(app_info)),
       helper_factory_(base::BindRepeating(&BookmarkAppHelperCreateWrapper)),
       data_retriever_(std::make_unique<web_app::WebAppDataRetriever>()) {}
@@ -148,11 +150,16 @@ void BookmarkAppInstallationTask::OnInstalled(
     ResultCallback result_callback,
     const Extension* extension,
     const WebApplicationInfo& web_app_info) {
+  if (extension) {
+    extension_ids_map_.Insert(app_info_.url, extension->id(),
+                              app_info_.install_source);
+    std::move(result_callback)
+        .Run(Result(web_app::InstallResultCode::kSuccess, extension->id()));
+    return;
+  }
   std::move(result_callback)
-      .Run(extension
-               ? Result(web_app::InstallResultCode::kSuccess, extension->id())
-               : Result(web_app::InstallResultCode::kFailedUnknownReason,
-                        base::nullopt));
+      .Run(Result(web_app::InstallResultCode::kFailedUnknownReason,
+                  base::nullopt));
 }
 
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_installation_task.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
+#include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 
 class Profile;
 enum class WebappInstallSource;
@@ -90,6 +91,8 @@ class BookmarkAppInstallationTask {
 
   Profile* profile_;
 
+  web_app::ExtensionIdsMap extension_ids_map_;
+
   const web_app::PendingAppManager::AppInfo app_info_;
 
   // We temporarily use a BookmarkAppHelper until the WebApp and WebShortcut

chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/installable/installable_data.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/extensions/web_app_extension_ids_map.h"
 #include "chrome/browser/web_applications/test/test_data_retriever.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -131,6 +132,10 @@ class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
     return app_installed;
   }
 
+  const std::string& app_id() {
+    return app_installation_result_->app_id.value();
+  }
+
   TestBookmarkAppHelper& test_helper() { return *test_helper_; }
 
   const Result& app_installation_result() { return *app_installation_result_; }
@@ -177,7 +182,13 @@ TEST_F(BookmarkAppInstallationTaskTest,
   test_helper().CompleteIconDownload();
   content::RunAllTasksUntilIdle();
 
+  base::Optional<std::string> id =
+      web_app::ExtensionIdsMap(profile()->GetPrefs())
+          .LookupExtensionId(app_url);
+
   EXPECT_TRUE(app_installed());
+  EXPECT_EQ(app_id(), id.value());
+
   EXPECT_TRUE(test_helper().create_shortcuts());
   EXPECT_FALSE(test_helper().forced_launch_type().has_value());
   EXPECT_TRUE(test_helper().is_default_app());
@@ -207,7 +218,12 @@ TEST_F(BookmarkAppInstallationTaskTest,
   test_helper().FailIconDownload();
   content::RunAllTasksUntilIdle();
 
+  base::Optional<std::string> id =
+      web_app::ExtensionIdsMap(profile()->GetPrefs())
+          .LookupExtensionId(app_url);
+
   EXPECT_FALSE(app_installed());
+  EXPECT_FALSE(id.has_value());
 }
 
 TEST_F(BookmarkAppInstallationTaskTest,

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -257,12 +257,9 @@ void PendingBookmarkAppManager::CurrentInstallationFinished(
       base::BindOnce(&PendingBookmarkAppManager::MaybeStartNextInstallation,
                      weak_ptr_factory_.GetWeakPtr()));
 
-  auto install_result_code = web_app::InstallResultCode::kFailedUnknownReason;
-  if (app_id) {
-    install_result_code = web_app::InstallResultCode::kSuccess;
-    const auto& info = current_task_and_callback_->task->app_info();
-    extension_ids_map_.Insert(info.url, app_id.value(), info.install_source);
-  }
+  auto install_result_code =
+      app_id ? web_app::InstallResultCode::kSuccess
+             : web_app::InstallResultCode::kFailedUnknownReason;
 
   std::unique_ptr<TaskAndCallback> task_and_callback;
   task_and_callback.swap(current_task_and_callback_);

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

@@ -125,7 +125,8 @@ class TestBookmarkAppInstallationTask : public BookmarkAppInstallationTask {
                                   bool succeeds)
       : BookmarkAppInstallationTask(profile, std::move(app_info)),
         profile_(profile),
-        succeeds_(succeeds) {}
+        succeeds_(succeeds),
+        extension_ids_map_(profile_->GetPrefs()) {}
   ~TestBookmarkAppInstallationTask() override = default;
 
   void InstallWebAppOrShortcutFromWebContents(
@@ -138,6 +139,8 @@ class TestBookmarkAppInstallationTask : public BookmarkAppInstallationTask {
       app_id = GenerateFakeAppId(app_info().url);
       ExtensionRegistry::Get(profile_)->AddEnabled(
           CreateDummyExtension(app_id));
+      extension_ids_map_.Insert(app_info().url, app_id,
+                                app_info().install_source);
     }
 
     std::move(on_install_called_).Run();
@@ -152,6 +155,7 @@ class TestBookmarkAppInstallationTask : public BookmarkAppInstallationTask {
  private:
   Profile* profile_;
   bool succeeds_;
+  web_app::ExtensionIdsMap extension_ids_map_;
 
   base::OnceClosure on_install_called_;