Don't warmup the app list if it hasn't been used in the past 28 days. [Win]

Don't warmup the app list if it hasn't been used in the past 28 days. [Win]

This CL adds a pref that tracks when the app list is last launched and
doesn't warm up the app list on Windows unless the app list has been
launched in the last 28 days.

BUG=440224
Committed: https://crrev.com/8f565846801e295986a5e18531498490f09acc6e
Cr-Commit-Position: refs/heads/master@{#324018}

[Mike Lerman, 2015-03-26 12:21:41]

mlerman@chromium.org changed reviewers:
+ mlerman@chromium.org

[Mike Lerman, 2015-03-26 12:21:42]

If I may do a drive by... 2 comments.

1 - Shouldn't canary reflect the reality that's on all the other channels? We
won't be able to measure the effectiveness of this CL of it doesn't do anything
on canary, and there's no point getting false jank signals on canary that
doesn't exist on other channels.

2 - Should this be a per-profile pref instead of global? On multi-profile
machines, just because one user likes using the app launcher doesn't mean all
users do. We'd need to find a way to do this without loading the Profile*,
perhaps using ProfileInfoCache or similarly have per-profile sections within the
main LocalState.

[calamity, 2015-04-01 04:34:46]

On 2015/03/26 12:21:42, Mike Lerman wrote:
> If I may do a drive by... 2 comments.
> 
> 1 - Shouldn't canary reflect the reality that's on all the other channels? We
> won't be able to measure the effectiveness of this CL of it doesn't do
anything
> on canary, and there's no point getting false jank signals on canary that
> doesn't exist on other channels.
> 
Yeah, good point.

> 2 - Should this be a per-profile pref instead of global? On multi-profile
> machines, just because one user likes using the app launcher doesn't mean all
> users do. We'd need to find a way to do this without loading the Profile*,
> perhaps using ProfileInfoCache or similarly have per-profile sections within
the
> main LocalState.

I think LocalState should be fine. The vast majority of users don't have
multiple profiles and the app list starts as the last app list user, not the
last chrome user, so warming up by the current chrome user doesn't make too much
sense anyway.

[calamity, 2015-04-01 04:34:56]

calamity@chromium.org changed reviewers:
+ tapted@chromium.org

[Mike Lerman, 2015-04-01 13:37:39]

On 2015/04/01 04:34:57, calamity wrote:

LGTM :) Thanks for taking a look into this!

[tapted, 2015-04-02 00:23:33]

https://codereview.chromium.org/1016503006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right):

https://codereview.chromium.org/1016503006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:77: // Migrate
chrome::kAppLauncherIsEnabled pref to
heads-up: you'll probably get a merge conflict here

https://codereview.chromium.org/1016503006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:352: if
(!IsAppLauncherEnabled())
Note this change should be in (dev channel) 43.0.2351.3, and might be worth
looking at uma before landing this to see if it's still needed.

Either way, we should compare uma stats (in 28 days time?) to see if this change
has an impact.

https://codereview.chromium.org/1016503006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:379: return false;
I think this needs to be smarter. How about just doing a RecordAppListLastLaunch
and continuing with the rest of the function. Otherwise there will be a huge
data spike the first time this code runs.

[tapted, 2015-04-07 02:39:16]

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right):

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:374: // Don't warm
up the app list if it hasn't been used for a while.
add ~"If the last launch is unknown, record it as "used" on the first warmup.

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:379:
RecordAppListLastLaunch();
pretty sure you'll need to expose the declaration of this somehow (protected
static on AppListServiceImpl maybe?)

[calamity, 2015-04-07 04:50:35]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[calamity, 2015-04-07 04:50:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/1016503006/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-04-07 04:50:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016503006/80001

[tapted, 2015-04-07 04:54:09]

lgtm

[commit-bot: I haz the power, 2015-04-07 05:51:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-07 05:51:09]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2015-04-07 06:15:33]

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right):

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:374: // Don't warm
up the app list if it hasn't been used for a while.
On 2015/04/07 02:39:16, tapted wrote:
> add ~"If the last launch is unknown, record it as "used" on the first warmup.

Done.

https://codereview.chromium.org/1016503006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:379:
RecordAppListLastLaunch();
On 2015/04/07 02:39:16, tapted wrote:
> pretty sure you'll need to expose the declaration of this somehow (protected
> static on AppListServiceImpl maybe?)

Oops sorry. I thought I'd finished and just left it lying around >_>

Made it public static along with the other record functions.

[calamity, 2015-04-07 06:18:41]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-04-07 06:18:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/1016503006/#ps100001
(title: "fix_unittests")

[commit-bot: I haz the power, 2015-04-07 06:18:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016503006/100001

[commit-bot: I haz the power, 2015-04-07 07:02:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-07 07:03:47]

Patchset 6 (id:??) landed as
https://crrev.com/8f565846801e295986a5e18531498490f09acc6e
Cr-Commit-Position: refs/heads/master@{#324018}