dpwa: Implement backward compatibility uninstall for Bookmark Apps.

Implement backward compatibility uninstall of Web Apps via
shadow Bookmark Apps.

On a web app uninstall we also must uninstall the shadow bookmark app.

All uninstall changes for the extensions registry get
propagated to the sync server (syncer::ModelType::APPS) and to all
devices where BMO is disabled.

Design doc: go/chrome-bmo-migration.

This is a re-land of
https://chromium-review.googlesource.com/c/chromium/src/+/2162507

We do only legacy uninstalls in this CL to fix the bug.

Bug: 1020037, 1086909
Change-Id: Ib65edc2762de38f050afedb00ffe06665845431b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2228371
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774552}

chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_view.cc

@@ -100,8 +100,6 @@ WebAppUninstallDialogDelegateView::WebAppUninstallDialogDelegateView(
   checkbox_ = AddChildView(std::move(checkbox));
 
   chrome::RecordDialogCreation(chrome::DialogIdentifier::EXTENSION_UNINSTALL);
-
-  ProcessAutoConfirmValue();
 }
 
 WebAppUninstallDialogDelegateView::~WebAppUninstallDialogDelegateView() {
@@ -244,6 +242,9 @@ void WebAppUninstallDialogViews::OnAllIconsRead(
 
   if (dialog_shown_callback_for_testing_)
     std::move(dialog_shown_callback_for_testing_).Run();
+
+  // This should be a tail call because it destroys |this|:
+  view_->ProcessAutoConfirmValue();
 }
 
 void WebAppUninstallDialogViews::OnWebAppUninstalled(

chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_view.h

@@ -47,6 +47,8 @@ class WebAppUninstallDialogDelegateView : public views::DialogDelegateView {
       const WebAppUninstallDialogDelegateView&) = delete;
   ~WebAppUninstallDialogDelegateView() override;
 
+  void ProcessAutoConfirmValue();
+
  private:
   // views::DialogDelegateView:
   gfx::Size CalculatePreferredSize() const override;
@@ -59,7 +61,6 @@ class WebAppUninstallDialogDelegateView : public views::DialogDelegateView {
   // Uninstalls the web app. Returns true on success.
   bool Uninstall();
   void ClearWebAppSiteData();
-  void ProcessAutoConfirmValue();
 
   void OnDialogAccepted();
   void OnDialogCanceled();

chrome/browser/web_applications/components/install_finalizer.cc

@@ -77,4 +77,9 @@ void InstallFinalizer::ReparentTab(const AppId& app_id,
                                              shortcut_created);
 }
 
+AppRegistrar& InstallFinalizer::registrar() const {
+  DCHECK(!is_legacy_finalizer());
+  return *registrar_;
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/install_finalizer.h

@@ -113,11 +113,15 @@ class InstallFinalizer {
   virtual ~InstallFinalizer() = default;
 
  protected:
-  AppRegistrar& registrar() const { return *registrar_; }
+  bool is_legacy_finalizer() const { return registrar_ == nullptr; }
+  AppRegistrar& registrar() const;
+
   WebAppUiManager& ui_manager() const { return *ui_manager_; }
   AppRegistryController& registry_controller() { return *registry_controller_; }
 
  private:
+  // If these pointers are nullptr then this is legacy install finalizer
+  // operating in standalone mode.
   AppRegistrar* registrar_ = nullptr;
   AppRegistryController* registry_controller_ = nullptr;
   WebAppUiManager* ui_manager_ = nullptr;

chrome/browser/web_applications/components/install_finalizer_unittest.cc

@@ -85,7 +85,7 @@ class InstallFinalizerUnitTest
     switch (GetParam()) {
       case ProviderType::kWebApps:
         finalizer_ = std::make_unique<WebAppInstallFinalizer>(
-            profile(), icon_manager_.get());
+            profile(), icon_manager_.get(), /*legacy_finalizer=*/nullptr);
         break;
       case ProviderType::kBookmarkApps:
         InitializeEmptyExtensionService(profile());

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -289,12 +289,13 @@ void BookmarkAppInstallFinalizer::OnExtensionInstalled(
 
   SetLaunchType(profile_, extension->id(), launch_type);
 
-  registry_controller().SetExperimentalTabbedWindowMode(
-      extension->id(), enable_experimental_tabbed_window);
-
   SetBookmarkAppIsLocallyInstalled(profile_, extension, is_locally_installed);
 
-  registrar().NotifyWebAppInstalled(extension->id());
+  if (!is_legacy_finalizer()) {
+    registry_controller().SetExperimentalTabbedWindowMode(
+        extension->id(), enable_experimental_tabbed_window);
+    registrar().NotifyWebAppInstalled(extension->id());
+  }
 
   std::move(callback).Run(extension->id(),
                           web_app::InstallResultCode::kSuccessNewInstall);
@@ -323,7 +324,8 @@ void BookmarkAppInstallFinalizer::OnExtensionUpdated(
     return;
   }
 
-  registrar().NotifyWebAppManifestUpdated(extension->id(), old_name);
+  if (!is_legacy_finalizer())
+    registrar().NotifyWebAppManifestUpdated(extension->id(), old_name);
 
   std::move(callback).Run(extension->id(),
                           web_app::InstallResultCode::kSuccessAlreadyInstalled);

chrome/browser/web_applications/extensions/bookmark_app_registrar.cc

@@ -30,11 +30,14 @@ namespace extensions {
 
 BookmarkAppRegistrar::BookmarkAppRegistrar(Profile* profile)
     : AppRegistrar(profile) {
-  extension_observer_.Add(ExtensionRegistry::Get(profile));
 }
 
 BookmarkAppRegistrar::~BookmarkAppRegistrar() = default;
 
+void BookmarkAppRegistrar::Start() {
+  extension_observer_.Add(ExtensionRegistry::Get(profile()));
+}
+
 bool BookmarkAppRegistrar::IsInstalled(const web_app::AppId& app_id) const {
   const Extension* extension = GetEnabledExtension(app_id);
   return extension && extension->from_bookmark();

chrome/browser/web_applications/extensions/bookmark_app_registrar.h

@@ -25,6 +25,7 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar,
   ~BookmarkAppRegistrar() override;
 
   // AppRegistrar:
+  void Start() override;
   bool IsInstalled(const web_app::AppId& app_id) const override;
   bool IsLocallyInstalled(const web_app::AppId& app_id) const override;
   bool WasInstalledByUser(const web_app::AppId& app_id) const override;

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -128,9 +128,12 @@ void SetWebAppFileHandlers(
 
 }  // namespace
 
-WebAppInstallFinalizer::WebAppInstallFinalizer(Profile* profile,
-                                               WebAppIconManager* icon_manager)
-    : profile_(profile),
+WebAppInstallFinalizer::WebAppInstallFinalizer(
+    Profile* profile,
+    WebAppIconManager* icon_manager,
+    std::unique_ptr<InstallFinalizer> legacy_finalizer)
+    : legacy_finalizer_(std::move(legacy_finalizer)),
+      profile_(profile),
       icon_manager_(icon_manager) {}
 
 WebAppInstallFinalizer::~WebAppInstallFinalizer() = default;
@@ -197,6 +200,8 @@ void WebAppInstallFinalizer::FinalizeInstall(
 
   SetWebAppManifestFieldsAndWriteData(web_app_info, std::move(web_app),
                                       std::move(commit_callback));
+
+  // TODO(crbug.com/1020037): Install shadow bookmark app in extensions.
 }
 
 void WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync(
@@ -325,6 +330,10 @@ void WebAppInstallFinalizer::UninstallExternalAppByUser(
   // should separate UninstallWebAppFromSyncByUser from
   // UninstallExternalAppByUser.
   UninstallWebApp(app_id, std::move(callback));
+
+  // Uninstall shadow bookmark app from this device and from the sync server.
+  if (legacy_finalizer_)
+    legacy_finalizer_->UninstallExternalAppByUser(app_id, base::DoNothing());
 }
 
 bool WebAppInstallFinalizer::WasExternalAppUninstalledByUser(
@@ -359,6 +368,8 @@ void WebAppInstallFinalizer::FinalizeUpdate(
 
   SetWebAppManifestFieldsAndWriteData(web_app_info, std::move(web_app),
                                       std::move(commit_callback));
+
+  // TODO(crbug.com/1020037): Update shadow bookmark app in extensions.
 }
 
 void WebAppInstallFinalizer::Start() {

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -26,8 +26,10 @@ class WebAppRegistrar;
 
 class WebAppInstallFinalizer final : public InstallFinalizer {
  public:
+  // |legacy_finalizer| can be nullptr (optional argument).
   WebAppInstallFinalizer(Profile* profile,
-                         WebAppIconManager* icon_manager);
+                         WebAppIconManager* icon_manager,
+                         std::unique_ptr<InstallFinalizer> legacy_finalizer);
   ~WebAppInstallFinalizer() override;
 
   // InstallFinalizer:
@@ -94,6 +96,8 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
 
   WebAppRegistrar& GetWebAppRegistrar() const;
 
+  std::unique_ptr<InstallFinalizer> legacy_finalizer_;
+
   Profile* const profile_;
   WebAppIconManager* const icon_manager_;
   bool started_ = false;

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -144,7 +144,7 @@ class WebAppInstallManagerTest : public WebAppTest {
                                                         std::move(file_utils));
 
     install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
-        profile(), icon_manager_.get());
+        profile(), icon_manager_.get(), /*legacy_finalizer=*/nullptr);
 
     shortcut_manager_ = std::make_unique<TestAppShortcutManager>(profile());
     file_handler_manager_ = std::make_unique<TestFileHandlerManager>(profile());

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -95,9 +95,9 @@ class WebAppInstallTaskTest : public WebAppTest {
 
     ui_manager_ = std::make_unique<TestWebAppUiManager>();
 
-    install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
-        profile(), icon_manager_.get());
-
+    install_finalizer_ =
+        std::make_unique<WebAppInstallFinalizer>(profile(), icon_manager_.get(),
+                                                 /*legacy_finalizer=*/nullptr);
     shortcut_manager_ = std::make_unique<TestAppShortcutManager>(profile());
     file_handler_manager_ = std::make_unique<TestFileHandlerManager>(profile());
 

chrome/browser/web_applications/web_app_migration_manager_browsertest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/web_applications/web_app_migration_manager.h"
 
 #include "base/files/file_util.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
@@ -13,11 +14,14 @@
 #include "base/test/bind_test_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/installable/installable_metrics.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/web_applications/test/ssl_test_utils.h"
+#include "chrome/browser/ui/web_applications/web_app_dialog_manager.h"
 #include "chrome/browser/ui/web_applications/web_app_dialog_utils.h"
+#include "chrome/browser/ui/web_applications/web_app_ui_manager_impl.h"
 #include "chrome/browser/web_applications/components/app_icon_manager.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/app_shortcut_manager.h"
@@ -35,6 +39,10 @@
 #include "content/public/test/browser_test.h"
 #include "content/public/test/test_launcher.h"
 #include "content/public/test/url_loader_interceptor.h"
+#include "extensions/browser/extension_dialog_auto_confirm.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/test_extension_registry_observer.h"
+#include "extensions/common/extension.h"
 #include "net/ssl/ssl_info.h"
 
 namespace web_app {
@@ -150,12 +158,32 @@ class WebAppMigrationManagerBrowserTest : public InProcessBrowserTest {
     return app_id;
   }
 
+  void UninstallWebAppAsUserViaMenu(const AppId& app_id) {
+    extensions::ScopedTestDialogAutoConfirm confirm{
+        extensions::ScopedTestDialogAutoConfirm::ACCEPT};
+
+    base::RunLoop run_loop;
+    ui_manager().dialog_manager().UninstallWebApp(
+        app_id, WebAppDialogManager::UninstallSource::kAppMenu,
+        browser()->window(), base::BindLambdaForTesting([&](bool success) {
+          EXPECT_TRUE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
   WebAppProvider& provider() {
     WebAppProvider* provider = WebAppProvider::Get(browser()->profile());
     DCHECK(provider);
     return *provider;
   }
 
+  WebAppUiManagerImpl& ui_manager() {
+    auto* ui_manager = WebAppUiManagerImpl::Get(browser()->profile());
+    DCHECK(ui_manager);
+    return *ui_manager;
+  }
+
   void AwaitRegistryReady() {
     base::RunLoop run_loop;
     provider().on_registry_ready().Post(FROM_HERE, run_loop.QuitClosure());
@@ -255,6 +283,43 @@ IN_PROC_BROWSER_TEST_F(WebAppMigrationManagerBrowserTest,
   }
 }
 
+IN_PROC_BROWSER_TEST_F(WebAppMigrationManagerBrowserTest,
+                       PRE_UninstallShadowBookmarkApp) {
+  EXPECT_TRUE(provider().registrar().AsBookmarkAppRegistrar());
+  AwaitRegistryReady();
+
+  // Install shadow bookmark app.
+  ui_test_utils::NavigateToURL(browser(), GURL{kSimpleManifestStartUrl});
+  AppId app_id = InstallWebAppAsUserViaOmnibox();
+
+  EXPECT_TRUE(provider().registrar().IsInstalled(app_id));
+  EXPECT_TRUE(ui_manager().dialog_manager().CanUninstallWebApp(app_id));
+}
+
+IN_PROC_BROWSER_TEST_F(WebAppMigrationManagerBrowserTest,
+                       UninstallShadowBookmarkApp) {
+  EXPECT_FALSE(provider().registrar().AsBookmarkAppRegistrar());
+  AwaitRegistryReady();
+
+  AppId app_id = GenerateAppIdFromURL(GURL{kSimpleManifestStartUrl});
+
+  EXPECT_TRUE(provider().registrar().IsInstalled(app_id));
+  EXPECT_TRUE(ui_manager().dialog_manager().CanUninstallWebApp(app_id));
+
+  auto* extensions_registry =
+      extensions::ExtensionRegistry::Get(browser()->profile());
+  extensions::TestExtensionRegistryObserver extensions_registry_observer(
+      extensions_registry);
+
+  UninstallWebAppAsUserViaMenu(app_id);
+  EXPECT_FALSE(provider().registrar().IsInstalled(app_id));
+
+  scoped_refptr<const extensions::Extension> extension =
+      extensions_registry_observer.WaitForExtensionUninstalled();
+  EXPECT_EQ(extension->id(), app_id);
+  EXPECT_EQ("Manifest test app", extension->short_name());
+}
+
 // TODO(crbug.com/1020037): Test policy installed bookmark apps with an external
 // install source to cover
 // WebAppMigrationManager::MigrateBookmarkAppInstallSource() logic.

chrome/browser/web_applications/web_app_provider.cc

@@ -203,10 +203,16 @@ void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
     registrar = std::move(mutable_registrar);
   }
 
+  auto legacy_finalizer =
+      std::make_unique<extensions::BookmarkAppInstallFinalizer>(profile);
+  legacy_finalizer->SetSubsystems(/*registrar=*/nullptr,
+                                  /*ui_manager=*/nullptr,
+                                  /*registry_controller=*/nullptr);
+
   auto icon_manager = std::make_unique<WebAppIconManager>(
       profile, *registrar, std::make_unique<FileUtilsWrapper>());
-  install_finalizer_ =
-      std::make_unique<WebAppInstallFinalizer>(profile, icon_manager.get());
+  install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
+      profile, icon_manager.get(), std::move(legacy_finalizer));
   file_handler_manager_ = std::make_unique<WebAppFileHandlerManager>(profile);
   shortcut_manager_ = std::make_unique<WebAppShortcutManager>(
       profile, icon_manager.get(), file_handler_manager_.get());