[Profiles] Send out less profile avatar related notifications

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 4e6803e2ceb57727fbdd9386df1918ab01c384ac..4d6cb1d5927d0b8d1e849767e3926eeb611e3533 100644
--- a/chrome/browser/profiles/profile_info_cache.cc
+++ b/chrome/browser/profiles/profile_info_cache.cc
@@ -135,6 +135,12 @@ void ReadBitmap(const base::FilePath& image_path,
   *out_image = new gfx::Image(image);
 }
 
+void CheckIfFileExists(const base::FilePath& file_path,

[Mike Lerman, 2015/01/27 16:39:15]

Not a huge fan of the name (sorry!). I feel like a method named "Check..."
should always return a boolean. This is "ExecuteCallbackIfFileExists", really.

[noms (inactive), 2015/01/28 02:18:35]

Done.

+                       const base::Closure& callback) {
+  if (!base::PathExists(file_path))

[Mike Lerman, 2015/01/27 16:39:15]

Do a CHECK that you're on FILE thread.

[noms (inactive), 2015/01/28 02:18:35]

Done.

+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+}
+
 void DeleteBitmap(const base::FilePath& image_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   base::DeleteFile(image_path, false);
@@ -214,7 +220,7 @@ void ProfileInfoCache::AddProfileToCache(
   sorted_keys_.insert(FindPositionForProfile(key, name), key);
 
   if (switches::IsNewAvatarMenu())
-    DownloadHighResAvatar(icon_index, profile_path);
+    DownloadHighResAvatarIfNeeded(icon_index, profile_path);
 
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
                     observer_list_,
@@ -536,9 +542,8 @@ void ProfileInfoCache::SetAvatarIconOfProfileAtIndex(size_t index,
 
   base::FilePath profile_path = GetPathOfProfileAtIndex(index);
 
-  // If needed, start downloading the high-res avatar.
   if (switches::IsNewAvatarMenu())
-    DownloadHighResAvatar(icon_index, profile_path);
+    DownloadHighResAvatarIfNeeded(icon_index, profile_path);
 
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
                     observer_list_,
@@ -693,7 +698,6 @@ void ProfileInfoCache::SetIsUsingGAIAPictureOfProfileAtIndex(size_t index,
   // This takes ownership of |info|.
   SetInfoForProfileAtIndex(index, info.release());
 
-  // Retrieve some info to update observers who care about avatar changes.
   base::FilePath profile_path = GetPathOfProfileAtIndex(index);
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
                     observer_list_,
@@ -848,7 +852,7 @@ void ProfileInfoCache::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kProfileInfoCache);
 }
 
-void ProfileInfoCache::DownloadHighResAvatar(
+void ProfileInfoCache::DownloadHighResAvatarIfNeeded(
     size_t icon_index,
     const base::FilePath& profile_path) {
   // Downloading is only supported on desktop.
@@ -856,25 +860,16 @@ void ProfileInfoCache::DownloadHighResAvatar(
   return;
 #endif
 
-  // TODO(noms): We should check whether the file already exists on disk
-  // before trying to re-download it. For now, since this is behind a flag and
-  // the resources are still changing, re-download it every time the profile
-  // avatar changes, to make sure we have the latest copy.
-  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;
-
-  // Start the download for this file. The cache takes ownership of the
-  // |avatar_downloader|, which will be deleted when the download completes, or
-  // if that never happens, when the ProfileInfoCache is destroyed.
-  ProfileAvatarDownloader* avatar_downloader = new ProfileAvatarDownloader(
-      icon_index,
-      profile_path,
-      this);
-  avatar_images_downloads_in_progress_[file_name] = avatar_downloader;
-  avatar_downloader->Start();
+  const base::FilePath file_path =
+      profiles::GetPathOfHighResAvatarAtIndex(icon_index);
+  base::Closure callback =
+      base::Bind(&ProfileInfoCache::DownloadHighResAvatar,
+                 AsWeakPtr(),
+                 profiles::GetDefaultAvatarIconFileNameAtIndex(icon_index),
+                 icon_index,
+                 profile_path);
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,

[Mike Lerman, 2015/01/27 16:39:15]

Rather that posting to FILE, consider using a BlockingPool:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

[noms (inactive), 2015/01/28 02:18:35]

I'd rather have all the posts in this file look the same (so all posts, or all
BlockingPool), and I'd rather not change them now. So I'm going to punt this
one, if that's ok :)

[Mike Lerman, 2015/01/28 18:32:56]

that's ok :)

+      base::Bind(&CheckIfFileExists, file_path, callback));
 }
 
 void ProfileInfoCache::SaveAvatarImageAtPath(
@@ -1015,6 +1010,30 @@ const gfx::Image* ProfileInfoCache::GetHighResAvatarOfProfileAtIndex(
                                    key, image_path);
 }
 
+void ProfileInfoCache::DownloadHighResAvatar(
+    const std::string file_name,

[Mike Lerman, 2015/01/27 16:39:15]

For simplicity, and to have this signature match DownloadHighResAvatarIfNeeded,
why not remove |file_name| and have this method call
GetDefaultAvatarIconFileNameAtIndex()?

[noms (inactive), 2015/01/28 02:18:35]

Done.

+    size_t icon_index,
+    const base::FilePath& profile_path) {
+  // Downloading is only supported on desktop.
+#if defined(OS_ANDROID) || defined(OS_IOS) || defined(OS_CHROMEOS)
+  return;
+#endif
+
+  // If the file is already being downloaded, don't start another download.
+  if (avatar_images_downloads_in_progress_[file_name])
+    return;
+
+  // Start the download for this file. The cache takes ownership of the
+  // |avatar_downloader|, which will be deleted when the download completes, or
+  // if that never happens, when the ProfileInfoCache is destroyed.
+  ProfileAvatarDownloader* avatar_downloader = new ProfileAvatarDownloader(
+      icon_index,
+      profile_path,
+      this);
+  avatar_images_downloads_in_progress_[file_name] = avatar_downloader;
+  avatar_downloader->Start();
+}
+
 const gfx::Image* ProfileInfoCache::LoadAvatarPictureFromPath(
     const base::FilePath& profile_path,
     const std::string& key,
@@ -1063,7 +1082,7 @@ void ProfileInfoCache::OnAvatarPictureLoaded(const base::FilePath& profile_path,
 
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
                     observer_list_,
-                    OnProfileAvatarChanged(profile_path));
+                    OnProfileHighResAvatarLoaded(profile_path));
 }
 
 void ProfileInfoCache::OnAvatarPictureSaved(
@@ -1077,8 +1096,8 @@ void ProfileInfoCache::OnAvatarPictureSaved(
       content::NotificationService::NoDetails());
 
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
-                    observer_list_,
-                    OnProfileAvatarChanged(profile_path));
+      observer_list_,
+      OnProfileHighResAvatarLoaded(profile_path));
 
   // 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.
@@ -1110,9 +1129,8 @@ void ProfileInfoCache::MigrateLegacyProfileNamesAndDownloadAvatars() {
       l10n_util::GetStringUTF16(IDS_LEGACY_DEFAULT_PROFILE_NAME));
 
   for (size_t i = 0; i < GetNumberOfProfiles(); i++) {
-    // If needed, start downloading the high-res avatar for this profile.
-    DownloadHighResAvatar(GetAvatarIconIndexOfProfileAtIndex(i),
-                          GetPathOfProfileAtIndex(i));
+    DownloadHighResAvatarIfNeeded(GetAvatarIconIndexOfProfileAtIndex(i),
+                                  GetPathOfProfileAtIndex(i));
 
     base::string16 name = base::i18n::ToLower(GetNameOfProfileAtIndex(i));
     if (name == default_profile_name || name == default_legacy_profile_name)