Add profile selector menu to app list.

chrome/browser/ui/app_list/app_list_view_delegate.cc

Index: chrome/browser/ui/app_list/app_list_view_delegate.cc
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index e53fa9bdd092c35d14cd52b1973d63fc9a93da9b..21204b6efca928f035fe0e8bd531639fc9ad71a5 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -6,10 +6,12 @@
 
 #include "base/callback.h"
 #include "base/files/file_path.h"
+#include "base/stl_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/feedback/feedback_util.h"
+#include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/apps_model_builder.h"
@@ -27,6 +29,7 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/user_metrics.h"
+#include "ui/app_list/search_box_model.h"
 
 #if defined(USE_ASH)
 #include "chrome/browser/ui/ash/app_list/app_sync_ui_state_watcher.h"
@@ -51,6 +54,20 @@ void CreateShortcutInWebAppDir(
 }
 #endif
 
+void PopulateUsers(ProfileInfoCache& profile_info,

[koz (OOO until 15th September), 2013/09/09 22:58:07]

Use a const ref or a pointer, non-const refs are against the style guide.

[calamity, 2013/09/10 22:50:46]

Done.

+                   base::FilePath active_profile_path,

[tapted, 2013/09/10 00:29:26]

nit: const-ref for this guy too ;)

[calamity, 2013/09/10 22:50:46]

Done.

+                   app_list::AppListModel::Users* users) {
+  const size_t count = profile_info.GetNumberOfProfiles();
+  for (size_t i = 0; i < count; ++i) {
+    app_list::AppListModel::User* user = new app_list::AppListModel::User();

[koz (OOO until 15th September), 2013/09/09 22:58:07]

nit: Use a scoped_ptr<> and then .release() it into |users|.

[calamity, 2013/09/10 22:50:46]

No longer using pointers.

+    user->name = profile_info.GetNameOfProfileAtIndex(i);
+    user->email = profile_info.GetUserNameOfProfileAtIndex(i);
+    user->profile_path = profile_info.GetPathOfProfileAtIndex(i);
+    user->active = active_profile_path == user->profile_path;
+    users->push_back(user);
+  }
+}
+
 }  // namespace
 
 AppListViewDelegate::AppListViewDelegate(AppListControllerDelegate* controller,
@@ -58,63 +75,85 @@ AppListViewDelegate::AppListViewDelegate(AppListControllerDelegate* controller,
     : controller_(controller),
       profile_(profile),
       model_(NULL) {
+  RegisterForNotifications();
+  g_browser_process->profile_manager()->GetProfileInfoCache().AddObserver(this);
+}
+
+AppListViewDelegate::~AppListViewDelegate() {
+  g_browser_process->
+      profile_manager()->GetProfileInfoCache().RemoveObserver(this);
+}
+
+void AppListViewDelegate::RegisterForNotifications() {
+  registrar_.RemoveAll();
   DCHECK(profile_);
+
   registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL,
                  content::Source<Profile>(profile_));
   registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_SIGNIN_FAILED,
                  content::Source<Profile>(profile_));
   registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
                  content::Source<Profile>(profile_));
-  g_browser_process->profile_manager()->GetProfileInfoCache().AddObserver(this);
-}
-
-AppListViewDelegate::~AppListViewDelegate() {
-  g_browser_process->
-      profile_manager()->GetProfileInfoCache().RemoveObserver(this);
 }
 
 void AppListViewDelegate::OnProfileChanged() {
-  model_->SetSignedIn(!signin_delegate_->NeedSignin());
-  ProfileInfoCache& cache =
-      g_browser_process->profile_manager()->GetProfileInfoCache();
-  // Populate the current user details.
-  size_t profile_index = cache.GetIndexOfProfileWithPath(profile_->GetPath());
-  // The profile won't exist in the cache if the current app list profile is
-  // being deleted.
-  if (profile_index == std::string::npos)
-    return;
+  model_->SetSignedIn(!GetSigninDelegate()->NeedSignin());
 
-  model_->SetCurrentUser(cache.GetNameOfProfileAtIndex(profile_index),
-                         cache.GetUserNameOfProfileAtIndex(profile_index));
+  // Populate the app list users.
+  app_list::AppListModel::Users users;
+  PopulateUsers(g_browser_process->profile_manager()->GetProfileInfoCache(),
+                profile_->GetPath(), &users);
+  model_->SetUsers(users);
 }
 
-void AppListViewDelegate::SetModel(app_list::AppListModel* model) {
-  if (model) {
-    model_ = model;
-    apps_builder_.reset(new AppsModelBuilder(profile_,
-                                             model->apps(),
-                                             controller_.get()));
-    apps_builder_->Build();
+void AppListViewDelegate::SetProfileByPath(const base::FilePath& profile_path) {
+  DCHECK(model_);
 
-    search_controller_.reset(new app_list::SearchController(
-        profile_, model->search_box(), model->results(), controller_.get()));
+  // The profile must be loaded before this is called.
+  profile_ =
+      g_browser_process->profile_manager()->GetProfileByPath(profile_path);
+  DCHECK(profile_);
 
-    signin_delegate_.reset(new ChromeSigninDelegate(profile_));
+  RegisterForNotifications();
+
+  apps_builder_->SetProfile(profile_);
+
+  search_controller_.reset(new app_list::SearchController(
+      profile_, model_->search_box(), model_->results(), controller_.get()));
+
+  signin_delegate_->SetProfile(profile_);

[tapted, 2013/09/10 00:29:26]

Why couldn't this just be signin_delegate_.reset(new
ChromeSigninDelegate(profile_)); ? It feels odd to still pass the profile to the
constructor, while allowing it to be changed.

[calamity, 2013/09/10 22:50:46]

SigninView holds onto this delegate when its constructed so the alternative
would be to either give SigninView the AppListViewDelegate so it can retrieve
the signin delegate when its updated or to reconstruct the SigninView on
rebuild.

[tapted, 2013/09/11 05:50:26]

Hm - so it does. Mac will fetch a new delegate in response to a signin status
change (via OnProfileChanged below).. but currently only if it switches from
true <-> false.

It looks the delegate will work OK with it as it is now, so I guess there's no
reason to change this. But it should be consistent. I suggest moving this line
into OnProfileChanged, along with the search_controller and
AppSyncUIStateWatcher parts here, and in InitModel(). Then remove the
constructor argument from ChromeSigninDelegate, and either .reset() it once in
the constructor, or just make it a regular member rather than a scoped_ptr (I'm
leaning slightly towards ditching scoped_ptr).

[calamity, 2013/09/13 18:02:35]

Done.

 
 #if defined(USE_ASH)
-    app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,
-                                                               model));
+  app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,
+                                                             model_));
 #endif
-    OnProfileChanged();
-  } else {
-    model_ = NULL;
-    apps_builder_.reset();
-    search_controller_.reset();
-    signin_delegate_.reset();
+  OnProfileChanged();
+  if (!model_->search_box()->text().empty())
+    StartSearch();

[tapted, 2013/09/10 00:29:26]

This feels funny here.. it feels like something the view code should be
responsible for, since there are some assumptions being made about whether the
view is in a state where it is ready to show a search to begin with.

In particular, if someone switches to a profile that is not signed in, it
probably shouldn't start searching.

Maybe moving it is too messy. In which case it probably needs a comment here.
Took me some thinking to figure out the rationale.

[xiyuan, 2013/09/10 01:54:33]

It does look strange. I would prefer to clear query when switching profile.

[calamity, 2013/09/10 22:50:46]

Clearing the search box means that if a user starts searching in the wrong
profile, they'll need to re-type their query to switch their search context. My
preference is leave the query intact.

The view only knows about the signin state through the signin delegate which we
have access to here (so we can do the signed in check here). The search only
refreshes the results in the search view so it shouldn't do anything funny to
the app launcher state.

WDYT?

[xiyuan, 2013/09/11 05:01:49]

Sounds reasonable.

[tapted, 2013/09/11 05:50:26]

I'm leaning slightly towards clearing results and forcing them to retype - but
I'll defer to xiyuan/UX. My reasoning: just refreshing results is a bit too
subtle. There's a chance it will provide the same results, and the interface
won't look like it switched at all. Showing users' "home" page of apps is a good
way to reassure them that the switch took place.

A new method on AppListModelObserver might be a nicer way to do it though -
there are already reactions to 'is signed in' keyed off that. Also, E.g. is
there more to do? What happens if a profile switch occurs when someone is on
their second page of apps - does pagination reset to the first page? But if it
all Just Works (and there are tests ;) ) I'm not really bothered with it being
here either.

[calamity, 2013/09/13 18:02:35]

Changed to clear search text on profile change for now. Page does indeed just go
back to page 0.

+}
+
+void AppListViewDelegate::InitModel(app_list::AppListModel* model) {
+  DCHECK(!model_);
+  DCHECK(model);
+  model_ = model;
+
+  // Initialize apps model.
+  apps_builder_.reset(new AppsModelBuilder(profile_,
+                                           model->apps(),
+                                           controller_.get()));
+  apps_builder_->Build();
+
+  search_controller_.reset(new app_list::SearchController(
+      profile_, model->search_box(), model->results(), controller_.get()));
+
+  signin_delegate_.reset(new ChromeSigninDelegate(profile_));
+
 #if defined(USE_ASH)
-    app_sync_ui_state_watcher_.reset();
+  app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,

[koz (OOO until 15th September), 2013/09/09 22:58:07]

What does this do?

[xiyuan, 2013/09/10 01:54:33]

This is for ChromeOS to show pulsing animation when user signs-in on a device
for the first time. If a profile is new,  and when sync is in progress or there
are pending extension installs, it sets AppListModel status to turn on/off the
pulsing animation. It does nothing if the profile is already created.

[koz (OOO until 15th September), 2013/09/10 15:21:36]

Cool, thanks for the explanation. Sounds like something it'd be good to expose
in the windows UI, too.

+                                                             model));
 #endif
-  }
+  // Initialize the profile information in the app list menu.
+  OnProfileChanged();
 }
 
 app_list::SigninDelegate* AppListViewDelegate::GetSigninDelegate() {
@@ -220,6 +259,11 @@ void AppListViewDelegate::OpenFeedback() {
                            chrome::kAppLauncherCategoryTag);
 }
 
+void AppListViewDelegate::ShowForProfileByPath(
+    const base::FilePath& profile_path) {
+  controller_->ShowForProfileByPath(profile_path);
+}
+
 void AppListViewDelegate::Observe(
     int type,
     const content::NotificationSource& source,
@@ -227,11 +271,12 @@ void AppListViewDelegate::Observe(
   OnProfileChanged();
 }
 
+void AppListViewDelegate::OnProfileAdded(const base::FilePath& profile_path) {
+  OnProfileChanged();
+}
+
 void AppListViewDelegate::OnProfileNameChanged(
     const base::FilePath& profile_path,
     const base::string16& old_profile_name) {
-  if (profile_->GetPath() != profile_path)
-    return;
-
   OnProfileChanged();
 }