Add UMA to track app launcher discoverability.

Add UMA to track app launcher discoverability.

We want to track effectiveness of the app launcher install education
pages in enabling users to find, and start using, the app launcher when
it is auto-enabled upon installing the first v2 packaged app. The
launcher is not auto-shown for the "first" install, to encourage users
to learn where it is.

This CL marks "how" the app launcher is enabled (e.g. command line flag,
webstore, etc.), and when the enable occurs. Then, when the user shows
the app list, the enable method and time taken to show it is recorded.

We also need to record users that "never" show the app list, or who see
the app list show automatically upon installing a _second_ app. One hour
is chosen as a timeout: if no value is recorded after one hour, a value
is recorded in the "last" bucket for the time taken.

BUG=314235
TBR=xiyuan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250348

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250938

[tapted, 2014-02-07 05:32:39]

Hi Ben, PTAL. Even has tests :o

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

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.h:80: // to show the app list. E.g.
the auto-show when installing an app. For UMA.
Nit: I think I know what the 'for uma' sentence means but it won't brr clear
outside the context of this cl. Maybe remove or expand.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:148: bool is_startup_check)
{
Nit: consider changing this parameter to was_launched. I think it is logically
equivalent but clearer and more flexible.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:149: if
(browser_shutdown::IsTryingToQuit())
Nit: can you add a comment explaining when this happens.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:318:
ENABLE_ALREADY_ENABLED);
This is unclear. I think it means that the app list was shown automatically
before it was shown manually. Is that right?

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:358:
RecordAppListDiscoverability(local_state_, true);
This doesn't seem like the right home for this.

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

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.h:80: // to show the app list. E.g.
the auto-show when installing an app. For UMA.
On 2014/02/09 21:08:07, benwells wrote:
> Nit: I think I know what the 'for uma' sentence means but it won't brr clear
> outside the context of this cl. Maybe remove or expand.

Done. (Expanded - I thought it could still be hard for someone to reason about
how to pick between Show or AutoShow without explaining why it existed).

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_disabled.cc (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_disabled.cc:43: AppListEnableSource
enale_source) OVERRIDE {}
whoops - fixed typo.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:148: bool is_startup_check)
{
On 2014/02/09 21:08:07, benwells wrote:
> Nit: consider changing this parameter to was_launched. I think it is logically
> equivalent but clearer and more flexible.

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.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:149: if
(browser_shutdown::IsTryingToQuit())
On 2014/02/09 21:08:07, benwells wrote:
> Nit: can you add a comment explaining when this happens.

Done.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:318:
ENABLE_ALREADY_ENABLED);
On 2014/02/09 21:08:07, benwells wrote:
> This is unclear. I think it means that the app list was shown automatically
> before it was shown manually. Is that right?

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.

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:358:
RecordAppListDiscoverability(local_state_, true);
On 2014/02/09 21:08:07, benwells wrote:
> This doesn't seem like the right home for this.

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.

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

lgtm

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/143683004/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:148: bool is_startup_check)
{
On 2014/02/10 02:54:29, tapted wrote:
> On 2014/02/09 21:08:07, benwells wrote:
> > Nit: consider changing this parameter to was_launched. I think it is
logically
> > equivalent but clearer and more flexible.
> 
> 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.

OK, sounds reasonable.

[tapted, 2014-02-11 03:00:39]

+xiyuan TBR for trivial/refactoring change in
chrome/browser/ui/ash/app_list/app_list_service_ash.cc

(also wanted to re-verify everything after testability changes - all checks out
- time for the CQ gauntlet)

[tapted, 2014-02-11 03:00:51]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-02-11 03:03:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/143683004/760001

[commit-bot: I haz the power, 2014-02-11 04:47:56]

Change committed as 250348

[grt (UTC plus 2), 2014-02-11 19:15:39]

A revert of this CL has been created in
https://codereview.chromium.org/134303006/ by grt@chromium.org.

The reason for reverting is: Doesn't compile Google Chrome-branded builds:

src\chrome\browser\ui\views\app_list\win\app_list_service_win.cc(459) : error
C2660: 'AppListServiceImpl::EnableAppList' : function does not take 1 arguments.

[tapted, 2014-02-12 03:01:56]

benwells: PTAL. Updated to not break the official+windows build

I started pulling the "we don't need app_host.exe, let's purge it" thread, and
things exploded - crrev/137713008/

I think it's also safe just to delete all the OFFICIAL_CHROME code in
app_list_service_win which is currently responsible for the migration from
app_host.exe. I went with this smaller fix just to keep those efforts separate,
but would be easily swayed.

Currently double-checking on a local official+windows checkout to ensure there's
nothing else (pretty sure this is it).

[benwells, 2014-02-12 05:21:25]

On 2014/02/12 03:01:56, tapted wrote:
> benwells: PTAL. Updated to not break the official+windows build
> 
> I started pulling the "we don't need app_host.exe, let's purge it" thread, and
> things exploded - crrev/137713008/
> 
> I think it's also safe just to delete all the OFFICIAL_CHROME code in
> app_list_service_win which is currently responsible for the migration from
> app_host.exe. I went with this smaller fix just to keep those efforts
separate,
> but would be easily swayed.
> 
> Currently double-checking on a local official+windows checkout to ensure
there's
> nothing else (pretty sure this is it).

lgtm.

[tapted, 2014-02-12 22:13:50]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-02-12 22:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/143683004/1140001

[commit-bot: I haz the power, 2014-02-13 04:13:28]

Change committed as 250938