[Profiles] Send out less profile avatar related notifications

chrome/browser/profiles/profile_info_cache.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile_info_cache.h"
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
@@ skipping 117 matching lines @@
   gfx::Image image = gfx::Image::CreateFrom1xPNGBytes(
       base::RefCountedString::TakeString(&image_data));
   if (image.IsEmpty()) {
     LOG(ERROR) << "Failed to decode PNG file.";
     return;
   }
 
   *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);
 }
 
 }  // namespace
 
 ProfileInfoCache::ProfileInfoCache(PrefService* prefs,
                                    const base::FilePath& user_data_dir)
     : prefs_(prefs),
@@ skipping 59 matching lines @@
   info->SetBoolean(kIsOmittedFromProfileListKey, !supervised_user_id.empty());
   info->SetBoolean(kProfileIsEphemeral, false);
   info->SetBoolean(kIsUsingDefaultNameKey, IsDefaultProfileName(name));
   // Assume newly created profiles use a default avatar.
   info->SetBoolean(kIsUsingDefaultAvatarKey, true);
   cache->SetWithoutPathExpansion(key, info.release());
 
   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_,
                     OnProfileAdded(profile_path));
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
       content::NotificationService::AllSources(),
       content::NotificationService::NoDetails());
 }
@@ skipping 301 matching lines @@
                                                      size_t icon_index) {
   scoped_ptr<base::DictionaryValue> info(
       GetInfoForProfileAtIndex(index)->DeepCopy());
   info->SetString(kAvatarIconKey,
       profiles::GetDefaultAvatarIconUrl(icon_index));
   // This takes ownership of |info|.
   SetInfoForProfileAtIndex(index, info.release());
 
   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_,
                     OnProfileAvatarChanged(profile_path));
 }
 
 void ProfileInfoCache::SetIsOmittedProfileAtIndex(size_t index,
                                                   bool is_omitted) {
   if (IsOmittedProfileAtIndex(index) == is_omitted)
     return;
@@ skipping 134 matching lines @@
 }
 
 void ProfileInfoCache::SetIsUsingGAIAPictureOfProfileAtIndex(size_t index,
                                                              bool value) {
   scoped_ptr<base::DictionaryValue> info(
       GetInfoForProfileAtIndex(index)->DeepCopy());
   info->SetBoolean(kUseGAIAPictureKey, value);
   // 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_,
                     OnProfileAvatarChanged(profile_path));
 }
 
 void ProfileInfoCache::SetProfileSigninRequiredAtIndex(size_t index,
                                                        bool value) {
   if (value == ProfileIsSigninRequiredAtIndex(index))
     return;
@@ skipping 134 matching lines @@
     names.push_back(name);
   }
   return names;
 }
 
 // static
 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.
 #if defined(OS_ANDROID) || defined(OS_IOS) || defined(OS_CHROMEOS)
   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(
     const gfx::Image* image,
     const std::string& key,
     const base::FilePath& image_path,
     const base::FilePath& profile_path) {
   cached_avatar_images_[key] = new gfx::Image(*image);
 
   scoped_ptr<ImageData> data(new ImageData);
@@ skipping 120 matching lines @@
 
   if (!strcmp(key.c_str(), profiles::GetNoHighResAvatarFileName()))
       return NULL;
 
   base::FilePath image_path =
       profiles::GetPathOfHighResAvatarAtIndex(avatar_index);
   return LoadAvatarPictureFromPath(GetPathOfProfileAtIndex(index),
                                    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,
     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];
   }
@@ skipping 28 matching lines @@
   }
   delete image;
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
       content::NotificationService::AllSources(),
       content::NotificationService::NoDetails());
 
   FOR_EACH_OBSERVER(ProfileInfoCacheObserver,
                     observer_list_,
-                    OnProfileAvatarChanged(profile_path));
+                    OnProfileHighResAvatarLoaded(profile_path));
 }
 
 void ProfileInfoCache::OnAvatarPictureSaved(
       const std::string& file_name,
       const base::FilePath& profile_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,
       content::NotificationService::AllSources(),
       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.
   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;
 }
 
@@ skipping 11 matching lines @@
   // depend on the names of all the other profiles in the info cache, so they
   // need to be re-computed after each rename.
   std::vector<base::FilePath> profiles_to_rename;
 
   const base::string16 default_profile_name = base::i18n::ToLower(
       l10n_util::GetStringUTF16(IDS_DEFAULT_PROFILE_NAME));
   const base::string16 default_legacy_profile_name = base::i18n::ToLower(
       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)
       profiles_to_rename.push_back(GetPathOfProfileAtIndex(i));
   }
 
   // Rename the necessary profiles.
   std::vector<base::FilePath>::const_iterator it;
   for (it = profiles_to_rename.begin(); it != profiles_to_rename.end(); ++it) {
     size_t profile_index = GetIndexOfProfileWithPath(*it);
     SetProfileIsUsingDefaultNameAtIndex(profile_index, true);
     // This will assign a new "Person %d" type name and re-sort the cache.
     SetNameOfProfileAtIndex(profile_index, ChooseNameForNewProfile(
         GetAvatarIconIndexOfProfileAtIndex(profile_index)));
   }
 #endif
 }