[Code Health] Refactor ChromeDownloadManagerDelegate

Replace usage of NOTIFICATION_CRX_INSTALLER_DONE in
ChromeDownloadManagerDelegate with a callback.

Bug: 1174728
Change-Id: I448d5ce33f0848cd80a63a94b00081a65ca361bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3975336
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Daniel d'Andrada <dandrader@google.com>
Cr-Commit-Position: refs/heads/main@{#1067464}

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -80,7 +80,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item_utils.h"
 #include "content/public/browser/download_manager.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/service_process_host.h"
 #include "content/public/common/origin_util.h"
@@ -118,8 +117,8 @@
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/webstore_installer.h"
-#include "extensions/browser/notification_types.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/user_script.h"
 #endif
 
 #if BUILDFLAG(ENABLE_OFFLINE_PAGES)
@@ -136,6 +135,11 @@ using safe_browsing::DownloadFileType;
 using safe_browsing::DownloadProtectionService;
 using ConnectionType = net::NetworkChangeNotifier::ConnectionType;
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+using extensions::CrxInstaller;
+using extensions::CrxInstallError;
+#endif
+
 namespace {
 
 #if !BUILDFLAG(IS_ANDROID)
@@ -732,16 +736,28 @@ bool ChromeDownloadManagerDelegate::ShouldOpenDownload(
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   if (download_crx_util::IsExtensionDownload(*item) &&
       !extensions::WebstoreInstaller::GetAssociatedApproval(*item)) {
-    scoped_refptr<extensions::CrxInstaller> crx_installer =
-        download_crx_util::OpenChromeExtension(profile_, *item);
+    scoped_refptr<CrxInstaller> installer(
+        download_crx_util::CreateCrxInstaller(profile_, *item));
 
-    // CRX_INSTALLER_DONE will fire when the install completes.  At that
-    // time, Observe() will call the passed callback.
-    registrar_.Add(
-        this, extensions::NOTIFICATION_CRX_INSTALLER_DONE,
-        content::Source<extensions::CrxInstaller>(crx_installer.get()));
+    if (download_crx_util::OffStoreInstallAllowedByPrefs(profile_, *item)) {
+      installer->set_off_store_install_allow_reason(
+          CrxInstaller::OffStoreInstallAllowedBecausePref);
+    }
+
+    auto token = base::UnguessableToken::Create();
+    running_crx_installs_[token] = installer;
+
+    installer->AddInstallerCallback(base::BindOnce(
+        &ChromeDownloadManagerDelegate::OnInstallerDone,
+        weak_ptr_factory_.GetWeakPtr(), token, std::move(callback)));
+
+    if (extensions::UserScript::IsURLUserScript(item->GetURL(),
+                                                item->GetMimeType())) {
+      installer->InstallUserScript(item->GetFullPath(), item->GetURL());
+    } else {
+      installer->InstallCrx(item->GetFullPath());
+    }
 
-    crx_installers_[crx_installer.get()] = std::move(callback);
     // The status text and percent complete indicator will change now
     // that we are installing a CRX.  Update observers so that they pick
     // up the change.
@@ -1475,24 +1491,23 @@ void ChromeDownloadManagerDelegate::CheckSavePackageScanningDone(
 }
 #endif  // FULL_SAFE_BROWSING
 
-// content::NotificationObserver implementation.
-void ChromeDownloadManagerDelegate::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  DCHECK_EQ(extensions::NOTIFICATION_CRX_INSTALLER_DONE, type);
-
-  registrar_.Remove(this, extensions::NOTIFICATION_CRX_INSTALLER_DONE, source);
+void ChromeDownloadManagerDelegate::OnInstallerDone(
+    const base::UnguessableToken& token,
+    content::DownloadOpenDelayedCallback callback,
+    const absl::optional<CrxInstallError>& error) {
+  scoped_refptr<CrxInstaller> installer;
+
+  {
+    auto iter = running_crx_installs_.find(token);
+    DCHECK(iter != running_crx_installs_.end());
+    installer = iter->second;
+    running_crx_installs_.erase(iter);
+  }
 
-  scoped_refptr<extensions::CrxInstaller> installer =
-      content::Source<extensions::CrxInstaller>(source).ptr();
-  content::DownloadOpenDelayedCallback callback =
-      std::move(crx_installers_[installer.get()]);
-  crx_installers_.erase(installer.get());
   std::move(callback).Run(installer->did_handle_successfully());
-#endif
 }
+#endif
 
 void ChromeDownloadManagerDelegate::OnDownloadTargetDetermined(
     uint32_t download_id,

chrome/browser/download/chrome_download_manager_delegate.h

@@ -17,6 +17,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/sequenced_task_runner.h"
+#include "base/unguessable_token.h"
 #include "build/build_config.h"
 #include "chrome/browser/download/download_completion_blocker.h"
 #include "chrome/browser/download/download_target_determiner_delegate.h"
@@ -29,8 +30,6 @@
 #include "components/safe_browsing/buildflags.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_manager_delegate.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "extensions/buildflags/buildflags.h"
 #include "ui/gfx/native_widget_types.h"
 
@@ -46,14 +45,16 @@ namespace content {
 class DownloadManager;
 }
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
 namespace extensions {
 class CrxInstaller;
+class CrxInstallError;
 }
+#endif
 
 // This is the Chrome side helper for the download system.
 class ChromeDownloadManagerDelegate
     : public content::DownloadManagerDelegate,
-      public content::NotificationObserver,
       public DownloadTargetDeterminerDelegate,
       public content::DownloadManager::Observer {
  public:
@@ -266,10 +267,13 @@ class ChromeDownloadManagerDelegate
       const base::FilePath& suggested_path,
       DownloadTargetDeterminerDelegate::ConfirmationCallback callback);
 
-  // content::NotificationObserver implementation.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  // Called when CrxInstaller in running_crx_installs_ finishes installation.
+  void OnInstallerDone(
+      const base::UnguessableToken& token,
+      content::DownloadOpenDelayedCallback callback,
+      const absl::optional<extensions::CrxInstallError>& error);
+#endif
 
   // Internal gateways for ShouldCompleteDownload().
   bool IsDownloadReadyForCompletion(
@@ -346,11 +350,9 @@ class ChromeDownloadManagerDelegate
   std::unique_ptr<DownloadPrefs> download_prefs_;
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  // Maps from pending extension installations to DownloadItem IDs.
-  typedef base::flat_map<extensions::CrxInstaller*,
-                         content::DownloadOpenDelayedCallback>
-      CrxInstallerMap;
-  CrxInstallerMap crx_installers_;
+  // CRX installs that are currently in progress.
+  std::map<base::UnguessableToken, scoped_refptr<extensions::CrxInstaller>>
+      running_crx_installs_;
 #endif
 
   // Outstanding callbacks to open file selection dialog.
@@ -359,8 +361,6 @@ class ChromeDownloadManagerDelegate
   // Whether a file picker dialog is showing.
   bool is_file_picker_showing_;
 
-  content::NotificationRegistrar registrar_;
-
   base::WeakPtrFactory<ChromeDownloadManagerDelegate> weak_ptr_factory_{this};
 };
 

chrome/browser/download/download_crx_util.cc

@@ -20,7 +20,6 @@
 #include "components/download/public/common/download_item.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item_utils.h"
-#include "content/public/browser/notification_service.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/user_script.h"
 
@@ -66,14 +65,14 @@ std::unique_ptr<ExtensionInstallPrompt> CreateExtensionInstallPrompt(
   }
 }
 
+}  // namespace
+
 bool OffStoreInstallAllowedByPrefs(Profile* profile, const DownloadItem& item) {
   return g_allow_offstore_install_for_testing ||
          extensions::ExtensionManagementFactory::GetForBrowserContext(profile)
              ->IsOffstoreInstallAllowed(item.GetURL(), item.GetReferrerUrl());
 }
 
-}  // namespace
-
 // Tests can call this method to inject a mock ExtensionInstallPrompt
 // to be used to confirm permissions on a downloaded CRX.
 void SetMockInstallPromptForTesting(
@@ -103,31 +102,6 @@ scoped_refptr<extensions::CrxInstaller> CreateCrxInstaller(
   return installer;
 }
 
-scoped_refptr<extensions::CrxInstaller> OpenChromeExtension(
-    Profile* profile,
-    const DownloadItem& download_item) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  scoped_refptr<extensions::CrxInstaller> installer(
-      CreateCrxInstaller(profile, download_item));
-
-  if (OffStoreInstallAllowedByPrefs(profile, download_item)) {
-    installer->set_off_store_install_allow_reason(
-        extensions::CrxInstaller::OffStoreInstallAllowedBecausePref);
-  }
-
-  if (extensions::UserScript::IsURLUserScript(download_item.GetURL(),
-                                              download_item.GetMimeType())) {
-    installer->InstallUserScript(download_item.GetFullPath(),
-                                 download_item.GetURL());
-  } else {
-    DCHECK(!WebstoreInstaller::GetAssociatedApproval(download_item));
-    installer->InstallCrx(download_item.GetFullPath());
-  }
-
-  return installer;
-}
-
 bool IsExtensionDownload(const DownloadItem& download_item) {
   if (download_item.GetTargetDisposition() ==
       DownloadItem::TARGET_DISPOSITION_PROMPT)

chrome/browser/download/download_crx_util.h

@@ -35,14 +35,6 @@ scoped_refptr<extensions::CrxInstaller> CreateCrxInstaller(
     Profile* profile,
     const download::DownloadItem& download_item);
 
-// Start installing a downloaded item item as a CRX (extension, theme, app,
-// ...).  The installer does work on the file thread, so the installation
-// is not complete when this function returns.  Returns the object managing
-// the installation.
-scoped_refptr<extensions::CrxInstaller> OpenChromeExtension(
-    Profile* profile,
-    const download::DownloadItem& download_item);
-
 // Returns true if this is an extension download. This also considers user
 // scripts to be extension downloads, since we convert those automatically.
 bool IsExtensionDownload(const download::DownloadItem& download_item);
@@ -56,6 +48,12 @@ bool IsTrustedExtensionDownload(Profile* profile,
 std::unique_ptr<base::AutoReset<bool>> OverrideOffstoreInstallAllowedForTesting(
     bool allowed);
 
+// Returns true if an offstore extension download should be allowed to proceed.
+// Takes into consideration what's set in
+// OverrideOffstoreInstallAllowedForTesting
+bool OffStoreInstallAllowedByPrefs(Profile* profile,
+                                   const download::DownloadItem& item);
+
 }  // namespace download_crx_util
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_CRX_UTIL_H_