Add profiles to wrench menu

chrome/browser/ui/toolbar/wrench_menu_model.cc

Index: chrome/browser/ui/toolbar/wrench_menu_model.cc
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc
index 6d97478615b78cb8e533a0621c5fe6e46cda60a5..741d624fa912a0af2ac9785fd13e5faebe32832f 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.cc
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc
@@ -9,6 +9,7 @@
 
 #include "base/command_line.h"
 #include "base/i18n/number_formatting.h"
+#include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ -17,6 +18,7 @@
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
@@ -24,6 +26,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/toolbar/encoding_menu_controller.h"
 #include "chrome/browser/upgrade_detector.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/profiling.h"
@@ -210,6 +213,111 @@ void BookmarkSubMenuModel::Build(Browser* browser) {
 
 
 ////////////////////////////////////////////////////////////////////////////////
+// ProfilesSubMenuModel
+
+ProfilesSubMenuModel::ProfilesSubMenuModel(
+    ui::SimpleMenuModel::Delegate* delegate, Browser* browser)
+    : SimpleMenuModel(this),
+      browser_(browser),
+      delegate_(delegate) {
+  Build();
+}
+
+void ProfilesSubMenuModel::Build() {

[sky, 2011/06/10 16:24:14]

Rename this Init and have the caller invoke it
(http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors)

[sail, 2011/06/11 01:27:37]

I agree but all the other classes in this file do the same thing so doing it
different here would be confusing.

+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  size_t count = profile_manager->GetNumberOfProfiles();
+  for (size_t i = 0; i < count; ++i) {
+    AddItem(COMMAND_SWITCH_TO_PROFILE + i,
+            profile_manager->GetNameOfProfileAtIndex(i));
+  }
+
+  AddSeparator();
+  AddItemWithStringId(IDC_CREATE_NEW_PROFILE,

[sky, 2011/06/10 16:24:14]

Are you sure you don't want this first? Having the new menu first then the list
of profiles better matches the bookmarks menu. The bookmarks menu has operations
related to the bookmarks first, then the bookmarks.

[sail, 2011/06/11 01:27:37]

This menu will mainly be used for switching between profiles so it makes sense
to put the "create new profile" command last.

For example, you look through all the profiles, if by the time you reach the end
and you haven't found the one you want yet then you choose "create new profile".

+                      IDS_PROFILES_CREATE_NEW_PROFILE_OPTION);
+}
+
+void ProfilesSubMenuModel::ExecuteCommand(int command_id) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  size_t index = command_id;
+  if (index < profile_manager->GetNumberOfProfiles()) {
+    FilePath userDataFolder;
+    PathService::Get(chrome::DIR_USER_DATA, &userDataFolder);
+    FilePath profile_path =
+        profile_manager->GetFilePathOfProfileAtIndex(index, userDataFolder);
+
+    if (profile_manager->GetProfileByPath(profile_path)) {
+      OpenWindowWithProfile(profile_path);
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          NewRunnableMethod(this, &ProfilesSubMenuModel::GetProfileOnIOThread,
+                            profile_path));

[Miranda Callahan, 2011/06/11 15:09:08]

You don't want to do this on the IO Thread -- see below.  What you want is
something that launches an existing profile asynchronously, such that the stuff
that touches the File System in profile creation takes place on the file thread.
 This will be very similar to ProfileManager::CreateMultiProfileAsync -- except
that instead of observing with a NewProfileLauncher that opens the sync signin
dialog after launch, observe with some other kind of launcher that calls back to
the OpenWindowWithProfile task.

See my comment below on GetProfileOnIOThread.

[sail, 2011/06/13 19:00:01]

Done.

+    }
+  } else {
+    delegate_->ExecuteCommand(command_id);
+  }
+}
+
+void ProfilesSubMenuModel::GetProfileOnIOThread(FilePath profile_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  ProfileManager* profile_manager = g_browser_process->profile_manager();

[sky, 2011/06/10 16:24:14]

Is ProfileManager, Profile and all the classes that Profile is going to create
really thread safe?

[sail, 2011/06/11 01:27:37]

The GetProfile() function is meant to be called on a IO thread. Miranda would
know more.

[Miranda Callahan, 2011/06/11 15:09:08]

I am trying to remember a discussion about this, but I don't remember this --
could you remind me if we ever decided this was going to be the case?  Certainly
at the moment GetProfile() should not be called on the IO Thread -- Scott is
correct that this is probably not thread safe.

See comment above for a suggestion on how to launch an existing profile that's
not currently loaded using asynchronous File Thread methods (and feel free to
steal from the code I wrote to do this from my menu CL from last week if you
want -- the LaunchExistingProfile code).

[sail, 2011/06/13 19:00:01]

Hey Miranda, we didn't discuss this. I just confused about what
ProfileManager::GetProfile() does. I switched the code to use
CreateProfileAsync() and it seems to work well. I'm testing on Windows right
now.

+  Profile* profile = profile_manager->GetProfile(profile_path);
+  if (profile) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        NewRunnableMethod(this, &ProfilesSubMenuModel::OpenWindowWithProfile,
+                          profile_path));
+  }
+}
+
+void ProfilesSubMenuModel::OpenWindowWithProfile(FilePath profile_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  Profile* profile = profile_manager->GetProfileByPath(profile_path);
+  if (!profile)
+    return;
+
+  Browser* browser = BrowserList::FindAnyBrowser(profile, false);

[sky, 2011/06/10 16:24:14]

Are you sure you don't want FindTabbedBrowser(profile, true)?

[sail, 2011/06/11 01:27:37]

Ahh, right. Switched to FindTabbedBrowser().
I don't think we want to match incognito windows. Incognito windows will have a
different avatar and it won't be clear that you actually switched to a different
profile.

[Miranda Callahan, 2011/06/11 15:09:08]

Could you rename this FindAnyBrowserWithProfile, to make absolutely clear what's
going on?

[Miranda Callahan, 2011/06/11 15:13:36]

nm -- I see that these are existing methods in BrowserList, and that you've
changed to FindTabbedProfile. Not a fan, but any change belongs in a different
CL.

+  if (browser) {
+    browser->window()->Activate();
+  } else {
+    Browser::NewWindowWithProfile(profile);
+  }
+}
+
+bool ProfilesSubMenuModel::IsCommandIdChecked(int command_id) const {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  size_t index = command_id;
+  if (index < profile_manager->GetNumberOfProfiles()) {
+    FilePath userDataFolder;
+    PathService::Get(chrome::DIR_USER_DATA, &userDataFolder);
+    FilePath profile_path =
+        profile_manager->GetFilePathOfProfileAtIndex(index, userDataFolder);
+
+    return browser_->GetProfile()->GetPath() == profile_path;
+  }
+  return delegate_->IsCommandIdChecked(command_id);
+}
+
+bool ProfilesSubMenuModel::IsCommandIdEnabled(int command_id) const {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  size_t index = command_id;
+  if (index < profile_manager->GetNumberOfProfiles())
+    return true;
+  return delegate_->IsCommandIdEnabled(command_id);
+}
+
+bool ProfilesSubMenuModel::GetAcceleratorForCommandId(
+    int command_id,
+    ui::Accelerator* accelerator) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  size_t index = command_id;
+  if (index < profile_manager->GetNumberOfProfiles())
+    return false;
+  return delegate_->GetAcceleratorForCommandId(command_id, accelerator);
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // WrenchMenuModel
 
 WrenchMenuModel::WrenchMenuModel(ui::AcceleratorProvider* provider,
@@ -227,6 +335,8 @@ WrenchMenuModel::WrenchMenuModel(ui::AcceleratorProvider* provider,
                  Source<HostZoomMap>(browser_->profile()->GetHostZoomMap()));
   registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
                  NotificationService::AllSources());
+  registrar_.Add(this, NotificationType::PROFILE_ADDED,
+                 NotificationService::AllSources());
 }
 
 WrenchMenuModel::~WrenchMenuModel() {
@@ -304,7 +414,14 @@ bool WrenchMenuModel::GetIconForCommandId(int command_id,
 }
 
 void WrenchMenuModel::ExecuteCommand(int command_id) {
-  browser_->ExecuteCommand(command_id);
+  switch (command_id) {
+    case IDC_CREATE_NEW_PROFILE:
+      ProfileManager::CreateMultiProfileAsync();
+      break;
+    default:
+      browser_->ExecuteCommand(command_id);
+      break;
+  }
 }
 
 bool WrenchMenuModel::IsCommandIdChecked(int command_id) const {
@@ -318,11 +435,15 @@ bool WrenchMenuModel::IsCommandIdChecked(int command_id) const {
 }
 
 bool WrenchMenuModel::IsCommandIdEnabled(int command_id) const {
-  if (command_id == IDC_SHOW_BOOKMARK_BAR) {
-    return !browser_->profile()->GetPrefs()->IsManagedPreference(
-        prefs::kEnableBookmarkBar);
+  switch (command_id) {
+    case IDC_SHOW_BOOKMARK_BAR:
+      return !browser_->profile()->GetPrefs()->IsManagedPreference(
+          prefs::kEnableBookmarkBar);
+    case IDC_CREATE_NEW_PROFILE:
+      return true;
+    default:
+      return browser_->command_updater()->IsCommandEnabled(command_id);
   }
-  return browser_->command_updater()->IsCommandEnabled(command_id);
 }
 
 bool WrenchMenuModel::IsCommandIdVisible(int command_id) const {
@@ -382,6 +503,16 @@ void WrenchMenuModel::Observe(NotificationType type,
     case NotificationType::NAV_ENTRY_COMMITTED:
       UpdateZoomControls();
       break;
+    case NotificationType::PROFILE_ADDED: {

[sky, 2011/06/10 16:24:14]

This will cause problems if the menu is showing. Seems like it would be better
to rebuild the model before showing and have the ProfilesMenuModel cache the
menu items.

[sail, 2011/06/11 01:27:37]

Agreed, removed.

+      int index = GetIndexOfCommandId(IDC_CREATE_NEW_PROFILE);
+      if (index == -1)
+        index = GetIndexOfCommandId(IDC_PROFILE_MENU);
+      if (index != -1) {
+        RemoveItemAt(index);
+        AddProfilesMenuItemAt(index);
+      }
+      break;
+    }
     default:
       NOTREACHED();
   }
@@ -458,6 +589,11 @@ void WrenchMenuModel::Build() {
   AddItemWithStringId(IDC_SHOW_DOWNLOADS, IDS_SHOW_DOWNLOADS);
   AddSeparator();
 
+#if !defined(OS_CHROMEOS)
+  AddProfilesMenuItemAt(GetItemCount());
+  AddSeparator();
+#endif
+
 #if defined(OS_CHROMEOS)
   AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);
 #elif defined(OS_MACOSX)
@@ -550,3 +686,15 @@ string16 WrenchMenuModel::GetSyncMenuLabel() const {
   return sync_ui_util::GetSyncMenuLabel(
       browser_->profile()->GetOriginalProfile()->GetProfileSyncService());
 }
+
+void WrenchMenuModel::AddProfilesMenuItemAt(int index) {
+  if (g_browser_process->profile_manager()->GetNumberOfProfiles() > 1) {

[sky, 2011/06/10 16:24:14]

Seems weird to me to change the type based on contents. Did you run this by Glen
and company?

[sail, 2011/06/11 01:27:37]

Yea, we went through this UI in the profile meeting last week. We can definitely
tweak this once it's checked in.

+    profiles_sub_menu_model_.reset(new ProfilesSubMenuModel(this, browser_));
+    InsertSubMenuWithStringIdAt(index, IDC_PROFILE_MENU, IDS_PROFILES_MENU,
+                                profiles_sub_menu_model_.get());
+  } else {
+    profiles_sub_menu_model_.reset();
+    InsertItemWithStringIdAt(index, IDC_CREATE_NEW_PROFILE,
+                             IDS_PROFILES_CREATE_NEW_PROFILE_OPTION);
+  }
+}