Refactor HashedExtensionMetrics into ExtensionsMetricsProvider.

Transforms HashedExtensionMetrics into a metrics::MetricsProvider, eliminating
direct knowledge of this class from MetricsLog. Renames the class to
ExtensionsMetricsProvider. Notably, changes the class to take in the
MetricsStateManager and later obtain the client ID from the manager rather than
directly taking in the ID; this change is necessary because the provider
instance is now constructed before the ID is determined.

BUG=374225

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

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

chrome/browser/metrics/extension_metrics.cc → chrome/browser/metrics/extensions_metrics_provider.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/metrics/extension_metrics.h"
+#include "chrome/browser/metrics/extensions_metrics_provider.h"
 
 #include <set>
 
@@ -10,7 +10,9 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/metrics_state_manager.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "components/metrics/metrics_log_base.h"
 #include "components/metrics/proto/system_profile.pb.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension_set.h"
@@ -29,14 +31,18 @@ const size_t kExtensionListBuckets = 1024;
 
 }  // namespace
 
-HashedExtensionMetrics::HashedExtensionMetrics(uint64 client_id)
-    : client_key_(client_id % kExtensionListClientKeys),
-      cached_profile_(NULL) {}
-HashedExtensionMetrics::~HashedExtensionMetrics() {}
+ExtensionsMetricsProvider::ExtensionsMetricsProvider(
+    metrics::MetricsStateManager* metrics_state_manager)
+    : metrics_state_manager_(metrics_state_manager), cached_profile_(NULL) {
+  DCHECK(metrics_state_manager_);
+}
+
+ExtensionsMetricsProvider::~ExtensionsMetricsProvider() {
+}
 
 // static
-int HashedExtensionMetrics::HashExtension(const std::string& extension_id,
-                                          uint32 client_key) {
+int ExtensionsMetricsProvider::HashExtension(const std::string& extension_id,
+                                             uint32 client_key) {
   DCHECK_LE(client_key, kExtensionListClientKeys);
   std::string message =
       base::StringPrintf("%u:%s", client_key, extension_id.c_str());
@@ -44,7 +50,7 @@ int HashedExtensionMetrics::HashExtension(const std::string& extension_id,
   return output % kExtensionListBuckets;
 }
 
-Profile* HashedExtensionMetrics::GetMetricsProfile() {
+Profile* ExtensionsMetricsProvider::GetMetricsProfile() {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   if (!profile_manager)
     return NULL;
@@ -68,7 +74,7 @@ Profile* HashedExtensionMetrics::GetMetricsProfile() {
 }
 
 scoped_ptr<extensions::ExtensionSet>
-HashedExtensionMetrics::GetInstalledExtensions() {
+ExtensionsMetricsProvider::GetInstalledExtensions() {
 #if defined(ENABLE_EXTENSIONS)
   // UMA reports do not support multiple profiles, but extensions are installed
   // per-profile.  We return the extensions installed in the primary profile.
@@ -83,20 +89,30 @@ HashedExtensionMetrics::GetInstalledExtensions() {
   return scoped_ptr<extensions::ExtensionSet>();
 }
 
-void HashedExtensionMetrics::WriteExtensionList(
+uint64 ExtensionsMetricsProvider::GetClientID() {
+  // TODO(blundell): Create a MetricsLogBase::ClientIDAsInt() API and call it
+  // here as well as in MetricsLogBases's population of the client_id field of
+  // the uma_proto.
+  return metrics::MetricsLogBase::Hash(metrics_state_manager_->client_id());
+}
+
+void ExtensionsMetricsProvider::ProvideSystemProfileMetrics(
     metrics::SystemProfileProto* system_profile) {
   scoped_ptr<extensions::ExtensionSet> extensions(GetInstalledExtensions());
   if (!extensions)
     return;
 
+  const int client_key = GetClientID() % kExtensionListClientKeys;
+
   std::set<int> buckets;
   for (extensions::ExtensionSet::const_iterator it = extensions->begin();
-       it != extensions->end(); ++it) {
-    buckets.insert(HashExtension((*it)->id(), client_key_));
+       it != extensions->end();
+       ++it) {
+    buckets.insert(HashExtension((*it)->id(), client_key));
   }
 
-  for (std::set<int>::const_iterator it = buckets.begin();
-       it != buckets.end(); ++it) {
+  for (std::set<int>::const_iterator it = buckets.begin(); it != buckets.end();
+       ++it) {
     system_profile->add_occupied_extension_bucket(*it);
   }
 }

chrome/browser/metrics/extension_metrics.h → chrome/browser/metrics/extensions_metrics_provider.h

@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_METRICS_EXTENSION_METRICS_H_
-#define CHROME_BROWSER_METRICS_EXTENSION_METRICS_H_
+#ifndef CHROME_BROWSER_METRICS_EXTENSIONS_METRICS_PROVIDER_H_
+#define CHROME_BROWSER_METRICS_EXTENSIONS_METRICS_PROVIDER_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
+#include "components/metrics/metrics_provider.h"
 
 class Profile;
 
@@ -17,31 +18,40 @@ class ExtensionSet;
 }
 
 namespace metrics {
+class MetricsStateManager;
 class SystemProfileProto;
 }
 
-// HashedExtensionMetrics groups various constants and functions used for
+// ExtensionsMetricsProvider groups various constants and functions used for
 // reporting extension IDs with UMA reports (after hashing the extension IDs
 // for privacy).
-class HashedExtensionMetrics {
+class ExtensionsMetricsProvider : public metrics::MetricsProvider {
  public:
-  explicit HashedExtensionMetrics(uint64 client_id);
-  virtual ~HashedExtensionMetrics();
+  // Holds on to |metrics_state_manager|, which must outlive this object, as a
+  // weak pointer.
+  explicit ExtensionsMetricsProvider(
+      metrics::MetricsStateManager* metrics_state_manager);
+  virtual ~ExtensionsMetricsProvider();
+
+  // metrics::MetricsProvider:
 
   // Writes the hashed list of installed extensions into the specified
   // SystemProfileProto object.
-  void WriteExtensionList(metrics::SystemProfileProto* system_profile);
+  virtual void ProvideSystemProfileMetrics(
+      metrics::SystemProfileProto* system_profile) OVERRIDE;
 
  protected:
-  // Retrieves the set of extensions installed in the current profile.  The
-  // default implementation should be fine, but this can be overridden for
-  // testing.
-  //
+  // Exposed for the sake of mocking in test code.
+
+  // Retrieves the set of extensions installed in the current profile.
   // TODO(mvrable): If metrics are ever converted to being per-profile, then
   // this should be updated to return extensions installed in a specified
   // profile.
   virtual scoped_ptr<extensions::ExtensionSet> GetInstalledExtensions();
 
+  // Retrieves the client ID.
+  virtual uint64 GetClientID();
+
   // Hashes the extension extension ID using the provided client key (which
   // must be less than kExtensionListClientKeys) and to produce an output value
   // between 0 and kExtensionListBuckets-1.
@@ -53,15 +63,15 @@ class HashedExtensionMetrics {
   // same value so that reported extensions are consistent.
   Profile* GetMetricsProfile();
 
-  // The key used when hashing extension identifiers, derived from client_id.
-  const int client_key_;
+  // The MetricsStateManager from which the client ID is obtained.
+  metrics::MetricsStateManager* metrics_state_manager_;
 
   // The profile for which extensions are gathered.  Once a profile is found
   // its value is cached here so that GetMetricsProfile() can return a
   // consistent value.
   Profile* cached_profile_;
 
-  DISALLOW_COPY_AND_ASSIGN(HashedExtensionMetrics);
+  DISALLOW_COPY_AND_ASSIGN(ExtensionsMetricsProvider);
 };
 
-#endif  // CHROME_BROWSER_METRICS_EXTENSION_METRICS_H_
+#endif  // CHROME_BROWSER_METRICS_EXTENSIONS_METRICS_PROVIDER_H_

chrome/browser/metrics/extension_metrics_unittest.cc → chrome/browser/metrics/extensions_metrics_provider_unittest.cc

@@ -2,10 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/metrics/extension_metrics.h"
+#include "chrome/browser/metrics/extensions_metrics_provider.h"
 
 #include <string>
 
+#include "base/prefs/testing_pref_service.h"
+#include "chrome/browser/metrics/metrics_state_manager.h"
 #include "components/metrics/proto/system_profile.pb.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
@@ -14,13 +16,18 @@
 
 namespace {
 
-class TestHashedExtensionMetrics : public HashedExtensionMetrics {
+bool IsMetricsReportingEnabled() {
+  return true;
+}
+
+class TestExtensionsMetricsProvider : public ExtensionsMetricsProvider {
  public:
-  explicit TestHashedExtensionMetrics(uint64 client_id)
-      : HashedExtensionMetrics(client_id) {}
+  explicit TestExtensionsMetricsProvider(
+      metrics::MetricsStateManager* metrics_state_manager)
+      : ExtensionsMetricsProvider(metrics_state_manager) {}
 
   // Makes the protected HashExtension method available to testing code.
-  using HashedExtensionMetrics::HashExtension;
+  using ExtensionsMetricsProvider::HashExtension;
 
  protected:
   // Override the GetInstalledExtensions method to return a set of extensions
@@ -56,29 +63,42 @@ class TestHashedExtensionMetrics : public HashedExtensionMetrics {
     extensions->Insert(extension);
     return extensions.Pass();
   }
+
+  // Override GetClientID() to return a specific value on which test
+  // expectations are based.
+  virtual uint64 GetClientID() OVERRIDE { return 0x3f1bfee9; }
 };
 
 }  // namespace
 
 // Checks that the hash function used to hide precise extension IDs produces
 // the expected values.
-TEST(HashedExtensionMetrics, HashExtension) {
-  EXPECT_EQ(978, TestHashedExtensionMetrics::HashExtension(
-      "ahfgeienlihckogmohjhadlkjgocpleb", 0));
-  EXPECT_EQ(10, TestHashedExtensionMetrics::HashExtension(
-      "ahfgeienlihckogmohjhadlkjgocpleb", 3817));
-  EXPECT_EQ(1007, TestHashedExtensionMetrics::HashExtension(
-      "pknkgggnfecklokoggaggchhaebkajji", 3817));
-  EXPECT_EQ(10, TestHashedExtensionMetrics::HashExtension(
-      "mdhofdjgenpkhlmddfaegdjddcecipmo", 3817));
+TEST(ExtensionsMetricsProvider, HashExtension) {
+  EXPECT_EQ(978,
+            TestExtensionsMetricsProvider::HashExtension(
+                "ahfgeienlihckogmohjhadlkjgocpleb", 0));
+  EXPECT_EQ(10,
+            TestExtensionsMetricsProvider::HashExtension(
+                "ahfgeienlihckogmohjhadlkjgocpleb", 3817));
+  EXPECT_EQ(1007,
+            TestExtensionsMetricsProvider::HashExtension(
+                "pknkgggnfecklokoggaggchhaebkajji", 3817));
+  EXPECT_EQ(10,
+            TestExtensionsMetricsProvider::HashExtension(
+                "mdhofdjgenpkhlmddfaegdjddcecipmo", 3817));
 }
 
 // Checks that the fake set of extensions provided by
-// TestHashedExtensionMetrics is encoded properly.
-TEST(HashedExtensionMetrics, SystemProtoEncoding) {
+// TestExtensionsMetricsProvider is encoded properly.
+TEST(ExtensionsMetricsProvider, SystemProtoEncoding) {
   metrics::SystemProfileProto system_profile;
-  TestHashedExtensionMetrics extension_metrics(0x3f1bfee9);
-  extension_metrics.WriteExtensionList(&system_profile);
+  TestingPrefServiceSimple local_state;
+  metrics::MetricsStateManager::RegisterPrefs(local_state.registry());
+  scoped_ptr<metrics::MetricsStateManager> metrics_state_manager(
+      metrics::MetricsStateManager::Create(&local_state,
+                base::Bind(&IsMetricsReportingEnabled)));
+  TestExtensionsMetricsProvider extension_metrics(metrics_state_manager.get());
+  extension_metrics.ProvideSystemProfileMetrics(&system_profile);
   ASSERT_EQ(2, system_profile.occupied_extension_bucket_size());
   EXPECT_EQ(10, system_profile.occupied_extension_bucket(0));
   EXPECT_EQ(1007, system_profile.occupied_extension_bucket(1));

chrome/browser/metrics/metrics_log.cc

@@ -25,7 +25,6 @@
 #include "base/time/time.h"
 #include "base/tracked_objects.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/metrics/extension_metrics.h"
 #include "chrome/common/pref_names.h"
 #include "components/metrics/metrics_provider.h"
 #include "components/metrics/metrics_service_client.h"
@@ -192,8 +191,7 @@ MetricsLog::MetricsLog(const std::string& client_id,
                      log_type,
                      client->GetVersionString()),
       client_(client),
-      creation_time_(base::TimeTicks::Now()),
-      extension_metrics_(uma_proto()->client_id()) {
+      creation_time_(base::TimeTicks::Now()) {
   uma_proto()->mutable_system_profile()->set_channel(client_->GetChannel());
 
 #if defined(OS_CHROMEOS)
@@ -387,8 +385,6 @@ void MetricsLog::RecordEnvironment(
   cpu->set_vendor_name(cpu_info.vendor_name());
   cpu->set_signature(cpu_info.signature());
 
-  extension_metrics_.WriteExtensionList(uma_proto()->mutable_system_profile());
-
   std::vector<ActiveGroupId> field_trial_ids;
   GetFieldTrialIds(&field_trial_ids);
   WriteFieldTrials(field_trial_ids, system_profile);

chrome/browser/metrics/metrics_log.h

@@ -12,12 +12,10 @@
 #include <vector>
 
 #include "base/basictypes.h"
-#include "chrome/browser/metrics/extension_metrics.h"
 #include "chrome/common/variations/variations_util.h"
 #include "components/metrics/metrics_log_base.h"
 #include "ui/gfx/size.h"
 
-class HashedExtensionMetrics;
 class PrefService;
 
 #if defined(OS_CHROMEOS)
@@ -149,9 +147,6 @@ class MetricsLog : public metrics::MetricsLogBase {
   // The time when the current log was created.
   const base::TimeTicks creation_time_;
 
-  // For including information on which extensions are installed in reports.
-  HashedExtensionMetrics extension_metrics_;
-
   DISALLOW_COPY_AND_ASSIGN(MetricsLog);
 };
 

chrome/browser/metrics/metrics_services_manager.cc

@@ -6,6 +6,7 @@
 
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/metrics/extensions_metrics_provider.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/metrics/metrics_state_manager.h"
 #include "chrome/browser/metrics/variations/variations_service.h"
@@ -31,6 +32,9 @@ MetricsService* MetricsServicesManager::GetMetricsService() {
     metrics_service_.reset(
         new MetricsService(GetMetricsStateManager(), &metrics_service_client_));
     metrics_service_client_.set_service(metrics_service_.get());
+    metrics_service_->RegisterMetricsProvider(
+        scoped_ptr<metrics::MetricsProvider>(
+            new ExtensionsMetricsProvider(GetMetricsStateManager())));
   }
   return metrics_service_.get();
 }

chrome/chrome_browser.gypi

@@ -1196,8 +1196,8 @@
         'browser/metrics/chrome_stability_metrics_provider.h',
         'browser/metrics/compression_utils.cc',
         'browser/metrics/compression_utils.h',
-        'browser/metrics/extension_metrics.cc',
-        'browser/metrics/extension_metrics.h',
+        'browser/metrics/extensions_metrics_provider.cc',
+        'browser/metrics/extensions_metrics_provider.h',
         'browser/metrics/field_trial_synchronizer.cc',
         'browser/metrics/field_trial_synchronizer.h',
         'browser/metrics/google_update_metrics_provider_win.cc',

chrome/chrome_tests_unit.gypi

@@ -1083,7 +1083,7 @@
         'browser/metrics/chrome_metrics_service_accessor_unittest.cc',
         'browser/metrics/cloned_install_detector_unittest.cc',
         'browser/metrics/compression_utils_unittest.cc',
-        'browser/metrics/extension_metrics_unittest.cc',
+        'browser/metrics/extensions_metrics_provider_unittest.cc',
         'browser/metrics/gpu_metrics_provider_unittest.cc',
         'browser/metrics/metrics_log_unittest.cc',
         'browser/metrics/metrics_service_unittest.cc',