Reset metrics if cellular uploads logic should be applied.

chrome/browser/metrics/chrome_metrics_service_client.cc

Index: chrome/browser/metrics/chrome_metrics_service_client.cc
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index 2b6e1e1885a1aca2e24a7eb3cb54a3cad530145d..4f87e47b1a6885c0cf4a6df7db27b1563db8578c 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -279,6 +279,19 @@ void ChromeMetricsServiceClient::LogPluginLoadingError(
 }
 
 void ChromeMetricsServiceClient::Initialize() {
+

[Alexei Svitkine (slow), 2015/05/07 21:06:53]

Nit: No blank line at start of function.

[gayane -on leave until 09-2017, 2015/05/07 21:25:59]

Done.

+  // Reset metrics reports before initializing metrics service so that metrics
+  // log manager is initialized correctly.

[Alexei Svitkine (slow), 2015/05/07 22:04:17]

This comment only explains why you're doing this here - and not *why* this is
being done, please mention the purpose of doing this.

[gayane -on leave until 09-2017, 2015/05/11 23:02:05]

Done.

+#if defined(OS_ANDROID)
+  PrefService* local_state = g_browser_process->local_state();
+  if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) &&

[Alexei Svitkine (slow), 2015/05/07 21:06:53]

Can you remind me where this pref will be initialized on Android?

[gayane -on leave until 09-2017, 2015/05/07 21:25:58]

This pref will be initialized as soon as new session for UMA logging will start.
https://cs.corp.google.com/#clankium/src/chrome/android/java/src/org/chromium...

and that happens after this code.

[Alexei Svitkine (slow), 2015/05/07 22:04:17]

I see - it's unfortunate that the two places are so decoupled. But I guess it's
hard to avoid given the current design - so please at least add a comment
mentioning what you said here (i.e. where the pref is initialized).

Can we do this from UmaSessionStats instead? In updatePreferences(), we already
check the state of the trials.

[gayane -on leave until 09-2017, 2015/05/11 23:02:05]

I guess we could do this in UmaSessionStats if I could refresh queues in
MetricsLogManager, because its constructor uses kMetricsInitialLogs and
kMetricsOngoingLogs to init queues. 

I could reset the prefs and call  MetricsLogManager.DiscardCurrentLog() if I
understood correctly.

Does this sound reasonable to you?

[Alexei Svitkine (slow), 2015/05/12 16:40:00]

That seems more complicated. I'm OK with the current approach now that you've
added the comment about where the pref is reset.

+    variations::GetVariationParamValue("UMA_EnableCellularLogUpload",

[Alexei Svitkine (slow), 2015/05/07 21:06:53]

Nit: Align.

[gayane -on leave until 09-2017, 2015/05/07 21:25:59]

Done.

+                                         "Enabled") == "true") {
+    local_state->ClearPref(metrics::prefs::kMetricsInitialLogs);
+    local_state->ClearPref(metrics::prefs::kMetricsOngoingLogs);
+  }
+#endif
+
   metrics_service_.reset(new metrics::MetricsService(
       metrics_state_manager_, this, g_browser_process->local_state()));
 
@@ -363,6 +376,15 @@ void ChromeMetricsServiceClient::Initialize() {
       scoped_ptr<metrics::MetricsProvider>(
           SigninStatusMetricsProvider::CreateInstance()));
 #endif  // !defined(OS_CHROMEOS) && !defined(OS_IOS)
+
+// Reset stability metrics after all providers are registered.

[Alexei Svitkine (slow), 2015/05/07 22:04:17]

Expand comment to mention *why* it's done after all providers are registered.
It's obvious from the code that it's done after all of them are registered.

[gayane -on leave until 09-2017, 2015/05/11 23:02:05]

Done.

+#if defined(OS_ANDROID)
+  if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) &&
+    variations::GetVariationParamValue("UMA_EnableCellularLogUpload",
+                                         "Enabled") == "true") {

[Alexei Svitkine (slow), 2015/05/07 21:06:53]

You're repeating this in two places - make a helper function at the top in an
anon namespace.

[gayane -on leave until 09-2017, 2015/05/07 21:25:58]

Done.

+    metrics_service_->ClearSavedStabilityMetrics();
+  }
+#endif
 }
 
 void ChromeMetricsServiceClient::OnInitTaskGotHardwareClass() {