[ExternallyInstalledWebAppPrefs] Decouple app list from the registrar.

This CL changes ExternallyInstalledWebAppPrefs::GetInstalledAppUrls to
BuildAppIdsMap which does not check for installation, to fix a dependency
cycle when querying for apps in the constructor of WebAppProvider.

This also represents a step towards refactoring the WebApp test environment.

Bug: 973324

Change-Id: Ia0ad9f29fe6694a548acd716885a7d538d376332
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1654676
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669102}

chrome/browser/web_applications/components/externally_installed_web_app_prefs.cc

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/values.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
@@ -94,7 +93,6 @@ bool ExternallyInstalledWebAppPrefs::HasAppId(const PrefService* pref_service,
 }
 
 // static
-
 bool ExternallyInstalledWebAppPrefs::HasAppIdWithInstallSource(
     const PrefService* pref_service,
     const AppId& app_id,
@@ -108,16 +106,16 @@ bool ExternallyInstalledWebAppPrefs::HasAppIdWithInstallSource(
 }
 
 // static
-std::vector<GURL> ExternallyInstalledWebAppPrefs::GetInstalledAppUrls(
-    Profile* profile,
+std::map<AppId, GURL> ExternallyInstalledWebAppPrefs::BuildAppIdsMap(
+    const PrefService* pref_service,
     InstallSource install_source) {
   const base::DictionaryValue* urls_to_dicts =
-      profile->GetPrefs()->GetDictionary(prefs::kWebAppsExtensionIDs);
+      pref_service->GetDictionary(prefs::kWebAppsExtensionIDs);
 
-  std::vector<GURL> installed_app_urls;
+  std::map<AppId, GURL> ids_to_urls;
 
   if (!urls_to_dicts) {
-    return installed_app_urls;
+    return ids_to_urls;
   }
 
   for (const auto& it : urls_to_dicts->DictItems()) {
@@ -138,16 +136,12 @@ std::vector<GURL> ExternallyInstalledWebAppPrefs::GetInstalledAppUrls(
       continue;
     }
 
-    auto* provider = web_app::WebAppProviderBase::GetProviderBase(profile);
-    DCHECK(provider);
-    if (!provider->registrar().IsInstalled(v->GetString())) {
-      continue;
-    }
-
-    installed_app_urls.emplace_back(it.first);
+    GURL url(it.first);
+    DCHECK(url.is_valid() && !url.is_empty());
+    ids_to_urls[v->GetString()] = url;
   }
 
-  return installed_app_urls;
+  return ids_to_urls;
 }
 
 ExternallyInstalledWebAppPrefs::ExternallyInstalledWebAppPrefs(

chrome/browser/web_applications/components/externally_installed_web_app_prefs.h

@@ -5,16 +5,16 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_EXTERNALLY_INSTALLED_WEB_APP_PREFS_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_EXTERNALLY_INSTALLED_WEB_APP_PREFS_H_
 
+#include <map>
 #include <string>
-#include <vector>
 
 #include "base/macros.h"
 #include "base/optional.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
 
 class GURL;
 class PrefService;
-class Profile;
 
 namespace user_prefs {
 class PrefRegistrySyncable;
@@ -39,9 +39,10 @@ class ExternallyInstalledWebAppPrefs {
                                         const AppId& app_id,
                                         InstallSource install_source);
 
-  // Returns the URLs of the apps that were installed from |install_source|.
-  static std::vector<GURL> GetInstalledAppUrls(Profile* profile,
-                                               InstallSource install_source);
+  // Returns the URLs of the apps that have been installed from
+  // |install_source|. Will still return apps that have been uninstalled.
+  static std::map<AppId, GURL> BuildAppIdsMap(const PrefService* pref_service,
+                                              InstallSource install_source);
 
   explicit ExternallyInstalledWebAppPrefs(PrefService* pref_service);
 

chrome/browser/web_applications/extensions/externally_installed_web_app_prefs_unittest.cc

@@ -56,11 +56,15 @@ class ExternallyInstalledWebAppPrefsTest
     extensions::ExtensionRegistry::Get(profile())->RemoveEnabled(id);
   }
 
-  std::vector<GURL> GetInstalledAppUrls(InstallSource install_source) {
-    std::vector<GURL> vec = ExternallyInstalledWebAppPrefs::GetInstalledAppUrls(
-        profile(), install_source);
-    std::sort(vec.begin(), vec.end());
-    return vec;
+  std::vector<GURL> GetAppUrls(InstallSource install_source) {
+    std::vector<GURL> urls;
+    for (const auto& id_and_url :
+         ExternallyInstalledWebAppPrefs::BuildAppIdsMap(profile()->GetPrefs(),
+                                                        install_source)) {
+      urls.push_back(id_and_url.second);
+    }
+    std::sort(urls.begin(), urls.end());
+    return urls;
   }
 
  private:
@@ -93,12 +97,9 @@ TEST_F(ExternallyInstalledWebAppPrefsTest, BasicOps) {
   EXPECT_FALSE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_c));
   EXPECT_FALSE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_d));
 
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kInternal));
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kExternalDefault));
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kExternalPolicy));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kInternal));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kExternalDefault));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kExternalPolicy));
 
   // Add some entries.
 
@@ -116,12 +117,10 @@ TEST_F(ExternallyInstalledWebAppPrefsTest, BasicOps) {
   EXPECT_TRUE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_c));
   EXPECT_FALSE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_d));
 
-  EXPECT_EQ(std::vector<GURL>({url_b}),
-            GetInstalledAppUrls(InstallSource::kInternal));
+  EXPECT_EQ(std::vector<GURL>({url_b}), GetAppUrls(InstallSource::kInternal));
   EXPECT_EQ(std::vector<GURL>({url_a, url_c}),
-            GetInstalledAppUrls(InstallSource::kExternalDefault));
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kExternalPolicy));
+            GetAppUrls(InstallSource::kExternalDefault));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kExternalPolicy));
 
   // Overwrite an entry.
 
@@ -138,15 +137,13 @@ TEST_F(ExternallyInstalledWebAppPrefsTest, BasicOps) {
   EXPECT_FALSE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_d));
 
   EXPECT_EQ(std::vector<GURL>({url_b, url_c}),
-            GetInstalledAppUrls(InstallSource::kInternal));
+            GetAppUrls(InstallSource::kInternal));
   EXPECT_EQ(std::vector<GURL>({url_a}),
-            GetInstalledAppUrls(InstallSource::kExternalDefault));
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kExternalPolicy));
+            GetAppUrls(InstallSource::kExternalDefault));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kExternalPolicy));
 
   // Uninstall an underlying extension. The ExternallyInstalledWebAppPrefs will
-  // still return positive for LookupAppId and HasAppId (as they ignore
-  // installed-ness), but GetInstalledAppUrls will skip over it.
+  // still return positive.
 
   SimulateUninstallApp(url_b);
 
@@ -160,12 +157,11 @@ TEST_F(ExternallyInstalledWebAppPrefsTest, BasicOps) {
   EXPECT_TRUE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_c));
   EXPECT_FALSE(ExternallyInstalledWebAppPrefs::HasAppId(prefs, id_d));
 
-  EXPECT_EQ(std::vector<GURL>({url_c}),
-            GetInstalledAppUrls(InstallSource::kInternal));
+  EXPECT_EQ(std::vector<GURL>({url_b, url_c}),
+            GetAppUrls(InstallSource::kInternal));
   EXPECT_EQ(std::vector<GURL>({url_a}),
-            GetInstalledAppUrls(InstallSource::kExternalDefault));
-  EXPECT_EQ(std::vector<GURL>({}),
-            GetInstalledAppUrls(InstallSource::kExternalPolicy));
+            GetAppUrls(InstallSource::kExternalDefault));
+  EXPECT_EQ(std::vector<GURL>({}), GetAppUrls(InstallSource::kExternalPolicy));
 }
 
 }  // namespace web_app

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -109,8 +109,15 @@ void PendingBookmarkAppManager::UninstallApps(
 
 std::vector<GURL> PendingBookmarkAppManager::GetInstalledAppUrls(
     web_app::InstallSource install_source) const {
-  return web_app::ExternallyInstalledWebAppPrefs::GetInstalledAppUrls(
-      profile_, install_source);
+  std::vector<GURL> installed_apps;
+  for (const auto& id_and_url :
+       web_app::ExternallyInstalledWebAppPrefs::BuildAppIdsMap(
+           profile_->GetPrefs(), install_source)) {
+    if (registrar_->IsInstalled(id_and_url.second))
+      installed_apps.push_back(id_and_url.second);
+  }
+
+  return installed_apps;
 }
 
 base::Optional<web_app::AppId> PendingBookmarkAppManager::LookupAppId(