Propogate RAPPOR permission changes

chrome/browser/metrics/metrics_services_manager.cc

 // Copyright 2014 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/metrics/metrics_services_manager.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 38 matching lines @@
 // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool thread
 // because it needs access to IO and cannot work from UI thread.
 void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {
   content::BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,
       base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, client_info));
 }
 
 }  // namespace
 
 MetricsServicesManager::MetricsServicesManager(PrefService* local_state)
-    : local_state_(local_state) {
+    : local_state_(local_state),
+      may_upload_(false),
+      may_record_(false) {
   DCHECK(local_state);
+  pref_change_registrar_.Init(local_state);
+  pref_change_registrar_.Add(rappor::prefs::kRapporEnabled,
+      base::Bind(&MetricsServicesManager::UpdateRapporService,
+                 base::Unretained(this)));

[Alexei Svitkine (slow), 2015/02/19 15:04:58]

Don't use Unretained. Instead, use a weak pointer factory.

[Steven Holte, 2015/02/19 20:25:12]

I'm not sure I understand the logic here.  Why here and not in
GetMetricsStateManager?  Since pref_change_registrar_ is member of "this", there
is enough information here to know that "this" will outlive the callback.

[Alexei Svitkine (slow), 2015/02/19 20:32:01]

You're right.

I was thinking of the case where the callback would be posted as a task. In that
case, in the meanwhile, this object, the registrar, and everything can go away
in the meanwhile without invalidating the posted task.

But since all the pref stuff is done on UI thread and there's no posting of
tasks, this is safe. 

Technically, it could break if impl. of PrefChangeRegistrar changes to post its
callback. But you're right that other places use this same pattern, so they
would all need to be updated anyway in such a case.

 }
 
 MetricsServicesManager::~MetricsServicesManager() {
 }
 
 metrics::MetricsService* MetricsServicesManager::GetMetricsService() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return GetChromeMetricsServiceClient()->metrics_service();
 }
 
@@ skipping 69 matching lines @@
   if (IsRapporEnabled(metrics_enabled)) {
     recording_level = metrics_enabled ?
                       rappor::FINE_LEVEL :
                       rappor::COARSE_LEVEL;
   }
 #endif  // defined(OS_IOS) || defined(OS_ANDROID)
 #endif  // defined(GOOGLE_CHROME_BUILD)
   return recording_level;
 }
 
+void MetricsServicesManager::UpdateRapporService() {
+  GetRapporService()->Update(GetRapporRecordingLevel(may_record_), may_upload_);

[Alexei Svitkine (slow), 2015/02/19 15:04:58]

I don't understand this.

Now that RAPPOR has its own pref, why is UMA's may_record_ and may_upload_
relevant anymore? Shouldn't it be checked the rappor pref instead? What am I
missing?

[Alexei Svitkine (slow), 2015/02/19 15:13:45]

Ah, just read your other CL and it makes sense now - since without the trial on,
we want to keep using UMA state. Can you add a comment to that extent?

[Steven Holte, 2015/02/19 20:25:12]

That's not the only reason.  Even with the new rappor pref, the UMA state
determines whether we send FINE metrics or only COARSE metrics.

[Alexei Svitkine (slow), 2015/02/19 20:32:01]

Acknowledged.

+}
+
 void MetricsServicesManager::UpdatePermissions(bool may_record,
                                                bool may_upload) {
+  may_record_ = may_record;
+  may_upload_ = may_upload;
+
   metrics::MetricsService* metrics = GetMetricsService();
 
   const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
 
   const bool only_do_metrics_recording =
       cmdline->HasSwitch(switches::kMetricsRecordingOnly) ||
       cmdline->HasSwitch(switches::kEnableBenchmarking);
 
   if (only_do_metrics_recording) {
     metrics->StartRecordingForTests();
     GetRapporService()->Update(rappor::FINE_LEVEL, false);
     return;
   }
 
   if (may_record) {
     if (!metrics->recording_active())
       metrics->Start();
 
     if (may_upload)
       metrics->EnableReporting();
     else
       metrics->DisableReporting();
   } else if (metrics->recording_active() || metrics->reporting_active()) {
     metrics->Stop();
   }
 
-  GetRapporService()->Update(GetRapporRecordingLevel(may_record), may_upload);
+  UpdateRapporService();
 }
 
 // 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 (base::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;
 }