[Profiles] Use high resolution avatars if they exist

[Profiles] Use high resolution avatars if they exist

If you're using the --new-profile-management flag, this CL uses a high 
resolution profile avatar, if it can be found. If it doesn't, it remains in the 
present status quo of using the small resolution avatar. If the flag isn't
enabled, the only changes are minor refactors, and no user changes are 
visible.

This is CL #1 of a multi-CL Voltron, that solves the avatar migration
problem as discussed here:
https://docs.google.com/a/google.com/document/d/1KU9gvlXg33rwcvr24NmReXwYRZ1KJhGmiMziyDHyttQ/edit?usp=sharing

TL;DR: the existing avatars are really tiny and can't really be displayed at
a higher size. We will on-demand download them to the user_data dir, and
if available, display them from there.

For testing, you can copy these avatars to user_data_dir/Avatars:
https://drive.google.com/folderview?id=0B1B1Up4p2NRMWE1tcHNsSmtPSlE&usp=sharing

BUG=305048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260384

[Roger Tawa OOO till Jul 10th, 2014-03-25 14:39:49]

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:124:
"chrome_avatar_sun_cloud.png",
Do we really need chrome_ prefix ?

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:497: }
GetGAIAPictureOfProfileAtIndex() and GetHighResAvatarOfProfileAtIndex() are
really similar.  Both are only called from GetAvatarIconOfProfileAtIndex().  I
think it might be better to combine them into one function like:

const gfx::Image* GetAvatarOfProfileAtIndex(
    const std::string& key,
    const base::FilePath& path)

and have GetAvatarIconOfProfileAtIndex() calculate the correct key and path
depending on whether IsUsingGAIAPictureOfProfileAtIndex(index) returns true or
not, and error conditions.  You probably don't need LoadAvatarPictureFromPath()
as a separate function either.

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:561: cached_avatar_images_[key] =
new gfx::Image();
I still think there is a potential memory leak here.  Can we guarantee that if
|*image| is null, that |cached_avatar_images_[key]| does not already contain an
image?  Can we add a DCHECK for this condition?

[noms (inactive), 2014-03-25 19:24:22]

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:124:
"chrome_avatar_sun_cloud.png",
Nope! On 2014/03/25 14:39:49, Roger Tawa wrote:
> Do we really need chrome_ prefix ?

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:497: }
Unfortunately GetGAIAPictureOfProfileAtIndex() is used by a bunch of other
classes that don't have access to things like the key and the path. However, I
moved more code to the common area. Is it better?

On 2014/03/25 14:39:49, Roger Tawa wrote:
> GetGAIAPictureOfProfileAtIndex() and GetHighResAvatarOfProfileAtIndex() are
> really similar.  Both are only called from GetAvatarIconOfProfileAtIndex().  I
> think it might be better to combine them into one function like:
> 
> const gfx::Image* GetAvatarOfProfileAtIndex(
>     const std::string& key,
>     const base::FilePath& path)
> 
> and have GetAvatarIconOfProfileAtIndex() calculate the correct key and path
> depending on whether IsUsingGAIAPictureOfProfileAtIndex(index) returns true or
> not, and error conditions.  You probably don't need
LoadAvatarPictureFromPath()
> as a separate function either.

https://codereview.chromium.org/210043005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:561: cached_avatar_images_[key] =
new gfx::Image();
I've just deleted |cached_avatar_images_[key]| in all cases. If it's null, it's
still fine, if it's not, it will get cleaned out and reassigned in both cases.

On 2014/03/25 14:39:49, Roger Tawa wrote:
> I still think there is a potential memory leak here.  Can we guarantee that if
> |*image| is null, that |cached_avatar_images_[key]| does not already contain
an
> image?  Can we add a DCHECK for this condition?

[noms (inactive), 2014-03-25 20:06:53]

+ Rachel for profiles review

[rpetterson, 2014-03-25 20:55:55]

profiles owners stamp lgtm

[Roger Tawa OOO till Jul 10th, 2014-03-27 15:28:49]

lgtm

[noms (inactive), 2014-03-27 15:29:04]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-03-27 15:32:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/210043005/150001

[commit-bot: I haz the power, 2014-03-27 15:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 15:59:00]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[noms (inactive), 2014-03-27 19:43:30]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:45:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/210043005/150001

[commit-bot: I haz the power, 2014-03-27 22:21:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:21:07]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[noms (inactive), 2014-03-29 16:22:02]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-03-29 16:22:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/210043005/150001

[commit-bot: I haz the power, 2014-03-29 17:46:04]

Change committed as 260384