Don't commit prefs::kStabilitySessionEndCompleted change immediately on chromeos

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 313de569207cbf3824c3368dcc2de80454acfef5..26b81bc96fbe88205de71feb1a9bcbb66e0508b2 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -737,13 +737,11 @@ void MetricsService::HandleIdleSinceLastTransmission(bool in_idle) {
 }
 
 void MetricsService::RecordStartOfSessionEnd() {
-  LogCleanShutdown();
-  RecordBooleanPrefValue(prefs::kStabilitySessionEndCompleted, false);
+  RecordStabilitySessionEndCompleted(false);
 }
 
 void MetricsService::RecordCompletedSessionEnd() {
-  LogCleanShutdown();
-  RecordBooleanPrefValue(prefs::kStabilitySessionEndCompleted, true);
+  RecordStabilitySessionEndCompleted(true);
 }
 
 #if defined(OS_ANDROID) || defined(OS_IOS)
@@ -1598,23 +1596,24 @@ void MetricsService::LogRendererHang() {
   IncrementPrefValue(prefs::kStabilityRendererHangCount);
 }
 
+void MetricsService::RecordStabilitySessionEndCompleted(bool value) {
+  PrefService* pref = g_browser_process->local_state();
+  DCHECK(pref);
+  // Redundant setting to assure that we always reset this value at shutdown
+  // (and that we don't use some alternate path, and not call
+  // RecordStartOfEndSession/RecordCompletedSessionEnd)
+  clean_shutdown_status_ = CLEANLY_SHUTDOWN;
+  pref->SetBoolean(prefs::kStabilitySessionEndCompleted, value);
+  RecordCurrentState(pref);
+  MarkAppCleanShutdownAndCommit();

[jar (doing other things), 2013/10/11 22:55:44]

I'm trying to understand what you are doing here, vs what the code used to do.

In the past, we *first* did a LogCleanShutdown() on lines 740 or 745.  This
function did a MarkAppCleanShutdownAndCommit(), and *then* set the pref.

Now, you first change the prefs, and then call MarkAppCleanShutdownAndCommit().

It appears you have changed the order (setting the state, before the commit). 


IMO, it would be great to add a file-level comment indicating the exact shutdown
sequence, and exactly what state is observable (via prefs) to deduce where (if
any) the previous run of the browser crashed (stopped updating prefs files).  We
could use that as the spec, verify that it is indeed what was currently done....
and then verify that it is preserved by your change.

WDYT?

+}
+
 bool MetricsService::UmaMetricsProperlyShutdown() {
   CHECK(clean_shutdown_status_ == CLEANLY_SHUTDOWN ||
         clean_shutdown_status_ == NEED_TO_SHUTDOWN);
   return clean_shutdown_status_ == CLEANLY_SHUTDOWN;
 }
 
-void MetricsService::LogCleanShutdown() {
-  // Redundant hack to write pref ASAP.
-  MarkAppCleanShutdownAndCommit();
-
-  // Redundant setting to assure that we always reset this value at shutdown
-  // (and that we don't use some alternate path, and not call LogCleanShutdown).
-  clean_shutdown_status_ = CLEANLY_SHUTDOWN;
-
-  RecordBooleanPrefValue(prefs::kStabilityExitedCleanly, true);
-}
-
 #if defined(OS_CHROMEOS)
 void MetricsService::LogChromeOSCrash(const std::string &crash_type) {
   if (crash_type == "user")
@@ -1754,16 +1753,6 @@ bool MetricsService::CanLogNotification() {
   return !chrome::IsOffTheRecordSessionActive();
 }
 
-void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {
-  DCHECK(IsSingleThreaded());
-
-  PrefService* pref = g_browser_process->local_state();
-  DCHECK(pref);
-
-  pref->SetBoolean(path, value);
-  RecordCurrentState(pref);
-}
-
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(prefs::kStabilityLastTimestampSec, Time::Now().ToTimeT());