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..d4d5a3fc0ce94b30b094e7290c0006d47255cd7c 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -6,12 +6,17 @@
 
 #include <list>
 
+#include "base/bind.h"
+#include "base/bind_helpers.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/extensions/feature_switch.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"
@@ -45,13 +50,21 @@ 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 FeatureSwitch enabled, then return
+  // immediately. Since we never register for any notifications, this ensures
+  // the ErrorConsole will never be enabled.
+  if (!FeatureSwitch::error_console()->IsEnabled())
+    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 +77,40 @@ 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<ExtensionError> error) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  const ExtensionError* error = scoped_error.release();
+  if (!enabled_ || !Extension::IdIsValid(error->extension_id()))
+    return;
+
+  ErrorList* extension_errors = &errors_[error->extension_id()];
+
+  // First, check if it's a duplicate.
+  for (ErrorList::iterator iter = extension_errors->begin();
+       iter != extension_errors->end(); ++iter) {
+    // If we find a duplicate error, remove the old error and add the new one,
+    // incrementing the occurrence count of the error. We use the new error
+    // for runtime errors, so we can link to the latest context, inspectable
+    // view, etc.
+    if (error->IsEqual(*iter)) {
+      error->set_occurrences((*iter)->occurrences() + 1);
+      delete *iter;
+      extension_errors->erase(iter);
+      break;
+    }
+  }
+
   // 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 (extension_errors->size() >= kMaxErrorsPerExtension) {
+    delete extension_errors->front();
+    extension_errors->pop_front();
   }
-  error_list->push_back(error);
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(error));
+  extension_errors->push_back(error.release());
+
+  FOR_EACH_OBSERVER(
+      Observer, observers_, OnErrorAdded(extension_errors->back()));
 }
 
 const ErrorConsole::ErrorList& ErrorConsole::GetErrorsForExtension(
@@ -98,6 +131,36 @@ void ErrorConsole::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void ErrorConsole::OnPrefChanged() {
+  bool should_enable =
+      profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode) &&
+      FeatureSwitch::error_console()->IsEnabled();

[Yoyo Zhou, 2013/08/27 17:28:00]

It doesn't make sense to test for the FeatureSwitch here if the listener isn't
registered if it's disabled. But if you choose to, you should return early if
it's disabled.

[Devlin, 2013/08/27 18:25:55]

My thought was to protect a little against ScopedOverrides for FeatureSwitches,
but that kinda opens a whole new can of worms, and since there's no
FeatureSwitch::Observer, it'd be hard to handle all the cases, so there's no
reason to handle just one.

[Yoyo Zhou, 2013/08/27 18:48:43]

Yeah, as you noted, because of the lack of observer, it can't be done.

+
+  if (should_enable && !enabled_)
+    Enable(ExtensionSystem::Get(profile_)->extension_service());
+  else if (!should_enable && 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_));
+}
+
+void ErrorConsole::Disable() {
+  notification_registrar_.RemoveAll();
+  RemoveAllErrors();
+  enabled_ = false;
+}
+
 void ErrorConsole::RemoveIncognitoErrors() {
   for (ErrorMap::iterator iter = errors_.begin();
        iter != errors_.end(); ++iter) {