Allow MetricsProviders to request an initial stability log.

components/metrics/metrics_service.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.
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
 // A MetricsService instance is typically created at application startup.  It is
@@ skipping 548 matching lines @@
 void MetricsService::InitializeMetricsState() {
   local_state_->SetString(metrics::prefs::kStabilityStatsVersion,
                           client_->GetVersionString());
   local_state_->SetInt64(metrics::prefs::kStabilityStatsBuildTime,
                          MetricsLog::GetBuildTime());
 
   log_manager_.LoadPersistedUnsentLogs();
 
   session_id_ = local_state_->GetInteger(metrics::prefs::kMetricsSessionID);
 
-  if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
-    IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
-    // Reset flag, and wait until we call LogNeedForCleanShutdown() before
-    // monitoring.
-    local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
+  if (ShouldPrepareStabilityLog()) {
+    if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {

[Alexei Svitkine (slow), 2014/09/09 15:34:28]

It's kind of confusing to have the check for this to be both in
ShouldPrepareStabilityLog() and here, since it relies on
ShouldPrepareStabilityLog() to check this pref which is not obvious by just
looking at the code here.

How about using the following code structure instead:

(in this function)

boolean exited_cleanly = true;
if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
  IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
  local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
  exited_cleanly = false;
}

if (!exited_cleanly || ProvidersHaveStabilityMetrics()) {
   ...
}

[Sigurður Ásgeirsson, 2014/09/09 17:13:40]

Yups, that's much clearer.

+      IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
+
+      // Reset flag, and wait until we call LogNeedForCleanShutdown() before
+      // monitoring.
+      local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
+    }
 
     // TODO(rtenneti): On windows, consider saving/getting execution_phase from
     // the registry.
     int execution_phase =
         local_state_->GetInteger(metrics::prefs::kStabilityExecutionPhase);
     UMA_HISTOGRAM_SPARSE_SLOWLY("Chrome.Browser.CrashedExecutionPhase",
                                 execution_phase);
 
     // If the previous session didn't exit cleanly, then prepare an initial

[Alexei Svitkine (slow), 2014/09/09 15:34:28]

Update comment.

[Sigurður Ásgeirsson, 2014/09/09 17:13:40]

Done.

     // stability log if UMA is enabled.
     if (state_manager_->IsMetricsReportingEnabled())
       PrepareInitialStabilityLog();
   }
 
   // Update session ID.
   ++session_id_;
   local_state_->SetInteger(metrics::prefs::kMetricsSessionID, session_id_);
 
   // Stability bookkeeping
@@ skipping 309 matching lines @@
       break;
 
     default:
       NOTREACHED();
       return;
   }
 
   DCHECK(log_manager_.has_staged_log());
 }
 
+bool MetricsService::ShouldPrepareStabilityLog() {
+  // Prepare a stability log if we had a crash in the previous session.
+  if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly))
+    return true;
+
+  // Or if any of the registered providers request it.
+  for (size_t i = 0; i < metrics_providers_.size(); ++i) {
+    if (metrics_providers_[i]->HasStabilityMetrics())
+      return true;
+  }
+
+  return false;
+}
+
 void MetricsService::PrepareInitialStabilityLog() {
   DCHECK_EQ(INITIALIZED, state_);
-  DCHECK_NE(0, local_state_->GetInteger(metrics::prefs::kStabilityCrashCount));
 
   scoped_ptr<MetricsLog> initial_stability_log(
       CreateLog(MetricsLog::INITIAL_STABILITY_LOG));
 
   // Do not call NotifyOnDidCreateMetricsLog here because the stability
   // log describes stats from the _previous_ session.
 
   if (!initial_stability_log->LoadSavedEnvironmentFromPrefs())
     return;
 
@@ skipping 260 matching lines @@
 void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {
   DCHECK(IsSingleThreaded());
   local_state_->SetBoolean(path, value);
   RecordCurrentState(local_state_);
 }
 
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(metrics::prefs::kStabilityLastTimestampSec,
                  Time::Now().ToTimeT());
 }