Add ErrorConsole UI for Extension Install Warnings

chrome/browser/extensions/error_console/error_console.cc

Index: chrome/browser/extensions/error_console/error_console.cc
diff --git a/chrome/browser/extensions/error_console/error_console.cc b/chrome/browser/extensions/error_console/error_console.cc
index cff063cd3adc45272e927bd7140317f31fcc40c1..2b13363c150e516c8c6f87b7baa7c511e35f195d 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -5,13 +5,18 @@
 #include "chrome/browser/extensions/error_console/error_console.h"
 
 #include <list>
+#include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/stl_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_set.h"
+#include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
@@ -37,6 +42,21 @@ void DeleteIncognitoErrorsFromList(ErrorConsole::ErrorList* list) {
   }
 }
 
+// Iterate through an error list and remove and delete all errors of a given
+// |type|.
+void DeleteErrorsOfTypeFromList(ErrorConsole::ErrorList* list,
+                                ExtensionError::Type type) {
+  ErrorConsole::ErrorList::iterator iter = list->begin();
+  while (iter != list->end()) {
+    if ((*iter)->type() == type) {
+      delete *iter;
+      iter = list->erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+}
+
 base::LazyInstance<ErrorConsole::ErrorList> g_empty_error_list =
     LAZY_INSTANCE_INITIALIZER;
 
@@ -45,13 +65,16 @@ base::LazyInstance<ErrorConsole::ErrorList> g_empty_error_list =
 void ErrorConsole::Observer::OnErrorConsoleDestroyed() {
 }
 
-ErrorConsole::ErrorConsole(Profile* profile) : profile_(profile) {
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_PROFILE_DESTROYED,
-                 content::NotificationService::AllBrowserContextsAndSources());
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
-                 content::Source<Profile>(profile_));
+ErrorConsole::ErrorConsole(Profile* profile,
+                           ExtensionService* extension_service)
+     : enabled_(false), profile_(profile) {
+  pref_registrar_.Init(profile_->GetPrefs());
+  pref_registrar_.Add(prefs::kExtensionsUIDeveloperMode,
+                      base::Bind(&ErrorConsole::OnPrefChanged,
+                                 base::Unretained(this)));
+
+  if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode))
+    Enable(extension_service);
 }
 
 ErrorConsole::~ErrorConsole() {
@@ -66,8 +89,20 @@ ErrorConsole* ErrorConsole::Get(Profile* profile) {
 
 void ErrorConsole::ReportError(scoped_ptr<const ExtensionError> scoped_error) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (!enabled_)
+    return;
 
   const ExtensionError* error = scoped_error.release();
+  const ErrorList& extension_errors =
+      GetErrorsForExtension(error->extension_id());
+  for (ErrorList::const_iterator iter = extension_errors.begin();
+       iter != extension_errors.end(); ++iter) {
+    if (error->IsEqual(*iter)) {  // duplicate error thrown; ignore it.
+      delete error;
+      return;
+    }
+  }
+
   // If there are too many errors for an extension already, limit ourselves to
   // the most recent ones.
   ErrorList* error_list = &errors_[error->extension_id()];
@@ -98,6 +133,61 @@ void ErrorConsole::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void ErrorConsole::OnPrefChanged() {
+  bool developer_mode =
+      profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode);
+
+  if (developer_mode && !enabled_)
+    Enable(ExtensionSystem::Get(profile_)->extension_service());
+  else if (!developer_mode && enabled_)
+    Disable();
+}
+
+void ErrorConsole::Enable(ExtensionService* extension_service) {
+  enabled_ = true;
+
+  notification_registrar_.Add(
+      this,
+      chrome::NOTIFICATION_PROFILE_DESTROYED,
+      content::NotificationService::AllBrowserContextsAndSources());
+  notification_registrar_.Add(
+      this,
+      chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
+      content::Source<Profile>(profile_));
+  notification_registrar_.Add(
+      this,
+      chrome::NOTIFICATION_EXTENSION_INSTALLED,
+      content::Source<Profile>(profile_));
+
+  if (extension_service) {
+    // Get manifest errors for extensions already installed.
+    const ExtensionSet* extensions = extension_service->extensions();
+    for (ExtensionSet::const_iterator iter = extensions->begin();
+         iter != extensions->end(); ++iter) {
+      AddManifestErrorsForExtension(iter->get());
+    }
+  }
+}
+
+void ErrorConsole::Disable() {
+  notification_registrar_.RemoveAll();
+  RemoveAllErrors();
+  enabled_ = false;
+}
+
+void ErrorConsole::AddManifestErrorsForExtension(const Extension* extension) {
+  const std::vector<InstallWarning>& warnings =
+      extension->install_warnings();
+  for (std::vector<InstallWarning>::const_iterator iter = warnings.begin();
+       iter != warnings.end(); ++iter) {
+    ReportError(scoped_ptr<const ExtensionError>(new ManifestParsingError(

[Yoyo Zhou, 2013/08/16 00:17:25]

Seeing this makes me wonder whether it's right to call these
ManifestParsingErrors. After all, they don't prevent the manifest from loading.
Thoughts?

[Devlin, 2013/08/16 23:56:54]

I've been wondering that as well...

How about a general rename from ManifestParsingError -> ManifestError (this will
also keep the door open for if we decide we want to include blocking errors from
ExtensionErrorReporter), and JavascriptRuntimeError -> RuntimeError?

Unfortunately, the "Error" parts (of these and ExtensionError) are still
somewhat misnomers, since they could be warnings or even just logs - but I can't
think of a more accurate blanket term.

[Yoyo Zhou, 2013/08/19 20:39:46]

ManifestError seems fine. If we want to have other kinds of runtime errors,
though, we can add them (or rename JavascriptRuntimeError) later.

[Devlin, 2013/08/19 21:42:39]

Done.

+        extension->id(),
+        base::UTF8ToUTF16(iter->message),
+        base::UTF8ToUTF16(iter->key),
+        base::UTF8ToUTF16(iter->specific))));
+  }
+}
+
 void ErrorConsole::RemoveIncognitoErrors() {
   for (ErrorMap::iterator iter = errors_.begin();
        iter != errors_.end(); ++iter) {
@@ -137,6 +227,23 @@ void ErrorConsole::Observe(int type,
       RemoveErrorsForExtension(
           content::Details<Extension>(details).ptr()->id());
       break;
+    case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
+      const InstalledExtensionInfo* info =
+          content::Details<InstalledExtensionInfo>(details).ptr();
+
+      // We don't want to have manifest errors from previous installs. We want
+      // to keep runtime errors, though, because extensions are reloaded on a
+      // refresh of chrome:extensions, and we don't want to wipe our history

[Yoyo Zhou, 2013/08/16 00:17:25]

Does this notification get sent when unpacked extensions are reloaded? (The
refresh of chrome://extensions only reloads unpacked extensions.)

[Devlin, 2013/08/16 23:56:54]

Yep, it does. Which means that any time the page was refreshed, all the runtime
errors would be wiped.

+      // whenever that happens.
+      ErrorMap::iterator iter = errors_.find(info->extension->id());
+      if (iter != errors_.end()) {
+        DeleteErrorsOfTypeFromList(&(iter->second),
+                                   ExtensionError::MANIFEST_PARSING_ERROR);
+      }
+
+      AddManifestErrorsForExtension(info->extension);
+      break;
+    }
     default:
       NOTREACHED();
   }