Add UMA to track app launcher discoverability.

chrome/browser/ui/app_list/app_list_service_impl.cc

Index: chrome/browser/ui/app_list/app_list_service_impl.cc
diff --git a/chrome/browser/ui/app_list/app_list_service_impl.cc b/chrome/browser/ui/app_list/app_list_service_impl.cc
index 043da34896f22c3d8a14cce14981d3f0535f6bdb..bfe29d0664f72e80b1f5dc02dd5d8bc5535e76ab 100644
--- a/chrome/browser/ui/app_list/app_list_service_impl.cc
+++ b/chrome/browser/ui/app_list/app_list_service_impl.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/apps/shortcut_manager.h"
 #include "chrome/browser/apps/shortcut_manager_factory.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/keep_alive_service.h"
 #include "chrome/browser/ui/app_list/keep_alive_service_impl.h"
@@ -27,6 +28,8 @@
 
 namespace {
 
+const int kDiscoverabilityTimeoutMinutes = 60;
+
 void SendAppListAppLaunch(int count) {
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Apps.AppListDailyAppLaunches", count, 1, 1000, 50);
@@ -66,6 +69,9 @@ void RecordDailyEventFrequency(
     const char* last_ping_pref,
     const char* count_pref,
     void (*send_callback)(int count)) {
+  if (!g_browser_process)
+    return;  // In a unit test.
+
   PrefService* local_state = g_browser_process->local_state();
 
   int count = local_state->GetInteger(count_pref);
@@ -138,13 +144,65 @@ class ProfileStoreImpl : public ProfileStore {
   base::WeakPtrFactory<ProfileStoreImpl> weak_factory_;
 };
 
+void RecordAppListDiscoverability(PrefService* local_state,
+                                  bool is_startup_check) {

[benwells, 2014/02/09 21:08:07]

Nit: consider changing this parameter to was_launched. I think it is logically
equivalent but clearer and more flexible.

[tapted, 2014/02/10 02:54:29]

At first I thought you meant ~was_(browser_process)_launched, which would also
fit. But I think you mean something like "was_app_list_shown"?

However, `is_startup_check` is also `false` for the timeout case. That is, it
really is the startup_check that is special -- that's the only time the function
should ever PostTask itself again. Guaranteeing this means we don't need a fudge
factor when there is (e.g.) 1 microsecond left on the timer due to
clock/scheduling inaccuracies.

[benwells, 2014/02/10 06:03:27]

OK, sounds reasonable.

+  if (browser_shutdown::IsTryingToQuit())

[benwells, 2014/02/09 21:08:07]

Nit: can you add a comment explaining when this happens.

[tapted, 2014/02/10 02:54:29]

Done.

+    return;
+
+  int64 enable_time_value = local_state->GetInt64(prefs::kAppListEnableTime);
+  if (enable_time_value == 0)
+    return;  // Already recorded or never enabled.
+
+  base::Time app_list_enable_time =
+      base::Time::FromInternalValue(enable_time_value);
+  if (is_startup_check) {
+    // When checking at startup, only clear and record the "timeout" case,
+    // otherwise wait for a timeout.
+    base::TimeDelta time_remaining = app_list_enable_time +
+        base::TimeDelta::FromMinutes(kDiscoverabilityTimeoutMinutes) -
+        base::Time::Now();
+    if (time_remaining > base::TimeDelta()) {
+      base::MessageLoop::current()->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&RecordAppListDiscoverability,
+                     base::Unretained(local_state),
+                     false),
+          time_remaining);
+      return;
+    }
+  }
+
+  local_state->SetInt64(prefs::kAppListEnableTime, 0);
+
+  AppListService::AppListEnableSource enable_source =
+      static_cast<AppListService::AppListEnableSource>(
+          local_state->GetInteger(prefs::kAppListEnableMethod));
+  if (enable_source == AppListService::ENABLE_FOR_APP_INSTALL) {
+    base::TimeDelta time_taken = base::Time::Now() - app_list_enable_time;
+    // This means the user "discovered" the app launcher naturally, after it was
+    // enabled on the first app install. Record how long it took to discover.
+    // Note that the last bucket is essentially "not discovered": subtract 1
+    // minute to account for clock inaccuracy.
+    UMA_HISTOGRAM_CUSTOM_TIMES(
+        "Apps.AppListTimeToDiscover",
+        time_taken,
+        base::TimeDelta::FromSeconds(1),
+        base::TimeDelta::FromMinutes(kDiscoverabilityTimeoutMinutes - 1),
+        10 /* bucket_count */);
+  }
+  UMA_HISTOGRAM_ENUMERATION(
+      "Apps.AppListHowEnabled",
+      enable_source,
+      AppListService::ENABLE_NUM_ENABLE_SOURCES);
+}
+
 }  // namespace
 
-// static
 void AppListServiceImpl::RecordAppListLaunch() {
   RecordDailyEventFrequency(prefs::kLastAppListLaunchPing,
                             prefs::kAppListLaunchCount,
                             &SendAppListLaunch);
+  RecordAppListDiscoverability(local_state_, false);
 }
 
 // static
@@ -255,13 +313,37 @@ void AppListServiceImpl::Show() {
                  weak_factory_.GetWeakPtr()));
 }
 
-void AppListServiceImpl::EnableAppList(Profile* initial_profile) {
+void AppListServiceImpl::AutoShowForProfile(Profile* requested_profile) {
+  local_state_->SetInteger(prefs::kAppListEnableMethod,
+                           ENABLE_ALREADY_ENABLED);

[benwells, 2014/02/09 21:08:07]

This is unclear. I think it means that the app list was shown automatically
before it was shown manually. Is that right?

[tapted, 2014/02/10 02:54:29]

Yep - I agree it sounds weird. This kinda made sense as a value for the enum
"EnableSource". E.g. when recording, to ask, "how was it enabled?". Also, since
this pref value is "sticky" (unlike the enable time) it makes sense even when
the shortcut _was_ discovered, and later shown by an app install. That is, a
user's "second" app install would always switch this pref value to
"ALREADY_ENABLED" (even when it was already recorded in UMA, and won't be
recorded again).

I've changed it to `ENABLE_SHOWN_UNDISCOVERED` which might be an improvement,
but I'm open to other suggestions :)

Then, for this to make sense, it feels like it would also be nice to keep the
discovery method, if one had already been recorded in UMA. So this function now
checks that too.

+  ShowForProfile(requested_profile);
+}
+
+void AppListServiceImpl::EnableAppList(Profile* initial_profile,
+                                       AppListEnableSource enable_source) {
   SetProfilePath(initial_profile->GetPath());
   if (local_state_->GetBoolean(prefs::kAppLauncherHasBeenEnabled))
     return;
 
   local_state_->SetBoolean(prefs::kAppLauncherHasBeenEnabled, true);
   CreateShortcut();
+
+  // UMA for launcher discoverability.
+  local_state_->SetInt64(prefs::kAppListEnableTime,
+                         base::Time::Now().ToInternalValue());
+  local_state_->SetInteger(prefs::kAppListEnableMethod,
+                           enable_source);
+  if (base::MessageLoop::current()) {
+    // Ensure a value is recorded if the user "never" shows the app list. Note
+    // there is no message loop in unit tests.
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&RecordAppListDiscoverability,
+                   base::Unretained(local_state_),
+                   false),
+        base::TimeDelta::FromMinutes(kDiscoverabilityTimeoutMinutes));
+  }
+
   AppShortcutManager* shortcut_manager =
       AppShortcutManagerFactory::GetForProfile(initial_profile);
   if (shortcut_manager)
@@ -273,11 +355,12 @@ void AppListServiceImpl::InvalidatePendingProfileLoads() {
 }
 
 void AppListServiceImpl::HandleCommandLineFlags(Profile* initial_profile) {
+  RecordAppListDiscoverability(local_state_, true);

[benwells, 2014/02/09 21:08:07]

This doesn't seem like the right home for this.

[tapted, 2014/02/10 02:54:29]

See how it feels now :). Renamed function to `PerformStartupChecks`. And
probably found a bug in the process: `SendUsageStats` was only being done at
startup for Windows - doh.

Folded SendUsageStats into PerformStartupChecks.

   if (command_line_.HasSwitch(switches::kResetAppListInstallState))
     local_state_->SetBoolean(prefs::kAppLauncherHasBeenEnabled, false);
 
   if (command_line_.HasSwitch(switches::kEnableAppList))
-    EnableAppList(initial_profile);
+    EnableAppList(initial_profile, ENABLE_VIA_COMMAND_LINE);
 }
 
 void AppListServiceImpl::SendUsageStats() {