Improvements to incognito and file access metrics for extensions: only record

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 ab16252f0df436508c00f4ed1db55d1644c44059..25b58fd421af59e106ab7312052ba963f16296be 100644
--- a/chrome/browser/extensions/installed_loader.cc
+++ b/chrome/browser/extensions/installed_loader.cc
@@ -443,8 +443,10 @@ void InstalledLoader::LoadAllExtensions() {
         extension, "Extensions.Permissions_Load");
 
     // For incognito and file access, skip anything that doesn't appear in
-    // settings.
-    if (extension->ShouldDisplayInExtensionSettings()) {
+    // settings. Also, policy-installed (and unpacked of course, checked above)
+    // extensions are boring.
+    if (extension->ShouldDisplayInExtensionSettings() &&
+        !Manifest::IsPolicyLocation(extension->location())) {
       if (extension->can_be_incognito_enabled()) {
         if (util::IsIncognitoEnabled(extension->id(), profile))
           ++incognito;
@@ -527,10 +529,15 @@ void InstalledLoader::LoadAllExtensions() {
                            disabled_for_permissions_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.NonWebStoreNewTabPageOverrides",
                            non_webstore_ntp_override_count);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoAllowed", incognito);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoNotAllowed", not_incognito);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessAllowed", file_access);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessNotAllowed", not_file_access);
+  if (incognito + not_incognito > 0) {

[jar (doing other things), 2014/05/20 17:43:34]

nit: the above is seemingly meant to be clever, but perhaps clearer would be:
if (incognito > 0 || not_incognito > 0)


This looks like it is changing what is recorded in the histograms.

Are you taking that change into account with your planned(?) use of the values?
If not, you'll get conflated data... and wish you had a new name for the
histogram.

[not at google - send to devlin, 2014/05/20 17:53:10]

We only just added this metric (a few days ago), so I'm ok with that.

[not at google - send to devlin, 2014/05/20 18:06:20]

I can change it but I think the existing condition and your suggestion are just
as understandable. I read "incognito + not_incognito" as "the number of
extensions that could have been enable in incognito", and this metric isn't
interesting if that's 0. I could add a comment to make that clearer.

[jar (doing other things), 2014/05/21 00:27:20]

If you're really enamoured with using the sum, you could rename the variables to
something like:

extension_count_for_incognito
extension_count_for_non_incognito

Adding those two named variables would then make sense, as they are both counts
(and hence non-negative), and it would be clearer why adding produced an
interesting sum (the positive sum clearly indicates if there are some
extensions).  

The current variable names made this less clear, and almost sounded more like
booleans (and their declaration gave no hint... I had to read code to find their
meanings).

[not at google - send to devlin, 2014/05/21 01:38:48]

That change SGTM. I'll come back to this CL tomorrow.

+    UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoAllowed", incognito);
+    UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoNotAllowed", not_incognito);
+  }
+  if (file_access + not_file_access > 0) {
+    UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessAllowed", file_access);
+    UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessNotAllowed",
+                             not_file_access);
+  }
 }
 
 int InstalledLoader::GetCreationFlags(const ExtensionInfo* info) {