Modifying the BackgroundModeManager to handle multiple profiles.

Modifying the BackgroundModeManager to handle multiple profiles.
The background mode manager is now owned by the broser
The status tray is also owned by the browser process since there is only one per browser. Previously it was owned by the profile, but now that there are multiple profiles, it makes sense to have the browser process own it.

Note: This CL does not unify the status tray into one icon.

TEST=unittests (added BackgroundModeManagerTest::MultiProfile)
BUG=80069, 82215
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86724

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

Notes from my early review. I suspect ultimately we'll want to refactor
BackgroundModeData into a more full-fledged per-profile object, but I think some
of that depends on what direction we go with the status tray UI.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.cc:69: Profile* profile_;
See my previous comment - do we really need to track profile_ here, given that
it's only used to pass back to the BackgroundApplicationListModel, which already
has a reference to this profile?

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.h:114: void
OnApplicationDataChanged(const Extension* extension, Profile* profile);
I'm curious why we need to pass in the Profile here, since
BackgroundApplicationListManager maintains its own reference to the parent
profile (I'm presuming it's invalid to invoke OnApplicationDataChanged() passing
a profile != profile_, correct?)

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:92: // If the profile data already
exists, this will overwrite it.
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?

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:109: 
nit: remove extra blank line

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
nit: should indent these lines correctly (line up with std:: not with open
paren). Also line continuations should be indented an extra 4 chars.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:125: delete it->second.applications_;
See previous note about having applications_ be a scoped_ptr<>

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:221: // The count here is across all
profiles since we must have brackground
nit: brackground->background

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:245: background_mode_data_.begin();
nit: indent properly (see above comment)

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:248: CreateStatusTrayIcon(it->first);
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.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:270: background_mode_data_.begin();
nit: indent properly

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:323: // first profile to get the
status tray.
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.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:370: // TODO(rlp): Add current profile
color.
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...

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:420: DCHECK(bmd->applications_);
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...

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:427: // TODO(rlp): FIX. We now have
mutliple applications.
Is there anything you need to fix here? It seems like you're doing the right
thing WRT profiles...

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:439: // We only use the version with
profile information.
Add NOTREACHED()? Alternatively, can you just not override this method?

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:77: BackgroundApplicationListModel*
applications_;
nit: struct member variables should not have trailing underscores (so:
status_icon, instead of status_icon_).

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:77: BackgroundApplicationListModel*
applications_;
Should this be a scoped_ptr<> so you don't have to manually delete it? Or, even
better, just have it not be allocated on the heap at all (would probably make
BMD into an object with a constructor so you could initialize applications_ with
the proper Profile).

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:103: // Overrides from
ProfileSatusMenuModel::Delegate implementation.
nit: ProfileStatusMenuModel (not ProfileSatus)

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:201: // Reference to our status tray
(owned by our parent profile). If null, the
Since we don't have a parent profile any more, this comment seems incorrect.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_status_menu_model.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_status_menu_model.cc:11:
ProfileStatusMenuModel::ProfileStatusMenuModel(Delegate* delegate, Profile*
profile)
line > 80 chars

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_status_menu_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_status_menu_model.h:6: #define
CHROME_BROWSER_PROFILES_PROFILE_STATUS_MENU_MODEL_H_
I'm not sure this is heavy-weight enough to deserve its own class and I don't
know that I'd put it in the profiles/ directory.

I would do one of the following instead:

1) Change this to be a general purpose template class that allows you to specify
any arbitrary type to be passed along with ExecuteCommand() (and possibly all
the other SimpleMenuModel delegate APIs) and put it in ui/base/models
2) Just make this class private to BackgroundModeManager.
3) Maybe rename BackgroundModeData to something like BackgroundModeForProfile,
make it a real class, implement SimpleMenuModel::Delegate on that class which
already has its own reference to the profile. You could even refactor all of the
status icon/context menu code into that class and out of BackgroundModeManager.

If we are going to continue having multiple status icons (one for each profile)
then i'd say #3 is the way to go. If we're going with Glen's suggestion to just
have a single status icon with multiple per-profile submenus, then probably #1
or #2 would be better because we'd need to keep the status icon functionality in
BackgroundModeManager.

[Andrew T Wilson (Slow), 2011-05-20 00:28:04]

One more note: app_controller_mac.mm contains logic to create a list of
background apps as a submenu under the Mac dock menu.

There's probably some work that needs to be done here to provide per-profile
lists.

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

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.cc:69: Profile* profile_;
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> See my previous comment - do we really need to track profile_ here, given that
> it's only used to pass back to the BackgroundApplicationListModel, which
already
> has a reference to this profile?

It doesn't pass back just to the BackgroundApplicationListModel. It also passes
back to the BackgroundModeManager (which also inherits
BackgroundApplicationListModel::Application) and needs a way to know which
profile's status icon to update.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.h:114: void
OnApplicationDataChanged(const Extension* extension, Profile* profile);
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> I'm curious why we need to pass in the Profile here, since
> BackgroundApplicationListManager maintains its own reference to the parent
> profile (I'm presuming it's invalid to invoke OnApplicationDataChanged()
passing
> a profile != profile_, correct?)

I also commented on the other comment. But BackgroundModeManager also inherts
from BackgroundApplicationListModel::Application and needs to know which profile
it's dealing with.

I'll add the check you mention to the BackgroundApplicationListModel definition
of this function though I don't think it ever gets called.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:92: // If the profile data already
exists, this will overwrite it.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> 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?

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:109: 
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: should indent these lines correctly (line up with std:: not with open
> paren). Also line continuations should be indented an extra 4 chars.

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.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:125: delete it->second.applications_;
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> See previous note about having applications_ be a scoped_ptr<>

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:221: // The count here is across all
profiles since we must have brackground
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: brackground->background

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:245: background_mode_data_.begin();
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: indent properly (see above comment)
 Done. See above comment.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:248: CreateStatusTrayIcon(it->first);
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> 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.

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.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:270: background_mode_data_.begin();
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: indent properly

Done. See above comment.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:323: // first profile to get the
status tray.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> 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.

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

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:370: // TODO(rlp): Add current profile
color.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> 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...

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

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:420: DCHECK(bmd->applications_);
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> 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...

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:427: // TODO(rlp): FIX. We now have
mutliple applications.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> Is there anything you need to fix here? It seems like you're doing the right
> thing WRT profiles...

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:439: // We only use the version with
profile information.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> Add NOTREACHED()? Alternatively, can you just not override this method?

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.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:77: BackgroundApplicationListModel*
applications_;
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: struct member variables should not have trailing underscores (so:
> status_icon, instead of status_icon_).

Not an issue any more.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:77: BackgroundApplicationListModel*
applications_;
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> Should this be a scoped_ptr<> so you don't have to manually delete it? Or,
even
> better, just have it not be allocated on the heap at all (would probably make
> BMD into an object with a constructor so you could initialize applications_
with
> the proper Profile).

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:103: // Overrides from
ProfileSatusMenuModel::Delegate implementation.
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> nit: ProfileStatusMenuModel (not ProfileSatus)

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.h:201: // Reference to our status tray
(owned by our parent profile). If null, the
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> Since we don't have a parent profile any more, this comment seems incorrect. 

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_status_menu_model.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_status_menu_model.cc:11:
ProfileStatusMenuModel::ProfileStatusMenuModel(Delegate* delegate, Profile*
profile)
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> line > 80 chars

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_status_menu_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_status_menu_model.h:6: #define
CHROME_BROWSER_PROFILES_PROFILE_STATUS_MENU_MODEL_H_
On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> I'm not sure this is heavy-weight enough to deserve its own class and I don't
> know that I'd put it in the profiles/ directory.
> 
> I would do one of the following instead:
> 
> 1) Change this to be a general purpose template class that allows you to
specify
> any arbitrary type to be passed along with ExecuteCommand() (and possibly all
> the other SimpleMenuModel delegate APIs) and put it in ui/base/models
> 2) Just make this class private to BackgroundModeManager.
> 3) Maybe rename BackgroundModeData to something like BackgroundModeForProfile,
> make it a real class, implement SimpleMenuModel::Delegate on that class which
> already has its own reference to the profile. You could even refactor all of
the
> status icon/context menu code into that class and out of
BackgroundModeManager.
> 
> If we are going to continue having multiple status icons (one for each
profile)
> then i'd say #3 is the way to go. If we're going with Glen's suggestion to
just
> have a single status icon with multiple per-profile submenus, then probably #1
> or #2 would be better because we'd need to keep the status icon functionality
in
> BackgroundModeManager.

So the real reason that it is its own class is because Miranda had need for a
similar one so the hope is that this one class will serve both our purposes.

1) It seemed pretty specific to be in base.
2) Miranda needed it in her class as well.
3) I'd pick this option. But it would eliminate others using a profile specific
menu. I'll let Miranda chime in on this one as to whether she wants the class or
not.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:155:
manager.OnBackgroundAppInstalled(NULL, &profile1);
What needs to be done to be able to call these private functions as done in the
tests above? For this test I get compile errors, but I don't for any of the
others.

[Miranda Callahan, 2011-05-20 15:16:53]

Mostly nits and a couple comments... thanks!

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:52: Profile* profile_);
nit: remove trailing _

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:237: // Only initiali status tray
icons for those profiles which actually
nit: initiali...

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:320: // first profile to get the
status tray.
looks like you changed this to use the g_browser_process -- comment change as
well?

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:485: Browser* browser =
BrowserList::GetLastActive();
This should be changed to BrowserList::GetLastActiveForProfile(profile), or it
may bring up a browser window associated with a different profile.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:488: browser =
BrowserList::GetLastActive();
...should probably also be GetLastActiveForProfile here too.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:494:
BackgroundModeManager::GetBackgroundModeInfo(
Join this line and the one below.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:185:
BackgroundModeManager::BackgroundModeInfo GetBackgroundModeInfo(
nit: say in comment what happens if it doesn't exist (is it created lazily? null
returned?)

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:211: // platform doesn't support status
icons.
nit: line breaking is a bit too aggressive

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:155:
manager.OnBackgroundAppInstalled(NULL, &profile1);
On 2011/05/20 05:53:17, rpetterson wrote:
> What needs to be done to be able to call these private functions as done in
the
> tests above? For this test I get compile errors, but I don't for any of the
> others.

Have you tried adding this line to background_mode_manager.h, around line 64?  

FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
                         MultiProfile);

[Miranda Callahan, 2011-05-20 15:20:17]

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_status_menu_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_status_menu_model.h:6: #define
CHROME_BROWSER_PROFILES_PROFILE_STATUS_MENU_MODEL_H_
On 2011/05/20 05:53:17, rpetterson wrote:
> On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > I'm not sure this is heavy-weight enough to deserve its own class and I
don't
> > know that I'd put it in the profiles/ directory.
> > 
> > I would do one of the following instead:
> > 
> > 1) Change this to be a general purpose template class that allows you to
> specify
> > any arbitrary type to be passed along with ExecuteCommand() (and possibly
all
> > the other SimpleMenuModel delegate APIs) and put it in ui/base/models
> > 2) Just make this class private to BackgroundModeManager.
> > 3) Maybe rename BackgroundModeData to something like
BackgroundModeForProfile,
> > make it a real class, implement SimpleMenuModel::Delegate on that class
which
> > already has its own reference to the profile. You could even refactor all of
> the
> > status icon/context menu code into that class and out of
> BackgroundModeManager.
> > 
> > If we are going to continue having multiple status icons (one for each
> profile)
> > then i'd say #3 is the way to go. If we're going with Glen's suggestion to
> just
> > have a single status icon with multiple per-profile submenus, then probably
#1
> > or #2 would be better because we'd need to keep the status icon
functionality
> in
> > BackgroundModeManager.
> 
> So the real reason that it is its own class is because Miranda had need for a
> similar one so the hope is that this one class will serve both our purposes.
> 
> 1) It seemed pretty specific to be in base.
> 2) Miranda needed it in her class as well.
> 3) I'd pick this option. But it would eliminate others using a profile
specific
> menu. I'll let Miranda chime in on this one as to whether she wants the class
or
> not.

Actually, I think that my menu class needs a vector of profiles, not a single
one -- I should have realized this when we talked yesterday, but my brain was
absent.  Let's do #3. I still like the separate icons for separate profiles
idea, based on my own usage -- but of course, we can change later if others come
to different conclusions on wider usage.

[Miranda Callahan, 2011-05-20 15:21:13]

On 2011/05/20 00:28:04, Andrew T Wilson wrote:
> One more note: app_controller_mac.mm contains logic to create a list of
> background apps as a submenu under the Mac dock menu.
> 
> There's probably some work that needs to be done here to provide per-profile
> lists.

Definitely -- the logic should be quite similar to this patch.

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

Just made a quick pass to respond to your previous comments (and Miranda's).
Sounds like you'll be doing some refactoring around removing ProfileMenuModel so
I'll do a more thorough pass once that's done - just let me know when it's ready
for another round.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.cc:69: Profile* profile_;
On 2011/05/20 05:53:17, rpetterson wrote:
> On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > See my previous comment - do we really need to track profile_ here, given
that
> > it's only used to pass back to the BackgroundApplicationListModel, which
> already
> > has a reference to this profile?
> 
> It doesn't pass back just to the BackgroundApplicationListModel. It also
passes
> back to the BackgroundModeManager (which also inherits
> BackgroundApplicationListModel::Application) and needs a way to know which
> profile's status icon to update.

But BackgroundApplicationListModel::Application never talks directly to
BackgroundModeManager - it always goes through BackgroundApplicationListModel,
which has the profile and can pass that along.

I think this is easier to think about if you rename
BackgroundApplicationListManager::OnApplicationList/DataChanged() to something
else so it doesn't get confused with
BackgroundApplicationListManager::Observer::OnApplicationList/DataChanged() -
the latter needs a Profile, the former doesn't.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.h:114: void
OnApplicationDataChanged(const Extension* extension, Profile* profile);
On 2011/05/20 05:53:17, rpetterson wrote:
> On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > I'm curious why we need to pass in the Profile here, since
> > BackgroundApplicationListManager maintains its own reference to the parent
> > profile (I'm presuming it's invalid to invoke OnApplicationDataChanged()
> passing
> > a profile != profile_, correct?)
> 
> I also commented on the other comment. But BackgroundModeManager also inherts
> from BackgroundApplicationListModel::Application and needs to know which
profile
> it's dealing with.
> 
> I'll add the check you mention to the BackgroundApplicationListModel
definition
> of this function though I don't think it ever gets called.

So, I think you still need to pass a Profile to
BackgroundApplicationListManager::Observer::OnApplicationDataChanged(). But
there's no reason why the private function in
BackgroundApplicationListManager::OnApplicationDataChanged() (which is
unfortunately named identically to the function in Observer) needs to take a
Profile.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
On 2011/05/20 05:53:17, rpetterson wrote:
> On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > nit: should indent these lines correctly (line up with std:: not with open
> > paren). Also line continuations should be indented an extra 4 chars.
> 
> 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.

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 :)

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:277: DCHECK(profile ==
profile_);
Keep in mind that this is *not* an implementation of the
Observer::OnApplicationDataChanged() interface, so this function does not have
to have the same signature (so you don't need to pass profile in here).

You might want to rename this as "SendApplicationDataChangedNotifications()" or
something similar just to avoid confusion in the future.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:155:
manager.OnBackgroundAppInstalled(NULL, &profile1);
Yeah, Miranda is correct. I really hate this pattern, btw, but it seems to be
the most common one used in Chrome.

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

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
On 2011/05/20 16:52:45, Andrew T Wilson wrote:
> On 2011/05/20 05:53:17, rpetterson wrote:
> > On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > > nit: should indent these lines correctly (line up with std:: not with open
> > > paren). Also line continuations should be indented an extra 4 chars.
> > 
> > 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.
> 
> 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 :)
> 

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]

One note, some of the checks for creating the status icon should clear up in the
next CL when I condense down to one status icon.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
File chrome/browser/background_application_list_model.h (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
chrome/browser/background_application_list_model.h:114: void
OnApplicationDataChanged(const Extension* extension, Profile* profile);
On 2011/05/20 16:52:45, Andrew T Wilson wrote:
> On 2011/05/20 05:53:17, rpetterson wrote:
> > On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > > I'm curious why we need to pass in the Profile here, since
> > > BackgroundApplicationListManager maintains its own reference to the parent
> > > profile (I'm presuming it's invalid to invoke OnApplicationDataChanged()
> > passing
> > > a profile != profile_, correct?)
> > 
> > I also commented on the other comment. But BackgroundModeManager also
inherts
> > from BackgroundApplicationListModel::Application and needs to know which
> profile
> > it's dealing with.
> > 
> > I'll add the check you mention to the BackgroundApplicationListModel
> definition
> > of this function though I don't think it ever gets called.
> 
> So, I think you still need to pass a Profile to
> BackgroundApplicationListManager::Observer::OnApplicationDataChanged(). But
> there's no reason why the private function in
> BackgroundApplicationListManager::OnApplicationDataChanged() (which is
> unfortunately named identically to the function in Observer) needs to take a
> Profile.

Done.

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
On 2011/05/20 16:56:45, Miranda Callahan wrote:
> On 2011/05/20 16:52:45, Andrew T Wilson wrote:
> > On 2011/05/20 05:53:17, rpetterson wrote:
> > > On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > > > nit: should indent these lines correctly (line up with std:: not with
open
> > > > paren). Also line continuations should be indented an extra 4 chars.
> > > 
> > > 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.
> > 
> > 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 :)
> > 
> 
> 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. :-)

:)

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:52: Profile* profile_);
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> nit: remove trailing _ 

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:277: DCHECK(profile ==
profile_);
On 2011/05/20 16:52:46, Andrew T Wilson wrote:
> Keep in mind that this is *not* an implementation of the
> Observer::OnApplicationDataChanged() interface, so this function does not have
> to have the same signature (so you don't need to pass profile in here).
> 
> You might want to rename this as "SendApplicationDataChangedNotifications()"
or
> something similar just to avoid confusion in the future.

Thanks for clearing up this confusion. I am renaming it and have pulled out the
profile parameter.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:237: // Only initiali status tray
icons for those profiles which actually
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> nit: initiali...

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:320: // first profile to get the
status tray.
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> looks like you changed this to use the g_browser_process -- comment change as
> well?

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:485: Browser* browser =
BrowserList::GetLastActive();
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> This should be changed to BrowserList::GetLastActiveForProfile(profile), or it
> may bring up a browser window associated with a different profile.

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:488: browser =
BrowserList::GetLastActive();
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> ...should probably also be GetLastActiveForProfile here too.

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:494:
BackgroundModeManager::GetBackgroundModeInfo(
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> Join this line and the one below.

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:185:
BackgroundModeManager::BackgroundModeInfo GetBackgroundModeInfo(
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> nit: say in comment what happens if it doesn't exist (is it created lazily?
null
> returned?)

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:211: // platform doesn't support status
icons.
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> nit: line breaking is a bit too aggressive

Done.

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:155:
manager.OnBackgroundAppInstalled(NULL, &profile1);
On 2011/05/20 15:16:53, Miranda Callahan wrote:
> On 2011/05/20 05:53:17, rpetterson wrote:
> > What needs to be done to be able to call these private functions as done in
> the
> > tests above? For this test I get compile errors, but I don't for any of the
> > others.
> 
> Have you tried adding this line to background_mode_manager.h, around line 64? 

> 
> FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
>                          MultiProfile);

Thanks!

[Miranda Callahan, 2011-05-23 15:57:39]

LGTM from my side!

On 2011/05/23 03:23:19, rpetterson wrote:
> One note, some of the checks for creating the status icon should clear up in
the
> next CL when I condense down to one status icon.
> 
>
http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
> File chrome/browser/background_application_list_model.h (right):
> 
>
http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_ap...
> chrome/browser/background_application_list_model.h:114: void
> OnApplicationDataChanged(const Extension* extension, Profile* profile);
> On 2011/05/20 16:52:45, Andrew T Wilson wrote:
> > On 2011/05/20 05:53:17, rpetterson wrote:
> > > On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > > > I'm curious why we need to pass in the Profile here, since
> > > > BackgroundApplicationListManager maintains its own reference to the
parent
> > > > profile (I'm presuming it's invalid to invoke OnApplicationDataChanged()
> > > passing
> > > > a profile != profile_, correct?)
> > > 
> > > I also commented on the other comment. But BackgroundModeManager also
> inherts
> > > from BackgroundApplicationListModel::Application and needs to know which
> > profile
> > > it's dealing with.
> > > 
> > > I'll add the check you mention to the BackgroundApplicationListModel
> > definition
> > > of this function though I don't think it ever gets called.
> > 
> > So, I think you still need to pass a Profile to
> > BackgroundApplicationListManager::Observer::OnApplicationDataChanged(). But
> > there's no reason why the private function in
> > BackgroundApplicationListManager::OnApplicationDataChanged() (which is
> > unfortunately named identically to the function in Observer) needs to take a
> > Profile.
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
> File chrome/browser/background_mode_manager.cc (right):
> 
>
http://codereview.chromium.org/6914021/diff/3001/chrome/browser/background_mo...
> chrome/browser/background_mode_manager.cc:121: background_mode_data_.begin();
> On 2011/05/20 16:56:45, Miranda Callahan wrote:
> > On 2011/05/20 16:52:45, Andrew T Wilson wrote:
> > > On 2011/05/20 05:53:17, rpetterson wrote:
> > > > On 2011/05/20 00:22:09, Andrew T Wilson wrote:
> > > > > nit: should indent these lines correctly (line up with std:: not with
> open
> > > > > paren). Also line continuations should be indented an extra 4 chars.
> > > > 
> > > > 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.
> > > 
> > > 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 :)
> > > 
> > 
> > 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. :-)
> 
> :)
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
> File chrome/browser/background_application_list_model.cc (right):
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
> chrome/browser/background_application_list_model.cc:52: Profile* profile_);
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > nit: remove trailing _ 
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_a...
> chrome/browser/background_application_list_model.cc:277: DCHECK(profile ==
> profile_);
> On 2011/05/20 16:52:46, Andrew T Wilson wrote:
> > Keep in mind that this is *not* an implementation of the
> > Observer::OnApplicationDataChanged() interface, so this function does not
have
> > to have the same signature (so you don't need to pass profile in here).
> > 
> > You might want to rename this as "SendApplicationDataChangedNotifications()"
> or
> > something similar just to avoid confusion in the future.
> 
> Thanks for clearing up this confusion. I am renaming it and have pulled out
the
> profile parameter.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> File chrome/browser/background_mode_manager.cc (right):
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.cc:237: // Only initiali status tray
> icons for those profiles which actually
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > nit: initiali...
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.cc:320: // first profile to get the
> status tray.
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > looks like you changed this to use the g_browser_process -- comment change
as
> > well?
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.cc:485: Browser* browser =
> BrowserList::GetLastActive();
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > This should be changed to BrowserList::GetLastActiveForProfile(profile), or
it
> > may bring up a browser window associated with a different profile.
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.cc:488: browser =
> BrowserList::GetLastActive();
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > ...should probably also be GetLastActiveForProfile here too.
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.cc:494:
> BackgroundModeManager::GetBackgroundModeInfo(
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > Join this line and the one below.
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> File chrome/browser/background_mode_manager.h (right):
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.h:185:
> BackgroundModeManager::BackgroundModeInfo GetBackgroundModeInfo(
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > nit: say in comment what happens if it doesn't exist (is it created lazily?
> null
> > returned?)
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager.h:211: // platform doesn't support
status
> icons.
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > nit: line breaking is a bit too aggressive
> 
> Done.
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> File chrome/browser/background_mode_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/6914021/diff/12032/chrome/browser/background_m...
> chrome/browser/background_mode_manager_unittest.cc:155:
> manager.OnBackgroundAppInstalled(NULL, &profile1);
> On 2011/05/20 15:16:53, Miranda Callahan wrote:
> > On 2011/05/20 05:53:17, rpetterson wrote:
> > > What needs to be done to be able to call these private functions as done
in
> > the
> > > tests above? For this test I get compile errors, but I don't for any of
the
> > > others.
> > 
> > Have you tried adding this line to background_mode_manager.h, around line
64? 
> 
> > 
> > FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
> >                          MultiProfile);
> 
> Thanks!

[Andrew T Wilson (Slow), 2011-05-23 20:35:40]

This is pretty close - I just have a few more questions/issues, with some style
nits in a few places.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:52: Profile* profile);
Profile is no longer needed.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:69: Profile* profile_;
We don't need a profile_ here any more.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:231:
background_mode_data_[profile]->applications_->AddObserver(this);
So, it looks like we mostly just delegate the handling of
BackgroundApplicationListManager notification off to BackgroundModeData. Is
there a reason why we need to listen to notifications in BackgroundModeManager
(why doesn't BackgroundModeData register itself as an observer instead)? Or is
having BackgroundModeManager listen to this notifications going to be useful
when you go back to having only a single status icon for all profiles?

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:268: 
Remove extra blank line

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:310: //  BackgroundModeManager,
BackgroundApplicationLisModel::Observer overrides
Nit: LisModel->ListModel

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:367: if
(it->second->HasBackgroundApp()) {
nit: Probably should remove curly braces since the rest of the file doesn't put
them around one-line if statements.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:534: BackgroundModeInfo bmd =
GetBackgroundModeInfo(profile);
If we were going to keep the multiple-status-icon functionality, then I'd say we
should move this code into BackgroundModeData. But since you're going to
implement single-status-icon support in an upcoming patch, it's fine to leave
this stuff in BackgroundModeManager since that's undoubtedly where it will end
up anyway.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:544:
BackgroundModeManager::GetBackgroundModeInfo(Profile* profile) {
nit: this is one of the few cases where you shouldn't indent a wrapped line
(function definitions should never be indented).

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:12: #include "base/memory/singleton.h"
Not needed as we are no longer a singleton (we are owned by BrowserProcessImpl).

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:122: typedef
linked_ptr<BackgroundModeData> BackgroundModeInfo;
I don't think we need/want the reference-counting aspects of linked_ptr (these
objects should not outlive the owner - BackgroundModeManager). I would suggest
either making this a scoped_ptr<> or else just have background_mode_data_ be a
map<Profile*, BackgroundModeData> - this would require having a no-param default
constructor and a separate Init() routine, though, so I'd probably go with
scoped_ptr.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:199: // not exist, returns an empty
BackgroundMdoeInfo.
nit: MdoeInfo->ModeInfo

I would change this to GetBackgroundModeData(Profile*) and have it return a
BackgroundModeData* (so we retain ownership within BackgroundModeManager).

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:55: // all icons now that
there are no apps
nit: period at end of sentence.

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:122: // call to
CreateStatusTray
nit: period at end of sentence.

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:159: // call to
CreateStatusTray
nit: period at end.

[rpetterson, 2011-05-24 02:18:00]

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
File chrome/browser/background_application_list_model.cc (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:52: Profile* profile);
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> Profile is no longer needed.

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_a...
chrome/browser/background_application_list_model.cc:69: Profile* profile_;
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> We don't need a profile_ here any more.

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:231:
background_mode_data_[profile]->applications_->AddObserver(this);
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> So, it looks like we mostly just delegate the handling of
> BackgroundApplicationListManager notification off to BackgroundModeData. Is
> there a reason why we need to listen to notifications in BackgroundModeManager
> (why doesn't BackgroundModeData register itself as an observer instead)? Or is
> having BackgroundModeManager listen to this notifications going to be useful
> when you go back to having only a single status icon for all profiles?

Yes, it will be useful in the future. Currently the only thing BMM does as a
result of being an observer for the application list is to update the status
tray icon(s). In the future, the BMM will be the only controlling the icon and
context menu contents so it makes sense to keep it the observer.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:268: 
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> Remove extra blank line

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:310: //  BackgroundModeManager,
BackgroundApplicationLisModel::Observer overrides
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> Nit: LisModel->ListModel

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:367: if
(it->second->HasBackgroundApp()) {
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: Probably should remove curly braces since the rest of the file doesn't
put
> them around one-line if statements.

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:534: BackgroundModeInfo bmd =
GetBackgroundModeInfo(profile);
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> If we were going to keep the multiple-status-icon functionality, then I'd say
we
> should move this code into BackgroundModeData. But since you're going to
> implement single-status-icon support in an upcoming patch, it's fine to leave
> this stuff in BackgroundModeManager since that's undoubtedly where it will end
> up anyway.

This was my thought exactly. I was going to move it there, but then realized it
was just making extra work since it it will eventually come back here. A similar
argument exists for CreateStatusTrayIcon.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:544:
BackgroundModeManager::GetBackgroundModeInfo(Profile* profile) {
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: this is one of the few cases where you shouldn't indent a wrapped line
> (function definitions should never be indented).

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:12: #include "base/memory/singleton.h"
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> Not needed as we are no longer a singleton (we are owned by
BrowserProcessImpl).

Done.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:122: typedef
linked_ptr<BackgroundModeData> BackgroundModeInfo;
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> I don't think we need/want the reference-counting aspects of linked_ptr (these
> objects should not outlive the owner - BackgroundModeManager). I would suggest
> either making this a scoped_ptr<> or else just have background_mode_data_ be a
> map<Profile*, BackgroundModeData> - this would require having a no-param
default
> constructor and a separate Init() routine, though, so I'd probably go with
> scoped_ptr.

scoped_ptr won't work in a map because it has to make copies. Similarly I can't
just put in BackgroundModeData objects becuase they have a scoped_ptr (to
applications_) as well so they can't be copied. The only two options I see are
BackgroundModeData* or linked_ptr. I would have used a shared_ptr, but when I
looked at the rest of the codebase, it seems typical to use linked_ptr in maps.

http://codereview.chromium.org/6914021/diff/19001/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:199: // not exist, returns an empty
BackgroundMdoeInfo.
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: MdoeInfo->ModeInfo
> 
> I would change this to GetBackgroundModeData(Profile*) and have it return a
> BackgroundModeData* (so we retain ownership within BackgroundModeManager).

I'm not quite following the change you're suggesting with regards to retaining
ownership. Perhaps this relates to the discussion about ptr types?

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:55: // all icons now that
there are no apps
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: period at end of sentence.

Done.

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:122: // call to
CreateStatusTray
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: period at end of sentence.

Done.

http://codereview.chromium.org/6914021/diff/25001/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:159: // call to
CreateStatusTray
On 2011/05/23 20:35:40, Andrew T Wilson wrote:
> nit: period at end.

Done.

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:165: if
(g_browser_process->local_state() && !skip_pref_registrar_init) {
I'm not thrilled about adding this bool here, but unfortunately the existence of
local_state() isn't always a sufficient test for unit_tests.

[Andrew T Wilson (Slow), 2011-05-24 20:49:28]

So, I'd like to understand why we need to create BackgroundModeManager in
TestingBrowserProcess now when previously we just returned NULL, and I'd really
like to get more comfortable about why we need that skip_pref_register_init
flag.

Can you send me a chat when you get a chance so we can discuss further? Those
are my only remaining concerns with this CL.

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:113: //EXPECT_CALL(manager,
CreateStatusTrayIcon(_));
nit: why is this commented out?

http://codereview.chromium.org/6914021/diff/30003/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6914021/diff/30003/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:131: background_mode_manager_.reset(new
BackgroundModeManager(&cl, true));
Not sure why we aren't returning NULL here any more? Seems like we really would
rather not create BackgroundModeManager in tests if we can avoid it.

[rpetterson, 2011-05-24 21:48:42]

Fixed up the items we discussed over chat as well (or rather reverted them back
to not needing skip_pref_registrar_init).

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6914021/diff/30003/chrome/browser/background_m...
chrome/browser/background_mode_manager_unittest.cc:113: //EXPECT_CALL(manager,
CreateStatusTrayIcon(_));
On 2011/05/24 20:49:28, Andrew T Wilson wrote:
> nit: why is this commented out?

Leftover from testing. Removed.

http://codereview.chromium.org/6914021/diff/30003/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6914021/diff/30003/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:131: background_mode_manager_.reset(new
BackgroundModeManager(&cl, true));
On 2011/05/24 20:49:28, Andrew T Wilson wrote:
> Not sure why we aren't returning NULL here any more? Seems like we really
would
> rather not create BackgroundModeManager in tests if we can avoid it.

Done.

[Andrew T Wilson (Slow), 2011-05-25 01:17:44]

LGTM, with the notes below and assuming the deletion of RegisterPrefs() was just
a merge error.

Thanks again for tackling this, and apologies for having so many comments :)

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:552: }
Did RegisterPrefs() get deleted accidentally? I think it should be re-added.

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:121: typedef
linked_ptr<BackgroundModeData> BackgroundModeInfo;
Add a comment about why we are using linked_ptr here (because of the map issue
you mentioned).

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:199:
BackgroundModeManager::BackgroundModeInfo GetBackgroundModeInfo(
The other thing I wanted to mention is that we probably should be returning
BackgroundModeData* from here, not a BackgroundModeInfo (dereference before
returning). The reason is that we really don't want calling
GetBackgroundModeInfo() to be changing the reference count, IMO -- we should
always retain ownership in background_mode_data_ and only expose weak pointers
outside that. Anyhow, not a big deal, so if you agree feel free to make this
change in a separate CL (and if you disagree just ignore this :)

[Andrew T Wilson (Slow), 2011-05-25 01:18:38]

Ooops, I see that you just moved RegisterPrefs(), you didn't delete it. So
ignore that comment.

[rpetterson, 2011-05-25 19:57:12]

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
File chrome/browser/background_mode_manager.h (right):

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:121: typedef
linked_ptr<BackgroundModeData> BackgroundModeInfo;
On 2011/05/25 01:17:44, Andrew T Wilson wrote:
> Add a comment about why we are using linked_ptr here (because of the map issue
> you mentioned).

Done.

http://codereview.chromium.org/6914021/diff/31015/chrome/browser/background_m...
chrome/browser/background_mode_manager.h:199:
BackgroundModeManager::BackgroundModeInfo GetBackgroundModeInfo(
On 2011/05/25 01:17:44, Andrew T Wilson wrote:
> The other thing I wanted to mention is that we probably should be returning
> BackgroundModeData* from here, not a BackgroundModeInfo (dereference before
> returning). The reason is that we really don't want calling
> GetBackgroundModeInfo() to be changing the reference count, IMO -- we should
> always retain ownership in background_mode_data_ and only expose weak pointers
> outside that. Anyhow, not a big deal, so if you agree feel free to make this
> change in a separate CL (and if you disagree just ignore this :)

I understand your point now. I'll include this in the next CL to unify the
status icons.