Enable profile switching for standalone App Launcher via the Settings App.

Enable profile switching for standalone App Launcher via the Settings App.

Before: http://i.imgur.com/Cp32FTO.png
After: http://i.imgur.com/V1y0XBz.png


BUG=166983, 169114, 167094
TEST=Launch chrome with the flag --show-app-list, then switch profiles by launching the settings app and using the "switch user" button. The app list should reappear under the new profile (it's possible to tell profiles apart from what apps are installed).

[tapted, 2013-01-17 04:23:30]

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.cc:335:
app_list_profile_dir.AppendASCII(chrome::kInitialProfile) :
Should this (as an extra step) first check prefs::kProfileLastUsed (perhaps
immediately setting kAppListProfile to that preference)?

This would stop it being recreated if someone deletes their default profile and
switches on the App Launcher. It also gives a default that is more likely to
match the currently running browser the first time it's launched.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.h:101: Profile* GetAppListProfile(const
FilePath& user_data_dir);
I think there is a case for putting this into app_list_util.h -- that will keep
the getter/setter in the same place.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_util.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_util.cc:9: namespace app_list_controller {
should be namespace chrome (there was an intervening revision that switched
this, so the patch was out of date in this regard)

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_util.h:20: void UpdateProfilePath(const
FilePath& profile_file_path);
just noticed this will be left undefined on chromeos (not sure how tough the
policy on this is). You could wrap it in ENABLE_SETTINGS_APP.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/manage_profile_handler.cc:22: #include
"chrome/browser/ui/app_list/app_list_util.h"
should perhaps be in the #ifdef below

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2235: extern const char kAppListProfile[] =
"app_list.profile";
this needs a comment (sorry - that was my bad in the initial patch)

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.h#ne...
chrome/common/pref_names.h:826: extern const char kAppListProfile[];
I considered putting this in an #if defined(ENABLE_APP_LIST), and ended up on
the "yes it should be" side of the fence

[benwells, 2013-01-17 04:30:29]

Does this work if Chrome / the app list is already running? It seems like there
are two problems:
- once the app list is initialized it is always used with that profile
- the only startup case that is changed is on chrome_browser_main, don't you
also have to change StartupBrowserCreator::ProcessCommandLineAlreadyRunning?

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.h:101: Profile* GetAppListProfile(const
FilePath& user_data_dir);
On 2013/01/17 04:23:31, tapted wrote:
> I think there is a case for putting this into app_list_util.h -- that will
keep
> the getter/setter in the same place.

I like having the getter/setter in the same place. I got the impression sail
didn't think this should go here from that other review.

[koz (OOO until 15th September), 2013-01-18 05:37:33]

I've made the startup profile loading code smarter and refactored the
AppListController's logic a bit. PTAL :-)

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.cc:335:
app_list_profile_dir.AppendASCII(chrome::kInitialProfile) :
On 2013/01/17 04:23:31, tapted wrote:
> Should this (as an extra step) first check prefs::kProfileLastUsed (perhaps
> immediately setting kAppListProfile to that preference)?
> 
> This would stop it being recreated if someone deletes their default profile
and
> switches on the App Launcher. It also gives a default that is more likely to
> match the currently running browser the first time it's launched.

That's a good idea. Done.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.h:101: Profile* GetAppListProfile(const
FilePath& user_data_dir);
On 2013/01/17 04:30:29, benwells wrote:
> On 2013/01/17 04:23:31, tapted wrote:
> > I think there is a case for putting this into app_list_util.h -- that will
> keep
> > the getter/setter in the same place.
> 
> I like having the getter/setter in the same place. I got the impression sail
> didn't think this should go here from that other review.

Done.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_util.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_util.cc:9: namespace app_list_controller {
On 2013/01/17 04:23:31, tapted wrote:
> should be namespace chrome (there was an intervening revision that switched
> this, so the patch was out of date in this regard)

Done.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_util.h:20: void UpdateProfilePath(const
FilePath& profile_file_path);
On 2013/01/17 04:23:31, tapted wrote:
> just noticed this will be left undefined on chromeos (not sure how tough the
> policy on this is). You could wrap it in ENABLE_SETTINGS_APP.

Done.

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/manage_profile_handler.cc:22: #include
"chrome/browser/ui/app_list/app_list_util.h"
On 2013/01/17 04:23:31, tapted wrote:
> should perhaps be in the #ifdef below

Done.

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2235: extern const char kAppListProfile[] =
"app_list.profile";
On 2013/01/17 04:23:31, tapted wrote:
> this needs a comment (sorry - that was my bad in the initial patch)

Done.

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/11968034/diff/1/chrome/common/pref_names.h#ne...
chrome/common/pref_names.h:826: extern const char kAppListProfile[];
On 2013/01/17 04:23:31, tapted wrote:
> I considered putting this in an #if defined(ENABLE_APP_LIST), and ended up on
> the "yes it should be" side of the fence

Done.

[benwells, 2013-01-18 06:18:45]

Some initial comments...

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_util.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_util.cc:16: }  // namespace
app_list_controller
Nit: app_list_controller->chrome

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_util.h:21: void UpdateProfilePath(const
FilePath& profile_file_path);
Nit UpdateAppListProfile might be a better name: more specific to the app list,
and is is not just updating the preference but the profile in use.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:591: FilePath user_data_dir
= profile_manager->user_data_dir();
Is user_data_dir used?

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:148: void
UpdateFromProfile();
This doesn't appear to be used.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:308:
base::Unretained(this), profile_load_sequence_id_),
Can we use a weak pointer instead?

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:326: #if
!defined(USE_AURA)
This #if is not needed (it is just needed when HWNDs are used)

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:388: void
AppListController::ActivateAppList() {
If UpdateFromProfile is not needed, can this move back into SowAppList?

[koz (OOO until 15th September), 2013-01-20 23:32:00]

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_util.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_util.cc:16: }  // namespace
app_list_controller
On 2013/01/18 06:18:46, benwells wrote:
> Nit: app_list_controller->chrome

Done.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_util.h:21: void UpdateProfilePath(const
FilePath& profile_file_path);
On 2013/01/18 06:18:46, benwells wrote:
> Nit UpdateAppListProfile might be a better name: more specific to the app
list,
> and is is not just updating the preference but the profile in use.

I've changed it to SetAppListProfile() (and made the follow-on renames).

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:591: FilePath user_data_dir
= profile_manager->user_data_dir();
On 2013/01/18 06:18:46, benwells wrote:
> Is user_data_dir used?

Oops, nope. Removed.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:148: void
UpdateFromProfile();
On 2013/01/18 06:18:46, benwells wrote:
> This doesn't appear to be used.

Done.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:308:
base::Unretained(this), profile_load_sequence_id_),
On 2013/01/18 06:18:46, benwells wrote:
> Can we use a weak pointer instead?

Done.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:326: #if
!defined(USE_AURA)
On 2013/01/18 06:18:46, benwells wrote:
> This #if is not needed (it is just needed when HWNDs are used)

Done.

https://codereview.chromium.org/11968034/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:388: void
AppListController::ActivateAppList() {
On 2013/01/18 06:18:46, benwells wrote:
> If UpdateFromProfile is not needed, can this move back into SowAppList?

Done.

[benwells, 2013-01-21 00:36:31]

I think just quibbles now...

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:847:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
Should this go here or in app list somewhere?

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:71: void
InitAppList(Profile* default_profile);
Why are these here and in app_list_util.h?

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:157: void
PopulateViewFromProfile();
Nit: this could take an input profile and set profile_ to that.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:290: 
Nit: extra blank line.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:300: // Invalidate
any pending profile path loads.
Add DCHECK On UI thread.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:343:
CHECK(current_view_ && app_list_is_showing_);
Just wondering, why not a DCHECK?

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:704: // last browser
profile used.
How does this deal with the profile having been since deleted? This might also
be a concern for chrome if the settings app can delete profiles.

[tapted, 2013-01-21 00:37:02]

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1083: 
nit: an extra line has crept in

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:77: void
UpdateProfilePath(const FilePath& profile_file_path);
should be SetAppListProfile

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.h:51: // |command_line| holds
the command line we need to process
nit: while you're here - comment is missing a period. But perhaps add a
description of cur_dir (also: should this be 'user_data_dir'?), and
startup_profile_dir.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.h:53: const CommandLine&
cmd_line,
nit: while you're here.. 'cmd_line' got abbreviated some time in the past and,
additionally, doesn't match the function comment. Should be command_line.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:280:
weak_factory_(this){
nit: missing space before '{', and I think convention is for '}' to be on this
line.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:290: 
nit: extra line can be removed

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:334:
DismissAppList();
Should the next three lines be only if profile != profile_ ? Since, ShowAppList
can be called multiple times for the same, loaded profile.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:687: // Create the
App list.
nit (while you're here): this comment is out of date. Can probably be removed
since the member functions now have comments.

[tapted, 2013-01-21 00:41:13]

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:71: void
InitAppList(Profile* default_profile);
On 2013/01/21 00:36:31, benwells wrote:
> Why are these here and in app_list_util.h?
Actually, that's a better point - (these were refactored out in
http://crrev.com/175961 which happened after the first patch iteration).

[koz (OOO until 15th September), 2013-01-21 23:23:15]

I added some code to keep the browser alive while we are asynchronously loading
a profile with the intent to show it.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:847:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
On 2013/01/21 00:36:31, benwells wrote:
> Should this go here or in app list somewhere?

Moved into app_list_util.h.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1083: 
On 2013/01/21 00:37:02, tapted wrote:
> nit: an extra line has crept in

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:71: void
InitAppList(Profile* default_profile);
On 2013/01/21 00:36:31, benwells wrote:
> Why are these here and in app_list_util.h?

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.h:51: // |command_line| holds
the command line we need to process
On 2013/01/21 00:37:02, tapted wrote:
> nit: while you're here - comment is missing a period. But perhaps add a
> description of cur_dir (also: should this be 'user_data_dir'?), and
> startup_profile_dir.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.h:53: const CommandLine&
cmd_line,
On 2013/01/21 00:37:02, tapted wrote:
> nit: while you're here.. 'cmd_line' got abbreviated some time in the past and,
> additionally, doesn't match the function comment. Should be command_line.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:157: void
PopulateViewFromProfile();
On 2013/01/21 00:36:31, benwells wrote:
> Nit: this could take an input profile and set profile_ to that.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:280:
weak_factory_(this){
On 2013/01/21 00:37:02, tapted wrote:
> nit: missing space before '{', and I think convention is for '}' to be on this
> line.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:290: 
On 2013/01/21 00:36:31, benwells wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:290: 
On 2013/01/21 00:37:02, tapted wrote:
> nit: extra line can be removed

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:300: // Invalidate
any pending profile path loads.
On 2013/01/21 00:36:31, benwells wrote:
> Add DCHECK On UI thread.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:334:
DismissAppList();
On 2013/01/21 00:37:02, tapted wrote:
> Should the next three lines be only if profile != profile_ ? Since,
ShowAppList
> can be called multiple times for the same, loaded profile.

Cool, good catch. I've changed PopulateViewFromProfile() to check profile ==
profile_ which should remove that inefficiency.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:343:
CHECK(current_view_ && app_list_is_showing_);
On 2013/01/21 00:36:31, benwells wrote:
> Just wondering, why not a DCHECK?

Not sure; this was inherited from Trent's change.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:687: // Create the
App list.
On 2013/01/21 00:37:02, tapted wrote:
> nit (while you're here): this comment is out of date. Can probably be removed
> since the member functions now have comments.

Done.

https://codereview.chromium.org/11968034/diff/17001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:704: // last browser
profile used.
On 2013/01/21 00:36:31, benwells wrote:
> How does this deal with the profile having been since deleted? This might also
> be a concern for chrome if the settings app can delete profiles.

Currently this will re-create the profile (as per ProfileManager::GetProfile()'s
semantics).

What happens in the browser if you delete the current profile is it goes and
closes all the browser windows (but not apps) for that profile. It also updates
the kProfileLastUsed preference so that it's still well-defined. I've added some
code to ProfileManager::ScheduleProfileForDeletion() to update the
kAppListProfile preference as well, which should prevent us from getting into a
funny state. I also added a TODO to move that code out of ProfileManager.

[benwells, 2013-01-22 01:21:58]

lgtm with a couple of comments.

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:62: #include
"chrome/browser/ui/app_list/app_list_util.h"
Nit: is this needed?

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:343:
ShowAppList(profile);
Can you remove this extra ShowAppList?

[koz (OOO until 15th September), 2013-01-22 02:23:49]

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:62: #include
"chrome/browser/ui/app_list/app_list_util.h"
On 2013/01/22 01:21:59, benwells wrote:
> Nit: is this needed?

Nope, removed.

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:343:
ShowAppList(profile);
On 2013/01/22 01:21:59, benwells wrote:
> Can you remove this extra ShowAppList?

Done.

[koz (OOO until 15th September), 2013-01-22 02:28:11]

+sky for OWNERS, in particular the changes to loading profiles at startup.

[tapted, 2013-01-22 03:01:08]

+estade for webui changes and OWNERS in /c/b/resources/options - he helped with
the earlier changes there for the settings app.

otherwise lgtm for general review, but I'll defer to OWNERS for
profile_manager.cc note below.

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:847:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
sorry for not noticing this sooner - but if these changes are going into
profile_manager.cc (and not app_list_controller_win.cc), this and the change in
ScheduleProfileForDeletion will also need an #ifdef guard, unless the #ifdef
guard around prefs::kAppListProfile in pref_names.h/.cc is taken out.

While there's precedent for #if defined OS_IOS/_WIN/_CHROMEOS in
profile_manager.cc already, I'm undecided whether more #ifdefs are a good thing.
I'll defer to someone who's been around longer than me. Perhaps on balance it
makes sense to remove the guard in pref_names.h for clarity, or perhaps having
guards in profile_manager.cc is good additional intrinsic documentation for why
the code is present.

https://codereview.chromium.org/11968034/diff/22002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/common/pref_names....
chrome/common/pref_names.cc:2238: #if defined(ENABLE_APP_LIST)
ENABLE_SETTINGS_APP or remove

https://codereview.chromium.org/11968034/diff/22002/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/common/pref_names....
chrome/common/pref_names.h:827: #if defined(ENABLE_APP_LIST)
should be ENABLE_SETTINGS_APP if we want to avoid it leaking to ChromeOS (or
removed if it's hurting clarity)

[sky, 2013-01-22 15:55:25]

Please elaborate on this. Are you saying we're going to start loading
all profiles at startup? If so, we shouldn't do that.

  -Scott

On Mon, Jan 21, 2013 at 6:28 PM,  <koz@chromium.org> wrote:
> +sky for OWNERS, in particular the changes to loading profiles at startup.
>
> https://codereview.chromium.org/11968034/

[tapted, 2013-01-22 16:11:26]

On 2013/01/22 15:55:25, sky wrote:
> Please elaborate on this. Are you saying we're going to start loading
> all profiles at startup? If so, we shouldn't do that.

(I'm up unusually late, so am chiming in for turnarounds). It will still just be
a single profile loaded, and the change to profile loading at startup should
just affect the case where the app list has been invoked by the user from a
taskbar shortcut. (This is the CrOS-style app launcher on windows)

The change adds a new local_pref for a profile bound to the app list.

In the Chrome-not-running case, no browser will be opened, and the startup
profile will be the app list profile path. In the chrome-already-running, the
app list profile may be loaded (asynchronously) if it is not already loaded in
the browser process.

[sky, 2013-01-22 16:15:21]

Ok, can you clarify what files in here you need me to look at?

[tapted, 2013-01-22 17:12:41]

On 2013/01/22 16:15:21, sky wrote:
> Ok, can you clarify what files in here you need me to look at?

(chiming in again..) sky, I think you are needed at least for
- c/b/chrome_browser_main.cc
- c/b/ui/ash/*/*
- c/b/ui/startup/startup_browser_creator*

@koz: I don't think these have owners either:
- c/b/profiles/profile_manager.{h,cc}
- c/b/ui/app_list/*

[tfarina, 2013-01-22 17:17:12]

On Tue, Jan 22, 2013 at 3:12 PM,  <tapted@chromium.org> wrote:
> @koz: I don't think these have owners either:
> - c/b/profiles/profile_manager.{h,cc}
sail and mirandac, erg

> - c/b/ui/app_list/*
xiyuan

> https://codereview.chromium.org/11968034/



-- 
Thiago

[sky, 2013-01-22 23:56:18]

LGTM - as Thiago says Xiyuan should revie the app_list changes.

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:314: FilePath GetStartupProfilePath(const
FilePath& user_data_dir,
Add description.

[koz (OOO until 15th September), 2013-01-23 00:07:55]

Great, thanks a million, Trent!

I've added sail@ for profile_manager.{h,cc} changes.

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/22002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:847:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
On 2013/01/22 03:01:08, tapted wrote:
> sorry for not noticing this sooner - but if these changes are going into
> profile_manager.cc (and not app_list_controller_win.cc), this and the change
in
> ScheduleProfileForDeletion will also need an #ifdef guard, unless the #ifdef
> guard around prefs::kAppListProfile in pref_names.h/.cc is taken out.
> 
> While there's precedent for #if defined OS_IOS/_WIN/_CHROMEOS in
> profile_manager.cc already, I'm undecided whether more #ifdefs are a good
thing.
> I'll defer to someone who's been around longer than me. Perhaps on balance it
> makes sense to remove the guard in pref_names.h for clarity, or perhaps having
> guards in profile_manager.cc is good additional intrinsic documentation for
why
> the code is present.

Good catch. I'll add the #ifdefs for now and see what sail says.

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:314: FilePath GetStartupProfilePath(const
FilePath& user_data_dir,
On 2013/01/22 23:56:18, sky wrote:
> Add description.

Done.

[xiyuan, 2013-01-23 00:09:33]

LGTM

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:12: class FilePath;
nit: seems not used

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:20: :
controller_(controller), profile_(profile) {}
nit: one member per line

[Evan Stade, 2013-01-23 00:16:21]

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/resources/...
File chrome/browser/resources/options/options_settings_app.js (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/resources/...
chrome/browser/resources/options/options_settings_app.js:9:
document.documentElement.classList.add('settings-app');
move this to OptionsPage.setIsSettingsApp. Also, don't have 2 sources of truth
(js value and css class)

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:324: return;
no need for this return

why is this function defined at all for non settings app?

[koz (OOO until 15th September), 2013-01-23 00:16:43]

Ta!

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:12: class FilePath;
On 2013/01/23 00:09:33, xiyuan wrote:
> nit: seems not used

Done.

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:20: :
controller_(controller), profile_(profile) {}
On 2013/01/23 00:09:33, xiyuan wrote:
> nit: one member per line

Done.

[koz (OOO until 15th September), 2013-01-23 02:58:58]

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/resources/...
File chrome/browser/resources/options/options_settings_app.js (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/resources/...
chrome/browser/resources/options/options_settings_app.js:9:
document.documentElement.classList.add('settings-app');
On 2013/01/23 00:16:21, Evan Stade wrote:
> move this to OptionsPage.setIsSettingsApp. Also, don't have 2 sources of truth
> (js value and css class)

Done.

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/24028/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:324: return;
On 2013/01/23 00:16:21, Evan Stade wrote:
> no need for this return
> 
> why is this function defined at all for non settings app?

Done.

[sail, 2013-01-23 16:55:24]

This CL needs:
  - tests
  - links to screenshots for all UI changes
  - remove #ifdefs

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:329: if
(command_line.HasSwitch(switches::kShowAppList))
why this change?

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:848:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
This should go in the app list code base. Not profiles. See
chrome::RegisterLocalState() for other people do this.

In general, keep your feature code in its own codebase.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1006: std::string
app_list_last_profile = local_state->GetString(
same, don't put your feature code here
if you need to do something when a profile is deleted then do the same thing
everyone else does, listen for a notification

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:106: FilePath
GetLastUsedProfilePath(const FilePath& user_data_dir);
The profile manager class is way to big right now. One problem is that it has
multiple ways to do the same thing. This makes it hard to maintain and hard to
understand.

Is there anyway we can avoid adding yet another way to get the last used
profile? If not can you remove GetLastUsedProfile and convert everyone to use
your new function?

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_util.h:19: #if defined(ENABLE_SETTINGS_APP)
don't scatter #ifdefs in code
instead have a single function that you can ask for feature availability

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:325: if
(!ProfileManager::IsMultipleProfilesEnabled())
The profile switch UI shouldn't be shown if multi-profiles is disabled.
This should be a DCHECK() instead.

[Evan Stade, 2013-01-23 22:18:33]

lgtm

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/resources/...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/resources/...
chrome/browser/resources/options/options_page.js:48: OptionsPage.settingsApp_ =
false;
remove

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.h:81: void
ProfilesAppListSwitch(const base::ListValue* args);
can you make this function and message name more similar to  an English sentence
in terms of word order?

[koz (OOO until 15th September), 2013-01-25 00:32:46]

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:329: if
(command_line.HasSwitch(switches::kShowAppList))
On 2013/01/23 16:55:24, sail wrote:
> why this change?

If chrome is started with the --show-app-list flag, it launches the app list
rather than a browser window. In this case we should load the profile used with
the app list instead of the last used browser profile.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:848:
prefs->RegisterStringPref(prefs::kAppListProfile, "");
On 2013/01/23 16:55:24, sail wrote:
> This should go in the app list code base. Not profiles. See
> chrome::RegisterLocalState() for other people do this.
> 
> In general, keep your feature code in its own codebase.

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1006: std::string
app_list_last_profile = local_state->GetString(
On 2013/01/23 16:55:24, sail wrote:
> same, don't put your feature code here
> if you need to do something when a profile is deleted then do the same thing
> everyone else does, listen for a notification

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:106: FilePath
GetLastUsedProfilePath(const FilePath& user_data_dir);
On 2013/01/23 16:55:24, sail wrote:
> The profile manager class is way to big right now. One problem is that it has
> multiple ways to do the same thing. This makes it hard to maintain and hard to
> understand.
> 
> Is there anyway we can avoid adding yet another way to get the last used
> profile? If not can you remove GetLastUsedProfile and convert everyone to use
> your new function?

I think the existence of these related functions is a result of the fact that
ProfileManager is responsible for loading profiles as well as managing these
preferences; there's already analogous methods GetDefaultProfile() and
GetDefaultProfileDir().

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/resources/...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/resources/...
chrome/browser/resources/options/options_page.js:48: OptionsPage.settingsApp_ =
false;
On 2013/01/23 22:18:33, Evan Stade wrote:
> remove

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_util.h:19: #if defined(ENABLE_SETTINGS_APP)
On 2013/01/23 16:55:24, sail wrote:
> don't scatter #ifdefs in code
> instead have a single function that you can ask for feature availability

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:325: if
(!ProfileManager::IsMultipleProfilesEnabled())
On 2013/01/23 16:55:24, sail wrote:
> The profile switch UI shouldn't be shown if multi-profiles is disabled.
> This should be a DCHECK() instead.

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.h:81: void
ProfilesAppListSwitch(const base::ListValue* args);
On 2013/01/23 22:18:33, Evan Stade wrote:
> can you make this function and message name more similar to  an English
sentence
> in terms of word order?

I've changed it to SwitchAppListProfile().

[sail, 2013-01-25 18:22:34]

Looks good overall. Still needs links to screenshots in CL description.

Also, you need tests for new functionality you've added. Here's a simple example
of how to write a test for multiple profiles:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/browser...

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:329: if
(command_line.HasSwitch(switches::kShowAppList))
On 2013/01/25 00:32:46, koz wrote:
> On 2013/01/23 16:55:24, sail wrote:
> > why this change?
> 
> If chrome is started with the --show-app-list flag, it launches the app list
> rather than a browser window. In this case we should load the profile used
with
> the app list instead of the last used browser profile.

This needs a comment.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:106: FilePath
GetLastUsedProfilePath(const FilePath& user_data_dir);
On 2013/01/25 00:32:46, koz wrote:
> On 2013/01/23 16:55:24, sail wrote:
> > The profile manager class is way to big right now. One problem is that it
has
> > multiple ways to do the same thing. This makes it hard to maintain and hard
to
> > understand.
> > 
> > Is there anyway we can avoid adding yet another way to get the last used
> > profile? If not can you remove GetLastUsedProfile and convert everyone to
use
> > your new function?
> 
> I think the existence of these related functions is a result of the fact that
> ProfileManager is responsible for loading profiles as well as managing these
> preferences; there's already analogous methods GetDefaultProfile() and
> GetDefaultProfileDir().

Argh. I'd like to avoid this. If you can't figure out a better solution then
lets at least match the naming convention in GetDefaultProfile*().

[tapted, 2013-01-26 01:10:08]

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:225: #if defined(ENABLE_APP_LIST)
This will be true on CrOS, so it should be ENABLE_SETTINGS_APP.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:226:
chrome::RegisterAppListPrefs(local_state);
Otherwise CrOS will need a definition of this in app_list_controller_ash.cc.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/resources/...
File chrome/browser/resources/options/options_settings_app.js (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/resources/...
chrome/browser/resources/options/options_settings_app.js:9:
document.documentElement.classList.add('settings-app');
should be removed.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:27:
ASSERT_FALSE(chrome::IsAppListVisible());
I think this test will run on CrOS, so app_list_controller_ash.cc will need an
implementation of IsAppListVisible().

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_util.h:38: // Returns true if the app list
is visible.
perhaps a note that this is only used for tests

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:780: #if
defined(ENABLE_APP_LIST)
this ifdef is redundant

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:337: #endif  // else
!defined(ENABLE_SETTINGS_APP)
comment should now be

  // defined(ENABLE_SETTINGS_APP)

[Alexei Svitkine (slow), 2013-01-28 19:00:34]

Should this have a TEST= line?

[koz (OOO until 15th September), 2013-01-29 00:34:11]

Thanks for the link sail. I ended up adding another browser_test to test the
profile switching aspect of the change as showing the app list is not unit test
friendly.

I've updated the CL description to have a TEST= field and point out that
screenshots can be found in the bug (I can't seem to hotlink directly to
attachments on crbug.com).

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:329: if
(command_line.HasSwitch(switches::kShowAppList))
On 2013/01/25 18:22:34, sail wrote:
> On 2013/01/25 00:32:46, koz wrote:
> > On 2013/01/23 16:55:24, sail wrote:
> > > why this change?
> > 
> > If chrome is started with the --show-app-list flag, it launches the app list
> > rather than a browser window. In this case we should load the profile used
> with
> > the app list instead of the last used browser profile.
> 
> This needs a comment.

Done.

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/11968034/diff/21005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:106: FilePath
GetLastUsedProfilePath(const FilePath& user_data_dir);
On 2013/01/25 18:22:34, sail wrote:
> On 2013/01/25 00:32:46, koz wrote:
> > On 2013/01/23 16:55:24, sail wrote:
> > > The profile manager class is way to big right now. One problem is that it
> has
> > > multiple ways to do the same thing. This makes it hard to maintain and
hard
> to
> > > understand.
> > > 
> > > Is there anyway we can avoid adding yet another way to get the last used
> > > profile? If not can you remove GetLastUsedProfile and convert everyone to
> use
> > > your new function?
> > 
> > I think the existence of these related functions is a result of the fact
that
> > ProfileManager is responsible for loading profiles as well as managing these
> > preferences; there's already analogous methods GetDefaultProfile() and
> > GetDefaultProfileDir().
> 
> Argh. I'd like to avoid this. If you can't figure out a better solution then
> lets at least match the naming convention in GetDefaultProfile*().

Unfortunately a better way doesn't jump out at me, so I've done the rename :-(

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:225: #if defined(ENABLE_APP_LIST)
On 2013/01/26 01:10:08, tapted wrote:
> This will be true on CrOS, so it should be ENABLE_SETTINGS_APP.

Done.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/resources/...
File chrome/browser/resources/options/options_settings_app.js (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/resources/...
chrome/browser/resources/options/options_settings_app.js:9:
document.documentElement.classList.add('settings-app');
On 2013/01/26 01:10:08, tapted wrote:
> should be removed.

Done.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:27:
ASSERT_FALSE(chrome::IsAppListVisible());
On 2013/01/26 01:10:08, tapted wrote:
> I think this test will run on CrOS, so app_list_controller_ash.cc will need an
> implementation of IsAppListVisible().

Done.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_util.h (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_util.h:38: // Returns true if the app list
is visible.
On 2013/01/26 01:10:08, tapted wrote:
> perhaps a note that this is only used for tests

I think it might have uses outside testing later so would rather not.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:780: #if
defined(ENABLE_APP_LIST)
On 2013/01/26 01:10:08, tapted wrote:
> this ifdef is redundant

Done.

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/11968034/diff/32027/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/manage_profile_handler.cc:337: #endif  // else
!defined(ENABLE_SETTINGS_APP)
On 2013/01/26 01:10:08, tapted wrote:
> comment should now be
> 
>   // defined(ENABLE_SETTINGS_APP)

Done.

[sail, 2013-01-29 18:28:55]

Code looks good to me.

> (I can't seem to hotlink directly to attachments on crbug.com).
It would be better to post screenshots to somewhere that you can link to.

Also, I'm not sure adding a "Switch User" button in Settings makes much sense.
There are several problems with this:
  - how would you know that "Switch User" has anything to do with app launcher?
  - what happens when you click the button? it's not clear, does the selected
profile become the default profile for the app launcher? How can I tell which is
the current default profile?
  - since the app launcher is supposed to be a different app, it's weird to have
to launch the browser to control it

https://codereview.chromium.org/11968034/diff/42001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11968034/diff/42001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:82:
DISALLOW_COPY_AND_ASSIGN(ShowAppListBrowserTest);
private:

[koz (OOO until 15th September), 2013-01-29 22:58:50]

On 2013/01/29 18:28:55, sail wrote:
> Code looks good to me.
> 
> > (I can't seem to hotlink directly to attachments on http://crbug.com).
> It would be better to post screenshots to somewhere that you can link to.
> 
> Also, I'm not sure adding a "Switch User" button in Settings makes much sense.
> There are several problems with this:
>   - how would you know that "Switch User" has anything to do with app
launcher?

The button is in the Settings App, which is only reachable from the app
launcher.

>   - what happens when you click the button? it's not clear, does the selected
> profile become the default profile for the app launcher? How can I tell which
is
> the current default profile?

Yes, it does make that profile the default profile for the app launcher. Right
now there is no specific UI for showing what the current default is (apart from
the settings app, which will highlight the currently active profile) - this UI
is temporary and needed for the dev preview of Chrome Apps and has been approved
by the UI team. Hopefully we will have the time to improve it soon.

>   - since the app launcher is supposed to be a different app, it's weird to
have
> to launch the browser to control it

It doesn't launch chrome, it launches the settings app, which is a subset of
chrome://settings bundled up in an app.

[koz (OOO until 15th September), 2013-01-29 22:59:02]

https://codereview.chromium.org/11968034/diff/42001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11968034/diff/42001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:82:
DISALLOW_COPY_AND_ASSIGN(ShowAppListBrowserTest);
On 2013/01/29 18:28:56, sail wrote:
> private:

Done.

[sail, 2013-01-29 23:05:10]

On 2013/01/29 22:58:50, koz wrote:
> On 2013/01/29 18:28:55, sail wrote:
> > Code looks good to me.
> > 
> > > (I can't seem to hotlink directly to attachments on http://crbug.com).
> > It would be better to post screenshots to somewhere that you can link to.
> > 
> > Also, I'm not sure adding a "Switch User" button in Settings makes much
sense.
> > There are several problems with this:
> >   - how would you know that "Switch User" has anything to do with app
> launcher?
> 
> The button is in the Settings App, which is only reachable from the app
> launcher.
> 
> >   - what happens when you click the button? it's not clear, does the
selected
> > profile become the default profile for the app launcher? How can I tell
which
> is
> > the current default profile?
> 
> Yes, it does make that profile the default profile for the app launcher. Right
> now there is no specific UI for showing what the current default is (apart
from
> the settings app, which will highlight the currently active profile) - this UI
> is temporary and needed for the dev preview of Chrome Apps and has been
approved
> by the UI team. Hopefully we will have the time to improve it soon.
> 
> >   - since the app launcher is supposed to be a different app, it's weird to
> have
> > to launch the browser to control it
> 
> It doesn't launch chrome, it launches the settings app, which is a subset of
> chrome://settings bundled up in an app.

Ahh, makes sense. Could you post a screenshot of the entire window to give
better context?

[koz (OOO until 15th September), 2013-01-29 23:27:19]

On 2013/01/29 23:05:10, sail wrote:
> On 2013/01/29 22:58:50, koz wrote:
> > On 2013/01/29 18:28:55, sail wrote:
> > > Code looks good to me.
> > > 
> > > > (I can't seem to hotlink directly to attachments on http://crbug.com).
> > > It would be better to post screenshots to somewhere that you can link to.
> > > 
> > > Also, I'm not sure adding a "Switch User" button in Settings makes much
> sense.
> > > There are several problems with this:
> > >   - how would you know that "Switch User" has anything to do with app
> > launcher?
> > 
> > The button is in the Settings App, which is only reachable from the app
> > launcher.
> > 
> > >   - what happens when you click the button? it's not clear, does the
> selected
> > > profile become the default profile for the app launcher? How can I tell
> which
> > is
> > > the current default profile?
> > 
> > Yes, it does make that profile the default profile for the app launcher.
Right
> > now there is no specific UI for showing what the current default is (apart
> from
> > the settings app, which will highlight the currently active profile) - this
UI
> > is temporary and needed for the dev preview of Chrome Apps and has been
> approved
> > by the UI team. Hopefully we will have the time to improve it soon.
> > 
> > >   - since the app launcher is supposed to be a different app, it's weird
to
> > have
> > > to launch the browser to control it
> > 
> > It doesn't launch chrome, it launches the settings app, which is a subset of
> > chrome://settings bundled up in an app.
> 
> Ahh, makes sense. Could you post a screenshot of the entire window to give
> better context?

Yep, done.

[sail, 2013-01-29 23:33:55]

> Yep, done.

The multi-user UI doesn't really make sense.
Why would you want the user to create a new profile from the app list settings
UI?
Can we get together and discuss this UI?

[sail, 2013-01-29 23:39:46]

On 2013/01/29 23:33:55, sail wrote:
> > Yep, done.
> 
> The multi-user UI doesn't really make sense.
> Why would you want the user to create a new profile from the app list settings
> UI?
> Can we get together and discuss this UI?

Also, a couple of questions:
  - what does this look like when the user has only 1 profile?
  - how do you open the app-launcher settings?

[sail, 2013-01-30 01:15:33]

Ok, we had a bunch of discussion about this in person. The conclusion was:
  - the UI in this CL should be temporary
  - the final UI we can is a profile switcher right in the Apps Launcher UI
  - I'm going to work on the profile switcher implementation. If I can get it
landed ASAP then we should be able to remove the temporary UI before M26 branch

On 2013/01/29 23:39:46, sail wrote:
> On 2013/01/29 23:33:55, sail wrote:
> > > Yep, done.
> > 
> > The multi-user UI doesn't really make sense.
> > Why would you want the user to create a new profile from the app list
settings
> > UI?
> > Can we get together and discuss this UI?
> 
> Also, a couple of questions:
>   - what does this look like when the user has only 1 profile?
>   - how do you open the app-launcher settings?

[sail, 2013-01-30 01:15:44]

LGTM