Mechanical refactor of CrxInstaller signatures. This will make

it easier to add additional configuration features to
CrxInstaller.

Review URL: http://codereview.chromium.org/875003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41503 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 138 insertions, 156 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index e083b60..998b252 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -2452,14 +2452,12 @@ void AutomationProvider::InstallExtension(const FilePath& crx_path,
                                       reply_message);
 
     const FilePath& install_dir = service->install_directory();
-    CrxInstaller::Start(crx_path,
-                        install_dir,
-                        Extension::INTERNAL,
-                        "",  // no expected id
-                        false,  // please do not delete crx file
-                        true,  // privilege increase allowed
-                        service,
-                        NULL);  // silent isntall, no UI
+    scoped_refptr<CrxInstaller> installer(
+        new CrxInstaller(install_dir,
+                         service,
+                         NULL));  // silent install, no UI
+    installer->set_allow_privilege_increase(true);
+    installer->InstallCrx(crx_path);
   } else {
     AutomationMsg_InstallExtension::WriteReplyParams(
         reply_message, AUTOMATION_MSG_EXTENSION_INSTALL_FAILED);
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 423f037..e9ead6c 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -1438,28 +1438,23 @@ void DownloadManager::OpenChromeExtension(const FilePath& full_path,
   ExtensionsService* service = profile_->GetExtensionsService();
   if (service) {
     NotificationService* nservice = NotificationService::current();
-      GURL nonconst_download_url = download_url;
-      nservice->Notify(NotificationType::EXTENSION_READY_FOR_INSTALL,
-                       Source<DownloadManager>(this),
-                       Details<GURL>(&nonconst_download_url));
+    GURL nonconst_download_url = download_url;
+    nservice->Notify(NotificationType::EXTENSION_READY_FOR_INSTALL,
+                     Source<DownloadManager>(this),
+                     Details<GURL>(&nonconst_download_url));
+
+    scoped_refptr<CrxInstaller> installer(
+        new CrxInstaller(service->install_directory(),
+                         service,
+                         new ExtensionInstallUI(profile_)));
+    installer->set_delete_source(true);
+
     if (UserScript::HasUserScriptFileExtension(download_url)) {
-      CrxInstaller::InstallUserScript(
-          full_path,
-          download_url,
-          service->install_directory(),
-          true,  // please delete crx on completion
-          service,
-          new ExtensionInstallUI(profile_));
+      installer->InstallUserScript(full_path, download_url);
     } else {
-      CrxInstaller::Start(
-          full_path,
-          service->install_directory(),
-          Extension::INTERNAL,
-          "",  // no expected id
-          true,  // please delete crx on completion
-          true,  // privilege increase allowed
-          service,
-          new ExtensionInstallUI(profile_));
+      installer->set_allow_privilege_increase(true);
+      installer->set_original_url(download_url);
+      installer->InstallCrx(full_path);
     }
   } else {
     TabContents* contents = NULL;
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index e4c5a93..71397f4 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -32,61 +32,12 @@ namespace {
   }
 }
 
-void CrxInstaller::Start(const FilePath& crx_path,
-                         const FilePath& install_directory,
-                         Extension::Location install_source,
-                         const std::string& expected_id,
-                         bool delete_source,
-                         bool allow_privilege_increase,
-                         ExtensionsService* frontend,
-                         ExtensionInstallUI* client) {
-  // Note: This object manages its own lifetime.
-  scoped_refptr<CrxInstaller> installer(
-      new CrxInstaller(crx_path, install_directory, delete_source, frontend,
-                       client));
-
-  installer->install_source_ = install_source;
-  installer->expected_id_ = expected_id;
-  installer->allow_privilege_increase_ = allow_privilege_increase;
-
-  scoped_refptr<SandboxedExtensionUnpacker> unpacker(
-      new SandboxedExtensionUnpacker(
-          installer->source_file_,
-          g_browser_process->resource_dispatcher_host(),
-          installer.get()));
-
-  ChromeThread::PostTask(
-      ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          unpacker.get(), &SandboxedExtensionUnpacker::Start));
-}
-
-void CrxInstaller::InstallUserScript(const FilePath& source_file,
-                                     const GURL& original_url,
-                                     const FilePath& install_directory,
-                                     bool delete_source,
-                                     ExtensionsService* frontend,
-                                     ExtensionInstallUI* client) {
-  scoped_refptr<CrxInstaller> installer(
-      new CrxInstaller(source_file, install_directory, delete_source, frontend,
-                       client));
-  installer->original_url_ = original_url;
-
-  ChromeThread::PostTask(
-      ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(installer.get(),
-                        &CrxInstaller::ConvertUserScriptOnFileThread));
-}
-
-CrxInstaller::CrxInstaller(const FilePath& source_file,
-                           const FilePath& install_directory,
-                           bool delete_source,
+CrxInstaller::CrxInstaller(const FilePath& install_directory,
                            ExtensionsService* frontend,
                            ExtensionInstallUI* client)
-    : source_file_(source_file),
-      install_directory_(install_directory),
+    : install_directory_(install_directory),
       install_source_(Extension::INTERNAL),
-      delete_source_(delete_source),
+      delete_source_(false),
       allow_privilege_increase_(false),
       create_app_shortcut_(false),
       frontend_(frontend),
@@ -111,6 +62,33 @@ CrxInstaller::~CrxInstaller() {
   }
 }
 
+void CrxInstaller::InstallCrx(const FilePath& source_file) {
+  source_file_ = source_file;
+
+  scoped_refptr<SandboxedExtensionUnpacker> unpacker(
+      new SandboxedExtensionUnpacker(
+          source_file,
+          g_browser_process->resource_dispatcher_host(),
+          this));
+
+  ChromeThread::PostTask(
+      ChromeThread::FILE, FROM_HERE,
+      NewRunnableMethod(
+          unpacker.get(), &SandboxedExtensionUnpacker::Start));
+}
+
+void CrxInstaller::InstallUserScript(const FilePath& source_file,
+                                     const GURL& original_url) {
+  DCHECK(!original_url.is_empty());
+
+  source_file_ = source_file;
+  original_url_ = original_url;
+
+  ChromeThread::PostTask(
+      ChromeThread::FILE, FROM_HERE,
+      NewRunnableMethod(this, &CrxInstaller::ConvertUserScriptOnFileThread));
+}
+
 void CrxInstaller::ConvertUserScriptOnFileThread() {
   std::string error;
   Extension* extension = ConvertUserScriptToExtension(source_file_,
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index 0b19fef..e2d4c02 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -33,54 +33,57 @@ class SkBitmap;
 // Additionally, we hold a reference to our own client so that it lives at least
 // long enough to receive the result of unpacking.
 //
-// TODO(aa): Pull out a frontend interface for testing?
+// IMPORTANT: Callers should keep a reference to a CrxInstaller while they are
+// working with it, eg:
+//
+// scoped_refptr<CrxInstaller> installer(new CrxInstaller(...));
+// installer->set_foo();
+// installer->set_bar();
+// installer->InstallCrx(...);
 class CrxInstaller
     : public SandboxedExtensionUnpackerClient,
       public ExtensionInstallUI::Delegate {
  public:
-  // Starts the installation of the crx file in |source_file| into
-  // |install_directory|.
-  //
-  // Other params:
-  //  install_source: The source of the install (external, --load-extension, etc
-  //  expected_id: Optional. If the caller knows what the ID of this extension
-  //               should be after unpacking, it can be specified here as a
-  //               sanity check.
-  //  delete_crx: Whether the crx should be deleted on completion.
-  //  frontend: The ExtensionsService to report the successfully installed
-  //            extension to.
-  //  client: Optional. If specified, will be used to confirm installation and
-  //          also notified of success/fail. Note that we hold a reference to
-  //          this, so it can outlive its creator (eg the UI).
-  static void Start(const FilePath& source_file,
-                    const FilePath& install_directory,
-                    Extension::Location install_source,
-                    const std::string& expected_id,
-                    bool delete_source,
-                    bool allow_privilege_increase,
-                    ExtensionsService* frontend,
-                    ExtensionInstallUI* client);
-
-  // Starts the installation of the user script file in |source_file| into
-  // |install_directory|. The script will be converted to an extension.
-  // See Start() for argument descriptions.
-  static void InstallUserScript(const FilePath& source_file,
-                                const GURL& original_url,
-                                const FilePath& install_directory,
-                                bool delete_source,
-                                ExtensionsService* frontend,
-                                ExtensionInstallUI* client);
+  // Constructor.  Extensions will be unpacked to |install_directory|.
+  // Extension objects will be sent to |frontend|, and any UI will be shown
+  // via |client|. For silent install, pass NULL for |client|.
+  CrxInstaller(const FilePath& install_directory,
+               ExtensionsService* frontend,
+               ExtensionInstallUI* client);
+
+  // Install the crx in |source_file|. Note that this will most likely
+  // complete asynchronously.
+  void InstallCrx(const FilePath& source_file);
+
+  // Install the user script in |source_file|. Note that this will most likely
+  // complete asynchronously.
+  void InstallUserScript(const FilePath& source_file,
+                         const GURL& original_url);
 
   // ExtensionInstallUI::Delegate
   virtual void InstallUIProceed(bool create_app_shortcut);
   virtual void InstallUIAbort();
 
+  const GURL& original_url() { return original_url_; }
+  void set_original_url(const GURL& val) { original_url_ = val; }
+
+  Extension::Location install_source() { return install_source_; }
+  void set_install_source(Extension::Location source) {
+    install_source_ = source;
+  }
+
+  const std::string& expected_id() { return expected_id_; }
+  void set_expected_id(const std::string& val) { expected_id_ = val; }
+
+  bool delete_source() { return delete_source_; }
+  void set_delete_source(bool val) { delete_source_ = val; }
+
+  bool allow_privilege_increase() { return allow_privilege_increase_; }
+  void set_allow_privilege_increase(bool val) {
+    allow_privilege_increase_ = val;
+  }
+
  private:
-  CrxInstaller(const FilePath& source_file,
-               const FilePath& install_directory,
-               bool delete_source,
-               ExtensionsService* frontend,
-               ExtensionInstallUI* client);
   ~CrxInstaller();
 
   // Converts the source user script to an extension.
@@ -111,7 +114,7 @@ class CrxInstaller
   // The file we're installing.
   FilePath source_file_;
 
-  // The URL the file was downloaded from. Only used for user scripts.
+  // The URL the file was downloaded from.
   GURL original_url_;
 
   // The directory extensions are installed to.
@@ -119,7 +122,7 @@ class CrxInstaller
 
   // The location the installation came from (bundled with Chromium, registry,
   // manual install, etc). This metadata is saved with the installation if
-  // successful.
+  // successful. Defaults to INTERNAL.
   Extension::Location install_source_;
 
   // For updates and external installs we have an ID we're expecting the
@@ -131,11 +134,16 @@ class CrxInstaller
   // allowed.
   bool extensions_enabled_;
 
-  // Whether we're supposed to delete the source file on destruction.
+  // Whether we're supposed to delete the source file on destruction. Defaults
+  // to false.
   bool delete_source_;
 
   // Whether privileges should be allowed to silently increaes from any
-  // previously installed version of the extension.
+  // previously installed version of the extension. This is used for things
+  // like external extensions, where extensions come with third-party software
+  // or are distributed by the network administrator. There is no UI shown
+  // for these extensions, so there shouldn't be UI for privilege increase,
+  // either. Defaults to false.
   bool allow_privilege_increase_;
 
   // Whether to create an app shortcut after successful installation. This is
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index 9f7cdcf..7041fab 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -128,15 +128,14 @@ bool ExtensionBrowserTest::InstallOrUpdateExtension(const std::string& id,
                   NotificationService::AllSources());
     registrar.Add(this, NotificationType::EXTENSION_INSTALL_ERROR,
                   NotificationService::AllSources());
-    CrxInstaller::Start(
-        path,
-        service->install_directory(),
-        Extension::INTERNAL,
-        id,
-        false,  // do not delete after install
-        id == "" ? true : false,  // only allow privilege increase for installs
-        service,
-        should_cancel ? new MockAbortExtensionInstallUI() : NULL);
+
+    scoped_refptr<CrxInstaller> installer(
+        new CrxInstaller(
+            service->install_directory(),
+            service,
+            should_cancel ? new MockAbortExtensionInstallUI() : NULL));
+    installer->set_expected_id(id);
+    installer->InstallCrx(path);
 
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE, new MessageLoop::QuitTask, kTimeoutMs);
diff --git a/chrome/browser/extensions/extension_browsertest.h b/chrome/browser/extensions/extension_browsertest.h
index 8a34903..51f5c59 100644
--- a/chrome/browser/extensions/extension_browsertest.h
+++ b/chrome/browser/extensions/extension_browsertest.h
@@ -36,8 +36,8 @@ class ExtensionBrowserTest
     return InstallOrUpdateExtension("", path, false, expected_change);
   }
 
-  // Same as above but passes an id to CrxInstaller::Start and does not allow
-  // a privilege increase.
+  // Same as above but passes an id to CrxInstaller and does not allow a
+  // privilege increase.
   bool UpdateExtension(const std::string& id, const FilePath& path,
                        int expected_change) {
     return InstallOrUpdateExtension(id, path, false, expected_change);
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index fc2dadf..8870a06 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -193,12 +193,12 @@ void ExtensionsService::Init() {
 }
 
 void ExtensionsService::InstallExtension(const FilePath& extension_path) {
-  CrxInstaller::Start(extension_path, install_directory_, Extension::INTERNAL,
-                      "",   // no expected id
-                      false,  // don't delete crx when complete
-                      true,  // allow privilege increase
-                      this,
-                      NULL);  // no client (silent install)
+  scoped_refptr<CrxInstaller> installer(
+      new CrxInstaller(install_directory_,
+                       this,  // frontend
+                       NULL));  // no client (silent install)
+  installer->set_allow_privilege_increase(true);
+  installer->InstallCrx(extension_path);
 }
 
 void ExtensionsService::UpdateExtension(const std::string& id,
@@ -209,12 +209,13 @@ void ExtensionsService::UpdateExtension(const std::string& id,
     return;
   }
 
-  CrxInstaller::Start(extension_path, install_directory_, Extension::INTERNAL,
-                      id,
-                      true,  // delete crx when complete
-                      false,  // do not allow upgrade of privileges
-                      this,
-                      NULL);  // no client (silent install)
+  scoped_refptr<CrxInstaller> installer(
+      new CrxInstaller(install_directory_,
+                       this,  // frontend
+                       NULL));  // no client (silent install)
+  installer->set_expected_id(id);
+  installer->set_delete_source(true);
+  installer->InstallCrx(extension_path);
 }
 
 void ExtensionsService::ReloadExtension(const std::string& extension_id) {
@@ -905,11 +906,14 @@ void ExtensionsService::OnExternalExtensionFound(const std::string& id,
     }
   }
 
-  CrxInstaller::Start(path, install_directory_, location, id,
-                      false,  // don't delete crx when complete
-                      true,  // allow privilege increase
-                      this,
-                      NULL);  // no client (silent install)
+  scoped_refptr<CrxInstaller> installer(
+      new CrxInstaller(install_directory_,
+                       this,  // frontend
+                       NULL));  // no client (silent install)
+  installer->set_install_source(location);
+  installer->set_expected_id(id);
+  installer->set_allow_privilege_increase(true);
+  installer->InstallCrx(path);
 }
 
 void ExtensionsService::ReportExtensionLoadError(
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index e866d26..e5cf7b4 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -798,13 +798,13 @@ TEST_F(ExtensionsServiceTest, InstallUserScript) {
              .AppendASCII("user_script_basic.user.js");
 
   ASSERT_TRUE(file_util::PathExists(path));
-  CrxInstaller::InstallUserScript(
+  scoped_refptr<CrxInstaller> installer(
+      new CrxInstaller(service_->install_directory(),
+                       service_,
+                       NULL));  // silent install
+  installer->InstallUserScript(
       path,
-      GURL("http://www.aaronboodman.com/scripts/user_script_basic.user.js"),
-      service_->install_directory(),
-      false,  // do not delete source
-      service_,
-      NULL);  // install UI
+      GURL("http://www.aaronboodman.com/scripts/user_script_basic.user.js"));
 
   loop_.RunAllPending();
   std::vector<std::string> errors = GetErrors();