Reset metrics if cellular uploads logic should be applied.

chrome/browser/metrics/chrome_metrics_service_client.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/chrome_metrics_service_client.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 261 matching lines @@
 void ChromeMetricsServiceClient::LogPluginLoadingError(
     const base::FilePath& plugin_path) {
 #if defined(ENABLE_PLUGINS)
   plugin_metrics_provider_->LogPluginLoadingError(plugin_path);
 #else
   NOTREACHED();
 #endif  // defined(ENABLE_PLUGINS)
 }
 
 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()));
 
   // Register metrics providers.
 #if defined(ENABLE_EXTENSIONS)
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(
           new ExtensionsMetricsProvider(metrics_state_manager_)));
 #endif
   metrics_service_->RegisterMetricsProvider(
@@ skipping 64 matching lines @@
       new SigninStatusMetricsProviderChromeOS;
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(signin_metrics_provider_cros));
 #endif  // defined(OS_CHROMEOS)
 
 #if !defined(OS_CHROMEOS) && !defined(OS_IOS)
   metrics_service_->RegisterMetricsProvider(
       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() {
   const base::Closure got_plugin_info_callback =
       base::Bind(&ChromeMetricsServiceClient::OnInitTaskGotPluginInfo,
                  weak_ptr_factory_.GetWeakPtr());
 
 #if defined(ENABLE_PLUGINS)
   plugin_metrics_provider_->GetPluginInformation(got_plugin_info_callback);
 #else
@@ skipping 149 matching lines @@
     case content::NOTIFICATION_LOAD_START:
     case content::NOTIFICATION_RENDERER_PROCESS_CLOSED:
     case content::NOTIFICATION_RENDER_WIDGET_HOST_HANG:
       metrics_service_->OnApplicationNotIdle();
       break;
 
     default:
       NOTREACHED();
   }
 }