Propogate RAPPOR permission changes

chrome/browser/metrics/metrics_services_manager.cc

Index: chrome/browser/metrics/metrics_services_manager.cc
diff --git a/chrome/browser/metrics/metrics_services_manager.cc b/chrome/browser/metrics/metrics_services_manager.cc
index 8f9384f3350131405e390fe51f1d6f06f9a4827b..8d7c720c2d81b0e40740a9ceadca9bd553c59f29 100644
--- a/chrome/browser/metrics/metrics_services_manager.cc
+++ b/chrome/browser/metrics/metrics_services_manager.cc
@@ -56,8 +56,14 @@ void PostStoreMetricsClientInfo(const metrics::ClientInfo& 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() {
@@ -147,8 +153,15 @@ rappor::RecordingLevel MetricsServicesManager::GetRapporRecordingLevel(
   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();
@@ -175,7 +188,7 @@ void MetricsServicesManager::UpdatePermissions(bool may_record,
     metrics->Stop();
   }
 
-  GetRapporService()->Update(GetRapporRecordingLevel(may_record), may_upload);
+  UpdateRapporService();
 }
 
 // TODO(asvitkine): This function does not report the correct value on Android,