desktop-pwas: Add AppRegistrar

AppRegistrar should be used by clients to query information about apps.
For example, if an app is installed.

AppRegistrar has two implementations BookmarkAppRegistrar and
WebAppRegistrar. BookmarkAppRegistrar is built on top of extensions.

PendingBookmarkApp uses BookmarkAppRegistrar to avoid querying the
ExtensionRegistry and ExtensionPrefs directly.

Bug: 916381

Change-Id: I0706bef37f2ecbf4b86d63eafe8de1dd168f440a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1498736
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637598}

chrome/browser/web_applications/components/BUILD.gn

@@ -4,6 +4,7 @@
 
 source_set("components") {
   sources = [
+    "app_registrar.h",
     "install_finalizer.h",
     "install_manager.h",
     "pending_app_manager.cc",

chrome/browser/web_applications/components/app_registrar.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_APP_REGISTRAR_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_APP_REGISTRAR_H_
+
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+
+namespace web_app {
+
+class AppRegistrar {
+ public:
+  virtual ~AppRegistrar() = default;
+
+  virtual void Init(base::OnceClosure callback) = 0;
+
+  // Returns true if the app with |app_id| is currently installed.
+  virtual bool IsInstalled(const AppId& app_id) const = 0;
+
+  // Returns true if the app with |app_id| was previously uninstalled by the
+  // user. For example, if a user uninstalls a default app ('default apps' are
+  // considered external apps), then this will return true.
+  virtual bool WasExternalAppUninstalledByUser(const AppId& app_id) const = 0;
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_APP_REGISTRAR_H_

chrome/browser/web_applications/extensions/BUILD.gn

@@ -12,6 +12,8 @@ source_set("extensions") {
     "bookmark_app_install_finalizer.h",
     "bookmark_app_installation_task.cc",
     "bookmark_app_installation_task.h",
+    "bookmark_app_registrar.cc",
+    "bookmark_app_registrar.h",
     "bookmark_app_tab_helper.cc",
     "bookmark_app_tab_helper.h",
     "bookmark_app_util.cc",

chrome/browser/web_applications/extensions/bookmark_app_registrar.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/extensions/bookmark_app_registrar.h"
+
+#include <utility>
+
+#include "base/callback_helpers.h"
+#include "chrome/browser/profiles/profile.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/one_shot_event.h"
+
+namespace extensions {
+
+BookmarkAppRegistrar::BookmarkAppRegistrar(Profile* profile)
+    : profile_(profile) {}
+
+BookmarkAppRegistrar::~BookmarkAppRegistrar() = default;
+
+void BookmarkAppRegistrar::Init(base::OnceClosure callback) {
+  ExtensionSystem::Get(profile_)->ready().Post(
+      FROM_HERE, base::AdaptCallbackForRepeating(std::move(callback)));
+}
+
+bool BookmarkAppRegistrar::IsInstalled(const web_app::AppId& app_id) const {
+  return ExtensionRegistry::Get(profile_)->GetInstalledExtension(app_id) !=
+         nullptr;
+}
+
+bool BookmarkAppRegistrar::WasExternalAppUninstalledByUser(
+    const web_app::AppId& app_id) const {
+  return ExtensionPrefs::Get(profile_)->IsExternalExtensionUninstalled(app_id);
+}
+
+}  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_registrar.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_REGISTRAR_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_REGISTRAR_H_
+
+#include "base/callback_forward.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
+
+class Profile;
+
+namespace extensions {
+
+class BookmarkAppRegistrar : public web_app::AppRegistrar {
+ public:
+  explicit BookmarkAppRegistrar(Profile* profile);
+  ~BookmarkAppRegistrar() override;
+
+  // AppRegistrar
+  void Init(base::OnceClosure callback) override;
+  bool IsInstalled(const web_app::AppId& app_id) const override;
+  bool WasExternalAppUninstalledByUser(
+      const web_app::AppId& app_id) const override;
+
+ private:
+  Profile* profile_;
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_REGISTRAR_H_

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -15,11 +15,10 @@
 #include "base/time/time.h"
 #include "chrome/browser/extensions/extension_service.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 "content/public/browser/navigation_controller.h"
 #include "content/public/browser/web_contents.h"
-#include "extensions/browser/extension_prefs.h"
-#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/uninstall_reason.h"
 
@@ -54,8 +53,11 @@ struct PendingBookmarkAppManager::TaskAndCallback {
   OnceInstallCallback callback;
 };
 
-PendingBookmarkAppManager::PendingBookmarkAppManager(Profile* profile)
+PendingBookmarkAppManager::PendingBookmarkAppManager(
+    Profile* profile,
+    web_app::AppRegistrar* registrar)
     : profile_(profile),
+      registrar_(registrar),
       extension_ids_map_(profile->GetPrefs()),
       web_contents_factory_(base::BindRepeating(&WebContentsCreateWrapper)),
       task_factory_(base::BindRepeating(&InstallationTaskCreateWrapper)),
@@ -150,12 +152,11 @@ void PendingBookmarkAppManager::SetTimerForTesting(
 
 base::Optional<bool> PendingBookmarkAppManager::IsExtensionPresentAndInstalled(
     const std::string& extension_id) {
-  if (ExtensionRegistry::Get(profile_)->GetExtensionById(
-          extension_id, ExtensionRegistry::EVERYTHING) != nullptr) {
+  if (registrar_->IsInstalled(extension_id)) {
     return base::Optional<bool>(true);
   }
-  if (ExtensionPrefs::Get(profile_)->IsExternalExtensionUninstalled(
-          extension_id)) {
+
+  if (registrar_->WasExternalAppUninstalledByUser(extension_id)) {
     return base::Optional<bool>(false);
   }
 

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -28,6 +28,10 @@ class RenderFrameHost;
 class WebContents;
 }  // namespace content
 
+namespace web_app {
+class AppRegistrar;
+}  // namespace web_app
+
 namespace extensions {
 
 // Implementation of web_app::PendingAppManager that manages the set of
@@ -43,7 +47,8 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
   using TaskFactory = base::RepeatingCallback<
       std::unique_ptr<BookmarkAppInstallationTask>(Profile*, AppInfo)>;
 
-  explicit PendingBookmarkAppManager(Profile* profile);
+  explicit PendingBookmarkAppManager(Profile* profile,
+                                     web_app::AppRegistrar* registrar_);
   ~PendingBookmarkAppManager() override;
 
   // web_app::PendingAppManager
@@ -91,6 +96,7 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
                    const base::string16& error_description) override;
 
   Profile* profile_;
+  web_app::AppRegistrar* registrar_;
   web_app::ExtensionIdsMap extension_ids_map_;
 
   WebContentsFactory web_contents_factory_;

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

@@ -16,9 +16,11 @@
 #include "base/test/bind_test_util.h"
 #include "base/timer/mock_timer.h"
 #include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_installation_task.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_registrar.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/crx_file/id_util.h"
@@ -189,6 +191,8 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
     test_extension_registry_observer_ =
         std::make_unique<TestExtensionRegistryObserver>(
             ExtensionRegistry::Get(profile()));
+
+    registrar_ = std::make_unique<extensions::BookmarkAppRegistrar>(profile());
   }
 
   void TearDown() override {
@@ -269,7 +273,8 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
 
   std::unique_ptr<PendingBookmarkAppManager>
   GetPendingBookmarkAppManagerWithTestFactories() {
-    auto manager = std::make_unique<PendingBookmarkAppManager>(profile());
+    auto manager = std::make_unique<PendingBookmarkAppManager>(
+        profile(), registrar_.get());
     manager->SetFactoriesForTesting(test_web_contents_creator(),
                                     successful_installation_task_creator());
     return manager;
@@ -339,6 +344,8 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
   PendingBookmarkAppManager::TaskFactory successful_installation_task_creator_;
   PendingBookmarkAppManager::TaskFactory failing_installation_task_creator_;
 
+  std::unique_ptr<web_app::AppRegistrar> registrar_;
+
   DISALLOW_COPY_AND_ASSIGN(PendingBookmarkAppManagerTest);
 };
 

chrome/browser/web_applications/web_app_provider.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/web_applications/components/web_app_audio_focus_id_map.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_registrar.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_tab_helper.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
@@ -77,40 +78,42 @@ void WebAppProvider::StartRegistry() {
   notification_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                               content::Source<Profile>(profile_));
 
-  if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions)) {
-    registrar_->Init(base::BindOnce(&WebAppProvider::OnRegistryReady,
-                                    weak_ptr_factory_.GetWeakPtr()));
-  } else {
-    extensions::ExtensionSystem::Get(profile_)->ready().Post(
-        FROM_HERE, base::BindRepeating(&WebAppProvider::OnRegistryReady,
-                                       weak_ptr_factory_.GetWeakPtr()));
-  }
+  registrar_->Init(base::BindOnce(&WebAppProvider::OnRegistryReady,
+                                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
   database_factory_ = std::make_unique<WebAppDatabaseFactory>(profile);
   database_ = std::make_unique<WebAppDatabase>(database_factory_.get());
-  registrar_ = std::make_unique<WebAppRegistrar>(database_.get());
+  auto web_app_registrar = std::make_unique<WebAppRegistrar>(database_.get());
   icon_manager_ = std::make_unique<WebAppIconManager>(
       profile, std::make_unique<FileUtilsWrapper>());
 
   auto install_finalizer = std::make_unique<WebAppInstallFinalizer>(
-      registrar_.get(), icon_manager_.get());
+      web_app_registrar.get(), icon_manager_.get());
   install_manager_ = std::make_unique<WebAppInstallManager>(
       profile, std::move(install_finalizer));
+
+  registrar_ = std::move(web_app_registrar);
 }
 
 void WebAppProvider::CreateBookmarkAppsSubsystems(Profile* profile) {
+  auto bookmark_app_registrar =
+      std::make_unique<extensions::BookmarkAppRegistrar>(profile);
+
   install_manager_ = std::make_unique<extensions::BookmarkAppInstallManager>();
 
   pending_app_manager_ =
-      std::make_unique<extensions::PendingBookmarkAppManager>(profile);
+      std::make_unique<extensions::PendingBookmarkAppManager>(
+          profile, bookmark_app_registrar.get());
 
   web_app_policy_manager_ = std::make_unique<WebAppPolicyManager>(
       profile, pending_app_manager_.get());
 
   system_web_app_manager_ = std::make_unique<SystemWebAppManager>(
       profile, pending_app_manager_.get());
+
+  registrar_ = std::move(bookmark_app_registrar);
 }
 
 void WebAppProvider::OnRegistryReady() {

chrome/browser/web_applications/web_app_provider.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -35,12 +36,12 @@ class InstallManager;
 class WebAppAudioFocusIdMap;
 class WebAppTabHelperBase;
 class SystemWebAppManager;
+class AppRegistrar;
 
 // Forward declarations for new extension-independent subsystems.
 class WebAppDatabase;
 class WebAppDatabaseFactory;
 class WebAppIconManager;
-class WebAppRegistrar;
 
 // Forward declarations for legacy extension-based subsystems.
 class WebAppPolicyManager;
@@ -108,10 +109,10 @@ class WebAppProvider : public KeyedService,
   std::unique_ptr<WebAppAudioFocusIdMap> audio_focus_id_map_;
   std::unique_ptr<WebAppDatabaseFactory> database_factory_;
   std::unique_ptr<WebAppDatabase> database_;
-  std::unique_ptr<WebAppRegistrar> registrar_;
   std::unique_ptr<WebAppIconManager> icon_manager_;
 
   // New generalized subsystems:
+  std::unique_ptr<AppRegistrar> registrar_;
   std::unique_ptr<InstallManager> install_manager_;
   std::unique_ptr<PendingAppManager> pending_app_manager_;
   std::unique_ptr<SystemWebAppManager> system_web_app_manager_;

chrome/browser/web_applications/web_app_registrar.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/web_applications/web_app_registrar.h"
 
+#include <utility>
+#include <vector>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
@@ -42,7 +45,7 @@ std::unique_ptr<WebApp> WebAppRegistrar::UnregisterApp(const AppId& app_id) {
   return web_app;
 }
 
-WebApp* WebAppRegistrar::GetAppById(const AppId& app_id) {
+WebApp* WebAppRegistrar::GetAppById(const AppId& app_id) const {
   auto kv = registry_.find(app_id);
   return kv == registry_.end() ? nullptr : kv->second.get();
 }
@@ -58,17 +61,27 @@ void WebAppRegistrar::UnregisterAll() {
   registry_.clear();
 }
 
-void WebAppRegistrar::Init(base::OnceClosure closure) {
+void WebAppRegistrar::Init(base::OnceClosure callback) {
   database_->OpenDatabase(base::BindOnce(&WebAppRegistrar::OnDatabaseOpened,
                                          weak_ptr_factory_.GetWeakPtr(),
-                                         std::move(closure)));
+                                         std::move(callback)));
 }
 
-void WebAppRegistrar::OnDatabaseOpened(base::OnceClosure closure,
+void WebAppRegistrar::OnDatabaseOpened(base::OnceClosure callback,
                                        Registry registry) {
   DCHECK(is_empty());
   registry_ = std::move(registry);
-  std::move(closure).Run();
+  std::move(callback).Run();
+}
+
+bool WebAppRegistrar::IsInstalled(const AppId& app_id) const {
+  return GetAppById(app_id) != nullptr;
+}
+
+bool WebAppRegistrar::WasExternalAppUninstalledByUser(
+    const AppId& app_id) const {
+  NOTIMPLEMENTED();
+  return false;
 }
 
 }  // namespace web_app

chrome/browser/web_applications/web_app_registrar.h

@@ -11,21 +11,22 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/abstract_web_app_database.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 
 namespace web_app {
 
 class WebApp;
 
-class WebAppRegistrar {
+class WebAppRegistrar : public AppRegistrar {
  public:
   explicit WebAppRegistrar(AbstractWebAppDatabase* database);
-  ~WebAppRegistrar();
+  ~WebAppRegistrar() override;
 
   void RegisterApp(std::unique_ptr<WebApp> web_app);
   std::unique_ptr<WebApp> UnregisterApp(const AppId& app_id);
 
-  WebApp* GetAppById(const AppId& app_id);
+  WebApp* GetAppById(const AppId& app_id) const;
 
   const Registry& registry() const { return registry_; }
 
@@ -34,10 +35,13 @@ class WebAppRegistrar {
   // Clears registry.
   void UnregisterAll();
 
-  void Init(base::OnceClosure closure);
+  // AppRegistrar
+  void Init(base::OnceClosure callback) override;
+  bool IsInstalled(const AppId& app_id) const override;
+  bool WasExternalAppUninstalledByUser(const AppId& app_id) const override;
 
  private:
-  void OnDatabaseOpened(base::OnceClosure closure, Registry registry);
+  void OnDatabaseOpened(base::OnceClosure callback, Registry registry);
 
   Registry registry_;
 

chrome/browser/web_applications/web_app_registrar_unittest.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/web_applications/web_app_registrar.h"
 
 #include <set>
+#include <string>
+#include <utility>
 
 #include "base/bind_helpers.h"
 #include "base/strings/string_number_conversions.h"
@@ -50,6 +52,7 @@ TEST(WebAppRegistrar, CreateRegisterUnregister) {
   auto registrar = std::make_unique<WebAppRegistrar>(database.get());
 
   EXPECT_EQ(nullptr, registrar->GetAppById(AppId()));
+  EXPECT_FALSE(registrar->GetAppById(AppId()));
 
   const GURL launch_url = GURL("https://example.com/path");
   const AppId app_id = GenerateAppIdFromURL(launch_url);
@@ -75,6 +78,7 @@ TEST(WebAppRegistrar, CreateRegisterUnregister) {
   EXPECT_TRUE(registrar->is_empty());
 
   registrar->RegisterApp(std::move(web_app));
+  EXPECT_TRUE(registrar->IsInstalled(app_id));
   WebApp* app = registrar->GetAppById(app_id);
 
   EXPECT_EQ(app_id, app->app_id());
@@ -88,19 +92,23 @@ TEST(WebAppRegistrar, CreateRegisterUnregister) {
   EXPECT_FALSE(registrar->is_empty());
 
   registrar->RegisterApp(std::move(web_app2));
+  EXPECT_TRUE(registrar->IsInstalled(app_id2));
   WebApp* app2 = registrar->GetAppById(app_id2);
   EXPECT_EQ(app_id2, app2->app_id());
   EXPECT_FALSE(registrar->is_empty());
 
   registrar->UnregisterApp(app_id);
+  EXPECT_FALSE(registrar->IsInstalled(app_id));
   EXPECT_EQ(nullptr, registrar->GetAppById(app_id));
   EXPECT_FALSE(registrar->is_empty());
 
   // Check that app2 is still registered.
   app2 = registrar->GetAppById(app_id2);
+  EXPECT_TRUE(registrar->IsInstalled(app_id2));
   EXPECT_EQ(app_id2, app2->app_id());
 
   registrar->UnregisterApp(app_id2);
+  EXPECT_FALSE(registrar->IsInstalled(app_id2));
   EXPECT_EQ(nullptr, registrar->GetAppById(app_id2));
   EXPECT_TRUE(registrar->is_empty());
 }