[Profiles] Use high resolution avatars if they exist

chrome/browser/profiles/profile_info_cache.cc

Index: chrome/browser/profiles/profile_info_cache.cc
diff --git a/chrome/browser/profiles/profile_info_cache.cc b/chrome/browser/profiles/profile_info_cache.cc
index 8055c9638c2b26452770a21239d0e41165f63bcb..b17dfe5c0ba8a4f076c64bea6be4d41873027a5f 100644
--- a/chrome/browser/profiles/profile_info_cache.cc
+++ b/chrome/browser/profiles/profile_info_cache.cc
@@ -10,6 +10,7 @@
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ -22,6 +23,7 @@
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/profile_management_switches.h"
 #include "content/public/browser/browser_thread.h"
@@ -60,6 +62,7 @@ const char kActiveTimeKey[] = "active_time";
 
 const char kDefaultUrlPrefix[] = "chrome://theme/IDR_PROFILE_AVATAR_";
 const char kGAIAPictureFileName[] = "Google Profile Picture.png";
+const char kHighResAvatarFolderName[] = "Avatars";
 
 const int kDefaultAvatarIconResources[] = {
   IDR_PROFILE_AVATAR_0,
@@ -90,6 +93,37 @@ const int kDefaultAvatarIconResources[] = {
   IDR_PROFILE_AVATAR_25,
 };
 
+// File names for the high-res avatar icon resources. In the same order as
+// the avatars in kDefaultAvatarIconResources.
+const char* kDefaultAvatarIconResourceFileNames[] = {
+  "chrome_avatar_generic.png",
+  "chrome_avatar_generic_aqua.png",
+  "chrome_avatar_generic_blue.png",
+  "chrome_avatar_generic_green.png",
+  "chrome_avatar_generic_orange.png",
+  "chrome_avatar_generic_purple.png",
+  "chrome_avatar_generic_red.png",
+  "chrome_avatar_generic_yellow.png",
+  "chrome_avatar_secret_agent.png",
+  "chrome_avatar_superhero.png",
+  "chrome_avatar_volley_ball.png",
+  "chrome_avatar_businessman.png",
+  "chrome_avatar_ninja.png",
+  "chrome_avatar_alien.png",
+  "chrome_avatar_smiley.png",
+  "chrome_avatar_flower.png",
+  "chrome_avatar_pizza.png",
+  "chrome_avatar_soccer.png",
+  "chrome_avatar_burger.png",
+  "chrome_avatar_cat.png",
+  "chrome_avatar_cupcake.png",
+  "chrome_avatar_dog.png",
+  "chrome_avatar_horse.png",
+  "chrome_avatar_margarita.png",
+  "chrome_avatar_note.png",
+  "chrome_avatar_sun_cloud.png",

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

Do we really need chrome_ prefix ?

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

Nope! On 2014/03/25 14:39:49, Roger Tawa wrote:

+};
+
 const size_t kDefaultAvatarIconsCount = arraysize(kDefaultAvatarIconResources);
 
 // The first 8 icons are generic.
@@ -152,6 +186,10 @@ void ReadBitmap(const base::FilePath& image_path,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   *out_image = NULL;
 
+  // If the path doesn't exist, don't even try reading it.
+  if (!base::PathExists(image_path))
+    return;
+
   std::string image_data;
   if (!base::ReadFileToString(image_path, &image_data)) {
     LOG(ERROR) << "Failed to read PNG file from disk.";
@@ -225,7 +263,7 @@ ProfileInfoCache::ProfileInfoCache(PrefService* prefs,
 
 ProfileInfoCache::~ProfileInfoCache() {
   STLDeleteContainerPairSecondPointers(
-      gaia_pictures_.begin(), gaia_pictures_.end());
+      cached_avatar_images_.begin(), cached_avatar_images_.end());
 }
 
 void ProfileInfoCache::AddProfileToCache(const base::FilePath& profile_path,
@@ -364,6 +402,13 @@ const gfx::Image& ProfileInfoCache::GetAvatarIconOfProfileAtIndex(
       return *image;
   }
 
+  // Use the high resolution version of the avatar if it exists.
+  if (switches::IsNewProfileManagement()) {
+    const gfx::Image* image = GetHighResAvatarOfProfileAtIndex(index);
+    if (image)
+      return *image;
+  }
+
   int resource_id = GetDefaultAvatarIconResourceIDAtIndex(
       GetAvatarIconIndexOfProfileAtIndex(index));
   return ResourceBundle::GetSharedInstance().GetNativeImageNamed(resource_id);
@@ -412,30 +457,58 @@ const gfx::Image* ProfileInfoCache::GetGAIAPictureOfProfileAtIndex(
   std::string key = CacheKeyFromProfilePath(path);
 
   // If the picture is already loaded then use it.
-  if (gaia_pictures_.count(key)) {
-    if (gaia_pictures_[key]->IsEmpty())
+  if (cached_avatar_images_.count(key)) {
+    if (cached_avatar_images_[key]->IsEmpty())
       return NULL;
-    return gaia_pictures_[key];
+    return cached_avatar_images_[key];
   }
 
   std::string file_name;
   GetInfoForProfileAtIndex(index)->GetString(
       kGAIAPictureFileNameKey, &file_name);
 
-  // If the picture is not on disk or it is already being loaded then return
-  // NULL.
-  if (file_name.empty() || gaia_pictures_loading_[key])
+  // If the picture is not on disk then return NULL.
+  if (file_name.empty())
     return NULL;
 
-  gaia_pictures_loading_[key] = true;
   base::FilePath image_path = path.AppendASCII(file_name);
+  LoadAvatarPictureFromPath(key, image_path);
+  return NULL;
+}
+
+const gfx::Image* ProfileInfoCache::GetHighResAvatarOfProfileAtIndex(
+    size_t index) const {
+  int avatar_index = GetAvatarIconIndexOfProfileAtIndex(index);
+  std::string key = kDefaultAvatarIconResourceFileNames[avatar_index];
+
+  // If the picture is already loaded then use it.
+  if (cached_avatar_images_.count(key)) {
+    if (cached_avatar_images_[key]->IsEmpty())
+      return NULL;
+    return cached_avatar_images_[key];
+  }
+
+  base::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  base::FilePath image_path =
+      user_data_dir.AppendASCII(kHighResAvatarFolderName).AppendASCII(key);
+  LoadAvatarPictureFromPath(key, image_path);
+  return NULL;
+}

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

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.

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

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:

+
+void ProfileInfoCache::LoadAvatarPictureFromPath(
+    const std::string& key,
+    const base::FilePath& image_path) const {
+  // If the picture is already being loaded then don't try loading it again.
+  if (cached_avatar_images_loading_[key])
+    return;
+  cached_avatar_images_loading_[key] = true;
+
   gfx::Image** image = new gfx::Image*;
   BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
       base::Bind(&ReadBitmap, image_path, image),
-      base::Bind(&ProfileInfoCache::OnGAIAPictureLoaded,
-          const_cast<ProfileInfoCache*>(this)->AsWeakPtr(), path, image));
-
-  return NULL;
+      base::Bind(&ProfileInfoCache::OnAvatarPictureLoaded,
+          const_cast<ProfileInfoCache*>(this)->AsWeakPtr(), key, image));
 }
 
 bool ProfileInfoCache::ProfileIsManagedAtIndex(size_t index) const {
@@ -474,19 +547,18 @@ bool ProfileInfoCache::ProfileIsUsingDefaultNameAtIndex(size_t index) const {
   return value;
 }
 
-void ProfileInfoCache::OnGAIAPictureLoaded(const base::FilePath& path,
-                                           gfx::Image** image) const {
+void ProfileInfoCache::OnAvatarPictureLoaded(const std::string& key,
+                                             gfx::Image** image) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  std::string key = CacheKeyFromProfilePath(path);
-  gaia_pictures_loading_[key] = false;
+  cached_avatar_images_loading_[key] = false;
 
   if (*image) {
-    delete gaia_pictures_[key];
-    gaia_pictures_[key] = *image;
+    delete cached_avatar_images_[key];
+    cached_avatar_images_[key] = *image;
   } else {
     // Place an empty image in the cache to avoid reloading it again.
-    gaia_pictures_[key] = new gfx::Image();
+    cached_avatar_images_[key] = new gfx::Image();

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

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]

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:

   }
   delete image;
 
@@ -705,10 +777,11 @@ void ProfileInfoCache::SetGAIAPictureOfProfileAtIndex(size_t index,
   std::string key = CacheKeyFromProfilePath(path);
 
   // Delete the old bitmap from cache.
-  std::map<std::string, gfx::Image*>::iterator it = gaia_pictures_.find(key);
-  if (it != gaia_pictures_.end()) {
+  std::map<std::string, gfx::Image*>::iterator it =
+      cached_avatar_images_.find(key);
+  if (it != cached_avatar_images_.end()) {
     delete it->second;
-    gaia_pictures_.erase(it);
+    cached_avatar_images_.erase(it);
   }
 
   std::string old_file_name;
@@ -725,7 +798,7 @@ void ProfileInfoCache::SetGAIAPictureOfProfileAtIndex(size_t index,
     }
   } else {
     // Save the new bitmap to disk.
-    gaia_pictures_[key] = new gfx::Image(*image);
+    cached_avatar_images_[key] = new gfx::Image(*image);
     scoped_ptr<ImageData> data(new ImageData);
     scoped_refptr<base::RefCountedMemory> png_data = image->As1xPNGBytes();
     data->assign(png_data->front(), png_data->front() + png_data->size());