WebApp: Plumb ExternalInstallSource argument for UninstallExternalWeb…

…App. This is a preparation CL to implement Uninstall for the new web apps. Bug: 901226 Change-Id: Icaa903d3a9d02929f1ab308e60693e031858134f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1921021 Commit-Queue: Alexey Baskakov <loyso@chromium.org> Reviewed-by: Eric Willigers <ericwilligers@chromium.org> Cr-Commit-Position: refs/heads/master@{#716440}
chromium · Nov 19, 2019 · 89240c3 · 89240c3
1 parent 0333405
commit 89240c3
Show file tree

Hide file tree

Showing 17 changed files with 56 additions and 26 deletions.
diff --git a/chrome/browser/web_applications/components/install_finalizer.h b/chrome/browser/web_applications/components/install_finalizer.h
@@ -19,6 +19,7 @@ class WebContents;
 
 namespace web_app {
 
+enum class ExternalInstallSource;
 enum class InstallResultCode;
 class AppRegistrar;
 class WebAppUiManager;
@@ -57,8 +58,10 @@ class InstallFinalizer {
 
   // Removes the external app for |app_url| from disk and registrar. Fails if
   // there is no installed external app for |app_url|.
-  virtual void UninstallExternalWebApp(const GURL& app_url,
-                                       UninstallWebAppCallback) = 0;
+  virtual void UninstallExternalWebApp(
+      const GURL& app_url,
+      ExternalInstallSource external_install_source,
+      UninstallWebAppCallback) = 0;
 
   virtual bool CanUserUninstallFromSync(const AppId& app_id) const = 0;
   virtual void UninstallWebAppFromSyncByUser(const AppId& app_id,

diff --git a/chrome/browser/web_applications/components/pending_app_manager.cc b/chrome/browser/web_applications/components/pending_app_manager.cc
@@ -91,7 +91,7 @@ void PendingAppManager::SynchronizeInstalledApps(
                          urls_to_remove.size() + desired_urls.size()));
 
   UninstallApps(
-      urls_to_remove,
+      urls_to_remove, install_source,
       base::BindRepeating(&PendingAppManager::UninstallForSynchronizeCallback,
                           weak_ptr_factory_.GetWeakPtr(), install_source));
   InstallApps(

diff --git a/chrome/browser/web_applications/components/pending_app_manager.h b/chrome/browser/web_applications/components/pending_app_manager.h
@@ -87,6 +87,7 @@ class PendingAppManager {
   // uninstall succeeded. Runs |callback| for every completed uninstallation -
   // whether or not the uninstallation actually succeeded.
   virtual void UninstallApps(std::vector<GURL> uninstall_urls,
+                             ExternalInstallSource install_source,
                              const UninstallCallback& callback) = 0;
 
   // Installs an app for each ExternalInstallOptions in

diff --git a/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc b/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc
@@ -148,7 +148,10 @@ void BookmarkAppInstallFinalizer::FinalizeUpdate(
 
 void BookmarkAppInstallFinalizer::UninstallExternalWebApp(
     const GURL& app_url,
+    web_app::ExternalInstallSource external_install_source,
     UninstallWebAppCallback callback) {
+  // Bookmark apps don't support app installation from different sources.
+  // |external_install_source| is ignored here.
   base::Optional<web_app::AppId> app_id =
       externally_installed_app_prefs_.LookupAppId(app_url);
   if (!app_id.has_value()) {

diff --git a/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h b/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h
@@ -42,8 +42,10 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
                                   UninstallWebAppCallback callback) override;
   void FinalizeUpdate(const WebApplicationInfo& web_app_info,
                       InstallFinalizedCallback callback) override;
-  void UninstallExternalWebApp(const GURL& app_url,
-                               UninstallWebAppCallback callback) override;
+  void UninstallExternalWebApp(
+      const GURL& app_url,
+      web_app::ExternalInstallSource external_install_source,
+      UninstallWebAppCallback callback) override;
   bool CanUserUninstallFromSync(const web_app::AppId& app_id) const override;
   void UninstallWebAppFromSyncByUser(const web_app::AppId& app_id,
                                      UninstallWebAppCallback) override;

diff --git a/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer_unittest.cc b/chrome/browser/web_applications/extensions/bookmark_app_install_finalizer_unittest.cc
@@ -45,7 +45,7 @@ const char kWebAppTitle[] = "Foo Title";
 class BookmarkAppInstallFinalizerTest : public ChromeRenderViewHostTestHarness {
  public:
   // Subclass that runs a closure when an extension is unpacked successfully.
-  // Useful for tests that want to trigger their own succeess/failure events.
+  // Useful for tests that want to trigger their own success/failure events.
   class FakeCrxInstaller : public CrxInstaller {
    public:
     explicit FakeCrxInstaller(Profile* profile)
@@ -374,7 +374,8 @@ TEST_F(BookmarkAppInstallFinalizerTest, UninstallExternalWebApp_Successful) {
 
   base::RunLoop run_loop;
   finalizer().UninstallExternalWebApp(
-      kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+      kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+      base::BindLambdaForTesting([&](bool uninstalled) {
         EXPECT_TRUE(uninstalled);
         EXPECT_EQ(0u, enabled_extensions().size());
         run_loop.Quit();
@@ -391,7 +392,8 @@ TEST_F(BookmarkAppInstallFinalizerTest, UninstallExternalWebApp_Multiple) {
   {
     base::RunLoop run_loop;
     finalizer().UninstallExternalWebApp(
-        kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+        kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+        base::BindLambdaForTesting([&](bool uninstalled) {
           EXPECT_TRUE(uninstalled);
           run_loop.Quit();
         }));
@@ -405,7 +407,8 @@ TEST_F(BookmarkAppInstallFinalizerTest, UninstallExternalWebApp_Multiple) {
   {
     base::RunLoop run_loop;
     finalizer().UninstallExternalWebApp(
-        kAlternateWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+        kAlternateWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+        base::BindLambdaForTesting([&](bool uninstalled) {
           EXPECT_TRUE(uninstalled);
           run_loop.Quit();
         }));
@@ -421,7 +424,8 @@ TEST_F(BookmarkAppInstallFinalizerTest,
 
   base::RunLoop run_loop;
   finalizer().UninstallExternalWebApp(
-      kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+      kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+      base::BindLambdaForTesting([&](bool uninstalled) {
         EXPECT_FALSE(uninstalled);
         run_loop.Quit();
       }));
@@ -432,7 +436,8 @@ TEST_F(BookmarkAppInstallFinalizerTest,
        UninstallExternalWebApp_FailsNeverInstalled) {
   base::RunLoop run_loop;
   finalizer().UninstallExternalWebApp(
-      kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+      kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+      base::BindLambdaForTesting([&](bool uninstalled) {
         EXPECT_FALSE(uninstalled);
         run_loop.Quit();
       }));
@@ -447,7 +452,8 @@ TEST_F(BookmarkAppInstallFinalizerTest,
   {
     base::RunLoop run_loop;
     finalizer().UninstallExternalWebApp(
-        kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+        kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+        base::BindLambdaForTesting([&](bool uninstalled) {
           EXPECT_TRUE(uninstalled);
           run_loop.Quit();
         }));
@@ -458,7 +464,8 @@ TEST_F(BookmarkAppInstallFinalizerTest,
   {
     base::RunLoop run_loop;
     finalizer().UninstallExternalWebApp(
-        kWebAppUrl, base::BindLambdaForTesting([&](bool uninstalled) {
+        kWebAppUrl, web_app::ExternalInstallSource::kExternalPolicy,
+        base::BindLambdaForTesting([&](bool uninstalled) {
           EXPECT_FALSE(uninstalled);
           run_loop.Quit();
         }));

diff --git a/chrome/browser/web_applications/extensions/pending_app_install_task_unittest.cc b/chrome/browser/web_applications/extensions/pending_app_install_task_unittest.cc
@@ -178,6 +178,7 @@ class TestPendingAppInstallFinalizer : public InstallFinalizer {
   }
 
   void UninstallExternalWebApp(const GURL& app_url,
+                               ExternalInstallSource external_install_source,
                                UninstallWebAppCallback callback) override {
     DCHECK(base::Contains(next_uninstall_external_web_app_results_, app_url));
     uninstall_external_web_app_urls_.push_back(app_url);

diff --git a/chrome/browser/web_applications/pending_app_install_task.cc b/chrome/browser/web_applications/pending_app_install_task.cc
@@ -121,7 +121,7 @@ void PendingAppInstallTask::UninstallPlaceholderApp(
 
   // Otherwise, uninstall the placeholder app.
   install_finalizer_->UninstallExternalWebApp(
-      install_options_.url,
+      install_options_.url, install_options_.install_source,
       base::BindOnce(&PendingAppInstallTask::OnPlaceholderUninstalled,
                      weak_ptr_factory_.GetWeakPtr(), web_contents,
                      std::move(result_callback)));

diff --git a/chrome/browser/web_applications/pending_app_manager_impl.cc b/chrome/browser/web_applications/pending_app_manager_impl.cc
@@ -61,13 +61,15 @@ void PendingAppManagerImpl::InstallApps(
 }
 
 void PendingAppManagerImpl::UninstallApps(std::vector<GURL> uninstall_urls,
+                                          ExternalInstallSource install_source,
                                           const UninstallCallback& callback) {
   for (auto& url : uninstall_urls) {
     finalizer()->UninstallExternalWebApp(
-        url, base::BindOnce(
-                 [](const UninstallCallback& callback, const GURL& app_url,
-                    bool uninstalled) { callback.Run(app_url, uninstalled); },
-                 callback, url));
+        url, install_source,
+        base::BindOnce(
+            [](const UninstallCallback& callback, const GURL& app_url,
+               bool uninstalled) { callback.Run(app_url, uninstalled); },
+            callback, url));
   }
 }
 

diff --git a/chrome/browser/web_applications/pending_app_manager_impl.h b/chrome/browser/web_applications/pending_app_manager_impl.h
@@ -45,6 +45,7 @@ class PendingAppManagerImpl : public PendingAppManager {
   void InstallApps(std::vector<ExternalInstallOptions> install_options_list,
                    const RepeatingInstallCallback& callback) override;
   void UninstallApps(std::vector<GURL> uninstall_urls,
+                     ExternalInstallSource install_source,
                      const UninstallCallback& callback) override;
   void Shutdown() override;
 

diff --git a/chrome/browser/web_applications/pending_app_manager_impl_unittest.cc b/chrome/browser/web_applications/pending_app_manager_impl_unittest.cc
@@ -341,14 +341,15 @@ class PendingAppManagerImplTest : public ChromeRenderViewHostTestHarness {
 
   std::vector<std::pair<GURL, bool>> UninstallAppsAndWait(
       PendingAppManager* pending_app_manager,
+      ExternalInstallSource install_source,
       std::vector<GURL> apps_to_uninstall) {
     std::vector<std::pair<GURL, bool>> results;
 
     base::RunLoop run_loop;
     auto barrier_closure =
         base::BarrierClosure(apps_to_uninstall.size(), run_loop.QuitClosure());
     pending_app_manager->UninstallApps(
-        std::move(apps_to_uninstall),
+        std::move(apps_to_uninstall), install_source,
         base::BindLambdaForTesting(
             [&](const GURL& url, bool successfully_uninstalled) {
               results.emplace_back(url, successfully_uninstalled);
@@ -960,7 +961,7 @@ TEST_F(PendingAppManagerImplTest, InstallApps_PendingInstallApps) {
   run_loop.Run();
 }
 
-TEST_F(PendingAppManagerImplTest, Install_PendingMulitpleInstallApps) {
+TEST_F(PendingAppManagerImplTest, Install_PendingMultipleInstallApps) {
   pending_app_manager_impl()->SetNextInstallationTaskResult(
       kFooWebAppUrl, InstallResultCode::kSuccessNewInstall);
   pending_app_manager_impl()->SetNextInstallationLaunchURL(kFooWebAppUrl,
@@ -1190,7 +1191,8 @@ TEST_F(PendingAppManagerImplTest, UninstallApps_Succeeds) {
   install_finalizer()->SetNextUninstallExternalWebAppResult(kFooWebAppUrl,
                                                             true);
   UninstallAppsResults results = UninstallAppsAndWait(
-      pending_app_manager_impl(), std::vector<GURL>{kFooWebAppUrl});
+      pending_app_manager_impl(), ExternalInstallSource::kExternalPolicy,
+      std::vector<GURL>{kFooWebAppUrl});
 
   EXPECT_EQ(results, UninstallAppsResults({{kFooWebAppUrl, true}}));
 
@@ -1202,7 +1204,8 @@ TEST_F(PendingAppManagerImplTest, UninstallApps_Fails) {
   install_finalizer()->SetNextUninstallExternalWebAppResult(kFooWebAppUrl,
                                                             false);
   UninstallAppsResults results = UninstallAppsAndWait(
-      pending_app_manager_impl(), std::vector<GURL>{kFooWebAppUrl});
+      pending_app_manager_impl(), ExternalInstallSource::kExternalPolicy,
+      std::vector<GURL>{kFooWebAppUrl});
   EXPECT_EQ(results, UninstallAppsResults({{kFooWebAppUrl, false}}));
 
   EXPECT_EQ(1u, uninstall_call_count());
@@ -1221,9 +1224,9 @@ TEST_F(PendingAppManagerImplTest, UninstallApps_Multiple) {
                                                             true);
   install_finalizer()->SetNextUninstallExternalWebAppResult(kBarWebAppUrl,
                                                             true);
-  UninstallAppsResults results =
-      UninstallAppsAndWait(pending_app_manager_impl(),
-                           std::vector<GURL>{kFooWebAppUrl, kBarWebAppUrl});
+  UninstallAppsResults results = UninstallAppsAndWait(
+      pending_app_manager_impl(), ExternalInstallSource::kExternalPolicy,
+      std::vector<GURL>{kFooWebAppUrl, kBarWebAppUrl});
   EXPECT_EQ(results, UninstallAppsResults(
                          {{kFooWebAppUrl, true}, {kBarWebAppUrl, true}}));
 
@@ -1250,7 +1253,8 @@ TEST_F(PendingAppManagerImplTest, UninstallApps_PendingInstall) {
   install_finalizer()->SetNextUninstallExternalWebAppResult(kFooWebAppUrl,
                                                             false);
   UninstallAppsResults uninstall_results = UninstallAppsAndWait(
-      pending_app_manager_impl(), std::vector<GURL>{kFooWebAppUrl});
+      pending_app_manager_impl(), ExternalInstallSource::kExternalPolicy,
+      std::vector<GURL>{kFooWebAppUrl});
   EXPECT_EQ(uninstall_results, UninstallAppsResults({{kFooWebAppUrl, false}}));
   EXPECT_EQ(1u, uninstall_call_count());
 

diff --git a/chrome/browser/web_applications/test/test_install_finalizer.cc b/chrome/browser/web_applications/test/test_install_finalizer.cc
@@ -55,6 +55,7 @@ void TestInstallFinalizer::FinalizeUninstallAfterSync(
 
 void TestInstallFinalizer::UninstallExternalWebApp(
     const GURL& app_url,
+    ExternalInstallSource external_install_source,
     UninstallWebAppCallback callback) {
   DCHECK(base::Contains(next_uninstall_external_web_app_results_, app_url));
   uninstall_external_web_app_urls_.push_back(app_url);

diff --git a/chrome/browser/web_applications/test/test_install_finalizer.h b/chrome/browser/web_applications/test/test_install_finalizer.h
@@ -36,6 +36,7 @@ class TestInstallFinalizer final : public InstallFinalizer {
   void FinalizeUpdate(const WebApplicationInfo& web_app_info,
                       InstallFinalizedCallback callback) override;
   void UninstallExternalWebApp(const GURL& app_url,
+                               ExternalInstallSource external_install_source,
                                UninstallWebAppCallback callback) override;
   bool CanUserUninstallFromSync(const AppId& app_id) const override;
   void UninstallWebAppFromSyncByUser(const AppId& app_id,

diff --git a/chrome/browser/web_applications/test/test_pending_app_manager.cc b/chrome/browser/web_applications/test/test_pending_app_manager.cc
@@ -75,6 +75,7 @@ void TestPendingAppManager::InstallApps(
 }
 
 void TestPendingAppManager::UninstallApps(std::vector<GURL> uninstall_urls,
+                                          ExternalInstallSource install_source,
                                           const UninstallCallback& callback) {
   auto weak_ptr = weak_ptr_factory_.GetWeakPtr();
   for (const auto& url : uninstall_urls) {

diff --git a/chrome/browser/web_applications/test/test_pending_app_manager.h b/chrome/browser/web_applications/test/test_pending_app_manager.h
@@ -53,6 +53,7 @@ class TestPendingAppManager : public PendingAppManager {
   void InstallApps(std::vector<ExternalInstallOptions> install_options_list,
                    const RepeatingInstallCallback& callback) override;
   void UninstallApps(std::vector<GURL> uninstall_urls,
+                     ExternalInstallSource install_source,
                      const UninstallCallback& callback) override;
   void Shutdown() override {}
 

diff --git a/chrome/browser/web_applications/web_app_install_finalizer.cc b/chrome/browser/web_applications/web_app_install_finalizer.cc
@@ -228,6 +228,7 @@ void WebAppInstallFinalizer::OnIconsDataDeleted(
 
 void WebAppInstallFinalizer::UninstallExternalWebApp(
     const GURL& app_url,
+    ExternalInstallSource external_install_source,
     UninstallWebAppCallback callback) {
   NOTIMPLEMENTED();
 }

diff --git a/chrome/browser/web_applications/web_app_install_finalizer.h b/chrome/browser/web_applications/web_app_install_finalizer.h
@@ -37,6 +37,7 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
   void FinalizeUpdate(const WebApplicationInfo& web_app_info,
                       InstallFinalizedCallback callback) override;
   void UninstallExternalWebApp(const GURL& app_url,
+                               ExternalInstallSource external_install_source,
                                UninstallWebAppCallback callback) override;
   bool CanUserUninstallFromSync(const AppId& app_id) const override;
   void UninstallWebAppFromSyncByUser(const AppId& app_id,