Refactor ProfileInfoCacheObserver interface and usage thereof.

Refactor ProfileInfoCacheObserver interface and usage thereof.

We want to simplify OnProfileAdded and OnProfileAvatarChanged so decouple the Observer interface a bit away from ProfileShortcutManagerWin. Instead, classes that implement the interface should fetch the global cache object and retrieve any additional information it needs from there when they are notified.

BUG=none
TEST=Ensure that ProfileInfoCacheTest and BackgroundModeManagerTest unit tests all pass.

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

[SteveT, 2011-12-21 17:12:45]

PTAL. A couple questions from me inline.

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const string16&
profile_base_dir) = 0;
I would love to remove |profile_base_dir| as well, which you can get by calling
ProfileInfoCache::CacheKeyFromProfilePath() on |profile_path|... Unfortunately,
this is a private member method. Are we cool with making this public?

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:31: const string16&
profile_base_dir) = 0;
As above.

[sail, 2011-12-21 17:51:10]

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const string16&
profile_base_dir) = 0;
Hi Steve, do you actually need the profile_base_dir here? If you need the unique
directory name that the profile is saved in then you can just get that from the
last component of profile_path. (something like FilePath::BaseName() i think).

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:23: const string16&
profile_name) = 0;
If possible, could you change all the callbacks to simply take the full profile
path? It seems like that would be all you need. The only exception would be
OnProfileWasRemoved and OnProfileNameChanged. For those two callbacks, they full
profile path should also be included.

[SteveT, 2011-12-21 20:47:03]

Over to you, PTAL.

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const string16&
profile_base_dir) = 0;
As I commented above - no we don't, but unfortunately we'll have to duplicate
some code to get it from |profile_path| unless we make CacheKeyFromProfilePath
public.

I'll do as you suggested, but let me know if it makes sense for us to make
CacheKeyFromProfilePath public to save some code duplication.

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:23: const string16&
profile_name) = 0;
Sounds good. All done - except I had to include the profile_name along side the
profile_path in OnProfileWasRemoved, as we are no longer able to retrieve the
name of the removed profile from the cache by using the old profile_path.

[sail, 2011-12-21 21:03:12]

lgtm

http://codereview.chromium.org/9020013/diff/7005/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/9020013/diff/7005/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:235:
UTF8ToUTF16(profile_path.BaseName().MaybeAsASCII());
since this code is windows only you don't need the UTF8ToUTF16() call. You can
just do profile_path.BaseName().value().

[sail, 2011-12-21 21:04:12]

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/9020013/diff/6002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const string16&
profile_base_dir) = 0;
On 2011/12/21 20:47:03, SteveT wrote:
> As I commented above - no we don't, but unfortunately we'll have to duplicate
> some code to get it from |profile_path| unless we make CacheKeyFromProfilePath
> public.
> 
> I'll do as you suggested, but let me know if it makes sense for us to make
> CacheKeyFromProfilePath public to save some code duplication.

I think the Cache key concept should remain private. Currently it's the profile
directory but there's no guarantee that it'll remain so.

[SteveT, 2011-12-22 02:59:07]

PTAL @ manage_profile_handler.cc:205. I left a comment explaining a change I had
to make to fix an issue I found while testing the refactored code.

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/manage_profile_handler.cc:205:
pref_service->SetInteger(prefs::kProfileAvatarIndex, new_icon_index);
I had to make this switch because SetInteger(kProfileAvatarIndex,...) sends a
notification that triggers updating the shortcut... however if we haven't set
IsUsingGAIAPicture to false before doing that, GetAvatarIcon returns the GAIA
pic rather than the default icon and the shortcut is incorrectly badged.

[sail, 2011-12-22 04:03:11]

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/manage_profile_handler.cc:205:
pref_service->SetInteger(prefs::kProfileAvatarIndex, new_icon_index);
On 2011/12/22 02:59:07, SteveT wrote:
> I had to make this switch because SetInteger(kProfileAvatarIndex,...) sends a
> notification that triggers updating the shortcut... however if we haven't set
> IsUsingGAIAPicture to false before doing that, GetAvatarIcon returns the GAIA
> pic rather than the default icon and the shortcut is incorrectly badged.

But wouldn't it eventually become correct once
SetIsUsingGAIAPictureOfProfileAtIndex() is called?

Does the shortcut manager have trouble if consecutive notifications are being
sent?

[SteveT, 2011-12-22 04:36:24]

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/manage_profile_handler.cc:205:
pref_service->SetInteger(prefs::kProfileAvatarIndex, new_icon_index);
Hm... So SetIsUsingGAIAPictureOfProfileAtIndex currently only notifies observers
if it is currently being set to true. I suppose with our new logic, we can have
it notify every time, instead.

That way (assuming we re-swamp the two above lines), the SetInteger call sets
the avatar index in place... and then the SetIsUsingGAIAPicture call triggers
the actual OnProfileAvatarChanged notification.

How does that sound?

[sail, 2011-12-22 04:48:28]

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/manage_profile_handler.cc:205:
pref_service->SetInteger(prefs::kProfileAvatarIndex, new_icon_index);
On 2011/12/22 04:36:24, SteveT wrote:
> Hm... So SetIsUsingGAIAPictureOfProfileAtIndex currently only notifies
observers

Really? That's weird. Similar to the name functions we should notify when ever
an API call will make an actual change.

http://codereview.chromium.org/9020013/diff/6017/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/manage_profile_handler.cc:205:
pref_service->SetInteger(prefs::kProfileAvatarIndex, new_icon_index);
> That way (assuming we re-swamp the two above lines), the SetInteger call sets
> the avatar index in place... and then the SetIsUsingGAIAPicture call triggers
> the actual OnProfileAvatarChanged notification.
> 
> How does that sound?

Sounds good. I'm not worried about the actual order of the calls. I was just
worried about the shortcut manager code being very fragile.

[SteveT, 2011-12-22 15:21:39]

I've removed the |value| check in SetIsUsingGAIAPIcture and surely enough, it
fixes the badging issue without having to flip those two lines in
profile_manager_handler.

Let me know if this looks good and I'll proceed with the commit.

[sail, 2011-12-22 17:36:31]

lgtm

[commit-bot: I haz the power, 2011-12-22 18:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/9020013/11001

[commit-bot: I haz the power, 2011-12-22 20:22:36]

Try job failure for 9020013-11001 (retry) on win_rel for step
"safe_browsing_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-22 20:28:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/9020013/11001

[commit-bot: I haz the power, 2011-12-22 22:05:18]

Change committed as 115621