Also fixes a bug where an externally installed

extension was loaded twice the first run after
installation.

I still ExtensionServiceBackend is begging to be split apart into individual tasks, this was just a quick spruce-up while I was in the file.

The main things I changed are:

* Moved extension_path_ from a member back into parameters where relevant. This just didn't feel right as state to me. It isn't relevant to all the methods the way alert_on_error_ and frontend_ are. This created the majority of the noise in the change, but is the least important.

* Renamed OnExtensionsLoadedFromDirectory to OnExtensionsLoaded since it is called in response to LoadExtension() too.

* Made install_directory_ be a parameter to backend's constructor instead of passed into each appropriate method. Again, this felt more appropriate because the install directory is relevant to the whole object.

* Hoisted checking for uninstalled external extensions up out of LoadExtension() into LoadExtensionsFromInstallDirectory(). It wasn't doing any harm in LoadExtension(), but it didn't seem relevant to other places where LoadExtension() is used (for example --load-extension and --install-extension).

* Hoisted installation success notification up out of InstallOrUpdateExtension() into InstallExtension(). This prevented having to pass around a be_noisy argument since InstallExtension() is only used for handling --install-extension, and InstallOrUpdateExtension() is just the private impl.

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

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

3 files changed, 194 insertions, 197 deletions

diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 3271920..5611594 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -68,8 +68,8 @@ const char kExternalInstallFile[] = "EXTERNAL_INSTALL";
 ExtensionsService::ExtensionsService(Profile* profile,
                                      UserScriptMaster* user_script_master)
     : message_loop_(MessageLoop::current()),
-      backend_(new ExtensionsServiceBackend),
       install_directory_(profile->GetPath().AppendASCII(kInstallDirectoryName)),
+      backend_(new ExtensionsServiceBackend(install_directory_)),
       profile_(profile),
       user_script_master_(user_script_master) {
 }
@@ -83,14 +83,12 @@ ExtensionsService::~ExtensionsService() {
 
 bool ExtensionsService::Init() {
 #if defined(OS_WIN)
-
   // TODO(port): ExtensionsServiceBackend::CheckForExternalUpdates depends on
   // the Windows registry.
   // TODO(erikkay): Should we monitor the registry during run as well?
   g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(backend_.get(),
           &ExtensionsServiceBackend::CheckForExternalUpdates,
-          install_directory_,
           scoped_refptr<ExtensionsServiceFrontendInterface>(this)));
 #endif
 
@@ -99,8 +97,7 @@ bool ExtensionsService::Init() {
   // from the frontend interface.
   g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(backend_.get(),
-          &ExtensionsServiceBackend::LoadExtensionsFromDirectory,
-          install_directory_,
+          &ExtensionsServiceBackend::LoadExtensionsFromInstallDirectory,
           scoped_refptr<ExtensionsServiceFrontendInterface>(this)));
 
   return true;
@@ -125,8 +122,6 @@ void ExtensionsService::InstallExtension(const FilePath& extension_path) {
       NewRunnableMethod(backend_.get(),
           &ExtensionsServiceBackend::InstallExtension,
           extension_path,
-          install_directory_,
-          true,  // alert_on_error
           scoped_refptr<ExtensionsServiceFrontendInterface>(this)));
 }
 
@@ -141,7 +136,7 @@ void ExtensionsService::LoadExtension(const FilePath& extension_path) {
           scoped_refptr<ExtensionsServiceFrontendInterface>(this)));
 }
 
-void ExtensionsService::OnExtensionsLoadedFromDirectory(
+void ExtensionsService::OnExtensionsLoaded(
     ExtensionList* new_extensions) {
   extensions_.insert(extensions_.end(), new_extensions->begin(),
                      new_extensions->end());
@@ -168,7 +163,8 @@ void ExtensionsService::OnExtensionsLoadedFromDirectory(
     }
   }
 
-  // Tell UserScriptMaster to kick off the first scan.
+  // Since user scripts may have changed, tell UserScriptMaster to kick off
+  // a scan.
   user_script_master_->StartScan();
 
   NotificationService::current()->Notify(
@@ -191,21 +187,19 @@ void ExtensionsService::OnExtensionInstalled(FilePath path, bool update) {
 
 // ExtensionsServicesBackend
 
-void ExtensionsServiceBackend::LoadExtensionsFromDirectory(
-    const FilePath& path_in,
+void ExtensionsServiceBackend::LoadExtensionsFromInstallDirectory(
     scoped_refptr<ExtensionsServiceFrontendInterface> frontend) {
-  FilePath path = path_in;
   frontend_ = frontend;
   alert_on_error_ = false;
 
-  if (!file_util::AbsolutePath(&path))
+  if (!file_util::AbsolutePath(&install_directory_))
     NOTREACHED();
 
   scoped_ptr<ExtensionList> extensions(new ExtensionList);
 
   // Create the <Profile>/Extensions directory if it doesn't exist.
-  if (!file_util::DirectoryExists(path)) {
-    file_util::CreateDirectory(path);
+  if (!file_util::DirectoryExists(install_directory_)) {
+    file_util::CreateDirectory(install_directory_);
     LOG(INFO) << "Created Extensions directory.  No extensions to install.";
     ReportExtensionsLoaded(extensions.release());
     return;
@@ -215,12 +209,25 @@ void ExtensionsServiceBackend::LoadExtensionsFromDirectory(
 
   // Find all child directories in the install directory and load their
   // manifests. Post errors and results to the frontend.
-  file_util::FileEnumerator enumerator(path,
+  file_util::FileEnumerator enumerator(install_directory_,
                                        false,  // not recursive
                                        file_util::FileEnumerator::DIRECTORIES);
-  for (extension_path_ = enumerator.Next(); !extension_path_.value().empty();
-       extension_path_ = enumerator.Next()) {
-    Extension* extension = LoadExtensionCurrentVersion();
+  FilePath extension_path;
+  for (extension_path = enumerator.Next(); !extension_path.value().empty();
+       extension_path = enumerator.Next()) {
+    std::string extension_id = WideToASCII(
+        extension_path.BaseName().ToWStringHack());
+    if (CheckExternalUninstall(extension_path, extension_id)) {
+      // TODO(erikkay): Possibly defer this operation to avoid slowing initial
+      // load of extensions.
+      UninstallExtension(extension_path);
+
+      // No error needs to be reported.  The extension effectively doesn't
+      // exist.
+      continue;
+    }
+
+    Extension* extension = LoadExtensionCurrentVersion(extension_path);
     if (extension)
       extensions->push_back(extension);
   }
@@ -237,14 +244,14 @@ void ExtensionsServiceBackend::LoadSingleExtension(
   // Explicit UI loads are always noisy.
   alert_on_error_ = true;
 
-  extension_path_ = path_in;
-  if (!file_util::AbsolutePath(&extension_path_))
+  FilePath extension_path = path_in;
+  if (!file_util::AbsolutePath(&extension_path))
     NOTREACHED();
 
   LOG(INFO) << "Loading single extension from " <<
-      WideToASCII(extension_path_.BaseName().ToWStringHack());
+      WideToASCII(extension_path.BaseName().ToWStringHack());
 
-  Extension* extension = LoadExtension();
+  Extension* extension = LoadExtension(extension_path);
   if (extension) {
     ExtensionList* extensions = new ExtensionList;
     extensions->push_back(extension);
@@ -252,27 +259,29 @@ void ExtensionsServiceBackend::LoadSingleExtension(
   }
 }
 
-Extension* ExtensionsServiceBackend::LoadExtensionCurrentVersion() {
+Extension* ExtensionsServiceBackend::LoadExtensionCurrentVersion(
+    const FilePath& extension_path) {
   std::string version_str;
-  if (!ReadCurrentVersion(extension_path_, &version_str)) {
-    ReportExtensionLoadError(StringPrintf("Could not read '%s' file.",
-        ExtensionsService::kCurrentVersionFileName));
+  if (!ReadCurrentVersion(extension_path, &version_str)) {
+    ReportExtensionLoadError(extension_path,
+        StringPrintf("Could not read '%s' file.",
+            ExtensionsService::kCurrentVersionFileName));
     return NULL;
   }
 
   LOG(INFO) << "  " <<
-      WideToASCII(extension_path_.BaseName().ToWStringHack()) <<
+      WideToASCII(extension_path.BaseName().ToWStringHack()) <<
       " version: " << version_str;
 
-  extension_path_ = extension_path_.AppendASCII(version_str);
-  return LoadExtension();
+  return LoadExtension(extension_path.AppendASCII(version_str));
 }
 
-Extension* ExtensionsServiceBackend::LoadExtension() {
+Extension* ExtensionsServiceBackend::LoadExtension(
+    const FilePath& extension_path) {
   FilePath manifest_path =
-      extension_path_.AppendASCII(Extension::kManifestFilename);
+      extension_path.AppendASCII(Extension::kManifestFilename);
   if (!file_util::PathExists(manifest_path)) {
-    ReportExtensionLoadError(Extension::kInvalidManifestError);
+    ReportExtensionLoadError(extension_path, Extension::kInvalidManifestError);
     return NULL;
   }
 
@@ -280,29 +289,19 @@ Extension* ExtensionsServiceBackend::LoadExtension() {
   std::string error;
   scoped_ptr<Value> root(serializer.Deserialize(&error));
   if (!root.get()) {
-    ReportExtensionLoadError(error);
+    ReportExtensionLoadError(extension_path, error);
     return NULL;
   }
 
   if (!root->IsType(Value::TYPE_DICTIONARY)) {
-    ReportExtensionLoadError(Extension::kInvalidManifestError);
+    ReportExtensionLoadError(extension_path, Extension::kInvalidManifestError);
     return NULL;
   }
 
-  scoped_ptr<Extension> extension(new Extension(extension_path_));
+  scoped_ptr<Extension> extension(new Extension(extension_path));
   if (!extension->InitFromValue(*static_cast<DictionaryValue*>(root.get()),
                                 &error)) {
-    ReportExtensionLoadError(error);
-    return NULL;
-  }
-
-  if (CheckExternalUninstall(extension_path_, extension->id())) {
-
-    // TODO(erikkay): Possibly defer this operation to avoid slowing initial
-    // load of extensions.
-    UninstallExtension(extension_path_);
-
-    // No error needs to be reported.  The extension effectively doesn't exist.
+    ReportExtensionLoadError(extension_path, error);
     return NULL;
   }
 
@@ -311,7 +310,7 @@ Extension* ExtensionsServiceBackend::LoadExtension() {
        extension->content_scripts().begin();
        iter != extension->content_scripts().end(); ++iter) {
     if (!file_util::PathExists(iter->path())) {
-      ReportExtensionLoadError(
+      ReportExtensionLoadError(extension_path,
           StringPrintf("Could not load content script '%s'.",
                        WideToUTF8(iter->path().ToWStringHack()).c_str()));
       return NULL;
@@ -322,9 +321,9 @@ Extension* ExtensionsServiceBackend::LoadExtension() {
 }
 
 void ExtensionsServiceBackend::ReportExtensionLoadError(
-    const std::string &error) {
+    const FilePath& extension_path, const std::string &error) {
   // TODO(port): note that this isn't guaranteed to work properly on Linux.
-  std::string path_str = WideToASCII(extension_path_.ToWStringHack());
+  std::string path_str = WideToASCII(extension_path.ToWStringHack());
   std::string message = StringPrintf("Could not load extension from '%s'. %s",
                                      path_str.c_str(), error.c_str());
   ExtensionErrorReporter::GetInstance()->ReportError(message, alert_on_error_);
@@ -334,17 +333,18 @@ void ExtensionsServiceBackend::ReportExtensionsLoaded(
     ExtensionList* extensions) {
   frontend_->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
       frontend_,
-      &ExtensionsServiceFrontendInterface::OnExtensionsLoadedFromDirectory,
+      &ExtensionsServiceFrontendInterface::OnExtensionsLoaded,
       extensions));
 }
 
 // The extension file format is a header, followed by the manifest, followed
 // by the zip file.  The header is a magic number, a version, the size of the
 // header, and the size of the manifest.  These ints are 4 byte little endian.
-DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
-  ScopedStdioHandle file(file_util::OpenFile(extension_path_, "rb"));
+DictionaryValue* ExtensionsServiceBackend::ReadManifest(
+    const FilePath& extension_path) {
+  ScopedStdioHandle file(file_util::OpenFile(extension_path, "rb"));
   if (!file.get()) {
-    ReportExtensionInstallError("no such extension file");
+    ReportExtensionInstallError(extension_path, "no such extension file");
     return NULL;
   }
 
@@ -358,15 +358,15 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
   // on a little endian machine with 4 byte alignment.
   len = fread(&header, 1, sizeof(ExtensionHeader), file.get());
   if (len < sizeof(ExtensionHeader)) {
-    ReportExtensionInstallError("invalid extension header");
+    ReportExtensionInstallError(extension_path, "invalid extension header");
     return NULL;
   }
   if (strncmp(kExtensionFileMagic, header.magic, sizeof(header.magic))) {
-    ReportExtensionInstallError("bad magic number");
+    ReportExtensionInstallError(extension_path, "bad magic number");
     return NULL;
   }
   if (header.version != Extension::kExpectedFormatVersion) {
-    ReportExtensionInstallError("bad version number");
+    ReportExtensionInstallError(extension_path, "bad version number");
     return NULL;
   }
   if (header.header_size > sizeof(ExtensionHeader))
@@ -389,11 +389,12 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
   std::string error;
   scoped_ptr<Value> val(json.Deserialize(&error));
   if (!val.get()) {
-    ReportExtensionInstallError(error);
+    ReportExtensionInstallError(extension_path, error);
     return NULL;
   }
   if (!val->IsType(Value::TYPE_DICTIONARY)) {
-    ReportExtensionInstallError("manifest isn't a JSON dictionary");
+    ReportExtensionInstallError(extension_path,
+                                "manifest isn't a JSON dictionary");
     return NULL;
   }
   DictionaryValue* manifest = static_cast<DictionaryValue*>(val.get());
@@ -403,14 +404,14 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
   // work.
   std::string id;
   if (!manifest->GetString(Extension::kIdKey, &id)) {
-    ReportExtensionInstallError("missing id key");
+    ReportExtensionInstallError(extension_path, "missing id key");
     return NULL;
   }
   FilePath dest_dir = install_directory_.AppendASCII(id.c_str());
   if (file_util::PathExists(dest_dir)) {
     std::string version;
     if (!manifest->GetString(Extension::kVersionKey, &version)) {
-      ReportExtensionInstallError("missing version key");
+      ReportExtensionInstallError(extension_path, "missing version key");
       return NULL;
     }
     std::string current_version;
@@ -422,11 +423,11 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
 
   std::string zip_hash;
   if (!manifest->GetString(Extension::kZipHashKey, &zip_hash)) {
-    ReportExtensionInstallError("missing zip_hash key");
+    ReportExtensionInstallError(extension_path, "missing zip_hash key");
     return NULL;
   }
   if (zip_hash.size() != kZipHashHexBytes) {
-    ReportExtensionInstallError("invalid zip_hash key");
+    ReportExtensionInstallError(extension_path, "invalid zip_hash key");
     return NULL;
   }
 
@@ -443,16 +444,17 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest() {
 
   std::vector<uint8> zip_hash_bytes;
   if (!HexStringToBytes(zip_hash, &zip_hash_bytes)) {
-    ReportExtensionInstallError("invalid zip_hash key");
+    ReportExtensionInstallError(extension_path, "invalid zip_hash key");
     return NULL;
   }
   if (zip_hash_bytes.size() != kZipHashBytes) {
-    ReportExtensionInstallError("invalid zip_hash key");
+    ReportExtensionInstallError(extension_path, "invalid zip_hash key");
     return NULL;
   }
   for (size_t i = 0; i < kZipHashBytes; ++i) {
     if (zip_hash_bytes[i] != hash[i]) {
-      ReportExtensionInstallError("zip_hash key didn't match zip hash");
+      ReportExtensionInstallError(extension_path,
+                                  "zip_hash key didn't match zip hash");
       return NULL;
     }
   }
@@ -487,14 +489,13 @@ bool ExtensionsServiceBackend::CheckCurrentVersion(
   scoped_ptr<Version> new_version(
       Version::GetVersionFromString(new_version_str));
   if (current_version->CompareTo(*new_version) >= 0) {
-
     // Verify that the directory actually exists.  If it doesn't we'll return
     // true so that the install code will repair the broken installation.
     // TODO(erikkay): A further step would be to verify that the extension
     // has actually loaded successfully.
     FilePath version_dir = dest_dir.AppendASCII(current_version_str);
     if (file_util::PathExists(version_dir)) {
-      ReportExtensionInstallError(
+      ReportExtensionInstallError(dest_dir, 
           "Existing version is already up to date.");
       return false;
     }
@@ -506,11 +507,10 @@ bool ExtensionsServiceBackend::InstallDirSafely(const FilePath& source_dir,
                                                 const FilePath& dest_dir) {
 
   if (file_util::PathExists(dest_dir)) {
-
     // By the time we get here, it should be safe to assume that this directory
     // is not currently in use (it's not the current active version).
     if (!file_util::Delete(dest_dir, true)) {
-      ReportExtensionInstallError(
+      ReportExtensionInstallError(source_dir,
           "Can't delete existing version directory.");
       return false;
     }
@@ -518,13 +518,15 @@ bool ExtensionsServiceBackend::InstallDirSafely(const FilePath& source_dir,
     FilePath parent = dest_dir.DirName();
     if (!file_util::DirectoryExists(parent)) {
       if (!file_util::CreateDirectory(parent)) {
-        ReportExtensionInstallError("Couldn't create extension directory.");
+        ReportExtensionInstallError(source_dir,
+                                    "Couldn't create extension directory.");
         return false;
       }
     }
   }
   if (!file_util::Move(source_dir, dest_dir)) {
-    ReportExtensionInstallError("Couldn't move temporary directory.");
+    ReportExtensionInstallError(source_dir,
+                                "Couldn't move temporary directory.");
     return false;
   }
 
@@ -541,13 +543,15 @@ bool ExtensionsServiceBackend::SetCurrentVersion(const FilePath& dest_dir,
     current_version.InsertBeforeExtension(FILE_PATH_LITERAL("_old"));
   if (file_util::PathExists(current_version_old)) {
     if (!file_util::Delete(current_version_old, false)) {
-      ReportExtensionInstallError("Couldn't remove CurrentVersion_old file.");
+      ReportExtensionInstallError(dest_dir,
+                                  "Couldn't remove CurrentVersion_old file.");
       return false;
     }
   }
   if (file_util::PathExists(current_version)) {
     if (!file_util::Move(current_version, current_version_old)) {
-      ReportExtensionInstallError("Couldn't move CurrentVersion file.");
+      ReportExtensionInstallError(dest_dir,
+                                  "Couldn't move CurrentVersion file.");
       return false;
     }
   }
@@ -566,7 +570,8 @@ bool ExtensionsServiceBackend::SetCurrentVersion(const FilePath& dest_dir,
         // TODO(erikkay): This is an ugly state to be in.  Try harder?
       }
     }
-    ReportExtensionInstallError("Couldn't create CurrentVersion file.");
+    ReportExtensionInstallError(dest_dir, 
+                                "Couldn't create CurrentVersion file.");
     return false;
   }
   return true;
@@ -574,44 +579,46 @@ bool ExtensionsServiceBackend::SetCurrentVersion(const FilePath& dest_dir,
 
 void ExtensionsServiceBackend::InstallExtension(
     const FilePath& extension_path,
-    const FilePath& install_dir,
-    bool alert_on_error,
     scoped_refptr<ExtensionsServiceFrontendInterface> frontend) {
   LOG(INFO) << "Installing extension " << extension_path.value();
 
   frontend_ = frontend;
-  alert_on_error_ = alert_on_error;
-  external_install_ = false;
-  extension_path_ = extension_path;
-  install_directory_ = install_dir;
+  alert_on_error_ = false;
 
-  InstallOrUpdateExtension(std::string());
+  bool was_update = false;
+  FilePath destination_path;
+  if (InstallOrUpdateExtension(extension_path,
+                               std::string(),  // no expected id
+                               &destination_path, &was_update))
+    ReportExtensionInstalled(destination_path.DirName(), was_update);
 }
 
-void ExtensionsServiceBackend::InstallOrUpdateExtension(
-    const std::string& expected_id) {
-  bool update = false;
+bool ExtensionsServiceBackend::InstallOrUpdateExtension(
+    const FilePath& source_file, const std::string& expected_id,
+    FilePath* version_dir, bool* was_update) {
+  if (was_update)
+    *was_update = false;
 
   // Read and verify the extension.
-  scoped_ptr<DictionaryValue> manifest(ReadManifest());
+  scoped_ptr<DictionaryValue> manifest(ReadManifest(source_file));
   if (!manifest.get()) {
-
     // ReadManifest has already reported the extension error.
-    return;
+    return false;
   }
   DictionaryValue* dict = manifest.get();
   Extension extension;
   std::string error;
   if (!extension.InitFromValue(*dict, &error)) {
-    ReportExtensionInstallError("Invalid extension manifest.");
-    return;
+    ReportExtensionInstallError(source_file,
+                                "Invalid extension manifest.");
+    return false;
   }
 
   // If an expected id was provided, make sure it matches.
   if (expected_id.length() && expected_id != extension.id()) {
-    ReportExtensionInstallError(
+    ReportExtensionInstallError(source_file, 
         "ID in new extension manifest does not match expected ID.");
-    return;
+    return false;
   }
 
   // <profile>/Extensions/<id>
@@ -620,8 +627,9 @@ void ExtensionsServiceBackend::InstallOrUpdateExtension(
   std::string current_version;
   if (ReadCurrentVersion(dest_dir, &current_version)) {
     if (!CheckCurrentVersion(version, current_version, dest_dir))
-      return;
-    update = true;
+      return false;
+    if (was_update)
+      *was_update = true;
   }
 
   // <profile>/Extensions/INSTALL_TEMP
@@ -630,56 +638,46 @@ void ExtensionsServiceBackend::InstallOrUpdateExtension(
   // Ensure we're starting with a clean slate.
   if (file_util::PathExists(temp_dir)) {
     if (!file_util::Delete(temp_dir, true)) {
-      ReportExtensionInstallError(
+      ReportExtensionInstallError(source_file,
           "Couldn't delete existing temporary directory.");
-      return;
+      return false;
     }
   }
   ScopedTempDir scoped_temp;
   scoped_temp.Set(temp_dir);
   if (!scoped_temp.IsValid()) {
-    ReportExtensionInstallError("Couldn't create temporary directory.");
-    return;
+    ReportExtensionInstallError(source_file, 
+                                "Couldn't create temporary directory.");
+    return false;
   }
 
   // <profile>/Extensions/INSTALL_TEMP/<version>
   FilePath temp_version = temp_dir.AppendASCII(version);
   file_util::CreateDirectory(temp_version);
-  if (!Unzip(extension_path_, temp_version, NULL)) {
-    ReportExtensionInstallError("Couldn't unzip extension.");
-    return;
+  if (!Unzip(source_file, temp_version, NULL)) {
+    ReportExtensionInstallError(source_file, "Couldn't unzip extension.");
+    return false;
   }
 
   // <profile>/Extensions/<dir_name>/<version>
-  FilePath version_dir = dest_dir.AppendASCII(version);
-  if (!InstallDirSafely(temp_version, version_dir))
-    return;
+  *version_dir = dest_dir.AppendASCII(version);
+  if (!InstallDirSafely(temp_version, *version_dir))
+    return false;
 
   if (!SetCurrentVersion(dest_dir, version)) {
-    if (!file_util::Delete(version_dir, true))
+    if (!file_util::Delete(*version_dir, true))
       LOG(WARNING) << "Can't remove " << dest_dir.value();
-    return;
-  }
-
-  if (external_install_) {
-
-    // To mark that this extension was installed from an external source,
-    // create a zero-length file.  At load time, this is used to indicate
-    // that the extension should be uninstalled.
-    // TODO(erikkay): move this into per-extension config storage when
-    // it appears.
-    FilePath marker = version_dir.AppendASCII(kExternalInstallFile);
-    file_util::WriteFile(marker, NULL, 0);
+    return false;
   }
 
-  ReportExtensionInstalled(dest_dir, update);
+  return true;
 }
 
 void ExtensionsServiceBackend::ReportExtensionInstallError(
-    const std::string &error) {
+    const FilePath& extension_path,  const std::string &error) {
 
   // TODO(erikkay): note that this isn't guaranteed to work properly on Linux.
-  std::string path_str = WideToASCII(extension_path_.ToWStringHack());
+  std::string path_str = WideToASCII(extension_path.ToWStringHack());
   std::string message =
       StringPrintf("Could not install extension from '%s'. %s",
                    path_str.c_str(), error.c_str());
@@ -687,7 +685,7 @@ void ExtensionsServiceBackend::ReportExtensionInstallError(
 }
 
 void ExtensionsServiceBackend::ReportExtensionInstalled(
-    FilePath path, bool update) {
+    const FilePath& path, bool update) {
   frontend_->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
       frontend_,
       &ExtensionsServiceFrontendInterface::OnExtensionInstalled,
@@ -695,9 +693,8 @@ void ExtensionsServiceBackend::ReportExtensionInstalled(
       update));
 
   // After it's installed, load it right away with the same settings.
-  extension_path_ = path;
   LOG(INFO) << "Loading extension " << path.value();
-  Extension* extension = LoadExtensionCurrentVersion();
+  Extension* extension = LoadExtensionCurrentVersion(path);
   if (extension) {
     // Only one extension, but ReportExtensionsLoaded can handle multiple,
     // so we need to construct a list.
@@ -716,7 +713,6 @@ void ExtensionsServiceBackend::ReportExtensionInstalled(
 // check that location for a .crx file, which it will then install locally if
 // a new version is available.
 void ExtensionsServiceBackend::CheckForExternalUpdates(
-    const FilePath& install_dir,
     scoped_refptr<ExtensionsServiceFrontendInterface> frontend) {
 
   // Note that this installation is intentionally silent (since it didn't
@@ -726,8 +722,6 @@ void ExtensionsServiceBackend::CheckForExternalUpdates(
   // they could install an extension manually themselves anyway.
   alert_on_error_ = false;
   frontend_ = frontend;
-  external_install_ = true;
-  install_directory_ = install_dir;
 
 #if defined(OS_WIN)
   HKEY reg_root = HKEY_LOCAL_MACHINE;
@@ -741,19 +735,28 @@ void ExtensionsServiceBackend::CheckForExternalUpdates(
       std::wstring extension_path;
       if (key.ReadValue(kRegistryExtensionPath, &extension_path)) {
         std::string id = WideToASCII(iterator.Name());
-        extension_path_ = FilePath(extension_path);
         std::wstring extension_version;
         if (key.ReadValue(kRegistryExtensionVersion, &extension_version)) {
-          if (ShouldInstall(id, WideToASCII(extension_version)))
-            InstallOrUpdateExtension(id);
+          if (ShouldInstall(id, WideToASCII(extension_version))) {
+            FilePath version_dir;
+            if (InstallOrUpdateExtension(FilePath(extension_path), id,
+                                         &version_dir, NULL)) {
+              // To mark that this extension was installed from an external
+              // source, create a zero-length file.  At load time, this is used
+              // to indicate that the extension should be uninstalled.
+              // TODO(erikkay): move this into per-extension config storage when
+              // it appears.
+              FilePath marker = version_dir.AppendASCII(
+                  kExternalInstallFile);
+              file_util::WriteFile(marker, NULL, 0);
+            }
+          }
         } else {
-
           // TODO(erikkay): find a way to get this into about:extensions
           LOG(WARNING) << "Missing value " << kRegistryExtensionVersion <<
               " for key " << key_path;
         }
       } else {
-
         // TODO(erikkay): find a way to get this into about:extensions
         LOG(WARNING) << "Missing value " << kRegistryExtensionPath <<
             " for key " << key_path;
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index 7489de7..be3158c 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -38,9 +38,9 @@ class ExtensionsServiceFrontendInterface
   // Load the extension from the directory |extension_path|.
   virtual void LoadExtension(const FilePath& extension_path) = 0;
 
-  // Called with results from LoadExtensionsFromDirectory(). The frontend
-  // takes ownership of the list.
-  virtual void OnExtensionsLoadedFromDirectory(ExtensionList* extensions) = 0;
+  // Called when extensions are loaded by the backend. The frontend takes
+  // ownership of the list.
+  virtual void OnExtensionsLoaded(ExtensionList* extensions) = 0;
 
   // Called with results from InstallExtension().
   // |is_update| is true if the installation was an update to an existing
@@ -71,7 +71,7 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface {
   virtual MessageLoop* GetMessageLoop();
   virtual void InstallExtension(const FilePath& extension_path);
   virtual void LoadExtension(const FilePath& extension_path);
-  virtual void OnExtensionsLoadedFromDirectory(ExtensionList* extensions);
+  virtual void OnExtensionsLoaded(ExtensionList* extensions);
   virtual void OnExtensionInstalled(FilePath path, bool is_update);
 
   // The name of the file that the current active version number is stored in.
@@ -85,15 +85,15 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface {
   // The message loop for the thread the ExtensionsService is running on.
   MessageLoop* message_loop_;
 
-  // The backend that will do IO on behalf of this instance.
-  scoped_refptr<ExtensionsServiceBackend> backend_;
-
   // The current list of installed extensions.
   ExtensionList extensions_;
 
   // The full path to the directory where extensions are installed.
   FilePath install_directory_;
 
+  // The backend that will do IO on behalf of this instance.
+  scoped_refptr<ExtensionsServiceBackend> backend_;
+
   // The profile associated with this set of extensions.
   Profile* profile_;
 
@@ -109,15 +109,14 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface {
 class ExtensionsServiceBackend
     : public base::RefCountedThreadSafe<ExtensionsServiceBackend> {
  public:
-  ExtensionsServiceBackend(){};
+  explicit ExtensionsServiceBackend(const FilePath& install_directory)
+      : install_directory_(install_directory) {};
 
-  // Loads extensions from a directory. The extensions are assumed to be
-  // unpacked in directories that are direct children of the specified path.
+  // Loads extensions from the install directory. The extensions are assumed to
+  // be unpacked in directories that are direct children of the specified path.
   // Errors are reported through ExtensionErrorReporter. On completion,
-  // OnExtensionsLoadedFromDirectory() is called with any successfully loaded
-  // extensions.
-  void LoadExtensionsFromDirectory(
-      const FilePath &path,
+  // OnExtensionsLoaded() is called with any successfully loaded extensions.
+  void LoadExtensionsFromInstallDirectory(
       scoped_refptr<ExtensionsServiceFrontendInterface> frontend);
 
   // Loads a single extension from |path| where |path| is the top directory of
@@ -131,54 +130,58 @@ class ExtensionsServiceBackend
       const FilePath &path,
       scoped_refptr<ExtensionsServiceFrontendInterface> frontend);
 
-  // Install the extension file at extension_path to install_dir.
-  // ReportExtensionInstallError is called on error.
-  // ReportExtensionInstalled is called on success.
+  // Install the extension file at |extension_path|. Errors are reported through
+  // ExtensionErrorReporter. ReportExtensionInstalled is called on success.
   void InstallExtension(
       const FilePath& extension_path,
-      const FilePath& install_dir,
-      bool alert_on_error,
       scoped_refptr<ExtensionsServiceFrontendInterface> frontend);
 
   // Check externally updated extensions for updates and install if necessary.
-  // ReportExtensionInstallError is called on error.
+  // Errors are reported through ExtensionErrorReporter. 
   // ReportExtensionInstalled is called on success.
   void CheckForExternalUpdates(
-      const FilePath& install_dir,
       scoped_refptr<ExtensionsServiceFrontendInterface> frontend);
 
  private:
-  // Load a single extension from |extension_path_|, the top directory of
+  // Load a single extension from |extension_path|, the top directory of
   // a specific extension where its manifest file lives.
-  Extension* LoadExtension();
+  Extension* LoadExtension(const FilePath& extension_path);
 
-  // Load a single extension from |extension_path_|, the top directory of
+  // Load a single extension from |extension_path|, the top directory of
   // a versioned extension where its Current Version file lives.
-  Extension* LoadExtensionCurrentVersion();
-
-  // Install a crx file at |extension_path_| into |install_directory_|.
-  // If |expected_id| is not empty, it's verified against the extension's
-  // manifest before installationl.  If the extension is already installed,
-  // install the new version only if its version number is greater than the
-  // current installed version.
-  void InstallOrUpdateExtension(const std::string& expected_id);
-
-  // Notify a frontend that there was an error loading an extension.
-  void ReportExtensionLoadError(const std::string& error);
-
-  // Notify a frontend that extensions were loaded.
+  Extension* LoadExtensionCurrentVersion(const FilePath& extension_path);
+
+  // Install a crx file at |source_file|. If |expected_id| is not empty, it's
+  // verified against the extension's manifest before installation. If the
+  // extension is already installed, install the new version only if its version
+  // number is greater than the current installed version. On success, sets
+  // |version_dir| to the versioned directory the extension was installed to and
+  // |was_update| to whether the extension turned out to be an update to an
+  // already installed version. Both |version_dir| and |was_update| can be NULL
+  // if the caller doesn't care.
+  bool InstallOrUpdateExtension(const FilePath& source_file,
+                                const std::string& expected_id,
+                                FilePath* version_dir,
+                                bool* was_update);
+
+  // Notify the frontend that there was an error loading an extension.
+  void ReportExtensionLoadError(const FilePath& extension_path,
+                                const std::string& error);
+
+  // Notify the frontend that extensions were loaded.
   void ReportExtensionsLoaded(ExtensionList* extensions);
 
-  // Notify a frontend that there was an error installing an extension.
-  void ReportExtensionInstallError(const std::string& error);
+  // Notify the frontend that there was an error installing an extension.
+  void ReportExtensionInstallError(const FilePath& extension_path,
+                                   const std::string& error);
 
-  // Notify a frontend that extensions were installed.
+  // Notify the frontend that extensions were installed.
   // |is_update| is true if this was an update to an existing extension.
-  void ReportExtensionInstalled(FilePath path, bool is_update);
+  void ReportExtensionInstalled(const FilePath& path, bool is_update);
 
   // Read the manifest from the front of the extension file.
   // Caller takes ownership of return value.
-  DictionaryValue* ReadManifest();
+  DictionaryValue* ReadManifest(const FilePath& extension_path);
 
   // Reads the Current Version file from |dir| into |version_string|.
   bool ReadCurrentVersion(const FilePath& dir, std::string* version_string);
@@ -219,18 +222,12 @@ class ExtensionsServiceBackend
   // The entry point is responsible for ensuring lifetime.
   ExtensionsServiceFrontendInterface* frontend_;
 
-  // The extension path being loaded or installed.
-  FilePath extension_path_;
-
   // The top-level extensions directory being installed to.
   FilePath install_directory_;
 
   // Whether errors result in noisy alerts.
   bool alert_on_error_;
 
-  // Whether the current install is from an external source.
-  bool external_install_;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionsServiceBackend);
 };
 
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 5544876..d74c060 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -54,11 +54,6 @@ class ExtensionsServiceTestFrontend
     : public ExtensionsServiceFrontendInterface {
  public:
 
-   ExtensionsServiceTestFrontend() {
-     file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("ext_test"),
-                                       &install_dir_);
-   }
-
   ~ExtensionsServiceTestFrontend() {
     for (ExtensionList::iterator iter = extensions_.begin();
          iter != extensions_.end(); ++iter) {
@@ -74,10 +69,6 @@ class ExtensionsServiceTestFrontend
     return &installed_;
   }
 
-  FilePath install_dir() {
-    return install_dir_;
-  }
-
   // ExtensionsServiceFrontendInterface
   virtual MessageLoop* GetMessageLoop() {
     return &message_loop_;
@@ -89,7 +80,7 @@ class ExtensionsServiceTestFrontend
   virtual void LoadExtension(const FilePath& extension_path) {
   }
 
-  virtual void OnExtensionsLoadedFromDirectory(ExtensionList* new_extensions) {
+  virtual void OnExtensionsLoaded(ExtensionList* new_extensions) {
     extensions_.insert(extensions_.end(), new_extensions->begin(),
                        new_extensions->end());
     delete new_extensions;
@@ -106,7 +97,7 @@ class ExtensionsServiceTestFrontend
                             ExtensionsServiceBackend* backend,
                             bool should_succeed) {
     ASSERT_TRUE(file_util::PathExists(path));
-    backend->InstallExtension(path, install_dir_, false,
+    backend->InstallExtension(path,
         scoped_refptr<ExtensionsServiceFrontendInterface>(this));
     message_loop_.RunAllPending();
     std::vector<std::string> errors = GetErrors();
@@ -131,7 +122,6 @@ class ExtensionsServiceTestFrontend
   MessageLoop message_loop_;
   ExtensionList extensions_;
   std::vector<FilePath> installed_;
-  FilePath install_dir_;
 };
 
 // make the test a PlatformTest to setup autorelease pools properly on mac
@@ -153,12 +143,13 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   extensions_path = extensions_path.AppendASCII("extensions");
   extensions_path = extensions_path.AppendASCII("good");
 
-  scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
+  scoped_refptr<ExtensionsServiceBackend> backend(
+      new ExtensionsServiceBackend(extensions_path));
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
       new ExtensionsServiceTestFrontend);
 
   std::vector<Extension*> extensions;
-  backend->LoadExtensionsFromDirectory(extensions_path,
+  backend->LoadExtensionsFromInstallDirectory(
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
 
@@ -217,12 +208,13 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
   extensions_path = extensions_path.AppendASCII("extensions");
   extensions_path = extensions_path.AppendASCII("bad");
 
-  scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
+  scoped_refptr<ExtensionsServiceBackend> backend(
+      new ExtensionsServiceBackend(extensions_path));
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
       new ExtensionsServiceTestFrontend);
 
   std::vector<Extension*> extensions;
-  backend->LoadExtensionsFromDirectory(extensions_path,
+  backend->LoadExtensionsFromInstallDirectory(
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
 
@@ -252,7 +244,11 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
 
-  scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
+  FilePath install_dir;
+  file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("ext_test"),
+                                   &install_dir);
+  scoped_refptr<ExtensionsServiceBackend> backend(
+      new ExtensionsServiceBackend(install_dir));
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
       new ExtensionsServiceTestFrontend);
 
@@ -290,7 +286,8 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
 
-  scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
+  scoped_refptr<ExtensionsServiceBackend> backend(
+      new ExtensionsServiceBackend(extensions_path));
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
       new ExtensionsServiceTestFrontend);