Remove OnRecordingDisabled() metrics client interface.

This was there so that crash client id could be cleared.
However, this was being called out of MetricsService::Stop(),
so it needed a special provision to not clear it when shutting
down, for example.

This change instead removes all of that plumbing and makes the
id clearing happen in the same place we clear the UMA client id,
when the user actually opts-out (as opposed to just metrics service
stopping). Unifies the prefs manipulation that happens when the
user changes the setting into a helper function.

As a follow up change after this lands, we should also make that
function be called on iOS.

BUG=635669
TBR=sgurun@chromium.org,nyquist@chromium.org,noyau@chromium.org

Review-Url: https://codereview.chromium.org/2248793002
Cr-Commit-Position: refs/heads/master@{#412568}

android_webview/browser/aw_metrics_service_client.cc

@@ -159,13 +159,11 @@ metrics::MetricsService* AwMetricsServiceClient::GetMetricsService() {
 
 // In Chrome, UMA and Breakpad are enabled/disabled together by the same
 // checkbox and they share the same client ID (a.k.a. GUID). SetMetricsClientId
-// and OnRecordingDisabled are intended to provide the ID to Breakpad. In
-// WebView, UMA and Breakpad are independent, so these are no-ops.
+// is intended to provide the ID to Breakpad. In WebView, UMA and Breakpad are
+// independent, so this is a no-op.
 
 void AwMetricsServiceClient::SetMetricsClientId(const std::string& client_id) {}
 
-void AwMetricsServiceClient::OnRecordingDisabled() {}
-
 bool AwMetricsServiceClient::IsOffTheRecordSessionActive() {
   // WebView has no incognito mode.
   return false;

android_webview/browser/aw_metrics_service_client.h

@@ -57,7 +57,6 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   // until initialization has asynchronously finished.
   metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;

blimp/engine/app/blimp_metrics_service_client.cc

@@ -90,13 +90,10 @@ metrics::MetricsService* BlimpMetricsServiceClient::GetMetricsService() {
 
 // In Chrome, UMA and Breakpad are enabled/disabled together by the same
 // checkbox and they share the same client ID (a.k.a. GUID).
-// This is not required by Blimp, so these are no-ops.
+// This is not required by Blimp, so this is a no-op.
 void BlimpMetricsServiceClient::SetMetricsClientId(
     const std::string& client_id) {}
 
-// Recording can not be disabled in Blimp, so this function is a no-op.
-void BlimpMetricsServiceClient::OnRecordingDisabled() {}
-
 bool BlimpMetricsServiceClient::IsOffTheRecordSessionActive() {
   // Blimp does not have incognito mode.
   return false;

blimp/engine/app/blimp_metrics_service_client.h

@@ -47,7 +47,6 @@ class BlimpMetricsServiceClient : public metrics::MetricsServiceClient,
   // metrics::MetricsServiceClient implementation.
   metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;

chrome/browser/android/metrics/uma_session_stats.cc

@@ -108,6 +108,8 @@ static void UpdateMetricsServiceState(JNIEnv* env,
   DCHECK(metrics);
 
   if (metrics->recording_active() != may_record) {
+    UpdateMetricsPrefsOnPermissionChange(may_record);
+
     // This function puts a consent file with the ClientID in the
     // data directory. The ID is passed to the renderer for crash
     // reporting when things go wrong.
@@ -117,17 +119,6 @@ static void UpdateMetricsServiceState(JNIEnv* env,
             may_record));
   }
 
-  // Clear the client id pref when opting out. Note: Mirrors code in
-  // metrics_reporting_state.cc. TODO(asvitkine): Unify.
-  if (!may_record) {
-    // Note: Clearing client id will not affect the running state (e.g. field
-    // trial randomization), as the pref is only read on startup.
-    g_browser_process->local_state()->ClearPref(
-        metrics::prefs::kMetricsClientID);
-    g_browser_process->local_state()->ClearPref(
-        metrics::prefs::kMetricsReportingEnabledTimestamp);
-  }
-
   g_browser_process->GetMetricsServicesManager()->UpdatePermissions(
       may_record, may_upload);
 }

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -284,13 +284,6 @@ void ChromeMetricsServiceClient::SetMetricsClientId(
   crash_keys::SetMetricsClientIdFromGUID(client_id);
 }
 
-void ChromeMetricsServiceClient::OnRecordingDisabled() {
-  // If we're shutting down, don't drop the metrics_client_id, so that late
-  // crashes won't lose it.
-  if (!g_browser_process->IsShuttingDown())
-    crash_keys::ClearMetricsClientId();
-}
-
 bool ChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
   return chrome::IsIncognitoSessionActive();
 }

chrome/browser/metrics/chrome_metrics_service_client.h

@@ -60,7 +60,6 @@ class ChromeMetricsServiceClient
   // metrics::MetricsServiceClient:
   metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;

chrome/browser/metrics/metrics_reporting_state.cc

@@ -9,6 +9,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/common/crash_keys.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "components/metrics/metrics_pref_names.h"
@@ -60,34 +61,16 @@ bool SetGoogleUpdateSettings(bool enabled) {
 void SetMetricsReporting(bool to_update_pref,
                          const OnMetricsReportingCallbackType& callback_fn,
                          bool updated_pref) {
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-
 #if !defined(OS_ANDROID)
   g_browser_process->local_state()->SetBoolean(
       metrics::prefs::kMetricsReportingEnabled, updated_pref);
 #endif  // !defined(OS_ANDROID)
 
-  // Clear the client id pref when opting out. Note: Mirrors code in
-  // uma_session_stats.cc. TODO(asvitkine): Unify.
-  if (!updated_pref) {
-    // Note: Clearing client id will not affect the running state (e.g. field
-    // trial randomization), as the pref is only read on startup.
-    g_browser_process->local_state()->ClearPref(
-        metrics::prefs::kMetricsClientID);
-    g_browser_process->local_state()->ClearPref(
-        metrics::prefs::kMetricsReportingEnabledTimestamp);
-  }
+  UpdateMetricsPrefsOnPermissionChange(updated_pref);
 
   // Uses the current state of whether reporting is enabled to enable services.
   g_browser_process->GetMetricsServicesManager()->UpdateUploadPermissions(true);
 
-  // When a user opts in to the metrics reporting service, the previously
-  // collected data should be cleared to ensure that nothing is reported before
-  // a user opts in and all reported data is accurate.
-  // TODO(asvitkine): This logic should be added to uma_session_stats.cc too.
-  if (updated_pref && metrics)
-    metrics->ClearSavedStabilityMetrics();
-
   if (to_update_pref == updated_pref) {
     RecordMetricsReportingHistogramValue(updated_pref ?
         METRICS_REPORTING_ENABLED : METRICS_REPORTING_DISABLED);
@@ -138,6 +121,24 @@ void ChangeMetricsReportingStateWithReply(
       base::Bind(&SetMetricsReporting, enabled, callback_fn));
 }
 
+void UpdateMetricsPrefsOnPermissionChange(bool metrics_enabled) {
+  if (metrics_enabled) {
+    // When a user opts in to the metrics reporting service, the previously
+    // collected data should be cleared to ensure that nothing is reported
+    // before a user opts in and all reported data is accurate.
+    g_browser_process->metrics_service()->ClearSavedStabilityMetrics();
+  } else {
+    // Clear the client id pref when opting out.
+    // Note: Clearing client id will not affect the running state (e.g. field
+    // trial randomization), as the pref is only read on startup.
+    g_browser_process->local_state()->ClearPref(
+        metrics::prefs::kMetricsClientID);
+    g_browser_process->local_state()->ClearPref(
+        metrics::prefs::kMetricsReportingEnabledTimestamp);
+    crash_keys::ClearMetricsClientId();
+  }
+}
+
 bool IsMetricsReportingPolicyManaged() {
   const PrefService* pref_service = g_browser_process->local_state();
   const PrefService::Preference* pref =

chrome/browser/metrics/metrics_reporting_state.h

@@ -26,6 +26,11 @@ void ChangeMetricsReportingStateWithReply(
     bool enabled,
     const OnMetricsReportingCallbackType& callback_fn);
 
+// Update metrics prefs on a permission (opt-in/out) change. When opting out,
+// this clears various client ids. When opting in, this resets saving crash
+// prefs, so as not to trigger upload of various stale data.
+void UpdateMetricsPrefsOnPermissionChange(bool metrics_enabled);
+
 // Returns whether MetricsReporting can be modified by the user (except
 // Android).
 bool IsMetricsReportingPolicyManaged();

chromecast/browser/metrics/cast_metrics_service_client.cc

@@ -116,9 +116,6 @@ void CastMetricsServiceClient::SetMetricsClientId(
 #endif
 }
 
-void CastMetricsServiceClient::OnRecordingDisabled() {
-}
-
 void CastMetricsServiceClient::StoreClientInfo(
     const ::metrics::ClientInfo& client_info) {
   const std::string& client_id = client_info.client_id;

chromecast/browser/metrics/cast_metrics_service_client.h

@@ -69,7 +69,6 @@ class CastMetricsServiceClient : public ::metrics::MetricsServiceClient,
   // ::metrics::MetricsServiceClient:
   ::metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;

components/metrics/metrics_service.cc

@@ -396,8 +396,6 @@ void MetricsService::DisableRecording() {
     return;
   recording_state_ = INACTIVE;
 
-  client_->OnRecordingDisabled();
-
   base::RemoveActionCallback(action_callback_);
 
   for (MetricsProvider* provider : metrics_providers_)

components/metrics/metrics_service_client.h

@@ -41,10 +41,6 @@ class MetricsServiceClient {
   // when metrics recording gets enabled.
   virtual void SetMetricsClientId(const std::string& client_id) = 0;
 
-  // Notifies the client that recording is disabled, so that other services
-  // (such as crash reporting) can clear any association with metrics.
-  virtual void OnRecordingDisabled() = 0;
-
   // Whether there's an "off the record" (aka "Incognito") session active.
   virtual bool IsOffTheRecordSessionActive() = 0;
 

components/metrics/test_metrics_service_client.cc

@@ -31,9 +31,6 @@ void TestMetricsServiceClient::SetMetricsClientId(
   client_id_ = client_id;
 }
 
-void TestMetricsServiceClient::OnRecordingDisabled() {
-}
-
 bool TestMetricsServiceClient::IsOffTheRecordSessionActive() {
   return false;
 }

components/metrics/test_metrics_service_client.h

@@ -26,7 +26,6 @@ class TestMetricsServiceClient : public MetricsServiceClient {
   // MetricsServiceClient:
   metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc

@@ -102,10 +102,6 @@ void IOSChromeMetricsServiceClient::SetMetricsClientId(
   crash_keys::SetMetricsClientIdFromGUID(client_id);
 }
 
-void IOSChromeMetricsServiceClient::OnRecordingDisabled() {
-  crash_keys::ClearMetricsClientId();
-}
-
 bool IOSChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
   return ::IsOffTheRecordSessionActive();
 }

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h

@@ -54,7 +54,6 @@ class IOSChromeMetricsServiceClient
   // metrics::MetricsServiceClient:
   metrics::MetricsService* GetMetricsService() override;
   void SetMetricsClientId(const std::string& client_id) override;
-  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;