Make AppBannerManagerDesktop listen to web app install events

This CL makes AppBannerManagerDesktop listen for web app installs
to determine whether to fire the appinstalled event to the renderer.
Previously it depended on every install UI flow to call OnInstall
because we didn't have this event to listen to internally.

Bug: 956810
Change-Id: If78c86c8c407a8777f59236685c388276ef0f724
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1657083
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669970}

chrome/browser/banners/app_banner_manager.cc

@@ -185,14 +185,16 @@ void AppBannerManager::OnInstall(bool is_native,
   DCHECK(installation_service);
   installation_service->OnInstall();
 
-  // We've triggered an installation, so reset bindings to ensure that any
-  // existing beforeinstallprompt events cannot trigger add to home screen.
-  ResetBindings();
+  // App has been installed (possibly by the user), page may no longer request
+  // install prompt.
+  binding_.Close();
 }
 
 void AppBannerManager::SendBannerAccepted() {
-  if (event_.is_bound())
+  if (event_.is_bound()) {
     event_->BannerAccepted(GetBannerType());
+    event_.reset();
+  }
 }
 
 void AppBannerManager::SendBannerDismissed() {
@@ -777,6 +779,9 @@ void AppBannerManager::ShowBanner() {
 }
 
 void AppBannerManager::DisplayAppBanner() {
+  // Prevent this from being called multiple times on the same connection.
+  binding_.Close();
+
   if (state_ == State::PENDING_PROMPT) {
     ShowBanner();
   } else if (state_ == State::SENDING_EVENT) {

chrome/browser/banners/app_banner_manager_desktop.cc

@@ -54,9 +54,15 @@ void AppBannerManagerDesktop::DisableTriggeringForTesting() {
 
 AppBannerManagerDesktop::AppBannerManagerDesktop(
     content::WebContents* web_contents)
-    : AppBannerManager(web_contents),
-      extension_registry_(extensions::ExtensionRegistry::Get(
-          web_contents->GetBrowserContext())) {}
+    : AppBannerManager(web_contents), registrar_observer_(this) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  extension_registry_ = extensions::ExtensionRegistry::Get(profile);
+  auto* provider = web_app::WebAppProviderBase::GetProviderBase(profile);
+  // May be null in unit tests e.g. TabDesktopMediaListTest.*.
+  if (provider)
+    registrar_observer_.Add(&provider->registrar());
+}
 
 AppBannerManagerDesktop::~AppBannerManagerDesktop() { }
 
@@ -147,6 +153,21 @@ void AppBannerManagerDesktop::OnEngagementEvent(
   AppBannerManager::OnEngagementEvent(web_contents, url, score, type);
 }
 
+void AppBannerManagerDesktop::OnWebAppInstalled(
+    const web_app::AppId& installed_app_id) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  auto* provider = web_app::WebAppProviderBase::GetProviderBase(profile);
+  DCHECK(provider);
+  web_app::AppId app_id = provider->registrar().FindAppIdForUrl(validated_url_);
+  if (!app_id.empty() && app_id == installed_app_id)
+    OnInstall(false /* is_native app */, blink::kWebDisplayModeStandalone);
+}
+
+void AppBannerManagerDesktop::OnAppRegistrarDestroyed() {
+  registrar_observer_.RemoveAll();
+}
+
 void AppBannerManagerDesktop::CreateWebApp(WebappInstallSource install_source) {
   content::WebContents* contents = web_contents();
   DCHECK(contents);
@@ -184,9 +205,6 @@ void AppBannerManagerDesktop::DidFinishCreatingWebApp(
   SendBannerAccepted();
   AppBannerSettingsHelper::RecordBannerInstallEvent(
       contents, GetAppIdentifier(), AppBannerSettingsHelper::WEB);
-
-  // OnInstall must be called last since it resets Mojo bindings.
-  OnInstall(false /* is_native app */, blink::kWebDisplayModeStandalone);
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(AppBannerManagerDesktop)

chrome/browser/banners/app_banner_manager_desktop.h

@@ -10,6 +10,8 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/banners/app_banner_manager.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
+#include "chrome/browser/web_applications/components/app_registrar_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace extensions {
@@ -25,7 +27,8 @@ namespace banners {
 // Manages web app banners for desktop platforms.
 class AppBannerManagerDesktop
     : public AppBannerManager,
-      public content::WebContentsUserData<AppBannerManagerDesktop> {
+      public content::WebContentsUserData<AppBannerManagerDesktop>,
+      public web_app::AppRegistrarObserver {
  public:
   ~AppBannerManagerDesktop() override;
 
@@ -70,9 +73,17 @@ class AppBannerManagerDesktop
                          double score,
                          SiteEngagementService::EngagementType type) override;
 
+  // web_app::AppRegistrarObserver:
+  void OnWebAppInstalled(const web_app::AppId& app_id) override;
+  void OnAppRegistrarDestroyed() override;
+
   void CreateWebApp(WebappInstallSource install_source);
 
   extensions::ExtensionRegistry* extension_registry_;
+
+  ScopedObserver<web_app::AppRegistrar, web_app::AppRegistrarObserver>
+      registrar_observer_;
+
   base::WeakPtrFactory<AppBannerManagerDesktop> weak_factory_{this};
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();

chrome/browser/banners/app_banner_manager_desktop_browsertest.cc

@@ -248,15 +248,6 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest, DestroyWebContents) {
 
 IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
                        InstallPromptAfterUserMenuInstall) {
-  // TODO(https://crbug.com/915043): Fix this test for the unified install
-  // codepath. This fails under unified install because the menu install path
-  // relies on extensions::TabHelper to call AppBannerManager::OnInstall which
-  // is no longer the case under unified install. This will be fixed in a follow
-  // up patch when AppBannerManager calls OnInstall by itself via
-  // AppRegistrarObserver::OnWebAppInstalled().
-  if (base::FeatureList::IsEnabled(features::kDesktopPWAsUnifiedInstall))
-    return;
-
   FakeAppBannerManagerDesktop* manager =
       FakeAppBannerManagerDesktop::CreateForWebContents(
           browser()->tab_strip_model()->GetActiveWebContents());

chrome/browser/extensions/tab_helper.cc

@@ -9,7 +9,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
-#include "chrome/browser/banners/app_banner_manager_desktop.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.h"
 #include "chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h"
@@ -61,7 +60,6 @@
 #include "extensions/common/manifest_handlers/icons_handler.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
-#include "third_party/blink/public/common/manifest/web_display_mode.h"
 #include "url/url_constants.h"
 
 using content::NavigationController;
@@ -192,18 +190,10 @@ void TabHelper::InvokeForContentRulesRegistries(const Func& func) {
 void TabHelper::FinishCreateBookmarkApp(
     const Extension* extension,
     const WebApplicationInfo& web_app_info) {
-  const bool success = (extension != nullptr);
-
-  if (success && web_app_info.open_as_window) {
-    // Send the 'appinstalled' event and ensure any beforeinstallpromptevent
-    // cannot trigger installation again.
-    banners::AppBannerManagerDesktop::FromWebContents(web_contents())
-        ->OnInstall(false /* is_native app */,
-                    blink::kWebDisplayModeStandalone);
-  }
   pending_web_app_action_ = NONE;
 
   const ExtensionId app_id = extension ? extension->id() : ExtensionId();
+  const bool success = (extension != nullptr);
   std::move(install_callback_).Run(app_id, success);
 }
 

chrome/browser/ui/views/page_action/pwa_install_view.cc

@@ -55,20 +55,9 @@ bool PwaInstallView::Update() {
 
 void PwaInstallView::OnExecuting(PageActionIconView::ExecuteSource source) {
   base::RecordAction(base::UserMetricsAction("PWAInstallIcon"));
-  content::WebContents* web_contents = GetWebContents();
-  // TODO(https://crbug.com/956810): Make AppBannerManager listen for
-  // installations instead of having to notify it from every installation UI
-  // surface.
-  auto* manager = banners::AppBannerManager::FromWebContents(web_contents);
-  web_app::CreateWebAppFromManifest(
-      web_contents, WebappInstallSource::OMNIBOX_INSTALL_ICON,
-      base::BindOnce(
-          [](base::WeakPtr<banners::AppBannerManager> manager,
-             const web_app::AppId& app_id, web_app::InstallResultCode code) {
-            if (manager && code == web_app::InstallResultCode::kSuccess)
-              manager->OnInstall(false, blink::kWebDisplayModeStandalone);
-          },
-          manager ? manager->GetWeakPtr() : nullptr));
+  web_app::CreateWebAppFromManifest(GetWebContents(),
+                                    WebappInstallSource::OMNIBOX_INSTALL_ICON,
+                                    base::DoNothing());
 }
 
 views::BubbleDialogDelegateView* PwaInstallView::GetBubble() const {