[dPWA] Consolidate CanUserUninstallWebApp()

This CL removes the CanUserUninstallWebApp() from WebAppInstallFinalizer and WebAppDialogManager and moves it to WebAppRegistrar. Because CanUserUninstallWebApp() is a method on WebApp the WebAppRegistrar is an appropriate place to wrap it with an extra "web app is installed" check. Bug: 1427340 Change-Id: If6cf1c92ce0dafbddf748deb9d68b046dbdf4a81 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4549022 Auto-Submit: Alan Cutter <alancutter@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/main@{#1147729}
chromium · May 23, 2023 · 6e5545f · 6e5545f
1 parent 29cfe4d
commit 6e5545f
Show file tree

Hide file tree

Showing 20 changed files with 30 additions and 58 deletions.
diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
@@ -737,7 +737,7 @@ IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
         WebAppProvider::GetForTest(browser()->profile());
     base::RunLoop run_loop;
 
-    ASSERT_TRUE(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+    ASSERT_TRUE(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
     provider->install_finalizer().UninstallWebApp(
         app_id, webapps::WebappUninstallSource::kAppMenu,
         base::BindLambdaForTesting([&](webapps::UninstallResultCode code) {

diff --git a/chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_view.cc b/chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_view.cc
@@ -167,7 +167,7 @@ void WebAppUninstallDialogDelegateView::Uninstall() {
   auto* provider = web_app::WebAppProvider::GetForWebApps(profile_);
   DCHECK(provider);
 
-  if (!provider->install_finalizer().CanUserUninstallWebApp(app_id_)) {
+  if (!provider->registrar_unsafe().CanUserUninstallWebApp(app_id_)) {
     std::exchange(dialog_, nullptr)->UninstallCancelled();
     return;
   }

diff --git a/chrome/browser/ui/web_applications/web_app_browser_controller.cc b/chrome/browser/ui/web_applications/web_app_browser_controller.cc
@@ -571,9 +571,7 @@ std::u16string WebAppBrowserController::GetFormattedUrlOrigin() const {
 }
 
 bool WebAppBrowserController::CanUserUninstall() const {
-  return WebAppUiManagerImpl::Get(&*provider_)
-      ->dialog_manager()
-      .CanUserUninstallWebApp(app_id());
+  return registrar().CanUserUninstallWebApp(app_id());
 }
 
 void WebAppBrowserController::Uninstall(

diff --git a/chrome/browser/ui/web_applications/web_app_browsertest.cc b/chrome/browser/ui/web_applications/web_app_browsertest.cc
@@ -1352,13 +1352,13 @@ IN_PROC_BROWSER_TEST_P(WebAppBrowserTest_ExternalPrefMigration,
   AppId app_id =
       provider->registrar_unsafe().LookupExternalAppId(install_url).value();
 
-  EXPECT_FALSE(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
 
   InstallWebAppFromPage(browser(), install_url);
 
   // Performing a user install on the page should not override the "policy"
   // install source.
-  EXPECT_FALSE(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
   const WebApp& web_app = *provider->registrar_unsafe().GetAppById(app_id);
   EXPECT_TRUE(web_app.IsSynced());
   EXPECT_TRUE(web_app.IsPolicyInstalledApp());

diff --git a/chrome/browser/ui/web_applications/web_app_dialog_manager.cc b/chrome/browser/ui/web_applications/web_app_dialog_manager.cc
@@ -22,14 +22,6 @@ WebAppDialogManager::WebAppDialogManager(Profile* profile)
 
 WebAppDialogManager::~WebAppDialogManager() = default;
 
-bool WebAppDialogManager::CanUserUninstallWebApp(const AppId& app_id) const {
-  auto* provider = WebAppProvider::GetForWebApps(profile_);
-  if (!provider)
-    return false;
-
-  return provider->install_finalizer().CanUserUninstallWebApp(app_id);
-}
-
 void WebAppDialogManager::UninstallWebApp(
     const AppId& app_id,
     webapps::WebappUninstallSource uninstall_source,

diff --git a/chrome/browser/ui/web_applications/web_app_dialog_manager.h b/chrome/browser/ui/web_applications/web_app_dialog_manager.h
@@ -35,7 +35,6 @@ class WebAppDialogManager {
 
   using Callback = base::OnceCallback<void(webapps::UninstallResultCode code)>;
 
-  bool CanUserUninstallWebApp(const AppId& app_id) const;
   // The uninstall dialog will be modal to |parent_window|, or a non-modal if
   // |parent_window| is nullptr.
   void UninstallWebApp(const AppId& app_id,

diff --git a/chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc b/chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc
@@ -88,7 +88,7 @@ void UninstallWebAppWithDialogFromStartupSwitch(const AppId& app_id,
   std::unique_ptr<ScopedKeepAlive> scoped_keep_alive =
       std::make_unique<ScopedKeepAlive>(KeepAliveOrigin::WEB_APP_UNINSTALL,
                                         KeepAliveRestartOption::DISABLED);
-  if (provider->install_finalizer().CanUserUninstallWebApp(app_id)) {
+  if (provider->registrar_unsafe().CanUserUninstallWebApp(app_id)) {
     WebAppUiManagerImpl::Get(provider)->dialog_manager().UninstallWebApp(
         app_id, webapps::WebappUninstallSource::kOsSettings,
         gfx::kNullNativeWindow,

diff --git a/chrome/browser/ui/web_applications/web_app_uninstall_browsertest.cc b/chrome/browser/ui/web_applications/web_app_uninstall_browsertest.cc
@@ -46,7 +46,7 @@ class WebAppUninstallBrowserTest : public WebAppControllerBrowserTest {
     WebAppProvider* const provider = WebAppProvider::GetForTest(profile());
     base::RunLoop run_loop;
 
-    DCHECK(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+    DCHECK(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
     provider->install_finalizer().UninstallWebApp(
         app_id, webapps::WebappUninstallSource::kAppMenu,
         base::BindLambdaForTesting([&](webapps::UninstallResultCode code) {
@@ -159,7 +159,7 @@ IN_PROC_BROWSER_TEST_F(WebAppUninstallBrowserTest, TwoUninstallCalls) {
   // Trigger app uninstall without waiting for result.
   WebAppProvider* const provider = WebAppProvider::GetForTest(profile());
   EXPECT_TRUE(provider->registrar_unsafe().IsInstalled(app_id));
-  DCHECK(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+  DCHECK(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
   provider->install_finalizer().UninstallWebApp(
       app_id, webapps::WebappUninstallSource::kAppMenu,
       base::BindLambdaForTesting([&](webapps::UninstallResultCode code) {

diff --git a/chrome/browser/ui/webui/app_home/app_home_page_handler.cc b/chrome/browser/ui/webui/app_home/app_home_page_handler.cc
@@ -400,7 +400,7 @@ app_home::mojom::AppInfoPtr AppHomePageHandler::CreateAppInfoPtrFromWebApp(
 
   app_info->store_page_url = absl::nullopt;
   app_info->may_uninstall =
-      web_app_provider_->install_finalizer().CanUserUninstallWebApp(app_id);
+      web_app_provider_->registrar_unsafe().CanUserUninstallWebApp(app_id);
   app_info->is_deprecated_app = false;
   return app_info;
 }
@@ -504,7 +504,7 @@ void AppHomePageHandler::ResetExtensionDialogState() {
 }
 
 void AppHomePageHandler::UninstallWebApp(const std::string& web_app_id) {
-  if (!web_app_provider_->install_finalizer().CanUserUninstallWebApp(
+  if (!web_app_provider_->registrar_unsafe().CanUserUninstallWebApp(
           web_app_id)) {
     LOG(ERROR) << "Attempt to uninstall a webapp that is non-usermanagable "
                   "was made. App id : "

diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
@@ -220,9 +220,7 @@ base::Value::Dict AppLauncherHandler::CreateWebAppInfo(
 
   GetWebAppBasicInfo(app_id, registrar, &dict);
 
-  dict.Set(
-      "mayDisable",
-      web_app_provider_->install_finalizer().CanUserUninstallWebApp(app_id));
+  dict.Set("mayDisable", registrar.CanUserUninstallWebApp(app_id));
   bool is_locally_installed = registrar.IsLocallyInstalled(app_id);
   dict.Set("mayChangeLaunchType", is_locally_installed);
 
@@ -991,7 +989,7 @@ void AppLauncherHandler::HandleUninstallApp(const base::Value::List& args) {
       !IsYoutubeExtension(extension_id)) {
     if (!extension_id_prompting_.empty())
       return;  // Only one prompt at a time.
-    if (!web_app_provider_->install_finalizer().CanUserUninstallWebApp(
+    if (!web_app_provider_->registrar_unsafe().CanUserUninstallWebApp(
             extension_id)) {
       LOG(ERROR) << "Attempt to uninstall a webapp that is non-usermanagable "
                  << "was made. App id : " << extension_id;

diff --git a/chrome/browser/web_applications/app_service/web_app_publisher_helper.cc b/chrome/browser/web_applications/app_service/web_app_publisher_helper.cc
@@ -404,9 +404,9 @@ void UninstallImpl(WebAppProvider* provider,
     return;
   }
 
-  WebAppDialogManager& web_app_dialog_manager =
-      web_app_ui_manager->dialog_manager();
-  if (web_app_dialog_manager.CanUserUninstallWebApp(app_id)) {
+  if (provider->registrar_unsafe().CanUserUninstallWebApp(app_id)) {
+    WebAppDialogManager& web_app_dialog_manager =
+        web_app_ui_manager->dialog_manager();
     webapps::WebappUninstallSource webapp_uninstall_source =
         WebAppPublisherHelper::ConvertUninstallSourceToWebAppUninstallSource(
             uninstall_source);
@@ -785,7 +785,7 @@ void WebAppPublisherHelper::UninstallWebApp(
 
   DCHECK(provider_);
   DCHECK(
-      provider_->install_finalizer().CanUserUninstallWebApp(web_app->app_id()));
+      provider_->registrar_unsafe().CanUserUninstallWebApp(web_app->app_id()));
   webapps::WebappUninstallSource webapp_uninstall_source =
       ConvertUninstallSourceToWebAppUninstallSource(uninstall_source);
   provider_->install_finalizer().UninstallWebApp(

diff --git a/chrome/browser/web_applications/extensions/externally_managed_app_install_task_unittest.cc b/chrome/browser/web_applications/extensions/externally_managed_app_install_task_unittest.cc
@@ -219,11 +219,6 @@ class TestExternallyManagedAppInstallFinalizer : public WebAppInstallFinalizer {
             }));
   }
 
-  bool CanUserUninstallWebApp(const AppId& app_id) const override {
-    NOTIMPLEMENTED();
-    return false;
-  }
-
   void UninstallWebApp(const AppId& app_id,
                        webapps::WebappUninstallSource uninstall_source,
                        UninstallWebAppCallback callback) override {

diff --git a/chrome/browser/web_applications/manifest_update_manager_browsertest.cc b/chrome/browser/web_applications/manifest_update_manager_browsertest.cc
@@ -1408,8 +1408,7 @@ IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
   )";
   OverrideManifest(kManifestTemplate, {"blue", kInstallableIconList});
   AppId app_id = InstallPolicyApp();
-  EXPECT_FALSE(
-      GetProvider().install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(GetProvider().registrar_unsafe().CanUserUninstallWebApp(app_id));
 
   OverrideManifest(kManifestTemplate, {"red", kInstallableIconList});
   EXPECT_EQ(GetResultAfterPageLoad(GetAppURL()),
@@ -1425,8 +1424,7 @@ IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
 
   // Policy installed apps should continue to be not uninstallable by the user
   // after updating.
-  EXPECT_FALSE(
-      GetProvider().install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(GetProvider().registrar_unsafe().CanUserUninstallWebApp(app_id));
 }
 
 IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
@@ -1443,8 +1441,7 @@ IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
   )";
   OverrideManifest(kManifestTemplate, {"blue", kInstallableIconList});
   AppId app_id = InstallKioskApp();
-  EXPECT_FALSE(
-      GetProvider().install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(GetProvider().registrar_unsafe().CanUserUninstallWebApp(app_id));
 
   OverrideManifest(kManifestTemplate, {"red", kInstallableIconList});
   EXPECT_EQ(GetResultAfterPageLoad(GetAppURL()),
@@ -1460,8 +1457,7 @@ IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
 
   // Kiosk installed apps should continue to be not uninstallable by the user
   // after updating.
-  EXPECT_FALSE(
-      GetProvider().install_finalizer().CanUserUninstallWebApp(app_id));
+  EXPECT_FALSE(GetProvider().registrar_unsafe().CanUserUninstallWebApp(app_id));
 }
 
 IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,

diff --git a/chrome/browser/web_applications/test/fake_install_finalizer.cc b/chrome/browser/web_applications/test/fake_install_finalizer.cc
@@ -77,11 +77,6 @@ void FakeInstallFinalizer::UninstallExternalWebAppByUrl(
                      }));
 }
 
-bool FakeInstallFinalizer::CanUserUninstallWebApp(const AppId& app_id) const {
-  NOTIMPLEMENTED();
-  return false;
-}
-
 void FakeInstallFinalizer::UninstallWebApp(
     const AppId& app_id,
     webapps::WebappUninstallSource uninstall_source,

diff --git a/chrome/browser/web_applications/test/fake_install_finalizer.h b/chrome/browser/web_applications/test/fake_install_finalizer.h
@@ -42,7 +42,6 @@ class FakeInstallFinalizer final : public WebAppInstallFinalizer {
       WebAppManagement::Type source,
       webapps::WebappUninstallSource uninstall_surface,
       UninstallWebAppCallback callback) override;
-  bool CanUserUninstallWebApp(const AppId& app_id) const override;
   void UninstallWebApp(const AppId& app_id,
                        webapps::WebappUninstallSource uninstall_source,
                        UninstallWebAppCallback callback) override;

diff --git a/chrome/browser/web_applications/test/web_app_install_test_utils.cc b/chrome/browser/web_applications/test/web_app_install_test_utils.cc
@@ -122,7 +122,7 @@ void UninstallWebApp(Profile* profile, const AppId& app_id) {
   WebAppProvider* const provider = WebAppProvider::GetForTest(profile);
   base::RunLoop run_loop;
 
-  DCHECK(provider->install_finalizer().CanUserUninstallWebApp(app_id));
+  DCHECK(provider->registrar_unsafe().CanUserUninstallWebApp(app_id));
   provider->install_finalizer().UninstallWebApp(
       app_id, webapps::WebappUninstallSource::kAppMenu,
       base::BindLambdaForTesting([&](webapps::UninstallResultCode code) {

diff --git a/chrome/browser/web_applications/web_app_install_finalizer.cc b/chrome/browser/web_applications/web_app_install_finalizer.cc
@@ -309,13 +309,6 @@ void WebAppInstallFinalizer::UninstallExternalWebAppByUrl(
                           uninstall_source, std::move(callback));
 }
 
-bool WebAppInstallFinalizer::CanUserUninstallWebApp(const AppId& app_id) const {
-  DCHECK(started_);
-
-  const WebApp* app = GetWebAppRegistrar().GetAppById(app_id);
-  return app ? app->CanUserUninstallWebApp() : false;
-}
-
 void WebAppInstallFinalizer::UninstallWebApp(
     const AppId& app_id,
     webapps::WebappUninstallSource webapp_uninstall_source,

diff --git a/chrome/browser/web_applications/web_app_install_finalizer.h b/chrome/browser/web_applications/web_app_install_finalizer.h
@@ -138,8 +138,6 @@ class WebAppInstallFinalizer {
                                webapps::WebappUninstallSource uninstall_surface,
                                UninstallWebAppCallback callback);
 
-  virtual bool CanUserUninstallWebApp(const AppId& app_id) const;
-
   virtual bool CanReparentTab(const AppId& app_id, bool shortcut_created) const;
   virtual void ReparentTab(const AppId& app_id,
                            bool shortcut_created,

diff --git a/chrome/browser/web_applications/web_app_registrar.cc b/chrome/browser/web_applications/web_app_registrar.cc
@@ -797,6 +797,11 @@ bool WebAppRegistrar::WasInstalledBySubApp(const AppId& app_id) const {
   return web_app && web_app->IsSubAppInstalledApp();
 }
 
+bool WebAppRegistrar::CanUserUninstallWebApp(const AppId& app_id) const {
+  const WebApp* web_app = GetAppById(app_id);
+  return web_app && web_app->CanUserUninstallWebApp();
+}
+
 bool WebAppRegistrar::IsAllowedLaunchProtocol(
     const AppId& app_id,
     const std::string& protocol_scheme) const {

diff --git a/chrome/browser/web_applications/web_app_registrar.h b/chrome/browser/web_applications/web_app_registrar.h
@@ -140,6 +140,10 @@ class WebAppRegistrar : public ProfileManagerObserver {
   // Returns true if the app was installed by the SubApp API.
   bool WasInstalledBySubApp(const AppId& app_id) const;
 
+  // Returns true if the app exists and is allowed to be uninstalled by the user
+  // e.g. it is not policy installed.
+  bool CanUserUninstallWebApp(const AppId& app_id) const;
+
   // Returns the AppIds and URLs of apps externally installed from
   // |install_source|.
   base::flat_map<AppId, base::flat_set<GURL>> GetExternallyInstalledApps(