Make installed_loader UMA less wasteful.

chrome/browser/extensions/installed_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/installed_loader.h"
 
 #include "base/files/file_path.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 41 matching lines @@
   UNPACKED_DIR,  // Unpacked directory.
   NEEDS_RELOCALIZATION,  // The locale has changed since we read this extension.
   CORRUPT_PREFERENCES,  // The manifest in the preferences is corrupt.
   NUM_MANIFEST_RELOAD_REASONS
 };
 
 // Used in histogram Extension.BackgroundPageType. Values may be added, as
 // long as existing values are not changed.
 enum BackgroundPageType {
   NO_BACKGROUND_PAGE = 0,
-  BACKGROUND_PAGE_PERSISTENT = 1,
-  EVENT_PAGE = 2,
+  BACKGROUND_PAGE_PERSISTENT,
+  EVENT_PAGE,
+  NUM_BACKGROUND_PAGE_TYPES
 };
 
 // Used in histogram Extensions.ExternalItemState. Values may be added, as
 // long as existing values are not changed.
 enum ExternalItemState {
   DEPRECATED_EXTERNAL_ITEM_DISABLED = 0,
   DEPRECATED_EXTERNAL_ITEM_ENABLED = 1,
   EXTERNAL_ITEM_WEBSTORE_DISABLED = 2,
   EXTERNAL_ITEM_WEBSTORE_ENABLED = 3,
   EXTERNAL_ITEM_NONWEBSTORE_DISABLED = 4,
@@ skipping 79 matching lines @@
     error = errors::kManifestUnreadable;
   }
 
   // Once installed, non-unpacked extensions cannot change their IDs (e.g., by
   // updating the 'key' field in their manifest).
   // TODO(jstritar): migrate preferences when unpacked extensions change IDs.
   if (extension.get() && !Manifest::IsUnpackedLocation(extension->location()) &&
       info.extension_id != extension->id()) {
     error = errors::kCannotChangeExtensionID;
     extension = NULL;
     content::RecordAction(UserMetricsAction("Extensions.IDChangedError"));

[not at google - send to devlin, 2014/05/21 22:31:07]

(see below... what even is "content::RecordAction"?)

   }
 
   // Check policy on every load in case an extension was blacklisted while
   // Chrome was not running.
   const ManagementPolicy* policy = extensions::ExtensionSystem::Get(
       extension_service_->profile())->management_policy();
   if (extension.get()) {
     Extension::DisableReason disable_reason = Extension::DISABLE_NONE;
     bool force_disabled = false;
     if (!policy->UserMayLoad(extension.get(), NULL)) {
@@ skipping 42 matching lines @@
     ExtensionInfo* info = extensions_info->at(i).get();
 
     // Skip extensions that were loaded from the command-line because we don't
     // want those to persist across browser restart.
     if (info->extension_location == Manifest::COMMAND_LINE)
       continue;
 
     ManifestReloadReason reload_reason = ShouldReloadExtensionManifest(*info);
     ++reload_reason_counts[reload_reason];
     UMA_HISTOGRAM_ENUMERATION("Extensions.ManifestReloadEnumValue",
-                              reload_reason, 100);
+                              reload_reason,
+                              NUM_MANIFEST_RELOAD_REASONS);
 
     if (reload_reason != NOT_NEEDED) {
       // Reloading an extension reads files from disk.  We do this on the
       // UI thread because reloads should be very rare, and the complexity
       // added by delaying the time when the extensions service knows about
       // all extensions is significant.  See crbug.com/37548 for details.
       // |allow_io| disables tests that file operations run on the file
       // thread.
       base::ThreadRestrictions::ScopedAllowIO allow_io;
 
@@ skipping 72 matching lines @@
        ++iter) {
     const Extension* extension = *iter;
     Manifest::Location location = extension->location();
     Manifest::Type type = extension->GetType();
 
     // For the first few metrics, include all extensions and apps (component,
     // unpacked, etc). It's good to know these locations, and it doesn't
     // muck up any of the stats. Later, though, we want to omit component and
     // unpacked, as they are less interesting.
     if (extension->is_app())
-      UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location, 100);
+      UMA_HISTOGRAM_ENUMERATION(
+          "Extensions.AppLocation", location, Manifest::NUM_LOCATIONS);
     else if (extension->is_extension())
-      UMA_HISTOGRAM_ENUMERATION("Extensions.ExtensionLocation", location, 100);
+      UMA_HISTOGRAM_ENUMERATION(
+          "Extensions.ExtensionLocation", location, Manifest::NUM_LOCATIONS);
 
     if (!ManifestURL::UpdatesFromGallery(extension)) {
       UMA_HISTOGRAM_ENUMERATION(
-          "Extensions.NonWebstoreLocation", location, 100);
+          "Extensions.NonWebstoreLocation", location, Manifest::NUM_LOCATIONS);
 
       // Check for inconsistencies if the extension was supposedly installed
       // from the webstore.
       enum {
         BAD_UPDATE_URL = 0,
         // This value was a mistake. Turns out sideloaded extensions can
         // have the from_webstore bit if they update from the webstore.
         DEPRECATED_IS_EXTERNAL = 1,
       };
       if (extension->from_webstore()) {
@@ skipping 27 matching lines @@
         URLOverrides::GetChromeURLOverrides(extension).count("newtab")) {
       ++non_webstore_ntp_override_count;
     }
 
     // Don't count unpacked extensions anymore, either.
     if (Manifest::IsUnpackedLocation(location))
       continue;
 
     UMA_HISTOGRAM_ENUMERATION("Extensions.ManifestVersion",
                               extension->manifest_version(),
                               10);

[jar (doing other things), 2014/05/21 22:12:45]

nit: I was curious about this bound (10)... but I think the histogram has not
yet been added to the xml file... so I really can't see if we've overrun, or
what is going on.

Please (in this CL) at least add this name and description to the xml file....
I'm fine with leaving this line as is for now (this CL)....

but if we really have large numbers, we'd probably switch to using:
UMA_HISTOGRAM_SPARSE_SLOWLY()
...but that is certainly a different CL.

[not at google - send to devlin, 2014/05/21 22:31:07]

Looks like there are actually 4 values that are being logged in this file but
don't have entries in histograms.xml:

1. Extensions.IDChangedError
2. Extensions.LoadAllUser
3. Extensions.ManifestReloadEnumValue
4. Extensions.ManifestVersion


(1) I might just delete, nobody has cared about it for a long time (the guy who
added it is long gone).
(2) I might add to histograms.xml in this patch
(3) I have no idea what this is even really trying to do, so I'd have problems
adding it to the histograms.xml file. I might just delete it (SGTY? It'll clean
up a bit more code in the process).
(4) We can actually get a decent approximation of this data by looking through
the items that are checked into the webstore, but it's conceivable we'll want to
know outside webstore, so I'll add to this patch.

[not at google - send to devlin, 2014/05/21 23:17:22]

Actually I'm going to delete (2). It's not very interesting, we already have
other data which is basically the same and clearly nobody has cared about it.

 
     // We might have wanted to count legacy packaged apps here, too, since they
     // are effectively extensions. Unfortunately, it's too late, as we don't
     // want to mess up the existing stats.
     if (type == Manifest::TYPE_EXTENSION) {
       UMA_HISTOGRAM_ENUMERATION("Extensions.BackgroundPageType",
                                 GetBackgroundPageType(extension),
-                                10);
+                                NUM_BACKGROUND_PAGE_TYPES);
     }
 
     // Using an enumeration shows us the total installed ratio across all users.
     // Using the totals per user at each startup tells us the distribution of
     // usage for each user (e.g. 40% of users have at least one app installed).
-    UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.LoadType", type, Manifest::NUM_LOAD_TYPES);
     switch (type) {
       case Manifest::TYPE_THEME:
         ++theme_count;
         break;
       case Manifest::TYPE_USER_SCRIPT:
         ++user_script_count;
         break;
       case Manifest::TYPE_HOSTED_APP:
         ++hosted_app_count;
         if (Manifest::IsExternalLocation(location)) {
@@ skipping 138 matching lines @@
 int InstalledLoader::GetCreationFlags(const ExtensionInfo* info) {
   int flags = extension_prefs_->GetCreationFlags(info->extension_id);
   if (!Manifest::IsUnpackedLocation(info->extension_location))
     flags |= Extension::REQUIRE_KEY;
   if (extension_prefs_->AllowFileAccess(info->extension_id))
     flags |= Extension::ALLOW_FILE_ACCESS;
   return flags;
 }
 
 }  // namespace extensions