[WebAppProvider] Create OnWebAppUninstalled event.

This patch creates the new OnWebAppUninstalled event. Follow up patches
will migrate systems that rely on this behavior from
OnWebAppWillBeUninstalled to OnWebAppUninstalled.

Patch also does a little cleanup in the sync integration tests.

Bug: 1162339, 1097050, 1126404, 1111533, 1159651, 1159482
Change-Id: I5081f25f0ad0d4aa9e4245023e8b00845d43d817
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606668Reviewed-by: Phillis Tang <phillis@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Jarryd Goodman <jarrydg@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840800}

chrome/browser/sync/test/integration/two_client_web_apps_bmo_sync_test.cc

@@ -193,7 +193,7 @@ class TwoClientWebAppsBMOSyncTest : public SyncTest {
 // Test is flaky (crbug.com/1097050)
 IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
                        DISABLED_SyncDoubleInstallation) {
-  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(SetupClients());
   ASSERT_TRUE(embedded_test_server()->Start());
   ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
 
@@ -203,6 +203,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
 
   EXPECT_EQ(app_id, app_id2);
 
+  ASSERT_TRUE(SetupSync());
+
   // Install a 'dummy' app & wait for installation to ensure sync has processed
   // the initial apps.
   InstallDummyAppAndWaitForSync(GURL("http://www.dummy.org/"), GetProfile(0),
@@ -252,7 +254,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
 #endif
 IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
                        MAYBE_SyncDoubleInstallationDifferentUserDisplayMode) {
-  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(SetupClients());
   ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
 
   WebApplicationInfo info;
@@ -268,10 +270,14 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
 
   EXPECT_EQ(app_id, app_id2);
 
-  // Install a 'dummy' app & wait for installation to ensure sync has processed
+  ASSERT_TRUE(SetupSync());
+
+  // Install a 'dummy' apps & wait for installation to ensure sync has processed
   // the initial apps.
-  InstallDummyAppAndWaitForSync(GURL("http://www.dummy.org/"), GetProfile(0),
+  InstallDummyAppAndWaitForSync(GURL("http://www.dummy1.org/"), GetProfile(0),
                                 GetProfile(1));
+  InstallDummyAppAndWaitForSync(GURL("http://www.dummy2.org/"), GetProfile(1),
+                                GetProfile(0));
 
   EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
 
@@ -320,7 +326,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest, DisplayMode) {
 #endif
 IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
                        MAYBE_DoubleInstallWithUninstall) {
-  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(SetupClients());
   ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
   ASSERT_TRUE(embedded_test_server()->Start());
 
@@ -329,6 +335,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest,
   AppId app_id2 = InstallAppAsUserInitiated(GetProfile(1));
   EXPECT_EQ(app_id, app_id2);
 
+  ASSERT_TRUE(SetupSync());
+
   // Uninstall the app from one of the profiles.
   UninstallWebApp(GetProfile(0), app_id);
 

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

@@ -271,6 +271,11 @@ class WebAppIntegrationBrowserTest
 
     app_menu_model->ExecuteCommand(WebAppMenuModel::kUninstallAppCommandId,
                                    /*event_flags=*/0);
+    // The |app_menu_model| must be destroyed here, as the |run_loop| waits
+    // until the app is fully uninstalled, which includes closing and deleting
+    // the app_browser_.
+    app_menu_model.reset();
+    app_browser_ = nullptr;
     run_loop.Run();
   }
 

chrome/browser/web_applications/components/app_registrar.cc

@@ -64,6 +64,11 @@ void AppRegistrar::NotifyWebAppsWillBeUpdatedFromSync(
     observer.OnWebAppsWillBeUpdatedFromSync(new_apps_state);
 }
 
+void AppRegistrar::NotifyWebAppUninstalled(const AppId& app_id) {
+  for (AppRegistrarObserver& observer : observers_)
+    observer.OnWebAppUninstalled(app_id);
+}
+
 void AppRegistrar::NotifyWebAppWillBeUninstalled(const AppId& app_id) {
   for (AppRegistrarObserver& observer : observers_)
     observer.OnWebAppWillBeUninstalled(app_id);

chrome/browser/web_applications/components/app_registrar.h

@@ -200,6 +200,7 @@ class AppRegistrar {
                                    base::StringPiece old_name);
   void NotifyWebAppsWillBeUpdatedFromSync(
       const std::vector<const WebApp*>& new_apps_state);
+  void NotifyWebAppUninstalled(const AppId& app_id);
   void NotifyWebAppWillBeUninstalled(const AppId& app_id);
   void NotifyWebAppLocallyInstalledStateChanged(const AppId& app_id,
                                                 bool is_locally_installed);

chrome/browser/web_applications/components/app_registrar_observer.h

@@ -35,11 +35,18 @@ class AppRegistrarObserver : public base::CheckedObserver {
   virtual void OnWebAppsWillBeUpdatedFromSync(
       const std::vector<const WebApp*>& new_apps_state) {}
 
-  // Called before a web app is uninstalled. |app_id| is still registered in the
-  // AppRegistrar. For bookmark apps, use BookmarkAppRegistrar::FindExtension to
-  // convert this |app_id| to Extension pointer.
+  // Called before a web app is uninstalled, before the uninstallation process
+  // begins. |app_id| is still registered in the AppRegistrar, and OS hooks have
+  // not yet been uninstalled. For bookmark apps, use
+  // BookmarkAppRegistrar::FindExtension to convert this |app_id| to Extension
+  // pointer.
   virtual void OnWebAppWillBeUninstalled(const AppId& app_id) {}
 
+  // Called after a web app is uninstalled. |app_id| is no longer registered in
+  // the AppRegistrar, all OS hooks are uninstalled, and icons have been
+  // deleted.
+  virtual void OnWebAppUninstalled(const AppId& app_id) {}
+
   // For bookmark apps, use BookmarkAppRegistrar::FindExtension to convert this
   // |app_id| to Extension pointer.
   virtual void OnWebAppProfileWillBeDeleted(const AppId& app_id) {}

chrome/browser/web_applications/test/web_app_install_observer.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/web_applications/test/web_app_install_observer.h"
+
 #include <memory>
 
 #include "base/run_loop.h"
@@ -122,6 +123,11 @@ void WebAppInstallObserver::SetWebAppInstalledWithOsHooksDelegate(
   app_installed_with_os_hooks_delegate_ = delegate;
 }
 
+void WebAppInstallObserver::SetWebAppWillBeUninstalledDelegate(
+    WebAppWillBeUninstalledDelegate delegate) {
+  app_will_be_uninstalled_delegate_ = delegate;
+}
+
 void WebAppInstallObserver::SetWebAppUninstalledDelegate(
     WebAppUninstalledDelegate delegate) {
   app_uninstalled_delegate_ = delegate;
@@ -162,6 +168,11 @@ void WebAppInstallObserver::OnWebAppsWillBeUpdatedFromSync(
 }
 
 void WebAppInstallObserver::OnWebAppWillBeUninstalled(const AppId& app_id) {
+  if (app_will_be_uninstalled_delegate_)
+    app_will_be_uninstalled_delegate_.Run(app_id);
+}
+
+void WebAppInstallObserver::OnWebAppUninstalled(const AppId& app_id) {
   listening_for_uninstall_app_ids_.erase(app_id);
   if (!listening_for_uninstall_app_ids_.empty())
     return;

chrome/browser/web_applications/test/web_app_install_observer.h

@@ -79,6 +79,11 @@ class WebAppInstallObserver final : public AppRegistrarObserver {
   void SetWebAppInstalledWithOsHooksDelegate(
       WebAppInstalledWithOsHooksDelegate delegate);
 
+  using WebAppWillBeUninstalledDelegate =
+      base::RepeatingCallback<void(const AppId& app_id)>;
+  void SetWebAppWillBeUninstalledDelegate(
+      WebAppWillBeUninstalledDelegate delegate);
+
   using WebAppUninstalledDelegate =
       base::RepeatingCallback<void(const AppId& app_id)>;
   void SetWebAppUninstalledDelegate(WebAppUninstalledDelegate delegate);
@@ -99,6 +104,7 @@ class WebAppInstallObserver final : public AppRegistrarObserver {
   void OnWebAppsWillBeUpdatedFromSync(
       const std::vector<const WebApp*>& new_apps_state) override;
   void OnWebAppWillBeUninstalled(const AppId& app_id) override;
+  void OnWebAppUninstalled(const AppId& app_id) override;
   void OnWebAppProfileWillBeDeleted(const AppId& app_id) override;
 
  private:
@@ -127,6 +133,7 @@ class WebAppInstallObserver final : public AppRegistrarObserver {
   WebAppInstalledDelegate app_installed_delegate_;
   WebAppInstalledWithOsHooksDelegate app_installed_with_os_hooks_delegate_;
   WebAppWillBeUpdatedFromSyncDelegate app_will_be_updated_from_sync_delegate_;
+  WebAppWillBeUninstalledDelegate app_will_be_uninstalled_delegate_;
   WebAppUninstalledDelegate app_uninstalled_delegate_;
   WebAppProfileWillBeDeletedDelegate app_profile_will_be_deleted_delegate_;
 

chrome/browser/web_applications/test/web_app_uninstall_waiter.cc

@@ -18,7 +18,7 @@ void WebAppUninstallWaiter::Wait() {
   run_loop_.Run();
 }
 
-void WebAppUninstallWaiter::OnWebAppWillBeUninstalled(const AppId& app_id) {
+void WebAppUninstallWaiter::OnWebAppUninstalled(const AppId& app_id) {
   if (app_id == app_id_)
     run_loop_.Quit();
 }

chrome/browser/web_applications/test/web_app_uninstall_waiter.h

@@ -20,7 +20,7 @@ class WebAppUninstallWaiter final : public AppRegistrarObserver {
   void Wait();
 
   // AppRegistrarObserver:
-  void OnWebAppWillBeUninstalled(const AppId& app_id) final;
+  void OnWebAppUninstalled(const AppId& app_id) final;
 
  private:
   AppId app_id_;

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -209,9 +209,10 @@ void WebAppInstallFinalizer::FinalizeUninstallAfterSync(
   DCHECK(!GetWebAppRegistrar().GetAppById(app_id));
 
   icon_manager_->DeleteData(
-      app_id, base::BindOnce(&WebAppInstallFinalizer::OnIconsDataDeleted,
-                             weak_ptr_factory_.GetWeakPtr(), app_id,
-                             std::move(callback)));
+      app_id,
+      base::BindOnce(
+          &WebAppInstallFinalizer::OnIconsDataDeletedAndWebAppUninstalled,
+          weak_ptr_factory_.GetWeakPtr(), app_id, std::move(callback)));
 }
 
 void WebAppInstallFinalizer::UninstallExternalWebApp(
@@ -330,9 +331,10 @@ void WebAppInstallFinalizer::OnUninstallOsHooks(
   update->DeleteApp(app_id);
 
   icon_manager_->DeleteData(
-      app_id, base::BindOnce(&WebAppInstallFinalizer::OnIconsDataDeleted,
-                             weak_ptr_factory_.GetWeakPtr(), app_id,
-                             std::move(callback)));
+      app_id,
+      base::BindOnce(
+          &WebAppInstallFinalizer::OnIconsDataDeletedAndWebAppUninstalled,
+          weak_ptr_factory_.GetWeakPtr(), app_id, std::move(callback)));
 }
 
 void WebAppInstallFinalizer::UninstallWebAppOrRemoveSource(
@@ -427,10 +429,11 @@ void WebAppInstallFinalizer::OnShortcutsMenuIconsDataWritten(
       std::move(update), std::move(commit_callback));
 }
 
-void WebAppInstallFinalizer::OnIconsDataDeleted(
+void WebAppInstallFinalizer::OnIconsDataDeletedAndWebAppUninstalled(
     const AppId& app_id,
     UninstallWebAppCallback callback,
     bool success) {
+  registrar().NotifyWebAppUninstalled(app_id);
   std::move(callback).Run(success);
 }
 

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -76,9 +76,9 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
                                        std::unique_ptr<WebApp> web_app,
                                        bool success);
 
-  void OnIconsDataDeleted(const AppId& app_id,
-                          UninstallWebAppCallback callback,
-                          bool success);
+  void OnIconsDataDeletedAndWebAppUninstalled(const AppId& app_id,
+                                              UninstallWebAppCallback callback,
+                                              bool success);
   void OnDatabaseCommitCompletedForInstall(InstallFinalizedCallback callback,
                                            AppId app_id,
                                            bool success);

chrome/browser/web_applications/web_app_sync_bridge.cc

@@ -535,6 +535,9 @@ void WebAppSyncBridge::ApplySyncChangesToRegistrar(
   // still registered at this stage.
   for (const AppId& app_id : update_local_data->apps_to_delete) {
     registrar_->NotifyWebAppWillBeUninstalled(app_id);
+    // TODO(https://crbug.com/1162349): Have the
+    // InstallDelegate::UninstallWebAppsAfterSync occur after OS hooks are
+    // uninstalled.
     os_integration_manager().UninstallAllOsHooks(app_id, base::DoNothing());
   }
 
@@ -556,6 +559,8 @@ void WebAppSyncBridge::ApplySyncChangesToRegistrar(
   // locally and not needed by other sources. We need to clean up disk data
   // (icons).
   if (!apps_unregistered.empty()) {
+    // TODO(https://crbug.com/1162349): Instead of calling this now, have this
+    // call occur after OS hooks are uninstalled.
     install_delegate_->UninstallWebAppsAfterSync(std::move(apps_unregistered),
                                                  base::DoNothing());
   }