Add unit tests for AppListServiceImpl.

Add unit tests for AppListServiceImpl.

TBR=sail@chromium.org

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

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

[koz (OOO until 15th September), 2013-09-26 03:12:01]

https://codereview.chromium.org/24707002/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_service_impl.cc (left):

https://codereview.chromium.org/24707002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_impl.cc:151: // Ensure we don't set
the pref to a managed user's profile path.
Note: this omission is intentional, as per my comments in
https://chromiumcodereview.appspot.com/24360013/.

[koz (OOO until 15th September), 2013-09-26 05:47:20]

https://codereview.chromium.org/24707002/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_service_impl.cc (left):

https://codereview.chromium.org/24707002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_impl.cc:151: // Ensure we don't set
the pref to a managed user's profile path.
On 2013/09/26 03:12:01, koz wrote:
> Note: this omission is intentional, as per my comments in
> https://chromiumcodereview.appspot.com/24360013/.

Never mind that - as per the discussion I've re-added this.

[benwells, 2013-09-26 06:55:29]

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:22: class
TestingAppListServiceImpl : public AppListServiceImpl {
Shouldn't this file be called app_list_service_unittest.cc?

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:87: virtual void
TearDown() OVERRIDE {
Is this empty override here for a reason? If so, state it, otherwise remove it.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:98:
TEST_F(AppListControllerUnitTest, EnablingStateIsPersisted) {
It seems like we could do a lot more testing. E.g. command line handling,
remembering which profile is active and handling profiles being removed, etc.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_service_impl.cc:111: case
Profile::CREATE_STATUS_CANCELED:
This previously decremented the pending profile load count (in the loader), but
now it doesn't. Is this a deliberate change?

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:90:
pickle->WriteFloat(it->scale());
What is this for? Is it an unrelated change?

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/profile_loader.h (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader.h:26: // This is useful for loading
profiles in response to user interaction where
Please add a TODO to remove this class and fold it into AppListServiceImpl, or
just do it.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/profile_loader_unittest.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader_unittest.cc:46:
TEST_F(ProfileLoaderUnittest, LoadASecondProfileBeforeTheFirstFinishes) {
Would be good to test InvalidatePendingProfileLoads as well.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader_unittest.cc:46:
TEST_F(ProfileLoaderUnittest, LoadASecondProfileBeforeTheFirstFinishes) {
Also would be good to test this classes interaction with the keepalive service.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/test/fake_profile_store.h (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/test/fake_profile_store.h:17: void
RunCallbackByPath(const base::FilePath& path, Profile* profile);
Can you add a comment about what this method does, or maybe give a better name?
Is it SignalProfileLoadComplete?

[koz (OOO until 15th September), 2013-09-26 22:12:46]

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:22: class
TestingAppListServiceImpl : public AppListServiceImpl {
On 2013/09/26 06:55:29, benwells wrote:
> Shouldn't this file be called app_list_service_unittest.cc?

Er, yes. Yes it should. Renamed.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:87: virtual void
TearDown() OVERRIDE {
On 2013/09/26 06:55:29, benwells wrote:
> Is this empty override here for a reason? If so, state it, otherwise remove
it.

Done.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_unittest.cc:98:
TEST_F(AppListControllerUnitTest, EnablingStateIsPersisted) {
On 2013/09/26 06:55:29, benwells wrote:
> It seems like we could do a lot more testing. E.g. command line handling,
> remembering which profile is active and handling profiles being removed, etc.

Done.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_service_impl.cc:111: case
Profile::CREATE_STATUS_CANCELED:
On 2013/09/26 06:55:29, benwells wrote:
> This previously decremented the pending profile load count (in the loader),
but
> now it doesn't. Is this a deliberate change?

Yes. All we need from the profile system is the ability to load profiles
asynchronously. We can keep track of whether or not there are pending loads
ourselves (which is what ProfileLoader does, currently).

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:90:
pickle->WriteFloat(it->scale());
On 2013/09/26 06:55:29, benwells wrote:
> What is this for? Is it an unrelated change?

For some reason the tests for this crash on my computer when I run them, though
not on the bots.

Ah, it's probably caused by bad test isolation: the tests should crash but
because they are run in a big group with other tests the global state has
changed by the time it gets to them and they pass.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/profile_loader.h (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader.h:26: // This is useful for loading
profiles in response to user interaction where
On 2013/09/26 06:55:29, benwells wrote:
> Please add a TODO to remove this class and fold it into AppListServiceImpl, or
> just do it.

Added TODO.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/profile_loader_unittest.cc (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader_unittest.cc:46:
TEST_F(ProfileLoaderUnittest, LoadASecondProfileBeforeTheFirstFinishes) {
On 2013/09/26 06:55:29, benwells wrote:
> Would be good to test InvalidatePendingProfileLoads as well.

Done.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/profile_loader_unittest.cc:46:
TEST_F(ProfileLoaderUnittest, LoadASecondProfileBeforeTheFirstFinishes) {
On 2013/09/26 06:55:29, benwells wrote:
> Also would be good to test this classes interaction with the keepalive
service.

Done.

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/test/fake_profile_store.h (right):

https://codereview.chromium.org/24707002/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/test/fake_profile_store.h:17: void
RunCallbackByPath(const base::FilePath& path, Profile* profile);
On 2013/09/26 06:55:29, benwells wrote:
> Can you add a comment about what this method does, or maybe give a better
name?
> Is it SignalProfileLoadComplete?

I added a pair of more general functions LoadProfile() and RemoveProfile() so we
can test situations where profiles are already loaded vs when they aren't, and
profile removal.

[benwells, 2013-09-27 09:00:39]

lgtm with the stuff made protected again.

Awesome to see all these tests! Plus its great to see chrome dependencies like
g_browser_process removed and injected as needed.

Please clean up the things like ProfileLoader / ProfileStore soon, and think
about that weird AppListViewWin thing as well (maybe that could be
AppListViewInterface, as it feels like that things public abstract interface).

https://codereview.chromium.org/24707002/diff/11001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_impl.h (left):

https://codereview.chromium.org/24707002/diff/11001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_impl.h:30: protected:
Can we bring back protected? I like keeping visibility at a minimum.

[koz (OOO until 15th September), 2013-09-30 00:53:27]

Thanks!

https://codereview.chromium.org/24707002/diff/11001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_impl.h (left):

https://codereview.chromium.org/24707002/diff/11001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_impl.h:30: protected:
On 2013/09/27 09:00:40, benwells wrote:
> Can we bring back protected? I like keeping visibility at a minimum.

Done. I've kept things that are public in the parent interface public though, as
I can't think of a reason not to and to do otherwise adds significantly to the
boilerplate required by tests.

[commit-bot: I haz the power, 2013-09-30 00:59:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/23001

[commit-bot: I haz the power, 2013-09-30 01:29:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[koz (OOO until 15th September), 2013-09-30 01:39:02]

TBRing sail@ for deletions from chrome/browser/profiles.

[commit-bot: I haz the power, 2013-09-30 01:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/23001

[commit-bot: I haz the power, 2013-09-30 02:09:23]

Retried try job too often on win7_aura for step(s) ash_unittests,
aura_unittests, browser_tests, compositor_unittests, content_browsertests,
content_unittests, interactive_ui_tests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-30 03:49:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/54001

[commit-bot: I haz the power, 2013-09-30 04:25:14]

Retried try job too often on win7_aura for step(s) ash_unittests,
aura_unittests, browser_tests, compositor_unittests, content_browsertests,
content_unittests, interactive_ui_tests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-30 04:34:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/36001

[commit-bot: I haz the power, 2013-09-30 05:19:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-30 06:24:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/41001

[commit-bot: I haz the power, 2013-09-30 07:03:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-30 08:13:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/102001

[commit-bot: I haz the power, 2013-09-30 09:10:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-30 09:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/120001

[sail, 2013-09-30 15:06:05]

lgtm

[commit-bot: I haz the power, 2013-09-30 16:26:54]

Change committed as 225992

[commit-bot: I haz the power, 2013-10-02 00:32:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/24707002/120001

[commit-bot: I haz the power, 2013-10-02 00:35:07]

Change committed as 226365