Record shutdown type UMA

chrome/browser/browser_shutdown.cc

 // Copyright (c) 2012 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/browser_shutdown.h"
 
 #include <stdint.h>
 
 #include <map>
 #include <string>
@@ skipping 152 matching lines @@
   // consider putting it in BrowserProcessImpl::EndSession.
   PrefService* prefs = g_browser_process->local_state();
 
   // Log the amount of times the user switched profiles during this session.
   ProfileMetrics::LogNumberOfProfileSwitches();
 
   metrics::MetricsService* metrics = g_browser_process->metrics_service();
   if (metrics)
     metrics->RecordCompletedSessionEnd();
 
-  if (g_shutdown_type > NOT_VALID && g_shutdown_num_processes > 0) {
-    // Record the shutdown info so that we can put it into a histogram at next
-    // startup.
-    prefs->SetInteger(prefs::kShutdownType, g_shutdown_type);
-    prefs->SetInteger(prefs::kShutdownNumProcesses, g_shutdown_num_processes);
-    prefs->SetInteger(prefs::kShutdownNumProcessesSlow,
-                      g_shutdown_num_processes_slow);
-  }
+  RecordShutdownInfoPrefs();
 
   // Check local state for the restart flag so we can restart the session below.
   bool restart_last_session = false;
   if (prefs->HasPrefPath(prefs::kRestartLastSessionOnShutdown)) {

[sky, 2016/09/12 16:58:20]

Don't we want to clear this one too?

[hashimoto, 2016/09/13 01:19:49]

I think kRestartLastSessionOnShutdown is different from kShutdownType,
kShutdownNumProcesses, and kShutdownNumProcessesSlow.
The former is used to control the behavior of the ongoing browser shutdown (btw
I couldn't understand why pref is used for this).
The latter is used to report stats about the shutdown on the next browser
startup.
IMHO mixing these two different groups of prefs makes the new function
RecordShutdownInfoPrefs unnecessarily complicated.

After all, SessionEnding() in application_lifetime.cc has been ignoring
kRestartLastSessionOnShutdown for long, and I think there is no strong reason to
change this.
WDYT?

[sky, 2016/09/13 02:17:59]

My understanding is that if kRestartLastSessionOnShutdown is true, then when you
exit the browser we restart it right away. kRestartLastSessionOnShutdown is set
to true in AttemptRestart. If for whatever reason AttemptRestart fails,
kRestartLastSessionOnShutdown is still true. If you don't change the common
shutdown code to clear this pref, then we may attempt to restart chrome at a
later date after the user logs off. That's why I think the session-end path
should also clear this pref.

I understand this is not an explicit part of the change you are after, but
sometimes when you move code around you see bugs and other things that need to
change. Since you're moving code a couple of lines above this it makes to fix it
at the same time.

[hashimoto, 2016/09/13 03:58:38]

OK, done.
Moved kRestartLastSessionOnShutdown related code to RecordShutdownInfoPrefs.

     restart_last_session =
         prefs->GetBoolean(prefs::kRestartLastSessionOnShutdown);
     prefs->ClearPref(prefs::kRestartLastSessionOnShutdown);
   }
 
   prefs->CommitPendingWrite();
 
 #if defined(ENABLE_RLZ)
   // Cleanup any statics created by RLZ. Must be done before NotificationService
   // is destroyed.
   rlz::RLZTracker::CleanupRlz();
 #endif
 
   return restart_last_session;
 }
 
+void RecordShutdownInfoPrefs() {
+  if (g_shutdown_type > NOT_VALID && g_shutdown_num_processes > 0) {
+    PrefService* prefs = g_browser_process->local_state();
+    // Record the shutdown info so that we can put it into a histogram at next
+    // startup.
+    prefs->SetInteger(prefs::kShutdownType, g_shutdown_type);
+    prefs->SetInteger(prefs::kShutdownNumProcesses, g_shutdown_num_processes);
+    prefs->SetInteger(prefs::kShutdownNumProcessesSlow,
+                      g_shutdown_num_processes_slow);
+  }
+}
+
 void ShutdownPostThreadsStop(int shutdown_flags) {
   delete g_browser_process;
   g_browser_process = NULL;
 
   // crbug.com/95079 - This needs to happen after the browser process object
   // goes away.
   ProfileManager::NukeDeletedProfilesFromDisk();
 
 #if defined(OS_CHROMEOS)
   chromeos::BootTimesRecorder::Get()->AddLogoutTimeMarker("BrowserDeleted",
@@ skipping 109 matching lines @@
   PrefService* prefs = g_browser_process->local_state();
   ShutdownType type =
       static_cast<ShutdownType>(prefs->GetInteger(prefs::kShutdownType));
   int num_procs = prefs->GetInteger(prefs::kShutdownNumProcesses);
   int num_procs_slow = prefs->GetInteger(prefs::kShutdownNumProcessesSlow);
   // clear the prefs immediately so we don't pick them up on a future run
   prefs->SetInteger(prefs::kShutdownType, NOT_VALID);
   prefs->SetInteger(prefs::kShutdownNumProcesses, 0);
   prefs->SetInteger(prefs::kShutdownNumProcessesSlow, 0);
 
+  UMA_HISTOGRAM_ENUMERATION("Shutdown.ShutdownType", type, kNumShutdownTypes);
+
   // Read and delete the file on the file thread.
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&ReadLastShutdownFile, type, num_procs, num_procs_slow));
 }
 
 void SetTryingToQuit(bool quitting) {
   g_trying_to_quit = quitting;
 
   if (quitting)
@@ skipping 26 matching lines @@
     base::trace_event::TraceConfig trace_config(
         command_line.GetSwitchValueASCII(switches::kTraceShutdown), "");
     content::TracingController::GetInstance()->StartTracing(
         trace_config,
         content::TracingController::StartTracingDoneCallback());
   }
   TRACE_EVENT0("shutdown", "StartShutdownTracing");
 }
 
 }  // namespace browser_shutdown