[Profiles] Download high-res avatars using the --new-profile-management flag

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 8220ede2a3654c6381734f33fad9805a922e25d7..f0c7d261a94237cebf9b2777d53c2b0bf39966e0 100644
--- a/chrome/browser/profiles/profile_info_cache.cc
+++ b/chrome/browser/profiles/profile_info_cache.cc
@@ -9,7 +9,6 @@
 #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"
@@ -21,15 +20,15 @@
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile_avatar_downloader.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
-#include "chrome/common/chrome_paths.h"
+#include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/common/pref_names.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
@@ -193,6 +192,9 @@ ProfileInfoCache::ProfileInfoCache(PrefService* prefs,
 ProfileInfoCache::~ProfileInfoCache() {
   STLDeleteContainerPairSecondPointers(
       cached_avatar_images_.begin(), cached_avatar_images_.end());
+  STLDeleteContainerPairSecondPointers(
+      avatar_images_downloads_in_progress_.begin(),
+      avatar_images_downloads_in_progress_.end());
 }
 
 void ProfileInfoCache::AddProfileToCache(const base::FilePath& profile_path,
@@ -332,7 +334,7 @@ const gfx::Image& ProfileInfoCache::GetAvatarIconOfProfileAtIndex(
   }
 
   // Use the high resolution version of the avatar if it exists.
-  if (switches::IsNewProfileManagement()) {
+  if (switches::IsNewAvatarMenu()) {
     const gfx::Image* image = GetHighResAvatarOfProfileAtIndex(index);
     if (image)
       return *image;
@@ -391,44 +393,6 @@ const gfx::Image* ProfileInfoCache::GetGAIAPictureOfProfileAtIndex(
   return LoadAvatarPictureFromPath(key, image_path);
 }
 
-const gfx::Image* ProfileInfoCache::GetHighResAvatarOfProfileAtIndex(
-    size_t index) const {
-  int avatar_index = GetAvatarIconIndexOfProfileAtIndex(index);
-  std::string key = profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index);
-
-  if (!strcmp(key.c_str(), profiles::GetNoHighResAvatarFileName()))
-    return NULL;
-
-  base::FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  base::FilePath image_path = user_data_dir.
-      AppendASCII(profiles::kHighResAvatarFolderName).AppendASCII(key);
-  return LoadAvatarPictureFromPath(key, image_path);
-}
-
-const gfx::Image* ProfileInfoCache::LoadAvatarPictureFromPath(
-    const std::string& key,
-    const base::FilePath& image_path) const {
-  // 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];
-  }
-
-  // If the picture is already being loaded then don't try loading it again.
-  if (cached_avatar_images_loading_[key])
-    return NULL;
-  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::OnAvatarPictureLoaded,
-          const_cast<ProfileInfoCache*>(this)->AsWeakPtr(), key, image));
-  return NULL;
-}
-
 bool ProfileInfoCache::ProfileIsManagedAtIndex(size_t index) const {
   return !GetManagedUserIdOfProfileAtIndex(index).empty();
 }
@@ -465,40 +429,6 @@ bool ProfileInfoCache::ProfileIsUsingDefaultNameAtIndex(size_t index) const {
   return value;
 }
 
-void ProfileInfoCache::OnAvatarPictureLoaded(const std::string& key,
-                                             gfx::Image** image) const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  cached_avatar_images_loading_[key] = false;
-
-  delete cached_avatar_images_[key];
-  if (*image) {
-    cached_avatar_images_[key] = *image;
-  } else {
-    // Place an empty image in the cache to avoid reloading it again.
-    cached_avatar_images_[key] = new gfx::Image();
-  }
-  delete image;
-
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
-      content::NotificationService::AllSources(),
-      content::NotificationService::NoDetails());
-}
-
-void ProfileInfoCache::OnGAIAPictureSaved(const base::FilePath& path,
-                                          bool* success) const  {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (*success) {
-    content::NotificationService::current()->Notify(
-        chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,
-        content::NotificationService::AllSources(),
-        content::NotificationService::NoDetails());
-  }
-  delete success;
-}
-
 bool ProfileInfoCache::IsUsingGAIAPictureOfProfileAtIndex(
     size_t index) const {
   bool value = false;
@@ -695,22 +625,10 @@ void ProfileInfoCache::SetGAIAPictureOfProfileAtIndex(size_t index,
     }
   } else {
     // Save the new bitmap to disk.
-    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());
-    if (!data->size()) {
-      LOG(ERROR) << "Failed to PNG encode the image.";
-    } else {
-      new_file_name = old_file_name.empty() ?
-          profiles::kGAIAPictureFileName : old_file_name;
-      base::FilePath image_path = path.AppendASCII(new_file_name);
-      bool* success = new bool;
-      BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
-          base::Bind(&SaveBitmap, data.release(), image_path, success),
-          base::Bind(&ProfileInfoCache::OnGAIAPictureSaved, AsWeakPtr(),
-                     path, success));
-    }
+    new_file_name =
+        old_file_name.empty() ? profiles::kGAIAPictureFileName : old_file_name;
+    base::FilePath image_path = path.AppendASCII(new_file_name);
+    SaveAvatarImageAtPath(image, key, image_path);
   }
 
   scoped_ptr<base::DictionaryValue> info(
@@ -931,6 +849,81 @@ void ProfileInfoCache::UpdateSortForProfileIndex(size_t index) {
       content::NotificationService::NoDetails());
 }
 
+const gfx::Image* ProfileInfoCache::GetHighResAvatarOfProfileAtIndex(
+    size_t index) const {
+  int avatar_index = GetAvatarIconIndexOfProfileAtIndex(index);
+  std::string key = profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index);

[msw, 2014/04/28 20:46:37]

This is no longer checked against GetNoHighResAvatarFileName , is that okay? (I
guess this is somehow related to GetPathOfHighResAvatarAtIndex using
GetDefaultAvatarIconResourceInfo instead of GetDefaultAvatarIconFileNameAtIndex
for the image path?)

[noms (inactive), 2014/04/29 19:12:27]

Argh, no, that was a rebase disaster. Thank you for catching it!
Fixed.
On 2014/04/28 20:46:37, msw wrote:

+  base::FilePath image_path =
+      profiles::GetPathOfHighResAvatarAtIndex(avatar_index);
+  return LoadAvatarPictureFromPath(key, image_path);
+}
+
+const gfx::Image* ProfileInfoCache::LoadAvatarPictureFromPath(
+    const std::string& key,
+    const base::FilePath& image_path) const {
+  // 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];
+  }
+
+  // If the picture is already being loaded then don't try loading it again.
+  if (cached_avatar_images_loading_[key])
+    return NULL;
+  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::OnAvatarPictureLoaded,
+          const_cast<ProfileInfoCache*>(this)->AsWeakPtr(), key, image));
+  return NULL;
+}
+
+void ProfileInfoCache::OnAvatarPictureLoaded(const std::string& key,
+                                             gfx::Image** image) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  cached_avatar_images_loading_[key] = false;
+  delete cached_avatar_images_[key];
+
+  if (*image) {
+    cached_avatar_images_[key] = *image;
+  } else {
+    // Place an empty image in the cache to avoid reloading it again.
+    cached_avatar_images_[key] = new gfx::Image();
+  }
+  delete image;
+
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
+      content::NotificationService::AllSources(),
+      content::NotificationService::NoDetails());
+}
+
+void ProfileInfoCache::OnAvatarPictureSaved(const std::string& file_name,
+                                            bool* success) {
+

[msw, 2014/04/28 20:46:37]

nit: remove new blank line

[noms (inactive), 2014/04/29 19:12:27]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (*success) {
+    content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,

[msw, 2014/04/28 20:46:37]

nit: these should be indented two more spaces.

[noms (inactive), 2014/04/29 19:12:27]

Done.

+      content::NotificationService::AllSources(),
+      content::NotificationService::NoDetails());
+  }
+  delete success;
+
+  // Remove the file from the list of downloads in progress. Note that this list
+  // only contains the high resolution avatars, and not the Gaia profile images.
+  if (!avatar_images_downloads_in_progress_[file_name])
+    return;
+
+  delete avatar_images_downloads_in_progress_[file_name];
+  avatar_images_downloads_in_progress_[file_name] = NULL;
+}
+
 // static
 std::vector<base::string16> ProfileInfoCache::GetProfileNames() {
   std::vector<base::string16> names;
@@ -952,3 +945,37 @@ std::vector<base::string16> ProfileInfoCache::GetProfileNames() {
 void ProfileInfoCache::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kProfileInfoCache);
 }
+
+void ProfileInfoCache::DownloadHighResAvatar(

[msw, 2014/04/28 20:46:37]

The order of all the function definitions in this file should match the
declaration order in the header.

[noms (inactive), 2014/04/29 19:12:27]

Done.

+    ProfileAvatarDownloader* avatar_downloader,
+    size_t icon_index) {
+  std::string file_name = profiles::GetDefaultAvatarIconFileNameAtIndex(
+      icon_index);
+  // If the file is already being downloaded, don't start another download.
+  if (avatar_images_downloads_in_progress_[file_name])
+    return;
+  avatar_images_downloads_in_progress_[file_name] = avatar_downloader;
+  avatar_downloader->Start();
+}
+
+void ProfileInfoCache::SaveAvatarImageAtPath(
+    const gfx::Image* image,
+    const std::string& key,
+    const base::FilePath& image_path) {
+

[msw, 2014/04/28 20:46:37]

nit: remove blank line

[noms (inactive), 2014/04/29 19:12:27]

Done.

+  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());
+
+  if (!data->size()) {
+    LOG(ERROR) << "Failed to PNG encode the image.";
+  } else {
+    bool* success = new bool;

[msw, 2014/04/28 20:46:37]

Why is this allocated here? SaveBitmap can call OnAvatarPictureSaved with its
own success value. I realize you haven't changed the original pattern, but I
would encourage you to revise this to something sane.

[noms (inactive), 2014/04/29 19:12:27]

I've changed it to passing the callback, and only running it in the successful
case. Is this better?

On 2014/04/28 20:46:37, msw wrote:

+    BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&SaveBitmap, data.release(), image_path, success),
+        base::Bind(&ProfileInfoCache::OnAvatarPictureSaved, AsWeakPtr(),
+                   key, success));
+  }
+}