WebApp: Plumb AppRegistrar to InstallFinalizer.

It allows BookmarkAppInstallFinalizer to mimic the "atomic write" of the
web app into the registry (BookmarkAppRegistrar).

The ideal behavior should match WebAppInstallFinalizer (that's our
future):
WebAppInstallFinalizer does real atomic write to disk (LevelDB) and
registers the web app in WebAppRegistrar.

Bug: 901226
Change-Id: Iaf9840cf02e9027f2822426c089a0b7a4e62803c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1732700
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683921}

chrome/browser/web_applications/components/app_registrar.cc

@@ -19,6 +19,14 @@ AppRegistrar::~AppRegistrar() {
     observer.OnAppRegistrarDestroyed();
 }
 
+WebAppRegistrar* AppRegistrar::AsWebAppRegistrar() {
+  return nullptr;
+}
+
+extensions::BookmarkAppRegistrar* AppRegistrar::AsBookmarkAppRegistrar() {
+  return nullptr;
+}
+
 void AppRegistrar::AddObserver(AppRegistrarObserver* observer) {
   observers_.AddObserver(observer);
 }

chrome/browser/web_applications/components/app_registrar.h

@@ -15,9 +15,14 @@
 class GURL;
 class Profile;
 
+namespace extensions {
+class BookmarkAppRegistrar;
+}
+
 namespace web_app {
 
 class AppRegistrarObserver;
+class WebAppRegistrar;
 
 enum class ExternalInstallSource;
 
@@ -28,6 +33,10 @@ class AppRegistrar {
 
   virtual void Init(base::OnceClosure callback) = 0;
 
+  // Safe downcasts. TODO(loyso): Subclass WebAppProvider to get rid of these:
+  virtual WebAppRegistrar* AsWebAppRegistrar();
+  virtual extensions::BookmarkAppRegistrar* AsBookmarkAppRegistrar();
+
   // Returns true if the app with the specified |start_url| is currently fully
   // locally installed. The provided |start_url| must exactly match the launch
   // URL for the app; this method does not consult the app scope or match URLs
@@ -80,10 +89,11 @@ class AppRegistrar {
   void AddObserver(AppRegistrarObserver* observer);
   void RemoveObserver(const AppRegistrarObserver* observer);
 
+  void NotifyWebAppInstalled(const AppId& app_id);
+
  protected:
   Profile* profile() const { return profile_; }
 
-  void NotifyWebAppInstalled(const AppId& app_id);
   void NotifyWebAppUninstalled(const AppId& app_id);
   void NotifyAppRegistrarShutdown();
 

chrome/browser/web_applications/components/install_finalizer.cc

@@ -9,7 +9,9 @@
 
 namespace web_app {
 
-void InstallFinalizer::SetSubsystems(WebAppUiManager* ui_manager) {
+void InstallFinalizer::SetSubsystems(AppRegistrar* registrar,
+                                     WebAppUiManager* ui_manager) {
+  registrar_ = registrar;
   ui_manager_ = ui_manager;
 }
 

chrome/browser/web_applications/components/install_finalizer.h

@@ -21,6 +21,7 @@ class WebContents;
 namespace web_app {
 
 enum class InstallResultCode;
+class AppRegistrar;
 class WebAppUiManager;
 
 // An abstract finalizer for the installation process, represents the last step.
@@ -28,8 +29,6 @@ class WebAppUiManager;
 // and registers an app.
 class InstallFinalizer {
  public:
-  void SetSubsystems(WebAppUiManager* ui_manager);
-
   using InstallFinalizedCallback =
       base::OnceCallback<void(const AppId& app_id, InstallResultCode code)>;
   using UninstallExternalWebAppCallback =
@@ -76,12 +75,20 @@ class InstallFinalizer {
       const AppId& app_id,
       const WebApplicationInfo& web_app_info) const = 0;
 
+  // TODO(loyso): This method should be protected and subclasses should call it
+  // (upcasting their final registrar type). Subclassing WebAppProvider will fix
+  // this.
+  virtual void SetSubsystems(AppRegistrar* registrar,
+                             WebAppUiManager* ui_manager);
+
   virtual ~InstallFinalizer() = default;
 
  protected:
+  AppRegistrar& registrar() const { return *registrar_; }
   WebAppUiManager& ui_manager() const { return *ui_manager_; }
 
  private:
+  AppRegistrar* registrar_ = nullptr;
   WebAppUiManager* ui_manager_ = nullptr;
 };
 

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -18,15 +18,14 @@
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.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_finalizer_utils.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_registrar.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/web_application_info.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
-#include "extensions/browser/install/crx_install_error.h"
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_id.h"
@@ -39,53 +38,8 @@
 
 namespace extensions {
 
-namespace {
-
-const Extension* GetExtensionById(Profile* profile,
-                                  const web_app::AppId& app_id) {
-  const Extension* app =
-      ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(app_id);
-  DCHECK(app);
-  return app;
-}
-
-void OnExtensionInstalled(
-    const GURL& app_url,
-    LaunchType launch_type,
-    bool is_locally_installed,
-    web_app::InstallFinalizer::InstallFinalizedCallback callback,
-    scoped_refptr<CrxInstaller> crx_installer,
-    const base::Optional<CrxInstallError>& error) {
-  if (error) {
-    std::move(callback).Run(web_app::AppId(),
-                            web_app::InstallResultCode::kFailedUnknownReason);
-    return;
-  }
-
-  const Extension* extension = crx_installer->extension();
-  DCHECK(extension);
-  if (extension !=
-      GetExtensionById(crx_installer->profile(), extension->id())) {
-    std::move(callback).Run(web_app::AppId(),
-                            web_app::InstallResultCode::kWebAppDisabled);
-    return;
-  }
-
-  DCHECK_EQ(AppLaunchInfo::GetLaunchWebURL(extension), app_url);
-
-  SetLaunchType(crx_installer->profile(), extension->id(), launch_type);
-
-  SetBookmarkAppIsLocallyInstalled(crx_installer->profile(), extension,
-                                   is_locally_installed);
-
-  std::move(callback).Run(extension->id(),
-                          web_app::InstallResultCode::kSuccess);
-}
-
-}  // namespace
-
 BookmarkAppInstallFinalizer::BookmarkAppInstallFinalizer(Profile* profile)
-    : profile_(profile), externally_installed_app_prefs_(profile->GetPrefs()) {
+    : externally_installed_app_prefs_(profile->GetPrefs()), profile_(profile) {
   crx_installer_factory_ = base::BindRepeating([](Profile* profile) {
     ExtensionService* extension_service =
         ExtensionSystem::Get(profile)->extension_service();
@@ -120,7 +74,8 @@ void BookmarkAppInstallFinalizer::FinalizeInstall(
   }
 
   crx_installer->set_installer_callback(base::BindOnce(
-      OnExtensionInstalled, web_app_info.app_url, launch_type,
+      &BookmarkAppInstallFinalizer::OnExtensionInstalled,
+      weak_ptr_factory_.GetWeakPtr(), web_app_info.app_url, launch_type,
       options.locally_installed, std::move(callback), crx_installer));
 
   switch (options.install_source) {
@@ -206,14 +161,14 @@ void BookmarkAppInstallFinalizer::CreateOsShortcuts(
     const web_app::AppId& app_id,
     bool add_to_desktop,
     CreateOsShortcutsCallback callback) {
-  const Extension* app = GetExtensionById(profile_, app_id);
+  const Extension* app = GetExtensionById(app_id);
   BookmarkAppCreateOsShortcuts(profile_, app, add_to_desktop,
                                std::move(callback));
 }
 
 bool BookmarkAppInstallFinalizer::CanReparentTab(const web_app::AppId& app_id,
                                                  bool shortcut_created) const {
-  const Extension* app = GetExtensionById(profile_, app_id);
+  const Extension* app = GetExtensionById(app_id);
   // Reparent the web contents into its own window only if that is the
   // app's launch type.
   if (!app ||
@@ -236,7 +191,7 @@ bool BookmarkAppInstallFinalizer::CanRevealAppShim() const {
 void BookmarkAppInstallFinalizer::RevealAppShim(const web_app::AppId& app_id) {
   DCHECK(CanRevealAppShim());
 #if defined(OS_MACOSX)
-  const Extension* app = GetExtensionById(profile_, app_id);
+  const Extension* app = GetExtensionById(app_id);
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           ::switches::kDisableHostedAppShimCreation)) {
     web_app::RevealAppShimInFinderForApp(profile_, app);
@@ -269,9 +224,58 @@ bool BookmarkAppInstallFinalizer::CanSkipAppUpdateForSync(
   return false;
 }
 
+void BookmarkAppInstallFinalizer::SetSubsystems(
+    web_app::AppRegistrar* registrar,
+    web_app::WebAppUiManager* ui_manager) {
+  registrar_ = registrar ? registrar->AsBookmarkAppRegistrar() : nullptr;
+  InstallFinalizer::SetSubsystems(registrar, ui_manager);
+}
+
 void BookmarkAppInstallFinalizer::SetCrxInstallerFactoryForTesting(
     CrxInstallerFactory crx_installer_factory) {
   crx_installer_factory_ = crx_installer_factory;
 }
 
+const Extension* BookmarkAppInstallFinalizer::GetExtensionById(
+    const web_app::AppId& app_id) const {
+  const Extension* app =
+      ExtensionRegistry::Get(profile_)->enabled_extensions().GetByID(app_id);
+  DCHECK(app);
+  return app;
+}
+
+void BookmarkAppInstallFinalizer::OnExtensionInstalled(
+    const GURL& app_url,
+    LaunchType launch_type,
+    bool is_locally_installed,
+    web_app::InstallFinalizer::InstallFinalizedCallback callback,
+    scoped_refptr<CrxInstaller> crx_installer,
+    const base::Optional<CrxInstallError>& error) {
+  if (error) {
+    std::move(callback).Run(web_app::AppId(),
+                            web_app::InstallResultCode::kFailedUnknownReason);
+    return;
+  }
+
+  const Extension* extension = crx_installer->extension();
+  DCHECK(extension);
+  if (extension != GetExtensionById(extension->id())) {
+    std::move(callback).Run(web_app::AppId(),
+                            web_app::InstallResultCode::kWebAppDisabled);
+    return;
+  }
+
+  DCHECK_EQ(AppLaunchInfo::GetLaunchWebURL(extension), app_url);
+
+  SetLaunchType(profile_, extension->id(), launch_type);
+
+  SetBookmarkAppIsLocallyInstalled(profile_, extension, is_locally_installed);
+
+  DCHECK(registrar_);
+  registrar_->NotifyWebAppInstalled(extension->id());
+
+  std::move(callback).Run(extension->id(),
+                          web_app::InstallResultCode::kSuccess);
+}
+
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -8,14 +8,20 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/externally_installed_web_app_prefs.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "extensions/browser/install/crx_install_error.h"
+#include "extensions/common/constants.h"
 
 class Profile;
 
 namespace extensions {
 
+class BookmarkAppRegistrar;
 class CrxInstaller;
+class Extension;
 
 // Class used to actually install the Bookmark App in the system.
 // TODO(loyso): Erase this subclass once crbug.com/877898 fixed.
@@ -44,6 +50,8 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
   bool CanSkipAppUpdateForSync(
       const web_app::AppId& app_id,
       const WebApplicationInfo& web_app_info) const override;
+  void SetSubsystems(web_app::AppRegistrar* registrar,
+                     web_app::WebAppUiManager* ui_manager) override;
 
   using CrxInstallerFactory =
       base::RepeatingCallback<scoped_refptr<CrxInstaller>(Profile*)>;
@@ -51,10 +59,24 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
       CrxInstallerFactory crx_installer_factory);
 
  private:
+  const Extension* GetExtensionById(const web_app::AppId& app_id) const;
+
+  void OnExtensionInstalled(
+      const GURL& app_url,
+      LaunchType launch_type,
+      bool is_locally_installed,
+      web_app::InstallFinalizer::InstallFinalizedCallback callback,
+      scoped_refptr<CrxInstaller> crx_installer,
+      const base::Optional<CrxInstallError>& error);
+
   CrxInstallerFactory crx_installer_factory_;
-  Profile* profile_;
   web_app::ExternallyInstalledWebAppPrefs externally_installed_app_prefs_;
 
+  Profile* profile_;
+  BookmarkAppRegistrar* registrar_ = nullptr;
+
+  base::WeakPtrFactory<BookmarkAppInstallFinalizer> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallFinalizer);
 };
 

chrome/browser/web_applications/extensions/bookmark_app_registrar.cc

@@ -34,6 +34,10 @@ void BookmarkAppRegistrar::Init(base::OnceClosure callback) {
   ExtensionSystem::Get(profile())->ready().Post(FROM_HERE, std::move(callback));
 }
 
+BookmarkAppRegistrar* BookmarkAppRegistrar::AsBookmarkAppRegistrar() {
+  return this;
+}
+
 bool BookmarkAppRegistrar::IsInstalled(const GURL& start_url) const {
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
   const ExtensionSet& extensions = registry->enabled_extensions();
@@ -80,15 +84,6 @@ int BookmarkAppRegistrar::CountUserInstalledApps() const {
   return CountUserInstalledBookmarkApps(profile());
 }
 
-void BookmarkAppRegistrar::OnExtensionInstalled(
-    content::BrowserContext* browser_context,
-    const extensions::Extension* extension,
-    bool is_update) {
-  DCHECK_EQ(browser_context, profile());
-  if (extension->from_bookmark())
-    NotifyWebAppInstalled(extension->id());
-}
-
 void BookmarkAppRegistrar::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,

chrome/browser/web_applications/extensions/bookmark_app_registrar.h

@@ -25,6 +25,7 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar,
 
   // AppRegistrar:
   void Init(base::OnceClosure callback) override;
+  BookmarkAppRegistrar* AsBookmarkAppRegistrar() override;
   bool IsInstalled(const GURL& start_url) const override;
   bool IsInstalled(const web_app::AppId& app_id) const override;
   bool WasExternalAppUninstalledByUser(
@@ -40,9 +41,6 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar,
   base::Optional<GURL> GetAppScope(const web_app::AppId& app_id) const override;
 
   // ExtensionRegistryObserver:
-  void OnExtensionInstalled(content::BrowserContext* browser_context,
-                            const Extension* extension,
-                            bool is_update) override;
   void OnExtensionUninstalled(content::BrowserContext* browser_context,
                               const Extension* extension,
                               UninstallReason reason) override;

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -45,9 +45,8 @@ void SetIcons(const WebApplicationInfo& web_app_info, WebApp* web_app) {
 
 }  // namespace
 
-WebAppInstallFinalizer::WebAppInstallFinalizer(WebAppRegistrar* registrar,
-                                               WebAppIconManager* icon_manager)
-    : registrar_(registrar), icon_manager_(icon_manager) {}
+WebAppInstallFinalizer::WebAppInstallFinalizer(WebAppIconManager* icon_manager)
+    : icon_manager_(icon_manager) {}
 
 WebAppInstallFinalizer::~WebAppInstallFinalizer() = default;
 
@@ -99,6 +98,8 @@ void WebAppInstallFinalizer::OnDataWritten(InstallFinalizedCallback callback,
   AppId app_id = web_app->app_id();
 
   registrar_->RegisterApp(std::move(web_app));
+  // TODO(loyso): NotifyWebAppInstalled should be a part of RegisterApp.
+  registrar_->NotifyWebAppInstalled(app_id);
 
   std::move(callback).Run(std::move(app_id), InstallResultCode::kSuccess);
 }
@@ -145,4 +146,10 @@ bool WebAppInstallFinalizer::CanSkipAppUpdateForSync(
   return true;
 }
 
+void WebAppInstallFinalizer::SetSubsystems(AppRegistrar* registrar,
+                                           WebAppUiManager* ui_manager) {
+  registrar_ = registrar ? registrar->AsWebAppRegistrar() : nullptr;
+  InstallFinalizer::SetSubsystems(registrar, ui_manager);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -21,8 +21,7 @@ class WebAppRegistrar;
 
 class WebAppInstallFinalizer final : public InstallFinalizer {
  public:
-  WebAppInstallFinalizer(WebAppRegistrar* registrar,
-                         WebAppIconManager* icon_manager);
+  explicit WebAppInstallFinalizer(WebAppIconManager* icon_manager);
   ~WebAppInstallFinalizer() override;
 
   // InstallFinalizer:
@@ -43,13 +42,15 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
   bool CanSkipAppUpdateForSync(
       const AppId& app_id,
       const WebApplicationInfo& web_app_info) const override;
+  void SetSubsystems(AppRegistrar* registrar,
+                     WebAppUiManager* ui_manager) override;
 
  private:
   void OnDataWritten(InstallFinalizedCallback callback,
                      std::unique_ptr<WebApp> web_app,
                      bool success);
 
-  WebAppRegistrar* registrar_;
+  WebAppRegistrar* registrar_ = nullptr;
   WebAppIconManager* icon_manager_;
 
   base::WeakPtrFactory<WebAppInstallFinalizer> weak_ptr_factory_{this};

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -126,9 +126,9 @@ class WebAppInstallTaskTest : public WebAppTest {
 
     ui_manager_ = std::make_unique<TestWebAppUiManager>();
 
-    install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
-        registrar_.get(), icon_manager_.get());
-    install_finalizer_->SetSubsystems(ui_manager_.get());
+    install_finalizer_ =
+        std::make_unique<WebAppInstallFinalizer>(icon_manager_.get());
+    install_finalizer_->SetSubsystems(registrar_.get(), ui_manager_.get());
 
     auto data_retriever = std::make_unique<TestDataRetriever>();
     data_retriever_ = data_retriever.get();

chrome/browser/web_applications/web_app_provider.cc

@@ -136,11 +136,8 @@ void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
   icon_manager_ = std::make_unique<WebAppIconManager>(
       profile, std::make_unique<FileUtilsWrapper>());
 
-  // TODO(crbug.com/973324): Once the WebAppInstallFinalizer can take an
-  // AppRegistrar instead of needing a WebAppRegistrar, move this wiring into
-  // ConnectSubsystems().
-  install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
-      web_app_registrar.get(), icon_manager_.get());
+  install_finalizer_ =
+      std::make_unique<WebAppInstallFinalizer>(icon_manager_.get());
   sync_manager_ = std::make_unique<WebAppSyncManager>();
 
   system_web_app_manager_ = std::make_unique<SystemWebAppManager>(profile);
@@ -165,7 +162,7 @@ void WebAppProvider::ConnectSubsystems() {
   DCHECK(!started_);
 
   install_manager_->SetSubsystems(registrar_.get(), install_finalizer_.get());
-  install_finalizer_->SetSubsystems(ui_manager_.get());
+  install_finalizer_->SetSubsystems(registrar_.get(), ui_manager_.get());
 
   // TODO(crbug.com/877898): Port all other managers to support BMO.
   if (!base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions)) {

chrome/browser/web_applications/web_app_registrar.cc

@@ -75,6 +75,10 @@ void WebAppRegistrar::OnDatabaseOpened(base::OnceClosure callback,
   std::move(callback).Run();
 }
 
+WebAppRegistrar* WebAppRegistrar::AsWebAppRegistrar() {
+  return this;
+}
+
 bool WebAppRegistrar::IsInstalled(const GURL& start_url) const {
   NOTIMPLEMENTED();
   return false;

chrome/browser/web_applications/web_app_registrar.h

@@ -37,6 +37,7 @@ class WebAppRegistrar : public AppRegistrar {
 
   // AppRegistrar:
   void Init(base::OnceClosure callback) override;
+  WebAppRegistrar* AsWebAppRegistrar() override;
   bool IsInstalled(const GURL& start_url) const override;
   bool IsInstalled(const AppId& app_id) const override;
   bool WasExternalAppUninstalledByUser(const AppId& app_id) const override;