Install platform apps into a separate data directory

chrome/browser/extensions/crx_installer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/crx_installer.h"
 
 #include <map>
 #include <set>
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/convert_user_script.h"
 #include "chrome/browser/extensions/convert_web_app.h"
 #include "chrome/browser/extensions/default_apps_trial.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/permissions_updater.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_file_util.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
@@ skipping 89 matching lines @@
 
 CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
                            ExtensionInstallUI* client)
     : install_directory_(frontend_weak->install_directory()),
       install_source_(Extension::INTERNAL),
       extensions_enabled_(frontend_weak->extensions_enabled()),
       delete_source_(false),
       create_app_shortcut_(false),
       frontend_weak_(frontend_weak),
       profile_(frontend_weak->profile()),
+      profile_path_(profile_->GetPath()),

[benwells, 2012/02/13 02:55:44]

Do you need this new field? Can you just call profile_->GetPath() in the one
place it is used?

       client_(client),
       apps_require_extension_mime_type_(false),
       allow_silent_install_(false),
       install_cause_(extension_misc::INSTALL_CAUSE_UNSET),
       creation_flags_(Extension::NO_FLAGS) {
 }
 
 CrxInstaller::~CrxInstaller() {
   // Delete the temp directory and crx file as necessary. Note that the
   // destructor might be called on any thread, so we post a task to the file
@@ skipping 330 matching lines @@
     }
   }
 
   // See how long extension install paths are.  This is important on
   // windows, because file operations may fail if the path to a file
   // exceeds a small constant.  See crbug.com/69693 .
   UMA_HISTOGRAM_CUSTOM_COUNTS(
     "Extensions.CrxInstallDirPathLength",
         install_directory_.value().length(), 0, 500, 100);
 
+  bool separate_data_dir_required =

[jeremy, 2012/02/12 09:40:28]

Could you add a comment explaining when this case kicks in.  It should be clear
to someone reading this function, under what conditions the flag will be true.

+      extension_->RequiresSeparateUserDataDirectory();
   FilePath version_dir = extension_file_util::InstallExtension(
       unpacked_extension_root_,
       extension_->id(),
       extension_->VersionString(),
-      install_directory_);
+      install_directory_,
+      separate_data_dir_required ? extension_file_util::DELETE_SOURCE_NO :
+                                   extension_file_util::DELETE_SOURCE_YES);
   if (version_dir.empty()) {
     ReportFailureFromFileThread(
         l10n_util::GetStringUTF16(
             IDS_EXTENSION_MOVE_DIRECTORY_TO_PROFILE_FAILED));
     return;
   }
 
+  // If the platform app requires its own data directory then we'll need to
+  // copy the extension into that data directory too.
+  if (separate_data_dir_required) {

[benwells, 2012/02/13 02:55:44]

What about unpacked extensions? Can this move into
extension_file_util::InstallExtension?

+    FilePath data_dir =
+        web_app::GetWebAppDataDirectory(profile_path_, *extension_);
+    FilePath profile_path = data_dir.AppendASCII(chrome::kInitialProfile);
+    FilePath extension_path =
+        profile_path.AppendASCII(ExtensionService::kInstallDirectoryName);
+    extension_file_util::InstallExtension(

[jeremy, 2012/02/12 09:40:28]

What if this call fails?

+        unpacked_extension_root_,
+        extension_->id(),
+        extension_->VersionString(),
+        extension_path,
+        extension_file_util::DELETE_SOURCE_YES);
+  }
+
   // This is lame, but we must reload the extension because absolute paths
   // inside the content scripts are established inside InitFromValue() and we
   // just moved the extension.
   // TODO(aa): All paths to resources inside extensions should be created
   // lazily and based on the Extension's root path at that moment.
   // TODO(rdevlin.cronin): Continue removing std::string errors and replacing
   // with string16
   std::string error;
   extension_ = extension_file_util::LoadExtension(
       version_dir,

[benwells, 2012/02/13 02:55:44]

Should this reload the extension from the new data directory or the original
extension?

I think at the moment we are just creating a copy of the extension in another
folder, so it can be run manually from there. In which case the call to
InstallExtension above can fail and not affect the flow of this function.

If that's the case, maybe a comment would help.

       install_source_,
       extension_->creation_flags() | Extension::REQUIRE_KEY,
       &error);
   CHECK(error.empty()) << error;
 
   ReportSuccessFromFileThread();
 }
 
 void CrxInstaller::ReportFailureFromFileThread(const string16& error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ skipping 83 matching lines @@
   // Some users (such as the download shelf) need to know when a
   // CRXInstaller is done.  Listening for the EXTENSION_* events
   // is problematic because they don't know anything about the
   // extension before it is unpacked, so they cannot filter based
   // on the extension.
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_CRX_INSTALLER_DONE,
       content::Source<CrxInstaller>(this),
       content::Details<const Extension>(extension));
 }