Let MetricsService know about some Android Activities

chrome/browser/metrics/metrics_service_android.cc

Index: chrome/browser/metrics/metrics_service_android.cc
diff --git a/chrome/browser/metrics/metrics_service_android.cc b/chrome/browser/metrics/metrics_service_android.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d49a63089dd408cd2ea613bc54b1e7e4901ff87d
--- /dev/null
+++ b/chrome/browser/metrics/metrics_service_android.cc
@@ -0,0 +1,143 @@
+// Copyright 2013 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/metrics_service.h"
+
+#include "base/metrics/histogram.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/android/activity_type_ids.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/common/pref_names.h"
+
+// static
+void MetricsService::RegisterPrefsAndroid(PrefRegistrySimple* registry) {
+  registry->RegisterIntegerPref(prefs::kStabilityForegroundActivityType,
+                                ActivityTypeIds::ACTIVITY_TYPE_NONE);
+  registry->RegisterIntegerPref(prefs::kStabilityLaunchedActivityFlags,
+                                ActivityTypeIds::ACTIVITY_FLAG_NONE);
+
+  registry->RegisterIntegerPref(prefs::kStabilityLaunchCountMainActivity, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityLaunchCountFullScreenActivity,
+                                0);
+
+  registry->RegisterIntegerPref(prefs::kStabilityCrashCountMainActivity, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityCrashCountFullScreenActivity,
+                                0);
+}
+
+void MetricsService::RecordAndroidStabilityPrefs() {
+  PrefService* pref = g_browser_process->local_state();
+
+  // Track which Activities were launched by the user.
+  int launched_activities =
+      pref->GetInteger(prefs::kStabilityLaunchedActivityFlags);
+  if (launched_activities & ActivityTypeIds::ACTIVITY_FLAG_MAIN)
+    IncrementPrefValue(prefs::kStabilityLaunchCountMainActivity);
+  if (launched_activities & ActivityTypeIds::ACTIVITY_FLAG_FULLSCREEN)
+    IncrementPrefValue(prefs::kStabilityLaunchCountFullScreenActivity);
+  pref->SetInteger(prefs::kStabilityLaunchedActivityFlags,
+                   ActivityTypeIds::ACTIVITY_FLAG_NONE);
+
+  // If an Activity was in the foreground when Chrome died, then Chrome was
+  // improperly shut down.  Record it as a crash for that particular Activity.
+  int foreground = pref->GetInteger(prefs::kStabilityForegroundActivityType);
+  switch (foreground) {
+    case ActivityTypeIds::ACTIVITY_TYPE_MAIN:
+      IncrementPrefValue(prefs::kStabilityCrashCountMainActivity);
+      break;
+    case ActivityTypeIds::ACTIVITY_TYPE_FULLSCREEN:
+      IncrementPrefValue(prefs::kStabilityCrashCountFullScreenActivity);
+      break;
+    default:
+      break;
+  }
+  pref->SetInteger(prefs::kStabilityForegroundActivityType,
+                   ActivityTypeIds::ACTIVITY_TYPE_NONE);
+}
+
+void MetricsService::RecordAndroidStabilityHistograms() {
+  PrefService* pref = g_browser_process->local_state();
+
+  // Track which Activities were launched by the user.
+  int main_launch_count =
+      pref->GetInteger(prefs::kStabilityLaunchCountMainActivity);
+  for (int i = 0; i < main_launch_count; ++i) {
+    UMA_HISTOGRAM_ENUMERATION("Chrome.Android.Activity.LaunchCounts",

[Alexei Svitkine (slow), 2013/12/23 16:28:01]

Nit: Make a helper function in the anon namespace for this that just takes in
the activity type id. (Histogram macros expand to a blob of code and its better
for code size to not repeat them - also prevents accidental typos in the name).

[gone, 2013/12/28 01:33:14]

Changed it to loop over the possibilities instead.

+                              ActivityTypeIds::ACTIVITY_TYPE_MAIN,
+                              ActivityTypeIds::ACTIVITY_TYPE_MAX_VALUE);
+  }
+  pref->SetInteger(prefs::kStabilityLaunchCountMainActivity, 0);
+
+  int full_screen_launch_count =
+      pref->GetInteger(prefs::kStabilityLaunchCountFullScreenActivity);
+  for (int i = 0; i < full_screen_launch_count; ++i) {
+    UMA_HISTOGRAM_ENUMERATION("Chrome.Android.Activity.LaunchCounts",
+                              ActivityTypeIds::ACTIVITY_TYPE_FULLSCREEN,
+                              ActivityTypeIds::ACTIVITY_TYPE_MAX_VALUE);
+  }
+  pref->SetInteger(prefs::kStabilityLaunchCountFullScreenActivity, 0);
+
+  // Track which Activities were in the foreground when Chrome died.
+  int main_crash_count =
+      pref->GetInteger(prefs::kStabilityCrashCountMainActivity);
+  for (int i = 0; i < main_crash_count; ++i) {
+    UMA_HISTOGRAM_ENUMERATION("Chrome.Android.Activity.CrashCounts",
+                              ActivityTypeIds::ACTIVITY_TYPE_MAIN,
+                              ActivityTypeIds::ACTIVITY_TYPE_MAX_VALUE);
+  }
+  pref->SetInteger(prefs::kStabilityCrashCountMainActivity, 0);
+
+  int full_screen_crash_count =
+      pref->GetInteger(prefs::kStabilityCrashCountFullScreenActivity);
+  for (int i = 0; i < full_screen_crash_count; ++i) {
+    UMA_HISTOGRAM_ENUMERATION("Chrome.Android.Activity.CrashCounts",
+                              ActivityTypeIds::ACTIVITY_TYPE_FULLSCREEN,
+                              ActivityTypeIds::ACTIVITY_TYPE_MAX_VALUE);
+  }
+  pref->SetInteger(prefs::kStabilityCrashCountFullScreenActivity, 0);
+}
+
+void MetricsService::OnForegroundActivityChanged(ActivityTypeIds::Type type) {
+  PrefService* pref = g_browser_process->local_state();
+
+  // Record that the Activity is now in the foreground.
+  pref->SetInteger(prefs::kStabilityForegroundActivityType, type);
+
+  // Record that the Activity was launched by ORing its flag representation with
+  // our record of the other actvities that had launched.
+  int launched_activities =
+      pref->GetInteger(prefs::kStabilityLaunchedActivityFlags);
+  switch (type) {
+    case ActivityTypeIds::ACTIVITY_TYPE_NONE:

[Alexei Svitkine (slow), 2013/12/23 16:28:01]

Can you add a comment explaining what this means?

[gone, 2013/12/28 01:33:14]

Added a more explicit ACTIVITY_UNKNOWN here to track that some other Activity we
haven't enumerated has been launched.

+      launched_activities |= ActivityTypeIds::ACTIVITY_FLAG_NONE;
+      break;
+    case ActivityTypeIds::ACTIVITY_TYPE_MAIN:
+      launched_activities |= ActivityTypeIds::ACTIVITY_FLAG_MAIN;
+      break;
+    case ActivityTypeIds::ACTIVITY_TYPE_FULLSCREEN:
+      launched_activities |= ActivityTypeIds::ACTIVITY_FLAG_FULLSCREEN;
+      break;
+    case ActivityTypeIds::ACTIVITY_TYPE_MAX_VALUE:
+      NOTREACHED() << "Unknown activity type encountered: " << type;

[Ilya Sherman, 2013/12/20 23:28:21]

nit: No need for the rest of the logging stmt now, since there's only one |type|
that can reach this code path.

[gone, 2013/12/28 01:33:14]

Redid this bit.  Should be cleaner now.

+      break;
+  }
+  pref->SetInteger(prefs::kStabilityLaunchedActivityFlags, launched_activities);

[Alexei Svitkine (slow), 2013/12/23 16:28:01]

Why use an intermediate pref for this, rather than incrementing
kStabilityLaunchCountMainActivity and kStabilityLaunchCountFullScreenActivity
here directly?

Is it to avoid double counting? If so, can you just keep |launched_activities| a
local var and record ones you've seen already and log the new ones?

[Alexei Svitkine (slow), 2013/12/23 16:29:52]

(I mean class instance var, rather than a local var.)

[gone, 2013/12/28 01:33:14]

tl;dr: Yeah, it's to avoid double-counting, but it's also done so that we don't
lose the stat during a crash.

Android sessions are currently defined as the time between when the main browser
Activity (i.e. browser window) is brought to the foreground and sent to the
background.  We define it this way because Android is free to kill the Chrome
process whenever it's not visible, meaning that it'd be difficult to properly
log a "clean shutdown".  This is also why getting a clean browser shutdown is
impractical on Android: if we try and perform a shutdown every time the app went
to the background, a user temporarily switching to another app would incur a
slow cold start every time they switched back.  Instead, we compromise and end
UMA sessions once in the background.

This is increasingly problematic, however, since we have other Activities that
can trigger a Chrome launch without ever bringing the main browser Activity to
the foreground (e.g. application shortcuts or FullScreenActivities). 
Consequently, these Activities are untracked by the metrics service and crashes
go unnoticed.

I'm trying to expand the definition of the UMA session to include when any
Chrome Activity is in the foreground through this CL, which is important because
the user can easily swap between the main browser Activity and a
FullScreenActivity without ever backgrounding Chrome, which feels like it should
count as a single session.  We don't want to count these as separate launches
because they're analogously just swapping between two desktop windows. 
Incrementing the counter whenever the foreground Activity would then result in
inflated counts.

Instead, we record a set of flags indicating which Activities were launched for
that given session and increment them when a new session starts.  Since we don't
immediately save the histograms out to disk, we store them in prefs to ensure we
don't lose them between crashes.

+
+  pref->CommitPendingWrite();
+}
+
+// static
+void MetricsService::DiscardOldStabilityStatsAndroid(PrefService* local_state) {
+  local_state->SetInteger(prefs::kStabilityForegroundActivityType,
+                          ActivityTypeIds::ACTIVITY_TYPE_NONE);
+  local_state->SetInteger(prefs::kStabilityLaunchedActivityFlags,
+                          ActivityTypeIds::ACTIVITY_FLAG_NONE);
+
+  local_state->SetInteger(prefs::kStabilityLaunchCountMainActivity, 0);
+  local_state->SetInteger(prefs::kStabilityLaunchCountFullScreenActivity, 0);
+
+  local_state->SetInteger(prefs::kStabilityCrashCountMainActivity, 0);
+  local_state->SetInteger(prefs::kStabilityCrashCountFullScreenActivity, 0);
+}