Don't try to download 'NothingToDownload' placeholder avatar.

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 887 matching lines @@
 
 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
   DCHECK(!disable_avatar_download_for_testing_);
 
+  // If this is the placeholder avatar, it is already included in the
+  // resources, so it doesn't need to be downloaded.

[Mike Lerman, 2015/05/13 20:53:59]

Can you append:
The avatar picture for this index will never be requested from disk by
GetHighResAvatarOfProfileAtIndex.

That makes a clear link between this condition and the check at line 1048.

+  if (icon_index == profiles::GetPlaceholderAvatarIndex())
+    return;
+
   const base::FilePath& file_path =
       profiles::GetPathOfHighResAvatarAtIndex(icon_index);
   base::Closure callback =
       base::Bind(&ProfileInfoCache::DownloadHighResAvatar,
                  AsWeakPtr(),
                  icon_index,
                  profile_path);
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       base::Bind(&RunCallbackIfFileMissing, file_path, callback));
 }
@@ skipping 111 matching lines @@
   std::string key = CacheKeyFromProfilePath(GetPathOfProfileAtIndex(index));
   std::vector<std::string>::iterator key_it =
       std::find(sorted_keys_.begin(), sorted_keys_.end(), key);
   DCHECK(key_it != sorted_keys_.end());
   sorted_keys_.erase(key_it);
   sorted_keys_.insert(FindPositionForProfile(key, name), key);
 }
 
 const gfx::Image* ProfileInfoCache::GetHighResAvatarOfProfileAtIndex(
     size_t index) const {
-  int avatar_index = GetAvatarIconIndexOfProfileAtIndex(index);
-  std::string key = profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index);
+  const size_t avatar_index = GetAvatarIconIndexOfProfileAtIndex(index);
 
   // If this is the placeholder avatar, it is already included in the
   // resources, so it doesn't need to be downloaded.
-  if (!strcmp(key.c_str(), profiles::GetNoHighResAvatarFileName())) {
+  if (avatar_index == profiles::GetPlaceholderAvatarIndex()) {
     return &ui::ResourceBundle::GetSharedInstance().GetImageNamed(
         profiles::GetPlaceholderAvatarIconResourceID());
   }
 
-  base::FilePath image_path =
+  const std::string file_name =
+      profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index);
+  const base::FilePath image_path =
       profiles::GetPathOfHighResAvatarAtIndex(avatar_index);
-  return LoadAvatarPictureFromPath(GetPathOfProfileAtIndex(index),
-                                   key, image_path);
+  return LoadAvatarPictureFromPath(GetPathOfProfileAtIndex(index), file_name,
+                                   image_path);
 }
 
 void ProfileInfoCache::DownloadHighResAvatar(
     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(erikchen): Remove ScopedTracker below once http://crbug.com/461175
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "461175 ProfileInfoCache::DownloadHighResAvatar::GetFileName"));
   const std::string file_name =

[Mike Lerman, 2015/05/12 19:59:12]

What happens if icon_index is the non-downloading icon? This isn't handled here.

[emaxx, 2015/05/13 12:08:16]

It would DCHECK inside GetDefaultAvatarIconFileNameAtIndex().
But, anyway, the DownloadHighResAvatar() is a private function that is
referenced only by DownloadHighResAvatarIfNeed() function, whose responsibility
is exactly caring about existing files and placeholder.
Do you think that we should nevertheless handle it here too?

[Mike Lerman, 2015/05/13 20:53:59]

Let's add a DCHECK(!file_name.empty()), to help protect against misuse of this
method. Doesn't hurt.

       profiles::GetDefaultAvatarIconFileNameAtIndex(icon_index);
   // If the file is already being downloaded, don't start another download.
   if (avatar_images_downloads_in_progress_.count(file_name))
     return;
 
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/461175
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile2(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "461175 ProfileInfoCache::DownloadHighResAvatar::MakeDownloader"));
@@ skipping 127 matching lines @@
   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
 }