Correctly indicate whether or not error collection is enabled in ErrorConsole

Fixes a problem  where we incorrectly reported whether or not error collection
was enabled for unpacked extensions.
BUG=365331

Review URL: https://codereview.chromium.org/245403003

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

3 files changed, 25 insertions, 27 deletions

diff --git a/chrome/browser/extensions/error_console/error_console.cc b/chrome/browser/extensions/error_console/error_console.cc
index bdfc201..73342d3 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -122,11 +122,7 @@ bool ErrorConsole::IsReportingEnabledForExtension(
   if (!enabled_ || !Extension::IdIsValid(extension_id))
     return false;
 
-  int mask = default_mask_;
-  // This call can fail if the preference isn't set, but we don't really care
-  // if it does, because we just use the default mask instead.
-  prefs_->ReadPrefAsInteger(extension_id, kStoreExtensionErrorsPref, &mask);
-  return mask != 0;
+  return GetMaskForExtension(extension_id) != 0;
 }
 
 void ErrorConsole::UseDefaultReportingForExtension(
@@ -140,11 +136,12 @@ void ErrorConsole::UseDefaultReportingForExtension(
 
 void ErrorConsole::ReportError(scoped_ptr<ExtensionError> error) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!enabled_ ||
-      !Extension::IdIsValid(error->extension_id()) ||
-      !ShouldReportErrorForExtension(error->extension_id(), error->type())) {
+  if (!enabled_ || !Extension::IdIsValid(error->extension_id()))
+    return;
+
+  int mask = GetMaskForExtension(error->extension_id());
+  if (!(mask & (1 << error->type())))
     return;
-  }
 
   const ExtensionError* weak_error = errors_.AddError(error.Pass());
   FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(weak_error));
@@ -285,28 +282,21 @@ void ErrorConsole::Observe(int type,
   }
 }
 
-bool ErrorConsole::ShouldReportErrorForExtension(
-    const std::string& extension_id, ExtensionError::Type type) const {
+int ErrorConsole::GetMaskForExtension(const std::string& extension_id) const {
   // Registered preferences take priority over everything else.
   int pref = 0;
-  if (prefs_->ReadPrefAsInteger(
-          extension_id, kStoreExtensionErrorsPref, &pref)) {
-    return (pref & (1 << type)) != 0;
-  }
-
-  // If the default mask says to report the error, do so.
-  if ((default_mask_ & (1 << type)) != 0)
-    return true;
+  if (prefs_->ReadPrefAsInteger(extension_id, kStoreExtensionErrorsPref, &pref))
+    return pref;
 
-  // One last check: If the extension is unpacked, we report all errors by
-  // default.
+  // If the extension is unpacked, we report all error types by default.
   const Extension* extension =
       ExtensionRegistry::Get(profile_)->GetExtensionById(
           extension_id, ExtensionRegistry::EVERYTHING);
   if (extension && extension->location() == Manifest::UNPACKED)
-    return true;
+    return (1 << ExtensionError::NUM_ERROR_TYPES) - 1;
 
-  return false;
+  // Otherwise, use the default mask.
+  return default_mask_;
 }
 
 }  // namespace extensions
diff --git a/chrome/browser/extensions/error_console/error_console.h b/chrome/browser/extensions/error_console/error_console.h
index 3176ce9..af2a865 100644
--- a/chrome/browser/extensions/error_console/error_console.h
+++ b/chrome/browser/extensions/error_console/error_console.h
@@ -145,10 +145,8 @@ class ErrorConsole : public content::NotificationObserver,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // Returns true if we should report the error of the given type for the
-  // extension.
-  bool ShouldReportErrorForExtension(const std::string& extension_id,
-                                     ExtensionError::Type type) const;
+  // Returns the applicable bit mask of reporting preferences for the extension.
+  int GetMaskForExtension(const std::string& extension_id) const;
 
   // Whether or not the error console should record errors. This is true if
   // the user is in developer mode, and at least one of the following is true:
diff --git a/chrome/browser/extensions/error_console/error_console_unittest.cc b/chrome/browser/extensions/error_console/error_console_unittest.cc
index dcfa60b..baf522b 100644
--- a/chrome/browser/extensions/error_console/error_console_unittest.cc
+++ b/chrome/browser/extensions/error_console/error_console_unittest.cc
@@ -231,6 +231,9 @@ TEST_F(ErrorConsoleUnitTest, TestDefaultStoringPrefs) {
       CreateNewRuntimeError(packed_extension->id(), "runtime error 1"));
   EXPECT_EQ(0u, error_console_->GetErrorsForExtension(
       packed_extension->id()).size());
+  // Also check that reporting settings are correctly returned.
+  EXPECT_FALSE(error_console_->IsReportingEnabledForExtension(
+      packed_extension->id()));
 
   // Errors should be reported by default for the unpacked extension.
   error_console_->ReportError(
@@ -239,6 +242,9 @@ TEST_F(ErrorConsoleUnitTest, TestDefaultStoringPrefs) {
       CreateNewRuntimeError(unpacked_extension->id(), "runtime error 2"));
   EXPECT_EQ(2u, error_console_->GetErrorsForExtension(
       unpacked_extension->id()).size());
+  // Also check that reporting settings are correctly returned.
+  EXPECT_TRUE(error_console_->IsReportingEnabledForExtension(
+      unpacked_extension->id()));
 
   // Registering a preference should override this for both types of extensions
   // (should be able to enable errors for packed, or disable errors for
@@ -250,6 +256,8 @@ TEST_F(ErrorConsoleUnitTest, TestDefaultStoringPrefs) {
       CreateNewRuntimeError(packed_extension->id(), "runtime error 3"));
   EXPECT_EQ(1u, error_console_->GetErrorsForExtension(
       packed_extension->id()).size());
+  EXPECT_TRUE(error_console_->IsReportingEnabledForExtension(
+      packed_extension->id()));
 
   error_console_->SetReportingForExtension(unpacked_extension->id(),
                                            ExtensionError::RUNTIME_ERROR,
@@ -259,6 +267,8 @@ TEST_F(ErrorConsoleUnitTest, TestDefaultStoringPrefs) {
   EXPECT_EQ(2u,  // We should still have the first two errors.
             error_console_->GetErrorsForExtension(
                 unpacked_extension->id()).size());
+  EXPECT_FALSE(error_console_->IsReportingEnabledForExtension(
+      unpacked_extension->id()));
 }
 
 }  // namespace extensions