MacPWAs: Fix multi-profile uninstall

Ensure that uninstalling an app will only cause the shim to go away if
this is the last profile for which that app is installed.

Make this distinction in web_app::internals::DeletePlatformShortcuts
vs web_app::internals::DeleteMultiProfileShortcutsForApp (single
vs multi profile). Add tests for this behavior.

Update ShortcutManager to
- Use a helper function UseAppShimRegistry to determine if an extension
  should be in the registry (only bookmark apps should).
- Register apps in OnExtensionLoaded. This is needed because we are
  introducing the AppShimRegistry after users may have already installed
  apps (so we need to register them sometime).
- Add hooks to call DeleteMultiProfileShortcutsForApp as needed.

Update WebAppShortcutCreator to
- Move a bunch of code into static helper functions (e.g, bundle id
  construction, using LaunchServices to look up apps, etc).
- Add tests for new behaviors

TBR=dominickn

Bug: 1029048
Change-Id: I85807fc2cedbe4fbbd297179a5e59c781794e84c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1938452Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719662}

chrome/browser/apps/platform_apps/app_shim_registry_mac.cc

@@ -102,6 +102,31 @@ void AppShimRegistry::OnAppQuit(const std::string& app_id,
   SetAppInfo(app_id, nullptr, &last_active_profiles);
 }
 
+std::set<std::string> AppShimRegistry::GetInstalledAppsForProfile(
+    const base::FilePath& profile) const {
+  std::set<std::string> result;
+  const base::DictionaryValue* app_shims =
+      GetPrefService()->GetDictionary(kAppShims);
+  if (!app_shims)
+    return result;
+  for (base::DictionaryValue::Iterator iter_app(*app_shims);
+       !iter_app.IsAtEnd(); iter_app.Advance()) {
+    const base::Value* installed_profiles_list =
+        iter_app.value().FindListKey(kInstalledProfiles);
+    if (!installed_profiles_list)
+      continue;
+    for (const auto& profile_path_value : installed_profiles_list->GetList()) {
+      if (!profile_path_value.is_string())
+        continue;
+      if (profile == GetFullProfilePath(profile_path_value.GetString())) {
+        result.insert(iter_app.key());
+        break;
+      }
+    }
+  }
+  return result;
+}
+
 void AppShimRegistry::SetPrefServiceAndUserDataDirForTesting(
     PrefService* pref_service,
     const base::FilePath& user_data_dir) {

chrome/browser/apps/platform_apps/app_shim_registry_mac.h

@@ -59,6 +59,11 @@ class AppShimRegistry {
   void OnAppQuit(const std::string& app_id,
                  std::set<base::FilePath> active_profiles);
 
+  // Return all apps installed for the specified profile. Used to delete apps
+  // when a profile is removed.
+  std::set<std::string> GetInstalledAppsForProfile(
+      const base::FilePath& profile) const;
+
   // Helper functions for testing.
   void SetPrefServiceAndUserDataDirForTesting(
       PrefService* pref_service,

chrome/browser/apps/platform_apps/app_shim_registry_mac_unittest.cc

@@ -112,4 +112,60 @@ TEST_F(AppShimRegistryTest, Lifetime) {
   EXPECT_EQ(0u, registry_->GetLastActiveProfilesForApp(app_id_a).size());
 }
 
+TEST_F(AppShimRegistryTest, InstalledAppsForProfile) {
+  const std::string app_id_a("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+  const std::string app_id_b("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
+  const base::FilePath profile_path_a("/x/y/z/Profile A");
+  const base::FilePath profile_path_b("/x/y/z/Profile B");
+  const base::FilePath profile_path_c("/x/y/z/Profile C");
+  std::set<std::string> apps;
+
+  // App A is installed for profiles B and C.
+  registry_->OnAppInstalledForProfile(app_id_a, profile_path_b);
+  registry_->OnAppInstalledForProfile(app_id_a, profile_path_c);
+  EXPECT_EQ(2u, registry_->GetInstalledProfilesForApp(app_id_a).size());
+  apps = registry_->GetInstalledAppsForProfile(profile_path_a);
+  EXPECT_TRUE(apps.empty());
+  apps = registry_->GetInstalledAppsForProfile(profile_path_b);
+  EXPECT_EQ(1u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_a));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_c);
+  EXPECT_EQ(1u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_a));
+
+  // App B is installed for profiles A and C.
+  registry_->OnAppInstalledForProfile(app_id_b, profile_path_a);
+  registry_->OnAppInstalledForProfile(app_id_b, profile_path_c);
+  apps = registry_->GetInstalledAppsForProfile(profile_path_a);
+  EXPECT_EQ(1u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_b));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_b);
+  EXPECT_EQ(1u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_a));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_c);
+  EXPECT_EQ(2u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_a));
+  EXPECT_EQ(1u, apps.count(app_id_b));
+
+  // Uninstall app A for profile B.
+  EXPECT_FALSE(registry_->OnAppUninstalledForProfile(app_id_a, profile_path_b));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_b);
+  EXPECT_TRUE(apps.empty());
+  apps = registry_->GetInstalledAppsForProfile(profile_path_c);
+  EXPECT_EQ(2u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_a));
+  EXPECT_EQ(1u, apps.count(app_id_b));
+
+  // Uninstall app A for profile C.
+  EXPECT_TRUE(registry_->OnAppUninstalledForProfile(app_id_a, profile_path_c));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_c);
+  EXPECT_EQ(1u, apps.size());
+  EXPECT_EQ(1u, apps.count(app_id_b));
+
+  // Uninstall app B for profile C.
+  EXPECT_FALSE(registry_->OnAppUninstalledForProfile(app_id_b, profile_path_c));
+  apps = registry_->GetInstalledAppsForProfile(profile_path_c);
+  EXPECT_TRUE(apps.empty());
+}
+
 }  // namespace

chrome/browser/apps/platform_apps/shortcut_manager.cc

@@ -38,6 +38,15 @@ using extensions::Extension;
 
 namespace {
 
+#if defined(OS_MACOSX)
+bool UseAppShimRegistry(content::BrowserContext* browser_context,
+                        const Extension* extension) {
+  if (browser_context->IsOffTheRecord())
+    return false;
+  return extension->is_app() && extension->from_bookmark();
+}
+#endif
+
 // This version number is stored in local prefs to check whether app shortcuts
 // need to be recreated. This might happen when we change various aspects of app
 // shortcuts like command-line flags or associated icons, binaries, etc.
@@ -105,6 +114,23 @@ AppShortcutManager::~AppShortcutManager() {
   }
 }
 
+void AppShortcutManager::OnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+#if defined(OS_MACOSX)
+  // Register installed apps as soon as their extension is loaded. This happens
+  // when the profile is loaded. This is redundant, because apps are registered
+  // when they are installed. It is necessary, however, because app registration
+  // was added long after app installation launched. This should be removed
+  // after shipping for a few versions (whereupon it may be assumed that most
+  // applications have been registered).
+  if (UseAppShimRegistry(browser_context, extension)) {
+    AppShimRegistry::Get()->OnAppInstalledForProfile(extension->id(),
+                                                     profile_->GetPath());
+  }
+#endif
+}
+
 void AppShortcutManager::OnExtensionWillBeInstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
@@ -114,8 +140,10 @@ void AppShortcutManager::OnExtensionWillBeInstalled(
     return;
 
 #if defined(OS_MACOSX)
-  AppShimRegistry::Get()->OnAppInstalledForProfile(extension->id(),
-                                                   profile_->GetPath());
+  if (UseAppShimRegistry(browser_context, extension)) {
+    AppShimRegistry::Get()->OnAppInstalledForProfile(extension->id(),
+                                                     profile_->GetPath());
+  }
 #endif
 
   // If the app is being updated, update any existing shortcuts but do not
@@ -134,11 +162,16 @@ void AppShortcutManager::OnExtensionUninstalled(
     const Extension* extension,
     extensions::UninstallReason reason) {
 #if defined(OS_MACOSX)
-  if (extension->is_app()) {
-    AppShimRegistry::Get()->OnAppUninstalledForProfile(extension->id(),
-                                                       profile_->GetPath());
-    // TODO(https://crbug.com/1001213): Plumb the return result through
-    // DeleteAllShortcuts, to appropriately delete multi-profile apps.
+  if (UseAppShimRegistry(browser_context, extension)) {
+    bool delete_multi_profile_shortcuts =
+        AppShimRegistry::Get()->OnAppUninstalledForProfile(extension->id(),
+                                                           profile_->GetPath());
+    if (delete_multi_profile_shortcuts) {
+      web_app::internals::GetShortcutIOTaskRunner()->PostTask(
+          FROM_HERE,
+          base::BindOnce(&web_app::internals::DeleteMultiProfileShortcutsForApp,
+                         extension->id()));
+    }
   }
 #endif
 
@@ -150,7 +183,23 @@ void AppShortcutManager::OnProfileWillBeRemoved(
   if (profile_path != profile_->GetPath())
     return;
 
-  // TODO(https://crbug.com/1001213): Update AppShimRegistry here.
+#if defined(OS_MACOSX)
+  // If any multi-profile app shims exist only for this profile, delete them.
+  std::set<std::string> apps_for_profile =
+      AppShimRegistry::Get()->GetInstalledAppsForProfile(profile_path);
+  for (const auto& app_id : apps_for_profile) {
+    bool delete_multi_profile_shortcuts =
+        AppShimRegistry::Get()->OnAppUninstalledForProfile(app_id,
+                                                           profile_path);
+    if (delete_multi_profile_shortcuts) {
+      web_app::internals::GetShortcutIOTaskRunner()->PostTask(
+          FROM_HERE,
+          base::BindOnce(&web_app::internals::DeleteMultiProfileShortcutsForApp,
+                         app_id));
+    }
+  }
+#endif
+
   web_app::internals::GetShortcutIOTaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(&web_app::internals::DeleteAllShortcutsForProfile,

chrome/browser/apps/platform_apps/shortcut_manager.h

@@ -36,6 +36,8 @@ class AppShortcutManager : public KeyedService,
   void UpdateShortcutsForAllAppsIfNeeded();
 
   // extensions::ExtensionRegistryObserver.
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const extensions::Extension* extension) override;
   void OnExtensionWillBeInstalled(content::BrowserContext* browser_context,
                                   const extensions::Extension* extension,
                                   bool is_update,

chrome/browser/web_applications/components/web_app_shortcut.cc

@@ -155,6 +155,13 @@ base::FilePath GetShortcutDataDir(const ShortcutInfo& shortcut_info) {
                                 shortcut_info.extension_id, shortcut_info.url);
 }
 
+#if !defined(OS_MACOSX)
+void DeleteMultiProfileShortcutsForApp(const std::string& app_id) {
+  // Multi-profile shortcuts exist only on macOS.
+  NOTREACHED();
+}
+#endif
+
 }  // namespace internals
 
 }  // namespace web_app

chrome/browser/web_applications/components/web_app_shortcut.h

@@ -135,6 +135,10 @@ void ScheduleCreatePlatformShortcuts(
 void DeletePlatformShortcuts(const base::FilePath& shortcut_data_path,
                              const ShortcutInfo& shortcut_info);
 
+// Delete the multi-profile (non-profile_scoped) shortcuts for the specified
+// app. This is the multi-profile complement of DeletePlatformShortcuts.
+void DeleteMultiProfileShortcutsForApp(const std::string& app_id);
+
 // Updates all the shortcuts we have added for this extension. This is the
 // platform specific implementation of the UpdateAllShortcuts function, and
 // is executed on the FILE thread.

chrome/browser/web_applications/components/web_app_shortcut_mac.h

@@ -96,7 +96,6 @@ class WebAppShortcutCreator {
 
   bool CreateShortcuts(ShortcutCreationReason creation_reason,
                        ShortcutLocations creation_locations);
-  void DeleteShortcuts();
 
   // Recreate the shortcuts where they are found on disk and in the profile
   // path. If |create_if_needed| is true, then create the shortcuts if no
@@ -122,14 +121,6 @@ class WebAppShortcutCreator {
   // Return true if the bundle for this app should be profile-agnostic.
   bool IsMultiProfile() const;
 
-  // Returns the bundle identifier to use for this app bundle.
-  std::string GetBundleIdentifier() const;
-
-  // Returns the profile-scoped app bundle identifier. For multi-profile apps,
-  // this will give the bundle identifier for shims that were created before
-  // multi-profile support was added.
-  std::string GetProfileScopedBundleIdentifier() const;
-
   // Copies the app loader template into a temporary directory and fills in all
   // relevant information. This works around a Finder bug where the app's icon
   // doesn't properly update.

chrome/browser/web_applications/components/web_app_shortcut_mac_unittest.mm

@@ -320,19 +320,46 @@ TEST_F(WebAppShortcutCreatorTest, UpdateBookmarkAppShortcut) {
   EXPECT_FALSE(base::PathExists(other_shim_path.Append("Contents")));
 }
 
-TEST_F(WebAppShortcutCreatorTest, DeleteShortcuts) {
-  base::ScopedTempDir other_folder_temp_dir;
-  EXPECT_TRUE(other_folder_temp_dir.CreateUniqueTempDir());
-  base::FilePath other_folder = other_folder_temp_dir.GetPath();
-  base::FilePath other_shim_path = other_folder.Append(shim_base_name_);
+TEST_F(WebAppShortcutCreatorTest, DeleteShortcutsSingleProfile) {
+  base::test::ScopedFeatureList scoped_features;
+  scoped_features.InitWithFeatures(
+      /*enabled_features=*/{},
+      /*disabled_features=*/{features::kAppShimMultiProfile});
+
+  base::FilePath other_shim_path =
+      shim_path_.DirName().Append("Copy of Shim.app");
+  NiceMock<WebAppShortcutCreatorMock> shortcut_creator(app_data_dir_,
+                                                       info_.get());
+
+  // Create an extra shim in another folder. It should be deleted since its
+  // bundle id matches.
+  std::vector<base::FilePath> bundle_by_id_paths;
+  bundle_by_id_paths.push_back(shim_path_);
+  bundle_by_id_paths.push_back(other_shim_path);
+  EXPECT_CALL(shortcut_creator, GetAppBundlesByIdUnsorted())
+      .WillRepeatedly(Return(bundle_by_id_paths));
+  EXPECT_TRUE(shortcut_creator.CreateShortcuts(SHORTCUT_CREATION_AUTOMATED,
+                                               ShortcutLocations()));
 
+  // Ensure the paths were created, and that they are destroyed.
+  EXPECT_TRUE(base::PathExists(shim_path_));
+  EXPECT_TRUE(base::PathExists(other_shim_path));
+  internals::DeleteMultiProfileShortcutsForApp(info_->extension_id);
+  EXPECT_TRUE(base::PathExists(shim_path_));
+  EXPECT_TRUE(base::PathExists(other_shim_path));
+  internals::DeletePlatformShortcuts(app_data_dir_, *info_);
+  EXPECT_FALSE(base::PathExists(shim_path_));
+  EXPECT_FALSE(base::PathExists(other_shim_path));
+}
+
+TEST_F(WebAppShortcutCreatorTest, DeleteShortcuts) {
+  base::FilePath other_shim_path =
+      shim_path_.DirName().Append("Copy of Shim.app");
   NiceMock<WebAppShortcutCreatorMock> shortcut_creator(app_data_dir_,
                                                        info_.get());
 
   // Create an extra shim in another folder. It should be deleted since its
   // bundle id matches.
-  std::string expected_bundle_id = kFakeChromeBundleId;
-  expected_bundle_id += ".app.Profile-1-" + info_->extension_id;
   std::vector<base::FilePath> bundle_by_id_paths;
   bundle_by_id_paths.push_back(shim_path_);
   bundle_by_id_paths.push_back(other_shim_path);
@@ -344,7 +371,10 @@ TEST_F(WebAppShortcutCreatorTest, DeleteShortcuts) {
   // Ensure the paths were created, and that they are destroyed.
   EXPECT_TRUE(base::PathExists(shim_path_));
   EXPECT_TRUE(base::PathExists(other_shim_path));
-  shortcut_creator.DeleteShortcuts();
+  internals::DeletePlatformShortcuts(app_data_dir_, *info_);
+  EXPECT_TRUE(base::PathExists(shim_path_));
+  EXPECT_TRUE(base::PathExists(other_shim_path));
+  internals::DeleteMultiProfileShortcutsForApp(info_->extension_id);
   EXPECT_FALSE(base::PathExists(shim_path_));
   EXPECT_FALSE(base::PathExists(other_shim_path));
 }