Revert 271798 "[Metrics] Make MetricsStateManager take a callbac..."

Breaks Google Chrome Linux x64 compile:
http://build.chromium.org/p/chromium.chrome/buildstatus?builder=Google%20Chrome%20Linux%20x64&number=50183

> [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

TBR=isherman@chromium.org

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

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

chrome/browser/metrics/metrics_service_unittest.cc

@@ -6,9 +6,10 @@
 
 #include <string>
 
-#include "base/bind.h"
+#include "base/command_line.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"
@@ -89,12 +90,8 @@ class MetricsServiceTest : public testing::Test {
  public:
   MetricsServiceTest()
       : testing_local_state_(TestingBrowserProcess::GetGlobal()),
-        is_metrics_reporting_enabled_(false),
-        metrics_state_manager_(
-            metrics::MetricsStateManager::Create(
-                GetLocalState(),
-                base::Bind(&MetricsServiceTest::is_metrics_reporting_enabled,
-                           base::Unretained(this)))) {
+        metrics_state_manager_(metrics::MetricsStateManager::Create(
+            GetLocalState())) {
   }
 
   virtual ~MetricsServiceTest() {
@@ -111,7 +108,9 @@ class MetricsServiceTest : public testing::Test {
 
   // Sets metrics reporting as enabled for testing.
   void EnableMetricsReporting() {
-    is_metrics_reporting_enabled_ = true;
+    // TODO(asvitkine): Refactor the code to not need this flag and delete it.
+    CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kEnableMetricsReportingForTesting);
   }
 
   // Waits until base::TimeTicks::Now() no longer equals |value|. This should
@@ -140,13 +139,8 @@ 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,18 +4,11 @@
 
 #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);
@@ -51,32 +44,7 @@ MetricsServicesManager::GetVariationsService() {
 
 metrics::MetricsStateManager* MetricsServicesManager::GetMetricsStateManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!metrics_state_manager_) {
-    metrics_state_manager_ = metrics::MetricsStateManager::Create(
-        local_state_,
-        base::Bind(&MetricsServicesManager::IsMetricsReportingEnabled,
-                   base::Unretained(this)));
-  }
+  if (!metrics_state_manager_)
+    metrics_state_manager_ = metrics::MetricsStateManager::Create(local_state_);
   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,9 +45,6 @@ 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,6 +19,10 @@
 #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 {
@@ -43,11 +47,8 @@ int GenerateLowEntropySource() {
 // static
 bool MetricsStateManager::instance_exists_ = false;
 
-MetricsStateManager::MetricsStateManager(
-    PrefService* local_state,
-    const base::Callback<bool(void)>& is_reporting_enabled_callback)
+MetricsStateManager::MetricsStateManager(PrefService* local_state)
     : local_state_(local_state),
-      is_reporting_enabled_callback_(is_reporting_enabled_callback),
       low_entropy_source_(kLowEntropySourceNotSet),
       entropy_source_returned_(ENTROPY_SOURCE_NONE) {
   ResetMetricsIDsIfNecessary();
@@ -64,7 +65,27 @@ MetricsStateManager::~MetricsStateManager() {
 }
 
 bool MetricsStateManager::IsMetricsReportingEnabled() {
-  return is_reporting_enabled_callback_.Run();
+  // 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;
 }
 
 void MetricsStateManager::ForceClientIdCreation() {
@@ -140,14 +161,11 @@ MetricsStateManager::CreateEntropyProvider() {
 
 // static
 scoped_ptr<MetricsStateManager> MetricsStateManager::Create(
-    PrefService* local_state,
-    const base::Callback<bool(void)>& is_reporting_enabled_callback) {
+    PrefService* local_state) {
   scoped_ptr<MetricsStateManager> result;
   // Note: |instance_exists_| is updated in the constructor and destructor.
-  if (!instance_exists_) {
-    result.reset(
-        new MetricsStateManager(local_state, is_reporting_enabled_callback));
-  }
+  if (!instance_exists_)
+    result.reset(new MetricsStateManager(local_state));
   return result.Pass();
 }
 

chrome/browser/metrics/metrics_state_manager.h

@@ -8,7 +8,6 @@
 #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"
@@ -28,6 +27,8 @@ 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
@@ -55,9 +56,7 @@ 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,
-      const base::Callback<bool(void)>& is_reporting_enabled_callback);
+  static scoped_ptr<MetricsStateManager> Create(PrefService* local_state);
 
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
@@ -79,13 +78,10 @@ class MetricsStateManager {
     ENTROPY_SOURCE_HIGH,
   };
 
-  // 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);
+  // 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);
 
   // Returns the low entropy source for this client. This is a random value
   // that is non-identifying amongst browser clients. This method will
@@ -108,9 +104,7 @@ class MetricsStateManager {
   static bool instance_exists_;
 
   // Weak pointer to the local state prefs store.
-  PrefService* const local_state_;
-
-  const base::Callback<bool(void)> is_reporting_enabled_callback_;
+  PrefService* local_state_;
 
   // 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,7 +7,6 @@
 #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"
@@ -20,32 +19,18 @@ namespace metrics {
 
 class MetricsStateManagerTest : public testing::Test {
  public:
-  MetricsStateManagerTest() : is_metrics_reporting_enabled_(false) {
+  MetricsStateManagerTest() {
     MetricsStateManager::RegisterPrefs(prefs_.registry());
   }
 
   scoped_ptr<MetricsStateManager> CreateStateManager() {
-    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;
+    return MetricsStateManager::Create(&prefs_).Pass();
   }
 
  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);
 };
 
@@ -74,7 +59,9 @@ TEST_F(MetricsStateManagerTest, EntropySourceUsed_Low) {
 }
 
 TEST_F(MetricsStateManagerTest, EntropySourceUsed_High) {
-  EnableMetricsReporting();
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableMetricsReportingForTesting);
+
   scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
   state_manager->CreateEntropyProvider();
   EXPECT_EQ(MetricsStateManager::ENTROPY_SOURCE_HIGH,

chrome/common/chrome_switches.cc

@@ -516,6 +516,12 @@ 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,6 +151,7 @@ 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[];