Refactor MetricsStateManager class out of MetricsService.

chrome/browser/chrome_browser_main.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/chrome_browser_main.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 544 matching lines @@
   // Must initialize metrics after labs have been converted into switches,
   // but before field trials are set up (so that client ID is available for
   // one-time randomized field trials).
 #if defined(ARCH_CPU_64_BITS)
   MetricsLog::set_version_extension("-64");
 #endif  // defined(ARCH_CPU_64_BITS)
 
   // Initialize FieldTrialList to support FieldTrials that use one-time
   // randomization.
   MetricsService* metrics = browser_process_->metrics_service();
-  MetricsService::ReportingState reporting_state =
-      IsMetricsReportingEnabled() ? MetricsService::REPORTING_ENABLED :
-                                    MetricsService::REPORTING_DISABLED;
-  if (reporting_state == MetricsService::REPORTING_ENABLED)
-    metrics->ForceClientIdCreation();  // Needed below.
   field_trial_list_.reset(
-      new base::FieldTrialList(
-          metrics->CreateEntropyProvider(reporting_state).release()));
+      new base::FieldTrialList(metrics->CreateEntropyProvider().release()));
 
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kEnableBenchmarking))
     base::FieldTrial::EnableBenchmarking();
 
   // Ensure any field trials specified on the command line are initialized.
   // Also stop the metrics service so that we don't pollute UMA.
   if (command_line->HasSwitch(switches::kForceFieldTrials)) {
     std::set<std::string> unforceable_field_trials;
 #if defined(OFFICIAL_BUILD)
@@ skipping 27 matching lines @@
   // This must be called after the local state is initialized.
   browser_field_trials_.SetupFieldTrials(local_state_);
 
   // Initialize FieldTrialSynchronizer system. This is a singleton and is used
   // for posting tasks via base::Bind. Its deleted when it goes out of scope.
   // Even though base::Bind does AddRef and Release, the object will not be
   // deleted after the Task is executed.
   field_trial_synchronizer_ = new FieldTrialSynchronizer();
 
   // Now that field trials have been created, initializes metrics recording.
-  metrics->InitializeMetricsRecordingState(reporting_state);
+  metrics->InitializeMetricsRecordingState();
 }
 
 // ChromeBrowserMainParts: |SetupMetricsAndFieldTrials()| related --------------
 
 void ChromeBrowserMainParts::StartMetricsRecording() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::StartMetricsRecording");
   MetricsService* metrics = g_browser_process->metrics_service();
 
   const bool only_do_metrics_recording =
       parsed_command_line_.HasSwitch(switches::kMetricsRecordingOnly) ||
       parsed_command_line_.HasSwitch(switches::kEnableBenchmarking);
   if (only_do_metrics_recording) {
     // If we're testing then we don't care what the user preference is, we turn
     // on recording, but not reporting, otherwise tests fail.
     metrics->StartRecordingForTests();
     return;
   }
 
   metrics->CheckForClonedInstall();
-
-  if (IsMetricsReportingEnabled())
-    metrics->Start();
-}
-
-bool ChromeBrowserMainParts::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;
-
-  bool enabled = false;
-  // The debug build doesn't send UMA logs when FieldTrials are forced.
-  if (command_line->HasSwitch(switches::kForceFieldTrials))
-    return 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;
+  const bool metrics_enabled = metrics->StartIfMetricsReportingEnabled();
+  if (metrics_enabled) {
+    browser_process_->rappor_service()->Start(
+        browser_process_->local_state(),
+        browser_process_->system_request_context());

[Ilya Sherman, 2014/05/02 05:12:38]

Hmm, are you sure that it's safe to move this code?  It looks like you might be
disabling RAPPOR on Android by doing so.

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

You're correct that this will disable RAPPOR on Android. I believe that should
be the only behaviour change as a result of this CL.

However, I believe we're not yet using RAPPOR for anything on Android, so I
think this is OK to do. I also actually believe that the previous code was wrong
because ChromeBrowserMainParts::IsMetricsReportingEnabled() didn't use the
correct prefs on Android for UMA enabled state. See http://crbug.com/362192

I think this is a good enough compromise for this CL (especially since it
actually makes things better - i.e. we're not incorrectly enabling RAPPOR when
UMA is off) and because it's very hard to do this refactoring without any
behaviour changes unfortunately. This is in part due to the current broken-ish
situation where different prefs are used on different platforms, so e.g. fixing
the logic will result in a functional change.

My plan to fix this down the line is the following:

-> Change IsMetricsReportingEnabled() to work correctly for Android.
-> Change Android private code to also go through
StartIfMetricsReportingEnabled().

For now, I've also added a TODO to both of our existing
IsMetricsReportingEnabled() functions about their current issues.

I'd prefer to fix the logic in follow-up CLs, so that it's not mixed in with
refactoring.

[Alexei Svitkine (slow), 2014/05/02 15:31:16]

Actually, I think my comment above is not quite correct. Due to the different
prefs issue, I think RAPPOR was already disabled on Android.

To confirm, I tried to look at Rappor.* histograms on Android and there was no
data for them. So I think this CL doesn't actually change any behavior.

+  }
 }
 
 void ChromeBrowserMainParts::RecordBrowserStartupTime() {
   // Don't record any metrics if UI was displayed before this point e.g.
   // warning dialogs.
   if (startup_metric_utils::WasNonBrowserUIDisplayed())
     return;
 
 #if defined(OS_ANDROID)
   // On Android the first run is handled in Java code, and the C++ side of
@@ skipping 410 matching lines @@
 
 int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreMainMessageLoopRunImpl");
   // Android updates the metrics service dynamically depending on whether the
   // application is in the foreground or not. Do not start here.
 #if !defined(OS_ANDROID)
   // Now that the file thread has been started, start recording.
   StartMetricsRecording();
 #endif
 
-  if (IsMetricsReportingEnabled()) {
-    browser_process_->rappor_service()->Start(
-        browser_process_->local_state(),
-        browser_process_->system_request_context());
-  }
-
   // Create watchdog thread after creating all other threads because it will
   // watch the other threads and they must be running.
   browser_process_->watchdog_thread();
 
   // Do any initializating in the browser process that requires all threads
   // running.
   browser_process_->PreMainMessageLoopRun();
 
   // Record last shutdown time into a histogram.
   browser_shutdown::ReadLastShutdownInfo();
@@ skipping 571 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif
 #endif
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }