Modifying the BackgroundModeManager to handle multiple profiles.

chrome/browser/background_mode_manager.cc

Index: chrome/browser/background_mode_manager.cc
===================================================================
--- chrome/browser/background_mode_manager.cc	(revision 85413)
+++ chrome/browser/background_mode_manager.cc	(working copy)
@@ -30,26 +30,36 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
+BackgroundModeManager::BackgroundModeData::BackgroundModeData()
+    : applications_(NULL),
+      status_icon_(NULL),
+      context_menu_(NULL),
+      context_menu_application_offset_(0) {
+}
+
+BackgroundModeManager::BackgroundModeData::~BackgroundModeData() {
+}
+
 void BackgroundModeManager::OnApplicationDataChanged(
-    const Extension* extension) {
-  UpdateContextMenuEntryIcon(extension);
+    const Extension* extension, Profile* profile) {
+  UpdateContextMenuEntryIcon(extension, profile);
 }
 
-void BackgroundModeManager::OnApplicationListChanged() {
-  UpdateStatusTrayIconContextMenu();
+void BackgroundModeManager::OnApplicationListChanged(Profile* profile) {
+  UpdateStatusTrayIconContextMenu(profile);
 }
 
-BackgroundModeManager::BackgroundModeManager(Profile* profile,
-                                             CommandLine* command_line)
-    : profile_(profile),
-      applications_(profile),
-      background_app_count_(0),
-      context_menu_(NULL),
-      context_menu_application_offset_(0),
+// static
+BackgroundModeManager* BackgroundModeManager::GetInstance() {
+  return Singleton<BackgroundModeManager>::get();
+}
+
+BackgroundModeManager::BackgroundModeManager()
+    : background_app_count_(0),
       in_background_mode_(false),
-      keep_alive_for_startup_(false),
-      status_tray_(NULL),
-      status_icon_(NULL) {
+      keep_alive_for_startup_(false) {
+
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
   // If background mode is currently disabled, just exit - don't listen for any
   // notifications.
   if (IsBackgroundModePermanentlyDisabled(command_line))
@@ -73,9 +83,16 @@
   // If the -keep-alive-for-test flag is passed, then always keep chrome running
   // in the background until the user explicitly terminates it, by acting as if
   // we loaded a background app.
-  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kKeepAliveForTest))
+  if (command_line->HasSwitch(switches::kKeepAliveForTest))
     OnBackgroundAppLoaded();
+}
 
+void BackgroundModeManager::RegisterProfile(Profile* profile) {
+
+  // If the profile data already exists, this will overwrite it.

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

Is it valuable to add a DCHECK(background_mode_data_.find(profile) ==
background_mode_data_.end()) just to ensure we don't ever call this twice for
the same profile?

[rpetterson, 2011/05/20 05:53:17]

Done.

+  background_mode_data_[profile].applications_ =
+      new BackgroundApplicationListModel(profile);
+
   // Listen for when extensions are loaded/unloaded so we can track the
   // number of background apps and modify our keep-alive and launch-on-startup
   // state appropriately.
@@ -90,16 +107,23 @@
   registrar_.Add(this, NotificationType::EXTENSIONS_READY,
                  Source<Profile>(profile));
 

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

nit: remove extra blank line

[rpetterson, 2011/05/20 05:53:17]

Done.

+
+  background_mode_data_[profile].applications_->AddObserver(this);
+
   // Listen for the application shutting down so we can decrement our KeepAlive
   // count.
   registrar_.Add(this, NotificationType::APP_TERMINATING,
                  NotificationService::AllSources());
-
-  applications_.AddObserver(this);
 }
 
 BackgroundModeManager::~BackgroundModeManager() {
-  applications_.RemoveObserver(this);
+  for (std::map<Profile*, BackgroundModeData>::iterator it =
+      background_mode_data_.begin();

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

nit: should indent these lines correctly (line up with std:: not with open
paren). Also line continuations should be indented an extra 4 chars.

[rpetterson, 2011/05/20 05:53:17]

I thought the 4 characters was from the start of the line which would be from
the "for" which is what it is now. But I think it's easier to read lined up with
the std:: so I've done that.

[Andrew T Wilson (Slow), 2011/05/20 16:52:45]

Typically wrapped for statement elements are indented to the start of the open
paren. Wrapped statements are indented another 4 characters on top of that (so,
4 spaces past the open paren).

However, I'm like the world's worst reviewer when it comes to style issues - I
often get them wrong. So you might want to get a second opinion :)

[Miranda Callahan, 2011/05/20 16:56:45]

I think that in this case you can choose either -- but people generally tend to
go with lining up at the start of the open paren for for statements, which we
all agree looks tidier, so -- yay. :-)

[rpetterson, 2011/05/23 03:23:19]

:)

+      it != background_mode_data_.end();
+      ++it) {
+    it->second.applications_->RemoveObserver(this);
+    delete it->second.applications_;

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

See previous note about having applications_ be a scoped_ptr<>

[rpetterson, 2011/05/20 05:53:17]

Done.

+  }
 
   // We're going away, so exit background mode (does nothing if we aren't in
   // background mode currently). This is primarily needed for unit tests,
@@ -140,8 +164,10 @@
         if (BackgroundApplicationListModel::IsBackgroundApp(*extension)) {
           // Extensions loaded after the ExtensionsService is ready should be
           // treated as new installs.
-          if (profile_->GetExtensionService()->is_ready())
-            OnBackgroundAppInstalled(extension);
+
+          Profile* profile = Source<Profile>(source).ptr();
+          if (profile->GetExtensionService()->is_ready())
+            OnBackgroundAppInstalled(extension, profile);
           OnBackgroundAppLoaded();
         }
       }
@@ -192,6 +218,8 @@
 void BackgroundModeManager::OnBackgroundAppLoaded() {
   // When a background app loads, increment our count and also enable
   // KeepAlive mode if the preference is set.
+  // The count here is across all profiles since we must have brackground

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

nit: brackground->background

[rpetterson, 2011/05/20 05:53:17]

Done.

+  // mode if there is even one.
   background_app_count_++;
   if (background_app_count_ == 1)
     StartBackgroundMode();
@@ -210,7 +238,15 @@
   BrowserList::StartKeepAlive();
 
   // Display a status icon to exit Chrome.
-  CreateStatusTrayIcon();
+  // TODO(rlp): Determine if we actually want to do this here.
+  // I suspect that we might want to create one for all current profiles
+  // loaded.
+  for (std::map<Profile*, BackgroundModeData>::iterator it =
+      background_mode_data_.begin();

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

nit: indent properly (see above comment)

[rpetterson, 2011/05/20 05:53:17]

 Done. See above comment.

+      it != background_mode_data_.end();
+      ++it) {
+    CreateStatusTrayIcon(it->first);

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

I think we should only create status tray icons for profiles that have
background apps, although this requires updating the set of status tray icons as
apps are loaded/unloaded. Or just have a single status tray icon with sub-menus
for the background apps for individual profiles, as Glen suggested.

[rpetterson, 2011/05/20 05:53:17]

There's an e-mail thread with Glen about using multiple icons that we're waiting
to hear back from him on. I can do that as a separate CL if he still wants one.
And in fact I'd rather do it as a separate CL since this one has already gotten
much larger.

With regards to the actual question, the status tray is currently updated
whenever the application list is changed (so I would assume on load/unload?)

I've updated so that it only creates the status icon when a background app is
loaded for that profile.

+  }
 }
 
 void BackgroundModeManager::OnBackgroundAppUnloaded() {
@@ -229,7 +265,13 @@
 
   // End KeepAlive mode and blow away our status tray icon.
   BrowserList::EndKeepAlive();
-  RemoveStatusTrayIcon();
+  // There is a status tray icon for each profile. Blow them all away.
+  for (std::map<Profile*, BackgroundModeData>::iterator it =
+      background_mode_data_.begin();

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

nit: indent properly

[rpetterson, 2011/05/20 05:53:17]

Done. See above comment.

+      it != background_mode_data_.end();
+      ++it) {
+    RemoveStatusTrayIcon(it->first);
+  }
 }
 
 void BackgroundModeManager::EnableBackgroundMode() {
@@ -251,7 +293,7 @@
 }
 
 void BackgroundModeManager::OnBackgroundAppInstalled(
-    const Extension* extension) {
+    const Extension* extension, Profile* profile) {
   // Background mode is disabled - don't do anything.
   if (!IsBackgroundModePrefEnabled())
     return;
@@ -263,7 +305,7 @@
 
   // Notify the user that a background app has been installed.
   if (extension)  // NULL when called by unit tests.
-    DisplayAppInstalledNotification(extension);
+    DisplayAppInstalledNotification(extension, profile);
 }
 
 void BackgroundModeManager::OnBackgroundAppUninstalled() {
@@ -273,46 +315,61 @@
     EnableLaunchOnStartup(false);
 }
 
-void BackgroundModeManager::CreateStatusTrayIcon() {
+void BackgroundModeManager::CreateStatusTrayIcon(Profile* profile) {
   // Only need status icons on windows/linux. ChromeOS doesn't allow exiting
   // Chrome and Mac can use the dock icon instead.
+
+  // All profiles should point to the same status tray so we can just use the
+  // first profile to get the status tray.

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

Not sure how you want this to work now - should StatusTray be a
lazily-initialized singleton then, or owned by the BrowserProcess? It seems
weird to go through a Profile to get a singleton. Failing that, we could just
move it out of the Profile and have BackgroundModeManager own it directly,
although that means it would be tricky if other parts of the system ever want to
display status icons.

I think the best solution might be to have the BrowserProcess own it, just like
it owns other things like the local state PrefService.

[rpetterson, 2011/05/20 05:53:17]

So previously, the profile owned the status tray reference. It's now owned by
the BrowserProcess.

 #if !defined(OS_MACOSX) && !defined(OS_CHROMEOS)
   if (!status_tray_)
-    status_tray_ = profile_->GetStatusTray();
+    status_tray_ = profile->GetStatusTray();
 #endif
 
   // If the platform doesn't support status icons, or we've already created
   // our status icon, just return.
-  if (!status_tray_ || status_icon_)
+  BackgroundModeData* bmd = &background_mode_data_[profile];
+  DCHECK(bmd->applications_);
+  if (!status_tray_ || bmd->status_icon_)
     return;
-  status_icon_ = status_tray_->CreateStatusIcon();
-  if (!status_icon_)
+
+  bmd->status_icon_ = status_tray_->CreateStatusIcon();
+  if (!bmd->status_icon_)
     return;
 
-  // Set the image and add ourselves as a click observer on it
+  // Set the image and add ourselves as a click observer on it.
+  // TODO(rlp): Status tray icon should have submenus for each profile.
   SkBitmap* bitmap = ResourceBundle::GetSharedInstance().GetBitmapNamed(
       IDR_STATUS_TRAY_ICON);
-  status_icon_->SetImage(*bitmap);
-  status_icon_->SetToolTip(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
-  UpdateStatusTrayIconContextMenu();
+  bmd->status_icon_->SetImage(*bitmap);
+  bmd->status_icon_->SetToolTip(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
+  UpdateStatusTrayIconContextMenu(profile);
 }
 
 void BackgroundModeManager::UpdateContextMenuEntryIcon(
-    const Extension* extension) {
-  if (!context_menu_)
+    const Extension* extension, Profile* profile) {
+  BackgroundModeData* bmd = &background_mode_data_[profile];
+  DCHECK(bmd->applications_);
+
+  if (!bmd->context_menu_)
     return;
-  context_menu_->SetIcon(
-      context_menu_application_offset_ + applications_.GetPosition(extension),
-      *(applications_.GetIcon(extension)));
-  status_icon_->SetContextMenu(context_menu_);  // for Update effect
+  bmd->context_menu_->SetIcon(
+      bmd->context_menu_application_offset_ +
+      bmd->applications_->GetPosition(extension),
+      *(bmd->applications_->GetIcon(extension)));
+
+  bmd->status_icon_->SetContextMenu(bmd->context_menu_);  // for Update effect
 }
 
-void BackgroundModeManager::UpdateStatusTrayIconContextMenu() {
-  if (!status_icon_)
+void BackgroundModeManager::UpdateStatusTrayIconContextMenu(Profile* profile) {
+  BackgroundModeData* bmd = &background_mode_data_[profile];
+  DCHECK(bmd->applications_);
+  if (!bmd->status_icon_)
     return;
 
+  // TODO(rlp): Add current profile color.

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

Not sure how you'd add a profile color, btw - if we keep multiple status icons,
then I guess you could change CreateStatusTrayIcon() to dynamically tint the
icon, or else if you transition to having only a single status tray icon you
could do something in this routine to put an appropriately colored icon/square
next to each profile's background apps...

[rpetterson, 2011/05/20 05:53:17]

Agreed. That's why it's a TODO :)

   // Create a context menu item for Chrome.
-  ui::SimpleMenuModel* menu = new ui::SimpleMenuModel(this);
+  ProfileStatusMenuModel* menu = new ProfileStatusMenuModel(this, profile);
   // Add About item
   menu->AddItem(IDC_ABOUT, l10n_util::GetStringFUTF16(IDS_ABOUT,
       l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)));
@@ -320,25 +377,25 @@
   menu->AddItemWithStringId(IDC_TASK_MANAGER, IDS_TASK_MANAGER);
   menu->AddSeparator();
   int position = 0;
-  context_menu_application_offset_ = menu->GetItemCount();
-  for (ExtensionList::const_iterator cursor = applications_.begin();
-       cursor != applications_.end();
+  bmd->context_menu_application_offset_ = menu->GetItemCount();
+  for (ExtensionList::const_iterator cursor = bmd->applications_->begin();
+       cursor != bmd->applications_->end();
        ++cursor, ++position) {
-    const SkBitmap* icon = applications_.GetIcon(*cursor);
-    DCHECK(position == applications_.GetPosition(*cursor));
+    const SkBitmap* icon = bmd->applications_->GetIcon(*cursor);
+    DCHECK(position == bmd->applications_->GetPosition(*cursor));
     const std::string& name = (*cursor)->name();
     menu->AddItem(position, UTF8ToUTF16(name));
     if (icon)
       menu->SetIcon(menu->GetItemCount() - 1, *icon);
   }
-  if (applications_.size() > 0)
+  if (bmd->applications_->size() > 0)
     menu->AddSeparator();
   menu->AddCheckItemWithStringId(
       IDC_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND,
       IDS_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND);
   menu->AddItemWithStringId(IDC_EXIT, IDS_EXIT);
-  context_menu_ = menu;
-  status_icon_->SetContextMenu(menu);
+  bmd->context_menu_ = menu;
+  bmd->status_icon_->SetContextMenu(menu);
 }
 
 bool BackgroundModeManager::IsCommandIdChecked(int command_id) const {
@@ -358,38 +415,44 @@
   return false;
 }
 
-void BackgroundModeManager::RemoveStatusTrayIcon() {
-  if (status_icon_)
-    status_tray_->RemoveStatusIcon(status_icon_);
-  status_icon_ = NULL;
-  context_menu_ = NULL;  // Do not delete, points within |status_icon_|.
+void BackgroundModeManager::RemoveStatusTrayIcon(Profile* profile) {
+  BackgroundModeData* bmd = &background_mode_data_[profile];
+  DCHECK(bmd->applications_);

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

You have these two lines in many places - maybe this is a good candidate for
refactoring into a helper routine:

BackgroundModeData* bmd = GetBackgroundModeDataForProfile(profile);

This is especially true if you make applications_ no longer allocated on the
heap, so you'd have to use DCHECK(background_mode_data.find(profile) !=
background_mode_data.end()) instead...

[rpetterson, 2011/05/20 05:53:17]

Done.

+  if (bmd->status_icon_)
+    status_tray_->RemoveStatusIcon(bmd->status_icon_);
+  bmd->status_icon_ = NULL;
+  bmd->context_menu_ = NULL;  // Do not delete, points within |status_icon_|.
 }
 
-void BackgroundModeManager::ExecuteApplication(int item) {
-  DCHECK(item >= 0 && item < static_cast<int>(applications_.size()));
-  Browser* browser = BrowserList::GetLastActive();
-  if (!browser) {
-    Browser::OpenEmptyWindow(profile_);
-    browser = BrowserList::GetLastActive();
-  }
-  const Extension* extension = applications_.GetExtension(item);
-  browser->OpenApplicationTab(profile_, extension, NEW_FOREGROUND_TAB);
+// TODO(rlp): FIX. We now have mutliple applications.

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

Is there anything you need to fix here? It seems like you're doing the right
thing WRT profiles...

[rpetterson, 2011/05/20 05:53:17]

Done.

+void BackgroundModeManager::ExecuteApplication(int item, Profile* profile) {
+  BackgroundModeData* bmd = &background_mode_data_[profile];
+  DCHECK(bmd->applications_);
+  DCHECK(item >= 0 && item < static_cast<int>(bmd->applications_->size()));
+
+  Browser* browser = GetBrowserWindow(profile);
+  const Extension* extension = bmd->applications_->GetExtension(item);
+  browser->OpenApplicationTab(profile, extension, NEW_FOREGROUND_TAB);
 }
 
 void BackgroundModeManager::ExecuteCommand(int item) {
+  // We only use the version with profile information.

[Andrew T Wilson (Slow), 2011/05/20 00:22:09]

Add NOTREACHED()? Alternatively, can you just not override this method?

[rpetterson, 2011/05/20 05:53:17]

I've added NOTREACHED(). In order to subclass of off ProfileStatusMenuModel
which subclasses off of SimpleMenuModel, it needs to be defined since it's
abstract.

+}
+
+void BackgroundModeManager::ExecuteCommand(int item, Profile* profile) {
   switch (item) {
     case IDC_ABOUT:
-      GetBrowserWindow()->OpenAboutChromeDialog();
+      GetBrowserWindow(profile)->OpenAboutChromeDialog();
       break;
     case IDC_EXIT:
       UserMetrics::RecordAction(UserMetricsAction("Exit"));
       BrowserList::CloseAllBrowsersAndExit();
       break;
     case IDC_OPTIONS:
-      GetBrowserWindow()->OpenOptionsDialog();
+      GetBrowserWindow(profile)->OpenOptionsDialog();
       break;
     case IDC_TASK_MANAGER:
-      GetBrowserWindow()->OpenTaskManager(true);
+      GetBrowserWindow(profile)->OpenTaskManager(true);
       break;
     case IDC_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND: {
       // Background mode must already be enabled (as otherwise this menu would
@@ -417,15 +480,15 @@
       break;
     }
     default:
-      ExecuteApplication(item);
+      ExecuteApplication(item, profile);
       break;
   }
 }
 
-Browser* BackgroundModeManager::GetBrowserWindow() {
+Browser* BackgroundModeManager::GetBrowserWindow(Profile* profile) {
   Browser* browser = BrowserList::GetLastActive();
   if (!browser) {
-    Browser::OpenEmptyWindow(profile_);
+    Browser::OpenEmptyWindow(profile);
     browser = BrowserList::GetLastActive();
   }
   return browser;