Fix TODOs: Get rid of ExtensionService::InstallExtension().

Also use a single delete function when removing extension files.

BUG=None
TEST=ExtensionServiceTest.*

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

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

4 files changed, 31 insertions, 46 deletions

diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 9a7cda5..1f494b0 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -39,12 +39,6 @@
 
 namespace {
 
-// Helper function to delete files. This is used to avoid ugly casts which
-// would be necessary with PostMessage since file_util::Delete is overloaded.
-static void DeleteFileHelper(const FilePath& path, bool recursive) {
-  file_util::Delete(path, recursive);
-}
-
 struct WhitelistedInstallData {
   WhitelistedInstallData() {}
   std::set<std::string> ids;
@@ -100,13 +94,15 @@ CrxInstaller::~CrxInstaller() {
   if (!temp_dir_.value().empty()) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        NewRunnableFunction(&DeleteFileHelper, temp_dir_, true));
+        NewRunnableFunction(
+            &extension_file_util::DeleteFile, temp_dir_, true));
   }
 
   if (delete_source_) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        NewRunnableFunction(&DeleteFileHelper, source_file_, false));
+        NewRunnableFunction(
+            &extension_file_util::DeleteFile, source_file_, false));
   }
 
   // Make sure the UI is deleted on the ui thread.
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index e2572b2..e15fa03 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -508,22 +508,6 @@ void ExtensionService::Init() {
   GarbageCollectExtensions();
 }
 
-void ExtensionService::InstallExtension(const FilePath& extension_path) {
-  scoped_refptr<CrxInstaller> installer(
-      new CrxInstaller(this,  // frontend
-                       NULL));  // no client (silent install)
-  installer->InstallCrx(extension_path);
-}
-
-namespace {
-  // TODO(akalin): Put this somewhere where both crx_installer.cc and
-  // this file can use it.
-  void DeleteFileHelper(const FilePath& path, bool recursive) {
-    CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    file_util::Delete(path, recursive);
-  }
-}  // namespace
-
 void ExtensionService::UpdateExtension(const std::string& id,
                                        const FilePath& extension_path,
                                        const GURL& download_url) {
@@ -540,7 +524,8 @@ void ExtensionService::UpdateExtension(const std::string& id,
     // that would do it for us.
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        NewRunnableFunction(&DeleteFileHelper, extension_path, false));
+        NewRunnableFunction(
+            extension_file_util::DeleteFile, extension_path, false));
     return;
   }
 
@@ -1560,7 +1545,7 @@ void ExtensionService::OnExtensionInstalled(const Extension* extension) {
       // load it.
       BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
-          NewRunnableFunction(&DeleteFileHelper, extension->path(), true));
+          NewRunnableFunction(&extension_file_util::DeleteFile, extension->path(), true));
       return;
     }
 
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index ac56b4f..547ae0b 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -221,14 +221,6 @@ class ExtensionService
   // not include terminated extensions.
   virtual const Extension* GetTerminatedExtension(const std::string& id);
 
-  // Install the extension file at |extension_path|.  Will install as an
-  // update if an older version is already installed.
-  // For fresh installs, this method also causes the extension to be
-  // immediately loaded.
-  // TODO(aa): This method can be removed. It is only used by the unit tests,
-  // and they could use CrxInstaller directly instead.
-  void InstallExtension(const FilePath& extension_path);
-
   // Updates a currently-installed extension with the contents from
   // |extension_path|.
   // TODO(aa): This method can be removed. ExtensionUpdater could use
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index b2b1623..ddef3cc 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -548,10 +548,22 @@ class ExtensionServiceTest
       PackAndInstallExtension(dir_path, FilePath(), should_succeed);
   }
 
+  // Create a CrxInstaller and start installation. To allow the install
+  // to happen, use loop_.RunAllPending();. Most tests will not use this
+  // method directly.  Instead, use InstallExtension(), which waits for
+  // the crx to be installed and does extra error checking.
+  void StartCrxInstall(const FilePath& crx_path) {
+    ASSERT_TRUE(file_util::PathExists(crx_path));
+    scoped_refptr<CrxInstaller> installer(
+        new CrxInstaller(service_,  // frontend
+                         NULL));  // no client (silent install)
+    installer->InstallCrx(crx_path);
+  }
+
   void InstallExtension(const FilePath& path,
                         bool should_succeed) {
     ASSERT_TRUE(file_util::PathExists(path));
-    service_->InstallExtension(path);
+    StartCrxInstall(path);
     loop_.RunAllPending();
     std::vector<std::string> errors = GetErrors();
     if (should_succeed) {
@@ -1783,7 +1795,7 @@ TEST_F(ExtensionServiceTest, Reinstall) {
 
   // A simple extension that should install without error.
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_TRUE(installed_);
@@ -1798,7 +1810,7 @@ TEST_F(ExtensionServiceTest, Reinstall) {
   ExtensionErrorReporter::GetInstance()->ClearErrors();
 
   // Reinstall the same version, it should overwrite the previous one.
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_TRUE(installed_);
@@ -1817,7 +1829,7 @@ TEST_F(ExtensionServiceTest, UpgradeSignedGood) {
   extensions_path = extensions_path.AppendASCII("extensions");
 
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_TRUE(installed_);
@@ -1827,7 +1839,7 @@ TEST_F(ExtensionServiceTest, UpgradeSignedGood) {
 
   // Upgrade to version 2.0
   path = extensions_path.AppendASCII("good2.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_TRUE(installed_);
@@ -1844,7 +1856,7 @@ TEST_F(ExtensionServiceTest, UpgradeSignedBad) {
   extensions_path = extensions_path.AppendASCII("extensions");
 
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_TRUE(installed_);
@@ -1854,8 +1866,8 @@ TEST_F(ExtensionServiceTest, UpgradeSignedBad) {
 
   // Try upgrading with a bad signature. This should fail during the unpack,
   // because the key will not match the signature.
-  path = extensions_path.AppendASCII("good2_bad_signature.crx");
-  service_->InstallExtension(path);
+  path = extensions_path.AppendASCII("bad_signature.crx");
+  StartCrxInstall(path);
   loop_.RunAllPending();
 
   ASSERT_FALSE(installed_);
@@ -2362,7 +2374,7 @@ TEST_F(ExtensionServiceTest, BlacklistedExtensionWillNotInstall) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
   EXPECT_EQ(0u, service_->extensions()->size());
   ValidateBooleanPref(good_crx, "blacklist", true);
@@ -2458,7 +2470,7 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyWillNotInstall) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
   EXPECT_EQ(0u, service_->extensions()->size());
 
@@ -2466,7 +2478,7 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyWillNotInstall) {
   whitelist->Append(Value::CreateStringValue(good_crx));
 
   // Ensure we can now install good_crx.
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
   EXPECT_EQ(1u, service_->extensions()->size());
 }
@@ -2480,7 +2492,7 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyRemovedIfRunning) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
   FilePath path = extensions_path.AppendASCII("good.crx");
-  service_->InstallExtension(path);
+  StartCrxInstall(path);
   loop_.RunAllPending();
   EXPECT_EQ(1u, service_->extensions()->size());