[dPWA] Explicitly pass source to uninstall, and require install source.

This patch fixes the incorrect assumption that we can determine the
installation source from the uninstallation location, and makes the
installation source a bit more required for install tasks and options.

This also updates some variable names from install_source to
install_surface, which is a more accurate name. Work to rename the whole
enum is in https://crbug.com/1313273.

The only functional change in this CL is in
externally_managed_app_install_task.cc:187, which sets the source of the
uninstall correctly now. This fixes an issue where replaced placeholder
apps aren't uninstalled correctly.

All other changes are purely refactoring.

Bug: 1312061
Change-Id: I82968bc91e12e8d60736ed32af9782312941d98e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3570646
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Tim Sergeant <tsergeant@chromium.org>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Edman Anjos <edman@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Phillis Tang <phillis@chromium.org>
Reviewed-by: Glenn Hartmann <hartmanng@chromium.org>
Commit-Queue: Glenn Hartmann <hartmanng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#989943}

chrome/browser/ash/android_sms/android_sms_app_setup_controller_impl.cc

@@ -78,7 +78,8 @@ void AndroidSmsAppSetupControllerImpl::PwaDelegate::RemovePwa(
   }
 
   provider->install_finalizer().UninstallExternalWebApp(
-      app_id, webapps::WebappUninstallSource::kInternalPreinstalled,
+      app_id, web_app::Source::kDefault,
+      webapps::WebappUninstallSource::kInternalPreinstalled,
       base::BindOnce(
           [](SuccessCallback callback, webapps::UninstallResultCode code) {
             std::move(callback).Run(code ==

chrome/browser/ash/app_mode/web_app/web_kiosk_app_launcher.cc

@@ -23,6 +23,7 @@
 #include "chromeos/ui/base/window_pin_type.h"
 #include "components/account_id/account_id.h"
 #include "components/webapps/browser/install_result_code.h"
+#include "components/webapps/browser/installable/installable_metrics.h"
 #include "ui/aura/window.h"
 #include "ui/base/page_transition_types.h"
 #include "url/origin.h"
@@ -74,7 +75,7 @@ void WebKioskAppLauncher::ContinueWithNetworkReady() {
       profile_,
       /*install_manager=*/nullptr,
       /*install_finalizer=*/nullptr, data_retriever_factory_.Run(),
-      /*registrar=*/nullptr);
+      /*registrar=*/nullptr, webapps::WebappInstallSource::MANAGEMENT_API);
   install_task_->LoadAndRetrieveWebAppInstallInfoWithIcons(
       WebKioskAppManager::Get()->GetAppByAccountId(account_id_)->install_url(),
       url_loader_.get(),

chrome/browser/ash/apps/apk_web_app_service.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/shelf/chrome_shelf_controller.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
@@ -252,7 +253,8 @@ void ApkWebAppService::UninstallWebApp(const web_app::AppId& web_app_id) {
   } else {
     DCHECK(provider_);
     provider_->install_finalizer().UninstallExternalWebApp(
-        web_app_id, webapps::WebappUninstallSource::kArc, base::DoNothing());
+        web_app_id, web_app::Source::kWebAppStore,
+        webapps::WebappUninstallSource::kArc, base::DoNothing());
   }
 }
 

chrome/browser/ash/child_accounts/time_limits/app_service_wrapper_unittest.cc

@@ -187,7 +187,7 @@ class AppServiceWrapperTest : public ::testing::Test {
       WebAppProvider::GetForTest(&profile_)
           ->install_finalizer()
           .UninstallExternalWebApp(
-              app_id.app_id(),
+              app_id.app_id(), web_app::Source::kDefault,
               webapps::WebappUninstallSource::kExternalPreinstalled,
               base::BindLambdaForTesting(
                   [&](webapps::UninstallResultCode code) {

chrome/browser/lacros/web_app_provider_bridge_lacros.cc

@@ -7,6 +7,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_id.h"
 #include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
@@ -74,8 +75,8 @@ void WebAppProviderBridgeLacros::WebAppUninstalledInArc(
             DCHECK(profile);
             auto* provider = web_app::WebAppProvider::GetForWebApps(profile);
             provider->install_finalizer().UninstallExternalWebApp(
-                app_id, webapps::WebappUninstallSource::kArc,
-                std::move(callback));
+                app_id, web_app::Source::kWebAppStore,
+                webapps::WebappUninstallSource::kArc, std::move(callback));
           },
           app_id, std::move(callback)));
 }

chrome/browser/ui/startup/startup_browser_creator_browsertest.cc

@@ -74,6 +74,7 @@
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/test/fake_web_app_provider.h"
 #include "chrome/browser/web_applications/test/web_app_test_observers.h"
+#include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_install_manager.h"
@@ -2230,8 +2231,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserWithWebAppTest,
   web_app::WebAppInstallFinalizer& web_app_finalizer =
       provider->install_finalizer();
 
-  web_app::WebAppInstallFinalizer::FinalizeOptions options;
-  options.install_source = webapps::WebappInstallSource::OMNIBOX_INSTALL_ICON;
+  web_app::WebAppInstallFinalizer::FinalizeOptions options(
+      webapps::WebappInstallSource::OMNIBOX_INSTALL_ICON);
 
   // Install web app set to open as a tab.
   {

chrome/browser/ui/web_applications/sub_apps_service_impl.cc

@@ -171,7 +171,8 @@ void SubAppsServiceImpl::Remove(const std::string& unhashed_app_id,
   }
 
   provider->install_finalizer().UninstallExternalWebApp(
-      sub_app_id, webapps::WebappUninstallSource::kSubApp,
+      sub_app_id, Source::Type::kSubApp,
+      webapps::WebappUninstallSource::kSubApp,
       base::BindOnce(&OnRemove, std::move(result_callback)));
 }
 

chrome/browser/web_applications/extensions/externally_managed_app_install_task_unittest.cc

@@ -190,10 +190,10 @@ class TestExternallyManagedAppInstallFinalizer : public WebAppInstallFinalizer {
     NOTREACHED();
   }
 
-  void UninstallExternalWebApp(
-      const AppId& app_id,
-      webapps::WebappUninstallSource external_install_source,
-      UninstallWebAppCallback callback) override {
+  void UninstallExternalWebApp(const AppId& app_id,
+                               Source::Type external_source,
+                               webapps::WebappUninstallSource uninstall_source,
+                               UninstallWebAppCallback callback) override {
     UnregisterApp(app_id);
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -203,7 +203,8 @@ class TestExternallyManagedAppInstallFinalizer : public WebAppInstallFinalizer {
 
   void UninstallExternalWebAppByUrl(
       const GURL& app_url,
-      webapps::WebappUninstallSource external_install_source,
+      Source::Type external_source,
+      webapps::WebappUninstallSource uninstall_source,
       UninstallWebAppCallback callback) override {
     DCHECK(base::Contains(next_uninstall_external_web_app_results_, app_url));
     uninstall_external_web_app_urls_.push_back(app_url);
@@ -436,7 +437,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallSucceeds) {
 
             EXPECT_EQ(web_app_info().user_display_mode, DisplayMode::kBrowser);
             EXPECT_EQ(webapps::WebappInstallSource::INTERNAL_DEFAULT,
-                      finalize_options().install_source);
+                      finalize_options().install_surface);
 
             run_loop.Quit();
           }));
@@ -545,7 +546,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallPreinstalledApp) {
                       EXPECT_TRUE(result.app_id.has_value());
 
                       EXPECT_EQ(webapps::WebappInstallSource::INTERNAL_DEFAULT,
-                                finalize_options().install_source);
+                                finalize_options().install_surface);
                       run_loop.Quit();
                     }));
 
@@ -571,7 +572,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallAppFromPolicy) {
                       EXPECT_TRUE(result.app_id.has_value());
 
                       EXPECT_EQ(webapps::WebappInstallSource::EXTERNAL_POLICY,
-                                finalize_options().install_source);
+                                finalize_options().install_surface);
                       run_loop.Quit();
                     }));
 
@@ -601,7 +602,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallPlaceholder) {
 
             EXPECT_EQ(1u, finalizer()->finalize_options_list().size());
             EXPECT_EQ(webapps::WebappInstallSource::EXTERNAL_POLICY,
-                      finalize_options().install_source);
+                      finalize_options().install_surface);
             const WebAppInstallInfo& web_app_info =
                 finalizer()->web_app_info_list().at(0);
 
@@ -639,7 +640,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallPlaceholderDefaultSource) {
 
             EXPECT_EQ(1u, finalizer()->finalize_options_list().size());
             EXPECT_EQ(webapps::WebappInstallSource::EXTERNAL_DEFAULT,
-                      finalize_options().install_source);
+                      finalize_options().install_surface);
             const WebAppInstallInfo& web_app_info =
                 finalizer()->web_app_info_list().at(0);
 
@@ -1013,7 +1014,7 @@ TEST_F(ExternallyManagedAppInstallTaskTest, InstallWithWebAppInfoSucceeds) {
 
         EXPECT_EQ(web_app_info().user_display_mode, DisplayMode::kStandalone);
         EXPECT_EQ(webapps::WebappInstallSource::SYSTEM_DEFAULT,
-                  finalize_options().install_source);
+                  finalize_options().install_surface);
 
         run_loop.Quit();
       }));

chrome/browser/web_applications/extensions/web_app_policy_manager_unittest.cc

@@ -290,12 +290,6 @@ ExternalInstallOptions GetCustomAppIconInstallOptions() {
 }
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
-Source::Type ConvertExternalInstallSourceToSourceType(
-    ExternalInstallSource external_install_source) {
-  return InferSourceFromMetricsInstallSource(
-      ConvertExternalInstallSourceToInstallSource(external_install_source));
-}
-
 }  // namespace
 
 enum class TestParam { kLacrosDisabled, kLacrosEnabled };
@@ -355,7 +349,7 @@ class WebAppPolicyManagerTest : public ChromeRenderViewHostTestHarness,
                 const auto install_source = install_options.install_source;
                 std::unique_ptr<WebApp> web_app = test::CreateWebApp(
                     install_url,
-                    ConvertExternalInstallSourceToSourceType(install_source));
+                    ConvertExternalInstallSourceToSource(install_source));
                 if (install_options.override_name)
                   web_app->SetName(install_options.override_name.value());
                 RegisterApp(std::move(web_app));
@@ -399,7 +393,7 @@ class WebAppPolicyManagerTest : public ChromeRenderViewHostTestHarness,
   void SimulatePreviouslyInstalledApp(const GURL& url,
                                       ExternalInstallSource install_source) {
     auto web_app = test::CreateWebApp(
-        url, ConvertExternalInstallSourceToSourceType(install_source));
+        url, ConvertExternalInstallSourceToSource(install_source));
     RegisterApp(std::move(web_app));
 
     externally_installed_app_prefs().Insert(

chrome/browser/web_applications/externally_installed_web_app_prefs.h

@@ -64,6 +64,7 @@ class ExternallyInstalledWebAppPrefs {
   // does not mean that there is no app for |url| just that there is no
   // *placeholder app*.
   absl::optional<AppId> LookupPlaceholderAppId(const GURL& url) const;
+
   void SetIsPlaceholder(const GURL& url, bool is_placeholder);
   bool IsPlaceholderApp(const AppId& app_id) const;
 

chrome/browser/web_applications/externally_managed_app_install_task.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_install_finalizer.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_install_manager.h"
@@ -183,6 +184,7 @@ void ExternallyManagedAppInstallTask::UninstallPlaceholderApp(
   // Otherwise, uninstall the placeholder app.
   install_finalizer_->UninstallExternalWebAppByUrl(
       install_options_.install_url,
+      ConvertExternalInstallSourceToSource(install_options_.install_source),
       webapps::WebappUninstallSource::kPlaceholderReplacement,
       base::BindOnce(&ExternallyManagedAppInstallTask::OnPlaceholderUninstalled,
                      weak_ptr_factory_.GetWeakPtr(), web_contents,
@@ -298,9 +300,9 @@ void ExternallyManagedAppInstallTask::FinalizePlaceholderInstall(
 
   web_app_info.user_display_mode = install_options_.user_display_mode;
 
-  WebAppInstallFinalizer::FinalizeOptions options;
-  options.install_source = ConvertExternalInstallSourceToInstallSource(
-      install_options_.install_source);
+  WebAppInstallFinalizer::FinalizeOptions options(
+      ConvertExternalInstallSourceToInstallSource(
+          install_options_.install_source));
   // Overwrite fields if we are doing a forced reinstall, because some
   // values (custom name or icon) might have changed.
   options.overwrite_existing_manifest_fields = install_options_.force_reinstall;

chrome/browser/web_applications/externally_managed_app_manager.cc

@@ -13,6 +13,7 @@
 #include "base/containers/contains.h"
 #include "base/stl_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/web_applications/web_app_install_utils.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "components/webapps/browser/install_result_code.h"
 
@@ -100,8 +101,7 @@ void ExternallyManagedAppManager::SynchronizeInstalledApps(
     bool has_same_external_source =
         registrar_->GetAppById(apps_it.first)
             ->GetSources()
-            .test(InferSourceFromMetricsInstallSource(
-                ConvertExternalInstallSourceToInstallSource(install_source)));
+            .test(ConvertExternalInstallSourceToSource(install_source));
     if (has_same_external_source)
       installed_urls.push_back(apps_it.second);
   }

chrome/browser/web_applications/externally_managed_app_manager_impl.cc

@@ -84,7 +84,8 @@ void ExternallyManagedAppManagerImpl::UninstallApps(
     const UninstallCallback& callback) {
   for (auto& url : uninstall_urls) {
     finalizer()->UninstallExternalWebAppByUrl(
-        url, ConvertExternalInstallSourceToUninstallSource(install_source),
+        url, ConvertExternalInstallSourceToSource(install_source),
+        ConvertExternalInstallSourceToUninstallSource(install_source),
         base::BindOnce(
             [](const UninstallCallback& callback, const GURL& app_url,
                webapps::UninstallResultCode code) {
@@ -208,9 +209,8 @@ void ExternallyManagedAppManagerImpl::MaybeStartNext() {
       {
         ScopedRegistryUpdate update(sync_bridge());
         WebApp* app_to_update = update->UpdateApp(app_id.value());
-        app_to_update->AddSource(InferSourceFromMetricsInstallSource(
-            ConvertExternalInstallSourceToInstallSource(
-                install_options.install_source)));
+        app_to_update->AddSource(ConvertExternalInstallSourceToSource(
+            install_options.install_source));
       }
       std::move(front->callback)
           .Run(install_options.install_url,

chrome/browser/web_applications/test/fake_install_finalizer.cc

@@ -49,7 +49,8 @@ void FakeInstallFinalizer::FinalizeUpdate(const WebAppInstallInfo& web_app_info,
 
 void FakeInstallFinalizer::UninstallExternalWebApp(
     const AppId& app_id,
-    webapps::WebappUninstallSource webapp_uninstall_source,
+    Source::Type source,
+    webapps::WebappUninstallSource uninstall_surface,
     UninstallWebAppCallback callback) {
   user_uninstalled_external_apps_.erase(app_id);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -59,7 +60,8 @@ void FakeInstallFinalizer::UninstallExternalWebApp(
 
 void FakeInstallFinalizer::UninstallExternalWebAppByUrl(
     const GURL& app_url,
-    webapps::WebappUninstallSource webapp_uninstall_source,
+    Source::Type source,
+    webapps::WebappUninstallSource uninstall_surface,
     UninstallWebAppCallback callback) {
   DCHECK(base::Contains(next_uninstall_external_web_app_results_, app_url));
   uninstall_external_web_app_urls_.push_back(app_url);

chrome/browser/web_applications/test/fake_install_finalizer.h

@@ -32,13 +32,14 @@ class FakeInstallFinalizer final : public WebAppInstallFinalizer {
                        InstallFinalizedCallback callback) override;
   void FinalizeUpdate(const WebAppInstallInfo& web_app_info,
                       InstallFinalizedCallback callback) override;
-  void UninstallExternalWebApp(
-      const AppId& app_id,
-      webapps::WebappUninstallSource external_install_source,
-      UninstallWebAppCallback callback) override;
+  void UninstallExternalWebApp(const AppId& app_id,
+                               Source::Type source,
+                               webapps::WebappUninstallSource uninstall_surface,
+                               UninstallWebAppCallback callback) override;
   void UninstallExternalWebAppByUrl(
       const GURL& app_url,
-      webapps::WebappUninstallSource external_install_source,
+      Source::Type source,
+      webapps::WebappUninstallSource uninstall_surface,
       UninstallWebAppCallback callback) override;
   void UninstallWithoutRegistryUpdateFromSync(
       const std::vector<AppId>& web_apps,

chrome/browser/web_applications/test/web_app_install_test_utils.cc

@@ -56,9 +56,9 @@ AppId InstallDummyWebApp(Profile* profile,
   web_app_info.description = base::UTF8ToUTF16(app_name);
   web_app_info.user_display_mode = DisplayMode::kStandalone;
 
-  WebAppInstallFinalizer::FinalizeOptions options;
+  WebAppInstallFinalizer::FinalizeOptions options(
+      webapps::WebappInstallSource::EXTERNAL_DEFAULT);
   options.bypass_os_hooks = true;
-  options.install_source = webapps::WebappInstallSource::EXTERNAL_DEFAULT;
 
   // In unit tests, we do not have Browser or WebContents instances.
   // Hence we use FinalizeInstall instead of InstallWebAppFromManifest