[Metrics] Make MetricsStateManager take a callback param to check if UMA is enabled.

Also, remove a no longer needed testing pref.

BUG=374296
TEST=compiles
R=asvitkine@chromium.org

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

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

chrome/browser/metrics/metrics_service_unittest.cc

@@ -6,10 +6,9 @@
 
 #include <string>
 
-#include "base/command_line.h"
+#include "base/bind.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/metrics/metrics_state_manager.h"
-#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -90,8 +89,12 @@ class MetricsServiceTest : public testing::Test {
  public:
   MetricsServiceTest()
       : testing_local_state_(TestingBrowserProcess::GetGlobal()),
-        metrics_state_manager_(metrics::MetricsStateManager::Create(
-            GetLocalState())) {
+        is_metrics_reporting_enabled_(false),
+        metrics_state_manager_(
+            metrics::MetricsStateManager::Create(
+                GetLocalState(),
+                base::Bind(&MetricsServiceTest::is_metrics_reporting_enabled,
+                           base::Unretained(this)))) {
   }
 
   virtual ~MetricsServiceTest() {
@@ -108,9 +111,7 @@ class MetricsServiceTest : public testing::Test {
 
   // Sets metrics reporting as enabled for testing.
   void EnableMetricsReporting() {
-    // TODO(asvitkine): Refactor the code to not need this flag and delete it.
-    CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableMetricsReportingForTesting);
+    is_metrics_reporting_enabled_ = true;
   }
 
   // Waits until base::TimeTicks::Now() no longer equals |value|. This should
@@ -139,8 +140,13 @@ class MetricsServiceTest : public testing::Test {
   }
 
  private:
+  bool is_metrics_reporting_enabled() const {
+    return is_metrics_reporting_enabled_;
+  }
+
   content::TestBrowserThreadBundle thread_bundle_;
   ScopedTestingLocalState testing_local_state_;
+  bool is_metrics_reporting_enabled_;
   scoped_ptr<metrics::MetricsStateManager> metrics_state_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsServiceTest);

chrome/browser/metrics/metrics_services_manager.cc

@@ -4,11 +4,18 @@
 
 #include "chrome/browser/metrics/metrics_services_manager.h"
 
+#include "base/command_line.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/metrics/metrics_state_manager.h"
 #include "chrome/browser/metrics/variations/variations_service.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "components/rappor/rappor_service.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#endif
+
 MetricsServicesManager::MetricsServicesManager(PrefService* local_state)
     : local_state_(local_state) {
   DCHECK(local_state);
@@ -44,7 +51,32 @@ MetricsServicesManager::GetVariationsService() {
 
 metrics::MetricsStateManager* MetricsServicesManager::GetMetricsStateManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!metrics_state_manager_)
-    metrics_state_manager_ = metrics::MetricsStateManager::Create(local_state_);
+  if (!metrics_state_manager_) {
+    metrics_state_manager_ = metrics::MetricsStateManager::Create(
+        local_state_,
+        base::Bind(&MetricsServicesManager::IsMetricsReportingEnabled,
+                   base::Unretained(this)));
+  }
   return metrics_state_manager_.get();
 }
+
+// TODO(asvitkine): This function does not report the correct value on Android,
+// see http://crbug.com/362192.
+bool MetricsServicesManager::IsMetricsReportingEnabled() const {
+  // If the user permits metrics reporting with the checkbox in the
+  // prefs, we turn on recording.  We disable metrics completely for
+  // non-official builds, or when field trials are forced.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kForceFieldTrials))
+    return false;
+
+  bool enabled = false;
+#if defined(GOOGLE_CHROME_BUILD)
+#if defined(OS_CHROMEOS)
+  chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
+                                            &enabled);
+#else
+  enabled = local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
+#endif  // #if defined(OS_CHROMEOS)
+#endif  // defined(GOOGLE_CHROME_BUILD)
+  return enabled;
+}

chrome/browser/metrics/metrics_services_manager.h

@@ -45,6 +45,9 @@ class MetricsServicesManager {
  private:
   metrics::MetricsStateManager* GetMetricsStateManager();
 
+  // Returns true iff metrics reporting is enabled.
+  bool IsMetricsReportingEnabled() const;
+
   // Ensures that all functions are called from the same thread.
   base::ThreadChecker thread_checker_;
 

chrome/browser/metrics/metrics_state_manager.cc

@@ -19,10 +19,6 @@
 #include "chrome/common/pref_names.h"
 #include "components/variations/caching_permuted_entropy_provider.h"
 
-#if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/settings/cros_settings.h"
-#endif
-
 namespace metrics {
 
 namespace {
@@ -47,8 +43,11 @@ int GenerateLowEntropySource() {
 // static
 bool MetricsStateManager::instance_exists_ = false;
 
-MetricsStateManager::MetricsStateManager(PrefService* local_state)
+MetricsStateManager::MetricsStateManager(
+    PrefService* local_state,
+    const base::Callback<bool(void)>& is_reporting_enabled_callback)
     : local_state_(local_state),
+      is_reporting_enabled_callback_(is_reporting_enabled_callback),
       low_entropy_source_(kLowEntropySourceNotSet),
       entropy_source_returned_(ENTROPY_SOURCE_NONE) {
   ResetMetricsIDsIfNecessary();
@@ -65,27 +64,7 @@ MetricsStateManager::~MetricsStateManager() {
 }
 
 bool MetricsStateManager::IsMetricsReportingEnabled() {
-  // If the user permits metrics reporting with the checkbox in the
-  // prefs, we turn on recording.  We disable metrics completely for
-  // non-official builds.  This can be forced with a flag.
-  const CommandLine* command_line = CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kEnableMetricsReportingForTesting))
-    return true;
-
-  // Disable metrics reporting when field trials are forced.
-  if (command_line->HasSwitch(switches::kForceFieldTrials))
-    return false;
-
-  bool enabled = false;
-#if defined(GOOGLE_CHROME_BUILD)
-#if defined(OS_CHROMEOS)
-  chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
-                                            &enabled);
-#else
-  enabled = local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
-#endif  // #if defined(OS_CHROMEOS)
-#endif  // defined(GOOGLE_CHROME_BUILD)
-  return enabled;
+  return is_reporting_enabled_callback_.Run();
 }
 
 void MetricsStateManager::ForceClientIdCreation() {
@@ -161,11 +140,14 @@ MetricsStateManager::CreateEntropyProvider() {
 
 // static
 scoped_ptr<MetricsStateManager> MetricsStateManager::Create(
-    PrefService* local_state) {
+    PrefService* local_state,
+    const base::Callback<bool(void)>& is_reporting_enabled_callback) {
   scoped_ptr<MetricsStateManager> result;
   // Note: |instance_exists_| is updated in the constructor and destructor.
-  if (!instance_exists_)
-    result.reset(new MetricsStateManager(local_state));
+  if (!instance_exists_) {
+    result.reset(
+        new MetricsStateManager(local_state, is_reporting_enabled_callback));
+  }
   return result.Pass();
 }
 

chrome/browser/metrics/metrics_state_manager.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
@@ -27,8 +28,6 @@ class MetricsStateManager {
   virtual ~MetricsStateManager();
 
   // Returns true if the user opted in to sending metric reports.
-  // TODO(asvitkine): This function does not report the correct value on
-  // Android, see http://crbug.com/362192.
   bool IsMetricsReportingEnabled();
 
   // Returns the client ID for this client, or the empty string if the user is
@@ -56,7 +55,9 @@ class MetricsStateManager {
 
   // Creates the MetricsStateManager, enforcing that only a single instance
   // of the class exists at a time. Returns NULL if an instance exists already.
-  static scoped_ptr<MetricsStateManager> Create(PrefService* local_state);
+  static scoped_ptr<MetricsStateManager> Create(
+      PrefService* local_state,
+      const base::Callback<bool(void)>& is_reporting_enabled_callback);
 
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
@@ -78,10 +79,13 @@ class MetricsStateManager {
     ENTROPY_SOURCE_HIGH,
   };
 
-  // Creates the MetricsStateManager with the given |local_state|. Clients
-  // should instead use Create(), which enforces a single instance of this class
-  // is alive at any given time.
-  explicit MetricsStateManager(PrefService* local_state);
+  // Creates the MetricsStateManager with the given |local_state|. Calls
+  // |is_reporting_enabled_callback| to query whether metrics reporting is
+  // enabled. Clients should instead use Create(), which enforces a single
+  // instance of this class is alive at any given time.
+  MetricsStateManager(
+      PrefService* local_state,
+      const base::Callback<bool(void)>& is_reporting_enabled_callback);
 
   // Returns the low entropy source for this client. This is a random value
   // that is non-identifying amongst browser clients. This method will
@@ -104,7 +108,9 @@ class MetricsStateManager {
   static bool instance_exists_;
 
   // Weak pointer to the local state prefs store.
-  PrefService* local_state_;
+  PrefService* const local_state_;
+
+  const base::Callback<bool(void)> is_reporting_enabled_callback_;
 
   // The identifier that's sent to the server with the log reports.
   std::string client_id_;

chrome/browser/metrics/metrics_state_manager_unittest.cc

@@ -7,6 +7,7 @@
 #include <ctype.h>
 #include <string>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/prefs/testing_pref_service.h"
 #include "chrome/common/chrome_switches.h"
@@ -19,18 +20,32 @@ namespace metrics {
 
 class MetricsStateManagerTest : public testing::Test {
  public:
-  MetricsStateManagerTest() {
+  MetricsStateManagerTest() : is_metrics_reporting_enabled_(false) {
     MetricsStateManager::RegisterPrefs(prefs_.registry());
   }
 
   scoped_ptr<MetricsStateManager> CreateStateManager() {
-    return MetricsStateManager::Create(&prefs_).Pass();
+    return MetricsStateManager::Create(
+        &prefs_,
+        base::Bind(&MetricsStateManagerTest::is_metrics_reporting_enabled,
+                   base::Unretained(this))).Pass();
+  }
+
+  // Sets metrics reporting as enabled for testing.
+  void EnableMetricsReporting() {
+    is_metrics_reporting_enabled_ = true;
   }
 
  protected:
   TestingPrefServiceSimple prefs_;
 
  private:
+  bool is_metrics_reporting_enabled() const {
+    return is_metrics_reporting_enabled_;
+  }
+
+  bool is_metrics_reporting_enabled_;
+
   DISALLOW_COPY_AND_ASSIGN(MetricsStateManagerTest);
 };
 
@@ -59,9 +74,7 @@ TEST_F(MetricsStateManagerTest, EntropySourceUsed_Low) {
 }
 
 TEST_F(MetricsStateManagerTest, EntropySourceUsed_High) {
-  CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kEnableMetricsReportingForTesting);
-
+  EnableMetricsReporting();
   scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
   state_manager->CreateEntropyProvider();
   EXPECT_EQ(MetricsStateManager::ENTROPY_SOURCE_HIGH,

chrome/common/chrome_switches.cc

@@ -516,12 +516,6 @@ const char kEnableIPv6[]                    = "enable-ipv6";
 // Enables experimentation with launching ephemeral apps via hyperlinks.
 const char kEnableLinkableEphemeralApps[]   = "enable-linkable-ephemeral-apps";
 
-// Enables metrics recording and reporting in the browser startup sequence, as
-// if this was an official Chrome build where the user allowed metrics
-// reporting. This is used for testing only.
-const char kEnableMetricsReportingForTesting[] =
-    "enable-metrics-reporting-for-testing";
-
 // Runs the Native Client inside the renderer process and enables GPU plugin
 // (internally adds lEnableGpuPlugin to the command line).
 const char kEnableNaCl[]                    = "enable-nacl";

chrome/common/chrome_switches.h

@@ -151,7 +151,6 @@ extern const char kEnableFastUnload[];
 extern const char kEnableIPv6[];
 extern const char kEnableLinkableEphemeralApps[];
 extern const char kEnableManagedStorage[];
-extern const char kEnableMetricsReportingForTesting[];
 extern const char kEnableNaCl[];
 extern const char kEnableNetBenchmarking[];
 extern const char kEnableNetworkTime[];