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

[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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271798

[Alexei Svitkine (slow), 2014-05-19 14:00:11]

LGTM, thanks

https://codereview.chromium.org/284293003/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/284293003/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_state_manager.h:110: PrefService* const
local_state_;
Nit: I don't think this type of const for pointers is very commonly used. It
looks confusing to someone not used to seeing this syntax, so I'd rather not
have it. (Unless it is indeed more widely used in Chromium that I think it is.)

[Ilya Sherman, 2014-05-19 14:03:36]

https://codereview.chromium.org/284293003/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_state_manager.h (right):

https://codereview.chromium.org/284293003/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_state_manager.h:110: PrefService* const
local_state_;
On 2014/05/19 14:00:11, Alexei Svitkine wrote:
> Nit: I don't think this type of const for pointers is very commonly used. It
> looks confusing to someone not used to seeing this syntax, so I'd rather not
> have it. (Unless it is indeed more widely used in Chromium that I think it
is.)

Codesearch suggests it's used a lot.  I like const things, so keeping :)

[Ilya Sherman, 2014-05-19 14:03:39]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-19 14:04:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/284293003/1

[Ilya Sherman, 2014-05-19 15:05:13]

/cc Colin

[commit-bot: I haz the power, 2014-05-19 19:06:25]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-19 19:33:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 19:34:01]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[Ilya Sherman, 2014-05-20 09:10:46]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 09:11:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/284293003/20001

[Ilya Sherman, 2014-05-20 09:13:02]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2014-05-20 09:13:40]

Alexei, mind taking another look?  I updated a couple of test files to fix
bustage.

[Alexei Svitkine (slow), 2014-05-20 09:26:52]

https://codereview.chromium.org/284293003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/284293003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:24: if
(command_line->HasSwitch(switches::kEnableMetricsReportingForTesting))
I think this can be removed now that tests aren't using this (including the pref
name entries).

[Ilya Sherman, 2014-05-20 09:32:21]

https://codereview.chromium.org/284293003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/284293003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:24: if
(command_line->HasSwitch(switches::kEnableMetricsReportingForTesting))
On 2014/05/20 09:26:53, Alexei Svitkine wrote:
> I think this can be removed now that tests aren't using this (including the
pref
> name entries).

Nice!  Done.

[Alexei Svitkine (slow), 2014-05-20 09:36:13]

not lgtm, found some things I missed before

Also, please update CL description to mention you're removing the pref.

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:23: if
(command_line->HasSwitch(switches::kForceFieldTrials))
command_line is not in scope

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:29:
chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
I think you also need to include a header for this.

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:32: enabled =
local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
Oops, just noticed that you don't have local_state_ anymore...

[Ilya Sherman, 2014-05-20 11:34:33]

Updated CL description as well.

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:23: if
(command_line->HasSwitch(switches::kForceFieldTrials))
On 2014/05/20 09:36:14, Alexei Svitkine wrote:
> command_line is not in scope

Heh, you can tell that I'm not a fan of compilation speed on this laptop :P

Fixed.

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:29:
chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
On 2014/05/20 09:36:14, Alexei Svitkine wrote:
> I think you also need to include a header for this.

Done.

https://codereview.chromium.org/284293003/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:32: enabled =
local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
On 2014/05/20 09:36:14, Alexei Svitkine wrote:
> Oops, just noticed that you don't have local_state_ anymore...

Done.

[Alexei Svitkine (slow), 2014-05-20 12:32:53]

lgtm

[Ilya Sherman, 2014-05-20 12:33:48]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 12:34:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/284293003/60001

[commit-bot: I haz the power, 2014-05-20 15:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 15:46:10]

Failed to apply patch for chrome/browser/metrics/metrics_service_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/metrics/metrics_service_unittest.cc
  Hunk #1 FAILED at 6.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/metrics/metrics_service_unittest.cc.rej

Patch:       chrome/browser/metrics/metrics_service_unittest.cc
Index: chrome/browser/metrics/metrics_service_unittest.cc
diff --git a/chrome/browser/metrics/metrics_service_unittest.cc
b/chrome/browser/metrics/metrics_service_unittest.cc
index
11ac829668c50c494cca10db61615a2c70357672..e2aef66c9414c6dcff12dbdc43a2a29ed702aa9a
100644
--- a/chrome/browser/metrics/metrics_service_unittest.cc
+++ b/chrome/browser/metrics/metrics_service_unittest.cc
@@ -6,11 +6,10 @@
 
 #include <string>
 
-#include "base/command_line.h"
+#include "base/bind.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/metrics/metrics_service_observer.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);

[Ilya Sherman, 2014-05-20 15:54:47]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 15:56:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/284293003/80001

[commit-bot: I haz the power, 2014-05-20 20:44:37]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-21 01:41:08]

Change committed as 271798