Add a centralized error reporter to the

extensions sytem. This can be called from any
component on any thread. The hope is that this
will encourage more thorough error handling.

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

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

8 files changed, 187 insertions, 91 deletions

diff --git a/chrome/browser/browser.scons b/chrome/browser/browser.scons
index 6bce4d9..7e94112 100644
--- a/chrome/browser/browser.scons
+++ b/chrome/browser/browser.scons
@@ -506,6 +506,8 @@ input_files = ChromeFileList([
   MSVSFilter('Extensions', [
     'extensions/extension.cc',
     'extensions/extension.h',
+    'extensions/extension_error_reporter.cc',
+    'extensions/extension_error_reporter.h',
     'extensions/extension_protocols.h',
     'extensions/extensions_service.cc',
     'extensions/extensions_service.h',
diff --git a/chrome/browser/browser.vcproj b/chrome/browser/browser.vcproj
index 1325f66..e5995c1 100644
--- a/chrome/browser/browser.vcproj
+++ b/chrome/browser/browser.vcproj
@@ -1906,6 +1906,14 @@
 				>
 			</File>
 			<File
+				RelativePath=".\extensions\extension_error_reporter.cc"
+				>
+			</File>
+			<File
+				RelativePath=".\extensions\extension_error_reporter.h"
+				>
+			</File>
+			<File
 				RelativePath=".\extensions\extension_protocols.cc"
 				>
 			</File>
diff --git a/chrome/browser/extensions/extension_error_reporter.cc b/chrome/browser/extensions/extension_error_reporter.cc
new file mode 100644
index 0000000..bd923f5
--- /dev/null
+++ b/chrome/browser/extensions/extension_error_reporter.cc
@@ -0,0 +1,65 @@
+// Copyright (c) 2006-2009 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/extension_error_reporter.h"
+
+#include "base/string_util.h"
+
+#if defined(OS_WIN)
+#include "chrome/common/win_util.h"
+#endif
+
+ExtensionErrorReporter* ExtensionErrorReporter::instance_ = NULL;
+
+// static
+void ExtensionErrorReporter::Init(bool enable_noisy_errors) {
+  if (!instance_) {
+    instance_ = new ExtensionErrorReporter(enable_noisy_errors);
+  }
+}
+
+// static
+ExtensionErrorReporter* ExtensionErrorReporter::GetInstance() {
+  CHECK(instance_) << "Init() was never called";
+  return instance_;
+}
+
+ExtensionErrorReporter::ExtensionErrorReporter(bool enable_noisy_errors)
+    : ui_loop_(MessageLoop::current()),
+      enable_noisy_errors_(enable_noisy_errors) {
+}
+
+void ExtensionErrorReporter::ReportError(const std::string& message,
+                                         bool be_noisy) {
+  // NOTE: There won't be a ui_loop_ in the unit test environment.
+  if (ui_loop_ && MessageLoop::current() != ui_loop_) {
+    ui_loop_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &ExtensionErrorReporter::ReportError, message,
+                          be_noisy));
+    return;
+  }
+
+  errors_.push_back(message);
+
+  // TODO(aa): Print the error message out somewhere better. I think we are
+  // going to need some sort of 'extension inspector'.
+  LOG(WARNING) << message;
+
+  if (enable_noisy_errors_ && be_noisy) {
+#if defined(OS_WIN)
+    win_util::MessageBox(NULL, UTF8ToWide(message), L"Extension error",
+        MB_OK | MB_SETFOREGROUND);
+#else
+    // TODO(port)
+#endif
+  }
+}
+
+const std::vector<std::string>* ExtensionErrorReporter::GetErrors() {
+  return &errors_;
+}
+
+void ExtensionErrorReporter::ClearErrors() {
+  errors_.clear();
+}
diff --git a/chrome/browser/extensions/extension_error_reporter.h b/chrome/browser/extensions/extension_error_reporter.h
new file mode 100644
index 0000000..737e276
--- /dev/null
+++ b/chrome/browser/extensions/extension_error_reporter.h
@@ -0,0 +1,52 @@
+// Copyright (c) 2006-2009 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_ERROR_REPORTER_H_
+#define CHROME_BROWSER_EXTENSIONS_ERROR_REPORTER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/message_loop.h"
+#include "base/ref_counted.h"
+
+// Exposes an easy way for the various components of the extension system to
+// report errors. This is a singleton that lives on the UI thread, with the
+// exception of ReportError() which may be called from any thread.
+// TODO(aa): Hook this up to about:extensions, when we have about:extensions.
+// TODO(aa): Consider exposing directly, or via a helper, to the renderer
+// process and plumbing the errors out to the browser.
+// TODO(aa): Add ReportError(extension_id, message, be_noisy), so that we can
+// report errors that are specific to a particular extension.
+class ExtensionErrorReporter
+  : public base::RefCountedThreadSafe<ExtensionErrorReporter> {
+ public:
+  // Initializes the error reporter. Must be called before any other methods
+  // and on the UI thread.
+  static void Init(bool enable_noisy_errors);
+
+  // Get the singleton instance.
+  static ExtensionErrorReporter* GetInstance();
+
+  // Report an error. Errors always go to LOG(INFO). Optionally, they can also
+  // cause a noisy alert box. This method can be called from any thread.
+  void ReportError(const std::string& message, bool be_noisy);
+
+  // Get the errors that have been reported so far.
+  const std::vector<std::string>* GetErrors();
+
+  // Clear the list of errors reported so far.
+  void ClearErrors();
+
+ private:
+  static ExtensionErrorReporter* instance_;
+
+  ExtensionErrorReporter(bool enable_noisy_errors);
+
+  MessageLoop* ui_loop_;
+  std::vector<std::string> errors_;
+  bool enable_noisy_errors_;
+};
+
+#endif  // CHROME_BROWSER_EXTENSIONS_ERROR_REPORTER_H_
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 4c1a40f..9fc0040 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -14,6 +14,7 @@
 #include "base/values.h"
 #include "net/base/file_stream.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/user_script_master.h"
 #include "chrome/browser/plugin_service.h"
 #include "chrome/common/json_value_serializer.h"
@@ -22,7 +23,6 @@
 
 #if defined(OS_WIN)
 #include "base/registry.h"
-#include "chrome/common/win_util.h"
 #endif
 
 // ExtensionsService
@@ -170,31 +170,6 @@ void ExtensionsService::OnExtensionsLoadedFromDirectory(
   delete new_extensions;
 }
 
-void ExtensionsService::OnExtensionLoadError(bool alert_on_error,
-                                             const std::string& error) {
-  // TODO(aa): Print the error message out somewhere better. I think we are
-  // going to need some sort of 'extension inspector'.
-  LOG(WARNING) << error;
-  if (alert_on_error) {
-#if defined(OS_WIN)
-    win_util::MessageBox(NULL, UTF8ToWide(error),
-        L"Extension load error", MB_OK | MB_SETFOREGROUND);
-#endif
-  }
-}
-
-void ExtensionsService::OnExtensionInstallError(bool alert_on_error,
-                                                const std::string& error) {
-  // TODO(erikkay): Print the error message out somewhere better.
-  LOG(WARNING) << error;
-  if (alert_on_error) {
-#if defined(OS_WIN)
-    win_util::MessageBox(NULL, UTF8ToWide(error),
-        L"Extension load error", MB_OK | MB_SETFOREGROUND);
-#endif
-  }
-}
-
 void ExtensionsService::OnExtensionInstalled(FilePath path, bool update) {
   NotificationService::current()->Notify(
       NotificationType::EXTENSION_INSTALLED,
@@ -339,14 +314,11 @@ Extension* ExtensionsServiceBackend::LoadExtension() {
 
 void ExtensionsServiceBackend::ReportExtensionLoadError(
     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 message = StringPrintf("Could not load extension from '%s'. %s",
                                      path_str.c_str(), error.c_str());
-  frontend_->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
-      frontend_, &ExtensionsServiceFrontendInterface::OnExtensionLoadError,
-      alert_on_error_, message));
+  ExtensionErrorReporter::GetInstance()->ReportError(message, alert_on_error_);
 }
 
 void ExtensionsServiceBackend::ReportExtensionsLoaded(
@@ -702,9 +674,7 @@ void ExtensionsServiceBackend::ReportExtensionInstallError(
   std::string message =
       StringPrintf("Could not install extension from '%s'. %s",
                    path_str.c_str(), error.c_str());
-  frontend_->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
-      frontend_, &ExtensionsServiceFrontendInterface::OnExtensionInstallError,
-      alert_on_error_, message));
+  ExtensionErrorReporter::GetInstance()->ReportError(message, alert_on_error_);
 }
 
 void ExtensionsServiceBackend::ReportExtensionInstalled(
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index c8f04b0..efba0086 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -37,18 +37,10 @@ class ExtensionsServiceFrontendInterface
   // Load the extension from the directory |extension_path|.
   virtual void LoadExtension(const FilePath& extension_path) = 0;
 
-  // Called when loading an extension fails.
-  virtual void OnExtensionLoadError(bool alert_on_error,
-                                    const std::string& message) = 0;
-
   // Called with results from LoadExtensionsFromDirectory(). The frontend
   // takes ownership of the list.
   virtual void OnExtensionsLoadedFromDirectory(ExtensionList* extensions) = 0;
 
-  // Called when installing an extension fails.
-  virtual void OnExtensionInstallError(bool alert_on_error,
-                                       const std::string& message) = 0;
-
   // Called with results from InstallExtension().
   // |is_update| is true if the installation was an update to an existing
   // installed extension rather than a new installation.
@@ -75,11 +67,7 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface {
   virtual MessageLoop* GetMessageLoop();
   virtual void InstallExtension(const FilePath& extension_path);
   virtual void LoadExtension(const FilePath& extension_path);
-  virtual void OnExtensionLoadError(bool alert_on_error,
-                                    const std::string& message);
   virtual void OnExtensionsLoadedFromDirectory(ExtensionList* extensions);
-  virtual void OnExtensionInstallError(bool alert_on_error,
-                                       const std::string& message);
   virtual void OnExtensionInstalled(FilePath path, bool is_update);
 
   // The name of the file that the current active version number is stored in.
@@ -118,7 +106,7 @@ class ExtensionsServiceBackend
 
   // Loads extensions from a directory. The extensions are assumed to be
   // unpacked in directories that are direct children of the specified path.
-  // Errors are reported through OnExtensionLoadError(). On completion,
+  // Errors are reported through ExtensionErrorReporter. On completion,
   // OnExtensionsLoadedFromDirectory() is called with any successfully loaded
   // extensions.
   void LoadExtensionsFromDirectory(
@@ -127,7 +115,7 @@ class ExtensionsServiceBackend
 
   // Loads a single extension from |path| where |path| is the top directory of
   // a specific extension where its manifest file lives.
-  // Errors are reported through OnExtensionLoadError(). On completion,
+  // Errors are reported through ExtensionErrorReporter. On completion,
   // OnExtensionsLoadedFromDirectory() is called with any successfully loaded
   // extensions.
   // TODO(erikkay): It might be useful to be able to load a packed extension
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index a49399d..b3ab36a 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "chrome/browser/extensions/extension.h"
+#include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/json_value_serializer.h"
@@ -26,6 +27,25 @@ struct ExtensionsOrder {
   }
 };
 
+static std::vector<std::string> GetErrors() {
+  const std::vector<std::string>* errors =
+      ExtensionErrorReporter::GetInstance()->GetErrors();
+  std::vector<std::string> ret_val;
+
+  for (std::vector<std::string>::const_iterator iter = errors->begin();
+       iter != errors->end(); ++iter) {
+    if (iter->find(".svn") == std::string::npos) {
+      ret_val.push_back(*iter);
+    }
+  }
+
+  // The tests rely on the errors being in a certain order, which can vary
+  // depending on how filesystem iteration works.
+  std::stable_sort(ret_val.begin(), ret_val.end());
+
+  return ret_val;
+}
+
 }  // namespace
 
 // A mock implementation of ExtensionsServiceFrontendInterface for testing the
@@ -46,10 +66,6 @@ class ExtensionsServiceTestFrontend
     }
   }
 
-  std::vector<std::string>* errors() {
-    return &errors_;
-  }
-
   ExtensionList* extensions() {
     return &extensions_;
   }
@@ -73,29 +89,13 @@ class ExtensionsServiceTestFrontend
   virtual void LoadExtension(const FilePath& extension_path) {
   }
 
-  virtual void OnExtensionLoadError(bool alert_on_error,
-                                    const std::string& message) {
-    // In the development environment, we get errors when trying to load
-    // extensions out of the .svn directories.
-    if (message.find(".svn") != std::string::npos)
-      return;
-
-    errors_.push_back(message);
-  }
-
   virtual void OnExtensionsLoadedFromDirectory(ExtensionList* new_extensions) {
     extensions_.insert(extensions_.end(), new_extensions->begin(),
                        new_extensions->end());
     delete new_extensions;
-    // In the tests we rely on extensions and errors being in particular order,
-    // which is not always the case (and is not guaranteed by used APIs).
+    // In the tests we rely on extensions being in particular order, which is
+    // not always the case (and is not guaranteed by used APIs).
     std::stable_sort(extensions_.begin(), extensions_.end(), ExtensionsOrder());
-    std::stable_sort(errors_.begin(), errors_.end());
-  }
-
-  virtual void OnExtensionInstallError(bool alert_on_error,
-                                       const std::string& message) {
-    errors_.push_back(message);
   }
 
   virtual void OnExtensionInstalled(FilePath path, bool is_update) {
@@ -109,32 +109,42 @@ class ExtensionsServiceTestFrontend
     backend->InstallExtension(path, install_dir_, false,
         scoped_refptr<ExtensionsServiceFrontendInterface>(this));
     message_loop_.RunAllPending();
+    std::vector<std::string> errors = GetErrors();
     if (should_succeed) {
       EXPECT_EQ(1u, installed_.size()) << path.value();
-      EXPECT_EQ(0u, errors_.size()) << path.value();
-      for (std::vector<std::string>::iterator err = errors_.begin();
-        err != errors_.end(); ++err) {
+      EXPECT_EQ(0u, errors.size()) << path.value();
+      for (std::vector<std::string>::iterator err = errors.begin();
+        err != errors.end(); ++err) {
         LOG(ERROR) << *err;
       }
     } else {
       EXPECT_EQ(0u, installed_.size()) << path.value();
-      EXPECT_EQ(1u, errors_.size()) << path.value();
+      EXPECT_EQ(1u, errors.size()) << path.value();
     }
+
     installed_.clear();
-    errors_.clear();
+    ExtensionErrorReporter::GetInstance()->ClearErrors();
   }
 
 
  private:
   MessageLoop message_loop_;
   ExtensionList extensions_;
-  std::vector<std::string> errors_;
   std::vector<FilePath> installed_;
   FilePath install_dir_;
 };
 
 // make the test a PlatformTest to setup autorelease pools properly on mac
-typedef PlatformTest ExtensionsServiceTest;
+class ExtensionsServiceTest : public testing::Test {
+ public:
+  static void SetUpTestCase() {
+    ExtensionErrorReporter::Init(false);  // no noisy errors
+  }
+
+  virtual void SetUp() {
+    ExtensionErrorReporter::GetInstance()->ClearErrors();
+  }
+};
 
 // Test loading good extensions from the profile directory.
 TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
@@ -152,9 +162,9 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
 
-  std::vector<std::string>* errors = frontend->errors();
-  for (std::vector<std::string>::iterator err = errors->begin();
-    err != errors->end(); ++err) {
+  std::vector<std::string> errors = GetErrors();
+  for (std::vector<std::string>::iterator err = errors.begin();
+    err != errors.end(); ++err) {
     LOG(ERROR) << *err;
   }
   ASSERT_EQ(3u, frontend->extensions()->size());
@@ -216,24 +226,24 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
 
-  EXPECT_EQ(4u, frontend->errors()->size());
+  EXPECT_EQ(4u, GetErrors().size());
   EXPECT_EQ(0u, frontend->extensions()->size());
 
-  EXPECT_TRUE(MatchPattern(frontend->errors()->at(0),
+  EXPECT_TRUE(MatchPattern(GetErrors()[0],
       std::string("Could not load extension from '*'. * ") +
-      JSONReader::kBadRootElementType)) << frontend->errors()->at(0);
+      JSONReader::kBadRootElementType)) << GetErrors()[0];
 
-  EXPECT_TRUE(MatchPattern(frontend->errors()->at(1),
+  EXPECT_TRUE(MatchPattern(GetErrors()[1],
       std::string("Could not load extension from '*'. ") +
-      Extension::kInvalidJsListError)) << frontend->errors()->at(1);
+      Extension::kInvalidJsListError)) << GetErrors()[1];
 
-  EXPECT_TRUE(MatchPattern(frontend->errors()->at(2),
+  EXPECT_TRUE(MatchPattern(GetErrors()[2],
       std::string("Could not load extension from '*'. ") +
-      Extension::kInvalidManifestError)) << frontend->errors()->at(2);
+      Extension::kInvalidManifestError)) << GetErrors()[2];
 
-  EXPECT_TRUE(MatchPattern(frontend->errors()->at(3),
+  EXPECT_TRUE(MatchPattern(GetErrors()[3],
       "Could not load extension from '*'. Could not read '*' file.")) <<
-      frontend->errors()->at(3);
+      GetErrors()[3];
 };
 
 // Test installing extensions.
@@ -289,7 +299,7 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
   backend->LoadSingleExtension(ext1,
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
-  EXPECT_EQ(0u, frontend->errors()->size());
+  EXPECT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, frontend->extensions()->size());
 
   FilePath no_manifest = extensions_path.AppendASCII("bad")
@@ -297,7 +307,6 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
   backend->LoadSingleExtension(no_manifest,
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
   frontend->GetMessageLoop()->RunAllPending();
-  EXPECT_EQ(1u, frontend->errors()->size());
+  EXPECT_EQ(1u, GetErrors().size());
   ASSERT_EQ(1u, frontend->extensions()->size());
 }
-
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index 86e9d16..9600c6c 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_manager.h"
+#include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/extensions/user_script_master.h"
 #include "chrome/browser/history/history.h"
@@ -367,6 +368,7 @@ void ProfileImpl::InitExtensions() {
     script_dir = script_dir.Append(chrome::kUserScriptsDirname);
   }
 
+  ExtensionErrorReporter::Init(true);  // allow noisy errors.
   user_script_master_ = new UserScriptMaster(
       g_browser_process->file_thread()->message_loop(), script_dir);
   extensions_service_ = new ExtensionsService(