Enabling sampling of UMA and crash reports on Android.

chrome/browser/android/metrics/uma_session_stats.cc

 // Copyright 2015 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/android/metrics/uma_session_stats.h"
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
@@ skipping 81 matching lines @@
   ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
       trial_name, group_name_hashes);
 }
 
 // Starts/stops the MetricsService when permissions have changed.
 // There are three possible states:
 // * Logs are being recorded and being uploaded to the server.
 // * Logs are being recorded, but not being uploaded to the server.
 //   This happens when we've got permission to upload on Wi-Fi but we're on a
 //   mobile connection (for example).
 // * Logs are neither being recorded or uploaded.

[Ilya Sherman, 2016/08/23 21:32:17]

Could you please document |consent_change| as well?

[Ilya Sherman, 2016/08/23 21:32:17]

This list sounds like there ought to be an assertion: DCHECK(may_record ||
!may_upload);

[jwd, 2016/08/23 23:00:58]

Done.

[jwd, 2016/08/23 23:00:59]

Re-worked some of the code to avoid that parameter.

[Ilya Sherman, 2016/08/24 05:02:50]

Now may_record is also unused.  Is it still needed here?  Is it important that
the code in ChangeMetricsReportingConsent() happens before the code in
UpdateMetricsServiceState()?  If so, is there something that guarantees that
ordering?  It would be good to document, as well, if there is an ordering
constraint.

[jwd, 2016/08/24 15:06:27]

Well, it's needed for the DCHECK ;P. Note, there is a deeper place in
MetricsServicesManager to move that DCHECK, but I'm not going to because it
violates how we use C++ side api.

There is an ordering constraint, if ChangeMetricsReportingConsent() gets called,
UpdateMetricsServiceState() should be called after it to affect the metrics
services. That's currently enforced on the Java side by
changeMetricsReportingConsent() calling updateMetricsServiceState() at the end.
I will add something to the comment for Change...().

-static void UpdateMetricsServiceState(JNIEnv* env,
-                                      const JavaParamRef<jobject>& obj,
+static void UpdateMetricsServiceState(JNIEnv*,
+                                      const JavaParamRef<jclass>&,
+                                      jboolean consent_change,
                                       jboolean may_record,
                                       jboolean may_upload) {
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-  DCHECK(metrics);
-
-  if (metrics->recording_active() != may_record) {
+  if (consent_change) {
     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.
+    // This function ensures a consent file in the data directory is either
+    // created, or deleted, depending on may_record. Starting up metrics
+    // services will ensure that the consent file contains the ClientID. The ID
+    // is passed to the renderer for crash reporting when things go wrong.
     content::BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,
         base::Bind(
             base::IgnoreResult(GoogleUpdateSettings::SetCollectStatsConsent),
             may_record));
   }
 
-  g_browser_process->GetMetricsServicesManager()->UpdatePermissions(
-      may_record, may_upload);
+  // This will also apply the consent state, taken from Chrome Local State
+  // prefs.
+  g_browser_process->GetMetricsServicesManager()->UpdateUploadPermissions(
+      may_upload);
 }
 
 // Renderer process crashed in the foreground.
 static void LogRendererCrash(JNIEnv*, const JavaParamRef<jclass>&) {
   DCHECK(g_browser_process);
   // Increment the renderer crash count in stability metrics.
   PrefService* pref = g_browser_process->local_state();
   DCHECK(pref);
   int value = pref->GetInteger(metrics::prefs::kStabilityRendererCrashCount);
   pref->SetInteger(metrics::prefs::kStabilityRendererCrashCount, value + 1);
@@ skipping 83 matching lines @@
   // We should have only one UmaSessionStats instance.
   DCHECK(!g_uma_session_stats);
   g_uma_session_stats = new UmaSessionStats();
   return reinterpret_cast<intptr_t>(g_uma_session_stats);
 }
 
 // Register native methods
 bool RegisterUmaSessionStats(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }