Provide UI for per-extension enabling/disabling of error collection.

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 2429af614a413b7bf187029e5a40e71f3e11dceb..8ac529074576aeb0b31b9ae26eb2caa187af5f73 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -37,9 +37,9 @@ namespace {
 // settings.
 const char kStoreExtensionErrorsPref[] = "store_extension_errors";
 
-// The default mask (for the time being) is to report everything.
-const int32 kDefaultMask = (1 << ExtensionError::MANIFEST_ERROR) |
-                           (1 << ExtensionError::RUNTIME_ERROR);
+// This is the default mask for which errors to report. That is, if an extension
+// does not have specific preference set, this will be used instead.
+const int32 kDefaultMask = 0;
 
 const char kAppsDeveloperToolsExtensionId[] =
     "ohmmkhmmmpcnpikjeljgnaoabkaalbgc";
@@ -86,19 +86,33 @@ void ErrorConsole::SetReportingForExtension(const std::string& extension_id,
     return;
 
   ErrorPreferenceMap::iterator pref = pref_map_.find(extension_id);

[not at google - send to devlin, 2014/04/16 21:28:31]

I'm not actually sure that having pref_map_  is much of an advantage over
querying Preferences directly. I presume that they do their own caching, it
wouldn't read from disk every time you access a preference.

[Devlin, 2014/04/16 22:50:41]

Oh, I know that we don't read from disk each time, but it's still a little more
complicated, just because you have to grab the extension dictionary, and then
grab the preference, and then do all the base::Value stuff, etc.  It's not a ton
of work, but since this will be queried each time an error is reported,
performance is important, and I think it might be best to keep it.  But, if you
feel strongly, I can change it.

[not at google - send to devlin, 2014/04/16 23:22:07]

I feel like we should have a helper function to get/set that bit in prefs.

[Devlin, 2014/04/16 23:58:22]

Per chat discussion, removed pref_map_ in favor of only using ExtensionPrefs.

+  int32 mask = pref == pref_map_.end() ? default_mask_ : pref->second;
+  mask = enabled ? mask | (1 << type) : mask &~(1 << type);

[not at google - send to devlin, 2014/04/16 21:28:31]

I find this pretty odd to read and parse. I would prefer

if (enabled)
  mask |= (1 << type);
else
  mask &= ~(1 << type);

much as it's an extra 3 lines...

[Devlin, 2014/04/16 22:50:41]

No!  Too many lines spoil the code! ;)

Done.

 
-  if (pref == pref_map_.end()) {
-    pref = pref_map_.insert(
-        std::pair<std::string, int32>(extension_id, default_mask_)).first;
+  SetExtensionPref(extension_id, mask);
+}
+
+void ErrorConsole::SetReportingForExtensionForAllTypes(
+    const std::string& extension_id, bool enabled) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!enabled_ || !Extension::IdIsValid(extension_id))
+    return;
+
+  int32 mask = 0;
+  if (enabled) {
+    mask |= (1 << ExtensionError::MANIFEST_ERROR) |
+            (1 << ExtensionError::RUNTIME_ERROR);

[not at google - send to devlin, 2014/04/16 21:28:31]

have a value like ExtensionError::NUM_ERROR or something then use mask |=
(ExtensionError::NUM_ERROR - 1);

[Devlin, 2014/04/16 22:50:41]

Hmm... it does break a few other things.  But I suppose it could be worth it.

[not at google - send to devlin, 2014/04/16 23:22:07]

you could also be mega-sneaky and do "mask = -1" or slightly less sneaky
MAX_UINT.

[Devlin, 2014/04/16 23:58:22]

masks are ints, not size_ts, so wouldn't -1 be something like 100...001?  (I
could be wrong - it's been awhile since I've done any fancy bitwise logic.)  And
I'm a little hesitant about UINT_MAX because of it's different definitions on
different architectures (I don't think it's one of the ones we redefine in
base/).

[not at google - send to devlin, 2014/04/17 00:15:53]

oh, read it as uint32, my bad.

   }
 
-  pref->second =
-      enabled ? pref->second | (1 << type) : pref->second &~(1 << type);
+  SetExtensionPref(extension_id, mask);
+}
 
-  ExtensionPrefs::Get(profile_)->UpdateExtensionPref(
-      extension_id,
-      kStoreExtensionErrorsPref,
-      base::Value::CreateIntegerValue(pref->second));
+bool ErrorConsole::IsReportingEnabledForExtension(
+    const std::string& extension_id) const {
+  return ShouldReportErrorForExtension(extension_id,
+                                       ExtensionError::MANIFEST_ERROR) ||
+         ShouldReportErrorForExtension(extension_id,
+                                       ExtensionError::RUNTIME_ERROR);

[not at google - send to devlin, 2014/04/16 21:28:31]

it would be nice if this method could look more like

return pref_map_[extension_id] > 0;

[Devlin, 2014/04/16 22:50:41]

Good point. Done.

 }
 
 void ErrorConsole::UseDefaultReportingForExtension(
@@ -116,16 +130,9 @@ void ErrorConsole::UseDefaultReportingForExtension(
 
 void ErrorConsole::ReportError(scoped_ptr<ExtensionError> error) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!enabled_ || !Extension::IdIsValid(error->extension_id()))
-    return;
-
-  ErrorPreferenceMap::const_iterator pref =
-      pref_map_.find(error->extension_id());
-  // Check the mask to see if we report the error. If we don't have a specific
-  // entry, use the default mask.
-  if ((pref == pref_map_.end() &&
-          ((default_mask_ & (1 << error->type())) == 0)) ||
-      (pref != pref_map_.end() && (pref->second & (1 << error->type())) == 0)) {
+  if (!enabled_ ||
+      !Extension::IdIsValid(error->extension_id()) ||
+      !ShouldReportErrorForExtension(error->extension_id(), error->type())) {
     return;
   }
 
@@ -269,4 +276,35 @@ void ErrorConsole::Observe(int type,
   }
 }
 
+void ErrorConsole::SetExtensionPref(const std::string& extension_id,
+                                    int32 mask) {
+  pref_map_[extension_id] = mask;
+  ExtensionPrefs::Get(profile_)->UpdateExtensionPref(
+      extension_id,
+      kStoreExtensionErrorsPref,
+      base::Value::CreateIntegerValue(mask));
+}
+
+bool ErrorConsole::ShouldReportErrorForExtension(
+    const std::string& extension_id, ExtensionError::Type type) const {
+  // Registered preferences take priority over everything else.
+  ErrorPreferenceMap::const_iterator pref = pref_map_.find(extension_id);
+  if (pref != pref_map_.end())
+    return (pref->second & (1 << type)) != 0;
+
+  // If the default mask says to report the error, do so.
+  if ((default_mask_ & (1 << type)) != 0)
+    return true;
+
+  // One last check: If the extension is unpacked, we report all errors by
+  // default.
+  const Extension* extension =
+      ExtensionRegistry::Get(profile_)->GetExtensionById(
+          extension_id, ExtensionRegistry::EVERYTHING);
+  if (extension && extension->location() == Manifest::UNPACKED)
+    return true;
+
+  return false;
+}
+
 }  // namespace extensions