MacPWAs: Use AppShimRegistry to populate Profiles menu

Prior to this change, we were asynchronously poking around in the
filesystem to see what apps are installs for which profiles. Just use
the AppShimRegistry.

Delete all of the asynchronous profile loading code, and rename
some of the menu functions to be more clear in their meaning.

TBR=dominickn

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

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc

@@ -254,9 +254,6 @@ struct ExtensionAppShimHandler::AppState {
   // The profile state for the profiles currently running this app.
   std::map<Profile*, std::unique_ptr<ProfileState>> profiles;
 
-  // The set of profiles for which this app is installed.
-  std::set<base::FilePath> installed_profiles;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(AppState);
 };
@@ -301,14 +298,6 @@ Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
   return profile && profile_manager->IsValidProfile(profile) ? profile : NULL;
 }
 
-void ExtensionAppShimHandler::Delegate::GetProfilesForAppAsync(
-    const std::string& app_id,
-    const std::vector<base::FilePath>& profile_paths_to_check,
-    base::OnceCallback<void(const std::vector<base::FilePath>&)> callback) {
-  web_app::GetProfilesForAppShim(app_id, profile_paths_to_check,
-                                 std::move(callback));
-}
-
 void ExtensionAppShimHandler::Delegate::LoadProfileAsync(
     const base::FilePath& full_path,
     base::OnceCallback<void(Profile*)> callback) {
@@ -1070,9 +1059,11 @@ void ExtensionAppShimHandler::OnBrowserSetLastActive(Browser* browser) {
   // is next to the new-active item).
   if (avatar_menu_)
     avatar_menu_->ActiveBrowserChanged(browser);
-  UpdateProfileMenuItems();
+  UpdateAllProfileMenus();
+}
 
-  // Update all multi-profile apps' profile menus.
+void ExtensionAppShimHandler::UpdateAllProfileMenus() {
+  RebuildProfileMenuItemsFromAvatarMenu();
   for (auto& iter_app : apps_) {
     AppState* app_state = iter_app.second.get();
     if (app_state->IsMultiProfile())
@@ -1080,7 +1071,7 @@ void ExtensionAppShimHandler::OnBrowserSetLastActive(Browser* browser) {
   }
 }
 
-void ExtensionAppShimHandler::UpdateProfileMenuItems() {
+void ExtensionAppShimHandler::RebuildProfileMenuItemsFromAvatarMenu() {
   if (!avatar_menu_) {
     ProfileManager* profile_manager = g_browser_process->profile_manager();
     avatar_menu_ = std::make_unique<AvatarMenu>(
@@ -1105,44 +1096,7 @@ void ExtensionAppShimHandler::OnAvatarMenuChanged(AvatarMenu* menu) {
   // Rebuild the profile menu to reflect changes (e.g, added or removed
   // profiles).
   DCHECK_EQ(avatar_menu_.get(), menu);
-  UpdateProfileMenuItems();
-
-  // Because profiles may have been added or removed, for each app, update the
-  // list of profiles for which that app is installed.
-  for (auto& iter_app : apps_) {
-    const std::string& app_id = iter_app.first;
-    AppState* app_state = iter_app.second.get();
-    if (!app_state->IsMultiProfile())
-      continue;
-    GetProfilesForAppAsync(
-        iter_app.first,
-        base::BindOnce(&ExtensionAppShimHandler::OnGotProfilesForApp,
-                       weak_factory_.GetWeakPtr(), app_id));
-  }
-}
-
-void ExtensionAppShimHandler::GetProfilesForAppAsync(
-    const std::string& app_id,
-    base::OnceCallback<void(const std::vector<base::FilePath>&)> callback) {
-  std::vector<base::FilePath> profile_paths_to_check;
-  for (const auto& item : profile_menu_items_)
-    profile_paths_to_check.push_back(item->profile_path);
-  delegate_->GetProfilesForAppAsync(app_id, profile_paths_to_check,
-                                    std::move(callback));
-}
-
-void ExtensionAppShimHandler::OnGotProfilesForApp(
-    const std::string& app_id,
-    const std::vector<base::FilePath>& profiles) {
-  auto found_app = apps_.find(app_id);
-  if (found_app == apps_.end())
-    return;
-  AppState* app_state = found_app->second.get();
-  DCHECK(app_state->IsMultiProfile());
-  app_state->installed_profiles.clear();
-  for (const auto& profile_path : profiles)
-    app_state->installed_profiles.insert(profile_path);
-  UpdateAppProfileMenu(app_state);
+  UpdateAllProfileMenus();
 }
 
 void ExtensionAppShimHandler::UpdateAppProfileMenu(AppState* app_state) {
@@ -1150,8 +1104,10 @@ void ExtensionAppShimHandler::UpdateAppProfileMenu(AppState* app_state) {
   // Include in |items| the profiles from |profile_menu_items_| for which this
   // app is installed, sorted by |menu_index|.
   std::vector<chrome::mojom::ProfileMenuItemPtr> items;
+  auto installed_profiles =
+      AppShimRegistry::Get()->GetInstalledProfilesForApp(app_state->app_id);
   for (const auto& item : profile_menu_items_) {
-    if (app_state->installed_profiles.count(item->profile_path))
+    if (installed_profiles.count(item->profile_path))
       items.push_back(item->Clone());
   }
   std::sort(items.begin(), items.end(), ProfileMenuItemComparator);
@@ -1195,6 +1151,10 @@ ExtensionAppShimHandler::GetOrCreateProfileState(
   }
   AppState* app_state = found_app->second.get();
 
+  // Initialize the profile menu.
+  if (is_multi_profile)
+    UpdateAppProfileMenu(app_state);
+
   auto found_profile = app_state->profiles.find(profile);
   if (found_profile == app_state->profiles.end()) {
     std::unique_ptr<AppShimHost> single_profile_host;
@@ -1208,16 +1168,6 @@ ExtensionAppShimHandler::GetOrCreateProfileState(
         app_state->profiles
             .insert(std::make_pair(profile, std::move(new_profile_state)))
             .first;
-    // Update the profile menu as each new profile uses this app. Note that
-    // ideally this should be done when when |multi_profile_host| is created
-    // above, and should be updated when apps are installed or uninstalled
-    // (but do not).
-    if (app_state->IsMultiProfile()) {
-      UpdateProfileMenuItems();
-      GetProfilesForAppAsync(
-          app_id, base::BindOnce(&ExtensionAppShimHandler::OnGotProfilesForApp,
-                                 weak_factory_.GetWeakPtr(), app_id));
-    }
   }
   return found_profile->second.get();
 }

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h

@@ -57,13 +57,6 @@ class ExtensionAppShimHandler : public AppShimHostBootstrap::Client,
     // Return the profile for |path|, only if it is already loaded.
     virtual Profile* ProfileForPath(const base::FilePath& path);
 
-    // Call |callback| with the list of profiles for which this app is
-    // installed.
-    virtual void GetProfilesForAppAsync(
-        const std::string& app_id,
-        const std::vector<base::FilePath>& profile_paths_to_check,
-        base::OnceCallback<void(const std::vector<base::FilePath>&)>);
-
     // Load a profile and call |callback| when completed or failed.
     virtual void LoadProfileAsync(const base::FilePath& path,
                                   base::OnceCallback<void(Profile*)> callback);
@@ -201,8 +194,12 @@ class ExtensionAppShimHandler : public AppShimHostBootstrap::Client,
   void set_delegate(Delegate* delegate);
   content::NotificationRegistrar& registrar() { return registrar_; }
 
+  // Called when profile menu items may have changed. Rebuilds the profile
+  // menu item list and sends updated lists to all apps.
+  void UpdateAllProfileMenus();
+
   // Update |profile_menu_items_| from |avatar_menu_|. Virtual for tests.
-  virtual void UpdateProfileMenuItems();
+  virtual void RebuildProfileMenuItemsFromAvatarMenu();
 
   // The list of all profiles that might appear in the menu.
   std::vector<chrome::mojom::ProfileMenuItemPtr> profile_menu_items_;
@@ -264,17 +261,6 @@ class ExtensionAppShimHandler : public AppShimHostBootstrap::Client,
                     const std::string& app_id,
                     LoadProfileAppCallback callback);
 
-  // Check to see for which profile paths in |profile_menu_items_| the app with
-  // |app_id| is installed, and return them as the argument to |callback|.
-  void GetProfilesForAppAsync(
-      const std::string& app_id,
-      base::OnceCallback<void(const std::vector<base::FilePath>&)> callback);
-
-  // Callback for the call to asynchronously query which profiles have an app
-  // installed.
-  void OnGotProfilesForApp(const std::string& app_id,
-                           const std::vector<base::FilePath>& profiles);
-
   // Update the profiles menu for the specified host.
   void UpdateAppProfileMenu(AppState* app_state);
 

chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc

@@ -51,15 +51,6 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
  public:
   virtual ~MockDelegate() { DCHECK(load_profile_callbacks_.empty()); }
 
-  void GetProfilesForAppAsync(
-      const std::string& app_id,
-      const std::vector<base::FilePath>& profile_paths_to_check,
-      base::OnceCallback<void(const std::vector<base::FilePath>&)> callback)
-      override {
-    get_profiles_for_app_callbacks_.push_back(
-        base::BindOnce(std::move(callback), profile_paths_to_check));
-  }
-
   MOCK_METHOD1(ProfileForPath, Profile*(const base::FilePath&));
   void LoadProfileAsync(const base::FilePath& path,
                         base::OnceCallback<void(Profile*)> callback) override {
@@ -141,22 +132,12 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
     return load_profile_callbacks_.erase(path);
   }
 
-  bool RunGetProfilesForAppCallback() {
-    if (get_profiles_for_app_callbacks_.empty())
-      return false;
-    std::move(get_profiles_for_app_callbacks_.front()).Run();
-    get_profiles_for_app_callbacks_.pop_front();
-    return true;
-  }
-
  private:
   ShimLaunchedCallback* launch_shim_callback_capture_ = nullptr;
   ShimTerminatedCallback* terminated_shim_callback_capture_ = nullptr;
   std::map<base::FilePath, base::OnceCallback<void(Profile*)>>
       load_profile_callbacks_;
   std::unique_ptr<AppShimHost> host_for_create_ = nullptr;
-
-  std::list<base::OnceClosure> get_profiles_for_app_callbacks_;
   bool allow_shim_to_connect_ = true;
 };
 
@@ -183,7 +164,7 @@ class TestingExtensionAppShimHandler : public ExtensionAppShimHandler {
     new_profile_menu_items_ = std::move(new_profile_menu_items);
     OnAvatarMenuChanged(nullptr);
   }
-  void UpdateProfileMenuItems() override {
+  void RebuildProfileMenuItemsFromAvatarMenu() override {
     profile_menu_items_.clear();
     for (const auto& item : new_profile_menu_items_)
       profile_menu_items_.push_back(item.Clone());
@@ -416,19 +397,16 @@ class ExtensionAppShimHandlerTestBase : public testing::Test {
       handler_->SetProfileMenuItems(std::move(items));
     }
 
-    printf("Adding A: %s\n", profile_path_a_.value().c_str());
     EXPECT_CALL(*delegate_, IsProfileLockedForPath(profile_path_a_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*delegate_, ProfileForPath(profile_path_a_))
         .WillRepeatedly(Return(&profile_a_));
 
-    printf("Adding B: %s\n", profile_path_b_.value().c_str());
     EXPECT_CALL(*delegate_, IsProfileLockedForPath(profile_path_b_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*delegate_, ProfileForPath(profile_path_b_))
         .WillRepeatedly(Return(&profile_b_));
 
-    printf("Adding C: %s\n", profile_path_c_.value().c_str());
     EXPECT_CALL(*delegate_, IsProfileLockedForPath(profile_path_c_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*delegate_, ProfileForPath(profile_path_c_))
@@ -1063,7 +1041,6 @@ TEST_F(ExtensionAppShimHandlerTestMultiProfile, MultiProfileSelectMenu) {
 }
 
 TEST_F(ExtensionAppShimHandlerTestMultiProfile, ProfileMenuOneProfile) {
-  // Set this app to be installed for profile A.
   {
     auto item_a = chrome::mojom::ProfileMenuItem::New();
     item_a->profile_path = profile_path_a_;
@@ -1074,12 +1051,15 @@ TEST_F(ExtensionAppShimHandlerTestMultiProfile, ProfileMenuOneProfile) {
     handler_->SetProfileMenuItems(std::move(items));
   }
 
+  // Set this app to be installed for profile A.
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdA,
+                                                   profile_path_a_);
+
   // When the app activates, a host is created. This will trigger building
   // the avatar menu.
   delegate_->SetHostForCreate(std::move(host_aa_unique_));
   EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(), false));
   handler_->OnAppActivated(&profile_a_, kTestAppIdA);
-  EXPECT_TRUE(delegate_->RunGetProfilesForAppCallback());
   EXPECT_EQ(host_aa_.get(), handler_->FindHost(&profile_a_, kTestAppIdA));
 
   // Launch the shim.
@@ -1092,7 +1072,6 @@ TEST_F(ExtensionAppShimHandlerTestMultiProfile, ProfileMenuOneProfile) {
   const auto& menu_items = host_aa_->test_app_shim_->profile_menu_items_;
 
   // We should have no menu items, because there is only one installed profile.
-  EXPECT_FALSE(delegate_->RunGetProfilesForAppCallback());
   EXPECT_TRUE(menu_items.empty());
 
   // Add profile B to the avatar menu and call the avatar menu observer update
@@ -1111,19 +1090,18 @@ TEST_F(ExtensionAppShimHandlerTestMultiProfile, ProfileMenuOneProfile) {
     items.push_back(std::move(item_b));
     handler_->SetProfileMenuItems(std::move(items));
   }
-  EXPECT_TRUE(delegate_->RunGetProfilesForAppCallback());
-  EXPECT_FALSE(delegate_->RunGetProfilesForAppCallback());
 
-  // We should now have 2 menu items. They should be sorted by menu_index,
-  // making b be before a.
+  // We should still only have no menu items, because the app is not installed
+  // for multiple profiles.
+  EXPECT_TRUE(menu_items.empty());
+
+  // Now install for profile B.
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdA,
+                                                   profile_path_b_);
+  handler_->OnAppActivated(&profile_b_, kTestAppIdA);
   EXPECT_EQ(menu_items.size(), 2u);
   EXPECT_EQ(menu_items[0]->profile_path, profile_path_b_);
   EXPECT_EQ(menu_items[1]->profile_path, profile_path_a_);
-
-  // Activate profile B. This should trigger re-building the avatar menu.
-  handler_->OnAppActivated(&profile_b_, kTestAppIdA);
-  EXPECT_TRUE(delegate_->RunGetProfilesForAppCallback());
-  EXPECT_FALSE(delegate_->RunGetProfilesForAppCallback());
 }
 
 TEST_F(ExtensionAppShimHandlerTestMultiProfile, FindProfileFromBadProfile) {

chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.h

@@ -32,17 +32,6 @@ bool MaybeRebuildShortcut(const base::CommandLine& command_line);
 void RevealAppShimInFinderForApp(Profile* profile,
                                  const extensions::Extension* app);
 
-// Callback made by GetProfilesForAppShim.
-using GetProfilesForAppShimCallback =
-    base::OnceCallback<void(const std::vector<base::FilePath>&)>;
-
-// Call |callback| with the subset of |profile_paths_to_check| for which the app
-// with |app_id| in installed.
-void GetProfilesForAppShim(
-    const std::string& app_id,
-    const std::vector<base::FilePath>& profile_paths_to_check,
-    GetProfilesForAppShimCallback callback);
-
 }  // namespace web_app
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_WEB_APP_EXTENSION_SHORTCUT_MAC_H_

chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.mm

@@ -153,38 +153,6 @@ void UpdateShortcutsForAllApps(Profile* profile, base::OnceClosure callback) {
   }
 }
 
-void GetProfilesForAppShim(
-    const std::string& app_id,
-    const std::vector<base::FilePath>& profile_paths_to_check,
-    GetProfilesForAppShimCallback callback) {
-  auto io_thread_lambda =
-      [](const std::string& app_id,
-         const std::vector<base::FilePath>& profile_paths_to_check,
-         GetProfilesForAppShimCallback callback) {
-        base::ScopedBlockingCall scoped_blocking_call(
-            FROM_HERE, base::BlockingType::MAY_BLOCK);
-
-        // Determine if an extension is installed for a profile by whether or
-        // not the subpath Extensions/|app_id| exists. We do this instead of
-        // checking the Profile's properties because the Profile may not be
-        // loaded yet.
-        std::vector<base::FilePath> profile_paths_with_app;
-        for (const auto& profile_path : profile_paths_to_check) {
-          base::FilePath extension_path =
-              profile_path.AppendASCII(extensions::kInstallDirectoryName)
-                  .AppendASCII(app_id);
-          if (base::PathExists(extension_path))
-            profile_paths_with_app.push_back(profile_path);
-        }
-        base::PostTask(
-            FROM_HERE, {content::BrowserThread::UI},
-            base::BindOnce(std::move(callback), profile_paths_with_app));
-      };
-  internals::GetShortcutIOTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(io_thread_lambda, app_id,
-                                profile_paths_to_check, std::move(callback)));
-}
-
 }  // namespace web_app
 
 namespace chrome {