ErrorConsole Switch, Duplicate Error Detection, Dev Mode Check

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..64ff3b069a7b0e43fd1669e86ca0180927cee0a9 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -6,16 +6,22 @@
 
 #include <list>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
 #include "base/lazy_instance.h"
+#include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension.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"
 #include "extensions/common/constants.h"
+#include "extensions/common/switches.h"
 
 namespace extensions {
 
@@ -45,13 +51,23 @@ 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) : enabled_(false),
+                                               profile_(profile) {
+  // If we don't have the necessary CommandLine switch enabled, then return

[Yoyo Zhou, 2013/08/27 00:41:07]

As I mentioned earlier today, take a look at FeatureSwitch:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

One of the benefits that didn't come to my mind at the time is it greatly
simplifies what you have to do when the switch goes from default-off to
default-on.

[Devlin, 2013/08/27 17:13:56]

Done.

+  // immediately. Since we never register for any notifications, this ensures
+  // the ErrorConsole will never be enabled.
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableErrorConsole)) {
+    return;
+  }
+
+  pref_registrar_.Init(profile_->GetPrefs());
+  pref_registrar_.Add(prefs::kExtensionsUIDeveloperMode,
+                      base::Bind(&ErrorConsole::OnPrefChanged,
+                                 base::Unretained(this)));
+
+  if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode))
+    Enable();
 }
 
 ErrorConsole::~ErrorConsole() {
@@ -64,20 +80,42 @@ ErrorConsole* ErrorConsole::Get(Profile* profile) {
   return ExtensionSystem::Get(profile)->error_console();
 }
 
-void ErrorConsole::ReportError(scoped_ptr<const ExtensionError> scoped_error) {
+void ErrorConsole::ReportError(scoped_ptr<const ExtensionError> error) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  const ExtensionError* error = scoped_error.release();
-  // If there are too many errors for an extension already, limit ourselves to
-  // the most recent ones.
-  ErrorList* error_list = &errors_[error->extension_id()];
-  if (error_list->size() >= kMaxErrorsPerExtension) {
-    delete error_list->front();
-    error_list->pop_front();
+  if (!enabled_ || !Extension::IdIsValid(error->extension_id()))
+    return;
+
+  // Keep a pointer to the error so we can reference it after |error|'s release
+  // without keeping track of an index in a list.
+  const ExtensionError* weak_error = error.get();
+  ErrorList* extension_errors = &errors_[error->extension_id()];
+
+  // First, check if it's a duplicate.
+  for (size_t i = 0; i < extension_errors->size(); ++i) {
+    // If we find a duplicate error, replace the error with the new version.

[Yoyo Zhou, 2013/08/27 00:41:07]

There's no way to see how often a duplicate error has occurred, right? It might
be nice to show a count, although I'm not sure what the UI should look like.

[Devlin, 2013/08/27 17:13:56]

Originally, I was holding off on any of the implementation until I had an idea
what the UI would be, but it's a simple enough addition to add in. Occurrence
count added.

+    // This can be useful for runtime errors, where we can link to the latest
+    // context, inspectable view, etc.
+    if (error->IsEqual(extension_errors->at(i))) {
+      delete extension_errors->at(i);
+      extension_errors->at(i) = error.release();

[Yoyo Zhou, 2013/08/27 00:41:07]

Could this violate our expectation that the error list contains the most recent
errors? e.g. say we're just showing the 3 most recent errors. We might not be
showing an error that repeated an older error for some reason. It seems to me
that adding the new error to the end would be preferable.

[Devlin, 2013/08/27 17:13:56]

Ooh, good point. Fixed.

+      break;
+    }
+  }
+
+  // If we didn't replace a previous error, then we add the error to the end of
+  // the list.
+  if (error.get()) {
+    // If there are too many errors for an extension already, limit ourselves to
+    // the most recent ones.
+    if (extension_errors->size() >= kMaxErrorsPerExtension) {
+      delete extension_errors->front();
+      extension_errors->pop_front();
+    }
+    extension_errors->push_back(error.release());
   }
-  error_list->push_back(error);
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(error));
+  FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(weak_error));
 }
 
 const ErrorConsole::ErrorList& ErrorConsole::GetErrorsForExtension(
@@ -98,6 +136,35 @@ void ErrorConsole::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void ErrorConsole::OnPrefChanged() {
+  bool developer_mode =
+      profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode);
+
+  if (developer_mode && !enabled_)
+    Enable();
+  else if (!developer_mode && enabled_)
+    Disable();
+}
+
+void ErrorConsole::Enable() {
+  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_));
+}
+
+void ErrorConsole::Disable() {
+  notification_registrar_.RemoveAll();
+  RemoveAllErrors();
+  enabled_ = false;
+}
+
 void ErrorConsole::RemoveIncognitoErrors() {
   for (ErrorMap::iterator iter = errors_.begin();
        iter != errors_.end(); ++iter) {