Update WebstoreInstaller to be ref counted.

chrome/browser/extensions/webstore_installer.cc

Index: chrome/browser/extensions/webstore_installer.cc
diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
index 7131cbf4cde33c1e6fa6e3570c610f5d6247b94d..e61c33d90945e517ff3af4310595b14591c7aad1 100644
--- a/chrome/browser/extensions/webstore_installer.cc
+++ b/chrome/browser/extensions/webstore_installer.cc
@@ -49,8 +49,19 @@ GURL GetWebstoreInstallUrl(
 }  // namespace
 
 
-WebstoreInstaller::WebstoreInstaller(Profile* profile)
-    : profile_(profile) {
+WebstoreInstaller::WebstoreInstaller(Profile* profile,
+                                     Delegate* delegate,
+                                     NavigationController* controller,
+                                     const std::string& id,
+                                     int flags)
+    : profile_(profile),
+      delegate_(delegate),
+      controller_(controller),
+      id_(id),
+      flags_(flags) {
+  download_url_ = GetWebstoreInstallUrl(id, flags & FLAG_INLINE_INSTALL ?
+      kInlineInstallSource : kDefaultInstallSource);
+
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
                  content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
@@ -59,61 +70,33 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile)
 
 WebstoreInstaller::~WebstoreInstaller() {}
 
-struct WebstoreInstaller::PendingInstall {
-  PendingInstall() : delegate(NULL) {}
-  PendingInstall(const std::string& id, const GURL& url, Delegate* delegate)
-      : id(id), download_url(url), delegate(delegate) {}
-  ~PendingInstall() {}
-
-  // The id of the extension.
-  std::string id;
+void WebstoreInstaller::Start() {
+  AddRef();  // Balanced in ReportSuccess and ReportFailure.
 
-  // The gallery download URL for the extension.
-  GURL download_url;
-
-  // The delegate for this install.
-  Delegate* delegate;
-};
-
-void WebstoreInstaller::InstallExtension(
-    const std::string& id, Delegate* delegate, int flags) {
-  if (!Extension::IdIsValid(id)) {
-    ReportFailure(id, kInvalidIdError);
+  if (!Extension::IdIsValid(id_)) {
+    ReportFailure(kInvalidIdError);
     return;
   }
 
-  Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
-  if (!browser) {
-    ReportFailure(id, kNoBrowserError);
-    return;
-  }
+  // The download url for the given extension is now contained in
+  // |download_url|. We will navigate the current tab to this url which will

[Mihai Parparita -not on Chrome, 2011/10/24 22:36:18]

This should be |download_url_|

[jstritar, 2011/10/25 02:45:04]

Done, and updated the comment to make a bit more sense.

+  // result in a download starting. Once completed it will go through the normal
+  // extension install flow.
+
+  // TODO(mihaip): For inline installs, we pretend like the referrer is the
+  // gallery, even though this could be an inline install, in order to pass the
+  // checks in ExtensionService::IsDownloadFromGallery. We should instead pass
+  // the real referrer, track if this is an inline install in the whitelist
+  // entry and look that up when checking that this is a valid download.
+  GURL referrer = controller_->GetActiveEntry()->url();
+  if (flags_ & FLAG_INLINE_INSTALL)
+    referrer = GURL(extension_urls::GetWebstoreItemDetailURLPrefix() + id_);
+
+  controller_->LoadURL(download_url_,
+                       referrer,
+                       content::PAGE_TRANSITION_LINK,
+                       std::string());
 
-  GURL install_url = GetWebstoreInstallUrl(id, flags & FLAG_INLINE_INSTALL ?
-      kInlineInstallSource : kDefaultInstallSource);
-  PendingInstall pending_install = CreatePendingInstall(
-      id, install_url, delegate);
-
-  // The download url for the given |id| is now contained in
-  // |pending_install.download_url|. We navigate the current tab to this url
-  // which will result in a download starting. Once completed it will go through
-  // the normal extension install flow.
-  NavigationController& controller =
-      browser->tabstrip_model()->GetActiveTabContents()->controller();
-
-  // TODO(mihaip, jstritar): For inline installs, we pretend like the referrer
-  // is the gallery, even though this could be an inline install, in order to
-  // pass the checks in ExtensionService::IsDownloadFromGallery. We should
-  // instead pass the real referrer, track if this is an inline install in the
-  // whitelist entry and look that up when checking that this is a valid
-  // download.
-  GURL referrer = controller.GetActiveEntry()->url();
-  if (flags & FLAG_INLINE_INSTALL)
-    referrer = GURL(extension_urls::GetWebstoreItemDetailURLPrefix() + id);
-
-  controller.LoadURL(install_url,
-                     referrer,
-                     content::PAGE_TRANSITION_LINK,
-                     std::string());
 }
 
 void WebstoreInstaller::Observe(int type,
@@ -124,7 +107,8 @@ void WebstoreInstaller::Observe(int type,
       CHECK(profile_->IsSameProfile(content::Source<Profile>(source).ptr()));
       const Extension* extension =
           content::Details<const Extension>(details).ptr();
-      ReportSuccess(extension->id());
+      if (id_ == extension->id())
+        ReportSuccess();
       break;
     }
 
@@ -134,12 +118,10 @@ void WebstoreInstaller::Observe(int type,
       if (!profile_->IsSameProfile(crx_installer->profile()))
         return;
 
-      std::string id = GetPendingInstallId(
-          crx_installer->original_download_url());
       const std::string* error =
           content::Details<const std::string>(details).ptr();
-      if (!id.empty())
-        ReportFailure(id, *error);
+      if (download_url_ == crx_installer->original_download_url())
+        ReportFailure(*error);
       break;
     }
 
@@ -148,55 +130,20 @@ void WebstoreInstaller::Observe(int type,
   }
 }
 
-bool WebstoreInstaller::ClearPendingInstall(
-    const std::string& id, PendingInstall* install) {
-  for (size_t i = 0; i < pending_installs_.size(); ++i) {
-    if (pending_installs_[i].id == id) {
-      *install = pending_installs_[i];
-      pending_installs_.erase(pending_installs_.begin() + i);
-      return true;
-    }
-  }
-  return false;
-}
+void WebstoreInstaller::ReportFailure(const std::string& error) {
+  if (delegate_)
+    delegate_->OnExtensionInstallFailure(id_, error);
 
-const WebstoreInstaller::PendingInstall&
-    WebstoreInstaller::CreatePendingInstall(
-        const std::string& id, GURL install_url, Delegate* delegate) {
-  PendingInstall pending_install(id, install_url, delegate);
-  pending_installs_.push_back(pending_install);
+  delegate_ = NULL;

[Mihai Parparita -not on Chrome, 2011/10/24 22:36:18]

Out of curiosity, why do you need to have delegate_ get nulled out? Is it to
prevent multiple success/failure calls to it? It seems like that shouldn't
happen, so having CHECKs for that is more appropriate (especially since the
installer will delete itself with the Release() calls).

[jstritar, 2011/10/25 02:45:04]

This was to make sure it doesn't get called twice. We shouldn't get multiple
notifications for the same extension install, and the installer should delete
itself... so I'll just get rid of it. 

 
-  return *(pending_installs_.end() - 1);
+  Release();  // Balanced in Start().
 }
 
+void WebstoreInstaller::ReportSuccess() {
+  if (delegate_)
+    delegate_->OnExtensionInstallSuccess(id_);
 
-std::string WebstoreInstaller::GetPendingInstallId(const GURL& url) {
-  if (url.is_empty())
-    return std::string();
-
-  for (size_t i = 0; i < pending_installs_.size(); ++i) {
-    if (pending_installs_[i].download_url == url)
-      return pending_installs_[i].id;
-  }
-
-  return std::string();
-}
-
-void WebstoreInstaller::ReportFailure(
-    const std::string& id, const std::string& error) {
-  PendingInstall install;
-  if (!ClearPendingInstall(id, &install))
-    return;
-
-  if (install.delegate)
-    install.delegate->OnExtensionInstallFailure(id, error);
-}
-
-void WebstoreInstaller::ReportSuccess(const std::string& id) {
-  PendingInstall install;
-  if (!ClearPendingInstall(id, &install))
-    return;
+  delegate_ = NULL;
 
-  if (install.delegate)
-    install.delegate->OnExtensionInstallSuccess(id);
+  Release();  // Balanced in Start().
 }