Fixing background mode manager to correctly display the name of the profile in the status icon. P...

Fixing background mode manager to correctly display the name of the profile in the status icon. Previously it was not being updated when Chrome first started up and it was also using the incorrect name. Now it gets the correct name from the profile info cache and also updates based on changes to the profile info cache.

BUG=101837
TEST=start chrome with background apps and the status icon will have names populated correctly, also BackgroundModeManagerTest::ProfileInfoCacheObserver

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113458

[Andrew T Wilson (Slow), 2011-12-06 00:46:03]

Also, is there any way to add some simple test coverage for this (even just
something simple like checking that GetBackgroundModeData(profile)->name() is
updated appropriately when the name changes?

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.cc:48: if (index !=
std::string::npos)
It seems weird to use std::string::npos here, which seems like it's specifically
for string indices? Is this the standard idiom we're using with
GetIndexOfProfileWithPath()?

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.cc:49: name_ =
cache->GetNameOfProfileAtIndex(index);
Also, it's somewhat odd to have this split in responsibilities between
BackgroundModeManager and BackgroundModeManagerData - it's not clear who is
responsible for setting the name (if the name is available at constructor time,
then BMD is responsible, otherwise BMM is responsible).

I'd suggest moving all of the "set the name" logic/responsibilities into one or
the other class - either have BMD register as an observer on ProfileInfoCache
and notify BackgroundModeManager when its name has changed, or else remove this
logic from the BMD constructor, and just have BackgroundModeManager always
explicitly call SetName() after creating BackgroundModeData in
RegisterProfile().

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.h:236: // If it does not
exist, returns NULL.
Update this comment since this cannot return NULL.

[rpetterson, 2011-12-06 05:52:33]

I've added a test which tests the three new major functions, OnProfileAdded,
OnProfileRemoved, and OnProfileNameChanged.

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.cc:48: if (index !=
std::string::npos)
On 2011/12/06 00:46:03, Andrew T Wilson wrote:
> It seems weird to use std::string::npos here, which seems like it's
specifically
> for string indices? Is this the standard idiom we're using with
> GetIndexOfProfileWithPath()?

I agree that it seems odd. I was originally checking if index was less than the
total number of profiles. But GetIndexOfProfileWithPath returns
std::string::npos if the path isn't found so I thought it best to use that.

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.cc:49: name_ =
cache->GetNameOfProfileAtIndex(index);
On 2011/12/06 00:46:03, Andrew T Wilson wrote:
> Also, it's somewhat odd to have this split in responsibilities between
> BackgroundModeManager and BackgroundModeManagerData - it's not clear who is
> responsible for setting the name (if the name is available at constructor
time,
> then BMD is responsible, otherwise BMM is responsible).
> 
> I'd suggest moving all of the "set the name" logic/responsibilities into one
or
> the other class - either have BMD register as an observer on ProfileInfoCache
> and notify BackgroundModeManager when its name has changed, or else remove
this
> logic from the BMD constructor, and just have BackgroundModeManager always
> explicitly call SetName() after creating BackgroundModeData in
> RegisterProfile().

I moved everything to the BMM. I would have preferred the BMD, but due to
removing from the list in OnProfileRemoved, it couldn't be in the BMD.

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/8802013/diff/5001/chrome/browser/background/ba...
chrome/browser/background/background_mode_manager.h:236: // If it does not
exist, returns NULL.
On 2011/12/06 00:46:03, Andrew T Wilson wrote:
> Update this comment since this cannot return NULL.

Done.

[Andrew T Wilson (Slow), 2011-12-06 06:46:01]

LGTM with a few nits.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:269: return
background_mode_data_[profile]->name();
Perhaps we should add a DCHECK(background_mode_data_.count(profile) > 0) to make
sure the passed profile exists in the map?

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:404:
UpdateStatusTrayIconContextMenu();
Should we break out of the loop here/early return?

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:410: // Remove the profile
from our map of profiles
nit: period at the end of the comment.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:413: // If a profile isn't
running a background app, it may not be in the map
nit: period at end of comment

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:425: // We check for if due
to unittests, but really this should only be called on
nit: "We check for if" - did you omit a word here?

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:71: string16
BackgroundModeDataName(Profile* profile);
This is fine, although since we're already exposing GetBackgroundModeData() to
the tests via FRIEND_TEST_ALL_PREFIXES() they could also just call
GetBackgroundModeData(profile)->name().

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:238: // If it does not
exist.
This comment seems truncated. Also, returning an iterator here is interesting -
no caller would ever actually use this to iterate, but instead it's returned so
the caller can efficiently remove the item from the underlying map if desired.
That's probably worth pointing out in the comment.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager_unittest.cc:281: EXPECT_EQ(1,
manager.NumberOfBackgroundModeData());
I like this test - it covers all the cases. One minor suggestion - one more
thing you could check is to ensure that the correct BackgroundModeData() was
removed via the call to OnProfileRemoved():

EXPECT_EQ(UTF8ToUTF16("p1new"), manager.BackgroundModeDataName(profile1));

If you paired this with my suggested DCHECK() in BackgroundModeDataName() to
ensure the profile already exists in the map, it would ensure that the expected
BMD remains in the map.

[rpetterson, 2011-12-06 20:09:04]

All nits taken care of. Thanks!

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:269: return
background_mode_data_[profile]->name();
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> Perhaps we should add a DCHECK(background_mode_data_.count(profile) > 0) to
make
> sure the passed profile exists in the map?

Function removed.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:404:
UpdateStatusTrayIconContextMenu();
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> Should we break out of the loop here/early return?

Done.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:410: // Remove the profile
from our map of profiles
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> nit: period at the end of the comment.

Done.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:413: // If a profile isn't
running a background app, it may not be in the map
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> nit: period at end of comment

Done.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.cc:425: // We check for if due
to unittests, but really this should only be called on
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> nit: "We check for if" - did you omit a word here?

Done.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:71: string16
BackgroundModeDataName(Profile* profile);
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> This is fine, although since we're already exposing GetBackgroundModeData() to
> the tests via FRIEND_TEST_ALL_PREFIXES() they could also just call
> GetBackgroundModeData(profile)->name().

Good point. I'd rather not have extraneous functions just for testing so this is
gone.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:238: // If it does not
exist.
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> This comment seems truncated. Also, returning an iterator here is interesting
-
> no caller would ever actually use this to iterate, but instead it's returned
so
> the caller can efficiently remove the item from the underlying map if desired.
> That's probably worth pointing out in the comment.

Done.

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/8802013/diff/10002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager_unittest.cc:281: EXPECT_EQ(1,
manager.NumberOfBackgroundModeData());
On 2011/12/06 06:46:02, Andrew T Wilson wrote:
> I like this test - it covers all the cases. One minor suggestion - one more
> thing you could check is to ensure that the correct BackgroundModeData() was
> removed via the call to OnProfileRemoved():
> 
> EXPECT_EQ(UTF8ToUTF16("p1new"), manager.BackgroundModeDataName(profile1));
> 
> If you paired this with my suggested DCHECK() in BackgroundModeDataName() to
> ensure the profile already exists in the map, it would ensure that the
expected
> BMD remains in the map.

That's a good idea. Done.

[commit-bot: I haz the power, 2011-12-07 19:01:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8802013/21001

[commit-bot: I haz the power, 2011-12-07 20:38:04]

Change committed as 113458