Refactor loading out of ExtensionService.

chrome/browser/extensions/unpacked_installer.cc

Index: chrome/browser/extensions/unpacked_installer.cc
diff --git a/chrome/browser/extensions/unpacked_installer.cc b/chrome/browser/extensions/unpacked_installer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5d2e66fc04e95f77bdd2d27a8af52ae2a52aaa99
--- /dev/null
+++ b/chrome/browser/extensions/unpacked_installer.cc
@@ -0,0 +1,213 @@
+// Copyright (c) 2011 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/unpacked_installer.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/file_util.h"
+#include "chrome/browser/extensions/extension_install_ui.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_file_util.h"
+
+namespace {
+
+// Manages an ExtensionInstallUI for a particular extension.
+class SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {

[Aaron Boodman, 2011/10/28 22:21:58]

I think that we can remove this. I believe the only place it is used in the code
(the only path where we end up with prompt_for_plugins_ = true) is in the
automation file. And in that case, it would only fire for extensions with
plugins. I think that was most likely not intended, or being used.

We definitely don't need this for the current functionality: We always show
prompts when installing packed, and never for installing unpacked.

[Matt Perry, 2011/10/28 22:28:55]

Actually, we show prompts for the first unpacked-load of an extension with
plugins. http://code.google.com/p/chromium/issues/detail?id=83273

[Yoyo Zhou, 2011/10/31 21:58:15]

prompt_for_plugins is supposed to be true by default. I must have lost that
somewhere!

+ public:
+  SimpleExtensionLoadPrompt(Profile* profile,
+                            base::WeakPtr<ExtensionService> extension_service,
+                            const Extension* extension);
+  ~SimpleExtensionLoadPrompt();
+
+  void ShowPrompt();
+
+  // ExtensionInstallUI::Delegate
+  virtual void InstallUIProceed();
+  virtual void InstallUIAbort(bool user_initiated);
+
+ private:
+  base::WeakPtr<ExtensionService> service_weak_;
+  scoped_ptr<ExtensionInstallUI> install_ui_;
+  scoped_refptr<const Extension> extension_;
+};
+
+SimpleExtensionLoadPrompt::SimpleExtensionLoadPrompt(
+    Profile* profile,
+    base::WeakPtr<ExtensionService> extension_service,
+    const Extension* extension)
+    : service_weak_(extension_service),
+      install_ui_(new ExtensionInstallUI(profile)),
+      extension_(extension) {
+}
+
+SimpleExtensionLoadPrompt::~SimpleExtensionLoadPrompt() {
+}
+
+void SimpleExtensionLoadPrompt::ShowPrompt() {
+  install_ui_->ConfirmInstall(this, extension_);
+}
+
+void SimpleExtensionLoadPrompt::InstallUIProceed() {
+  if (service_weak_.get())
+    service_weak_->OnExtensionInstalled(
+        extension_, false, -1);  // Not from web store.
+  delete this;
+}
+
+void SimpleExtensionLoadPrompt::InstallUIAbort(bool user_initiated) {
+  delete this;
+}
+
+}  // namespace
+
+UnpackedInstaller::UnpackedInstaller(
+    base::WeakPtr<ExtensionService> extension_service)
+    : service_weak_(extension_service) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+UnpackedInstaller::~UnpackedInstaller() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+        BrowserThread::CurrentlyOn(BrowserThread::FILE));
+}
+
+void UnpackedInstaller::Load(const FilePath& extension_path) {
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+      base::Bind(&UnpackedInstaller::LoadSingleExtension,
+                 this, extension_path));
+}
+
+void UnpackedInstaller::LoadFromCommandLine(

[Aaron Boodman, 2011/10/28 22:21:58]

This only shares OnLoadSingleExtension with the rest of this class. Otherwise it
is totally different. Seems like it should be its own thing.

[Yoyo Zhou, 2011/10/31 21:58:15]

This actually does exactly the same thing as the rest of the class, but
synchronously. Maybe I should clarify that in the header comments.

[Aaron Boodman, 2011/10/31 22:33:25]

What I meant is that it doesn't rely on anything else in this class, so it seems
it could be separate if not for OnLoadSingleExtension.

In the glorious future, when the stupid plugin prompt goes away, there wouldn't
be anything in OnExtensionLoaded, so it seems it might be possible to pull this
out.

But since that isn't true now, nevermind me.

+    const FilePath& path_in) {
+  if (!service_weak_.get())
+    return;
+  // Load extensions from the command line synchronously to avoid a race
+  // between extension loading and loading an URL from the command line.
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+
+  FilePath extension_path = path_in;
+  file_util::AbsolutePath(&extension_path);
+
+  std::string id = Extension::GenerateIdForPath(extension_path);
+  bool allow_file_access =
+      Extension::ShouldAlwaysAllowFileAccess(Extension::LOAD);
+  if (service_weak_->extension_prefs()->HasAllowFileAccessSetting(id))
+    allow_file_access = service_weak_->extension_prefs()->AllowFileAccess(id);
+
+  int flags = Extension::NO_FLAGS;
+  if (allow_file_access)
+    flags |= Extension::ALLOW_FILE_ACCESS;
+  if (Extension::ShouldDoStrictErrorChecking(Extension::LOAD))
+    flags |= Extension::STRICT_ERROR_CHECKS;
+
+  std::string error;
+  scoped_refptr<const Extension> extension(extension_file_util::LoadExtension(
+      extension_path,
+      Extension::LOAD,
+      flags,
+      &error));
+
+  if (!extension) {
+    service_weak_->ReportExtensionLoadError(extension_path, error, true);
+    return;
+  }
+
+  OnLoadSingleExtension(extension);
+}
+
+void UnpackedInstaller::LoadSingleExtension(const FilePath& extension_path) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  FilePath absolute_path = extension_path;
+  file_util::AbsolutePath(&absolute_path);
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(&UnpackedInstaller::CheckExtensionFileAccess,

[Aaron Boodman, 2011/10/28 22:21:58]

Add a TODO that it would be nice to remove this extra ping-pong, but we need the
flag to know what to pass to extension_file_util::Load().

[Yoyo Zhou, 2011/10/31 21:58:15]

Done.

+                 this, absolute_path));
+}
+
+void UnpackedInstaller::CheckExtensionFileAccess(
+    const FilePath& extension_path) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::string id = Extension::GenerateIdForPath(extension_path);
+  // Unpacked extensions default to allowing file access, but if that has been
+  // overridden, don't reset the value.
+  bool allow_file_access =
+      Extension::ShouldAlwaysAllowFileAccess(Extension::LOAD);
+  if (service_weak_->extension_prefs()->HasAllowFileAccessSetting(id))
+    allow_file_access = service_weak_->extension_prefs()->AllowFileAccess(id);
+
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+      base::Bind(
+          &UnpackedInstaller::LoadSingleExtensionWithFileAccess,
+          this, extension_path, allow_file_access));
+}
+
+void UnpackedInstaller::LoadSingleExtensionWithFileAccess(
+    const FilePath& extension_path,
+    bool allow_file_access) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  int flags = allow_file_access ?
+      Extension::ALLOW_FILE_ACCESS : Extension::NO_FLAGS;
+  if (Extension::ShouldDoStrictErrorChecking(Extension::LOAD))
+    flags |= Extension::STRICT_ERROR_CHECKS;
+  std::string error;
+  scoped_refptr<const Extension> extension(extension_file_util::LoadExtension(
+      extension_path,
+      Extension::LOAD,
+      flags,
+      &error));
+
+  if (!extension) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(
+            &UnpackedInstaller::ReportExtensionLoadError,
+            this,
+            extension_path, error));
+    return;
+  }
+
+  // Report this as an installed extension so that it gets remembered in the

[Aaron Boodman, 2011/10/28 22:21:58]

Comment is no longer needed since it is obvious from the name/purpose of this
class. (woo)

[Yoyo Zhou, 2011/10/31 21:58:15]

Done.

+  // prefs.
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &UnpackedInstaller::OnLoadSingleExtension,
+          this, extension));
+}
+
+void UnpackedInstaller::ReportExtensionLoadError(
+    const FilePath& extension_path, const std::string &error) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!service_weak_.get())
+    return;
+  service_weak_->ReportExtensionLoadError(extension_path, error, true);
+}
+
+void UnpackedInstaller::OnLoadSingleExtension(
+    const scoped_refptr<const Extension>& extension) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!service_weak_.get())
+    return;
+  const ExtensionList* disabled_extensions =
+      service_weak_->disabled_extensions();
+  if (service_weak_->show_extensions_prompts() &&
+      prompt_for_plugins_ &&
+      !extension->plugins().empty() &&
+      std::find(disabled_extensions->begin(),
+                disabled_extensions->end(),
+                extension) !=
+      disabled_extensions->end()) {
+    SimpleExtensionLoadPrompt* prompt = new SimpleExtensionLoadPrompt(
+        service_weak_->profile(),
+        service_weak_,
+        extension);
+    prompt->ShowPrompt();
+    return;  // continues in SimpleExtensionLoadPrompt::InstallUI*
+  }
+  // Not from web store.

[Aaron Boodman, 2011/10/28 22:21:58]

Is this comment still legit? I don't see how it relates to above check.

[Yoyo Zhou, 2011/10/31 21:58:15]

I believe the comment is about the 'false' parameter.

[Aaron Boodman, 2011/10/31 22:33:25]

Ah, can you format it like this then:

extension,
false,  // not from web store
-1);  // magic

[Yoyo Zhou, 2011/11/01 21:50:57]

Done.

+  service_weak_->OnExtensionInstalled(extension, false, -1);
+}