Additional metrics for disabled extensions and content verification

chrome/browser/extensions/installed_loader.cc

Index: chrome/browser/extensions/installed_loader.cc
diff --git a/chrome/browser/extensions/installed_loader.cc b/chrome/browser/extensions/installed_loader.cc
index feb8fe7d4880b0352bbec3d73b1912e964cfeeaa..95c39566e3d322bf5e65bf536d48464aca8f8948 100644
--- a/chrome/browser/extensions/installed_loader.cc
+++ b/chrome/browser/extensions/installed_loader.cc
@@ -134,6 +134,22 @@ void RecordCreationFlags(const Extension* extension) {
   }
 }
 
+// Records the diable reasons for a single extension grouped by

[Ken Rockot(use gerrit already), 2014/07/24 03:56:02]

nit: disable

[asargent_no_longer_on_chrome, 2014/07/24 17:08:12]

Done.

+// Extension::DisableReason.
+void RecordDisableReasons(int reasons) {
+  // |reasons| is a bitmask with values from Extension::DisabledReason
+  // which are increasing powers of 2.
+  int shift = 0;
+  for (int reason = (1 << shift); reason < Extension::DISABLE_REASON_LAST;
+       reason = (1 << ++shift)) {

[Ken Rockot(use gerrit already), 2014/07/24 03:56:02]

What about:

for (int reason = 1; reason < ...; reason <<= 1) {

instead?

[asargent_no_longer_on_chrome, 2014/07/24 17:08:12]

Done.

+    if ((reason == 0 && reasons == 0) || reasons & reason)

[Ken Rockot(use gerrit already), 2014/07/24 03:56:02]

OK now I'm confused; I don't think reason can ever be 0 here.

Here's my guess for what the whole function should be, if I understand the
intent correctly:

void RecordDisableReasons(int reasons) {
  if (reasons == Extension::DISABLE_NONE) {
    UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason",
Extension::DISABLE_NONE, Extension::DISABLE_REASON_LAST);
    return;
  }
  for (int reason = 1; reason < Extension::DISABLE_REASON_LAST; reason <<= 1) {
    if (reasons & reason) {
      UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason", reason,
Extension::DISABLE_REASON_LAST);
    }
  }
}

[asargent_no_longer_on_chrome, 2014/07/24 17:08:12]

And this is why we have code review! I did some refactoring and hadn't re-tested
the 0 case. Thanks - fixed. 

+      UMA_HISTOGRAM_ENUMERATION(
+          "Extensions.DisableReason", reason, Extension::DISABLE_REASON_LAST);
+    if (reasons == 0)
+      break;
+  }
+}
+
 }  // namespace
 
 InstalledLoader::InstalledLoader(ExtensionService* extension_service)
@@ -473,6 +489,7 @@ void InstalledLoader::LoadAllExtensions() {
     if (extension_prefs_->DidExtensionEscalatePermissions((*ex)->id())) {
       ++disabled_for_permissions_count;
     }
+    RecordDisableReasons(extension_prefs_->GetDisableReasons((*ex)->id()));
     if (Manifest::IsExternalLocation((*ex)->location())) {
       // See loop above for ENABLED.
       if (ManifestURL::UpdatesFromGallery(*ex)) {
@@ -542,6 +559,8 @@ void InstalledLoader::LoadAllExtensions() {
     UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessNotAllowed",
                              file_access_not_allowed_count);
   }
+  UMA_HISTOGRAM_COUNTS_100("Extensions.CorruptExtensionTotalDisables",
+                           extension_prefs_->GetCorruptedDisableCount());
 }
 
 int InstalledLoader::GetCreationFlags(const ExtensionInfo* info) {