Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side

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 44 matching lines @@
 #include "chrome/browser/component_updater/supervised_user_whitelist_installer.h"
 #include "chrome/browser/component_updater/swiftshader_component_installer.h"
 #include "chrome/browser/component_updater/widevine_cdm_component_installer.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/gpu/gl_string_manager.h"
 #include "chrome/browser/gpu/three_d_api_observer.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/memory/tab_manager.h"
 #include "chrome/browser/metrics/field_trial_synchronizer.h"
+#include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/browser/metrics/thread_watcher.h"
 #include "chrome/browser/nacl_host/nacl_browser_delegate_impl.h"
 #include "chrome/browser/net/crl_set_fetcher.h"
 #include "chrome/browser/performance_monitor/performance_monitor.h"
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/power/process_power_collector.h"
 #include "chrome/browser/prefs/chrome_pref_service_factory.h"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/prefs/pref_metrics_service.h"
@@ skipping 650 matching lines @@
     case version_info::Channel::DEV:
     case version_info::Channel::BETA:
     case version_info::Channel::STABLE:
       // Don't enable instrumentation.
       break;
   }
 
   // Register a synthetic field trial for the sampling profiler configuration
   // that was already chosen.
   sampling_profiler_config_.RegisterSyntheticFieldTrial();
+
+#if defined(OS_CHROMEOS)
+  // Set kMetricsReportingEnabled based on kStatsReportingPref device setting as
+  // it is more reliable for Chrome OS.

[Mattias Nissler (ping if slow), 2015/10/28 09:51:15]

suggestion: s/it is more reliable for Chrome OS/it is the source of truth on
Chrome OS/.

[gayane -on leave until 09-2017, 2015/10/28 15:00:07]

Done.

+  bool enable_metrics = false;
+  chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
+                                            &enable_metrics);

[Mattias Nissler (ping if slow), 2015/10/28 09:51:15]

I'd expect an observer to be installed here to re-sync the pref after the device
setting changes?

[gayane -on leave until 09-2017, 2015/10/28 15:00:06]

you mean besides initialization? My understanding is that there is no scenario
when kStatsReportingPref will change which needs to also affect
kMetricsReportingEnabled.

Now kStatsReportingPref changes its value only in two cases. 
 - First when policy is applied. in this case kMetricsReportingEnabled gets
directly changed by policy
 - Second when user changes kMetricsReportingEnabled then kStatsReportingPref
gets updated accordingly.

[Mattias Nissler (ping if slow), 2015/10/29 10:06:14]

I guess you're referring to the CrosSettings::SetBoolean call in
browser_options_handler.cc here, correct? While in theory what you say is
correct, the assumption that whatever Chrome writes is fragile. I'd prefer a
solution that enables/disables metrics reporting based on the actual ground
truth value reported by device settings, and not just Chrome's idea what it
should be.

For example, we do have some code in session_manager that may sanitize certain
device settings. While it currently doesn't touch the metrics reporting flag,
the existence of that code shows that second-guessing the behavior of device
settings is not the best of ideas.

[Alexei Svitkine (slow), 2015/10/29 17:04:14]

Can the observer change be done in a later CL? I think it would be a bit
non-trivial, since it also requires toggling UMA service state and other things,
and for that kind of more complicated logic we probably need test coverage. How
about having a TODO comment with an associated crbug for this CL? Especially
that you say it can't change right with the current code right now.

[Mattias Nissler (ping if slow), 2015/11/02 11:09:58]

Note that the old code does handle device settings changes in
device_settings_provider.cc and switches metrics reporting state appropriately,
so this CL would essentially be a step back on this aspect. For this reason, I'm
feeling a bit uncomfortable with a bug + TODO - how about we make the functional
change as part of this CL and then postpone test coverage to a follow-up?

+  InitiateMetricsReportingChange(enable_metrics,
+                                 OnMetricsReportingCallbackType());
+#endif
 }
 
 // ChromeBrowserMainParts: |SetupMetricsAndFieldTrials()| related --------------
 
 void ChromeBrowserMainParts::StartMetricsRecording() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::StartMetricsRecording");
 
   g_browser_process->metrics_service()->CheckForClonedInstall(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
 
@@ skipping 1103 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif  // defined(OS_CHROMEOS)
 #endif  // defined(OS_ANDROID)
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }