Refactor ProfileInfoCache in c/b/profiles

chrome/browser/profiles/profile_avatar_downloader.cc

Index: chrome/browser/profiles/profile_avatar_downloader.cc
diff --git a/chrome/browser/profiles/profile_avatar_downloader.cc b/chrome/browser/profiles/profile_avatar_downloader.cc
index 69f4a4c94130debd1adb8a4ae19a419b63130465..46b255d82c1eed5e471c54cc307f4655c8f52977 100644
--- a/chrome/browser/profiles/profile_avatar_downloader.cc
+++ b/chrome/browser/profiles/profile_avatar_downloader.cc
@@ -19,10 +19,11 @@ const char kHighResAvatarDownloadUrlPrefix[] =
 ProfileAvatarDownloader::ProfileAvatarDownloader(
     size_t icon_index,
     const base::FilePath& profile_path,
-    ProfileInfoCache* cache)
+    ProfileAttributesStorage* storage)
     : icon_index_(icon_index),
       profile_path_(profile_path),
-      cache_(cache) {
+      storage_(storage) {
+  DCHECK(storage);
   GURL url(std::string(kHighResAvatarDownloadUrlPrefix) +
            profiles::GetDefaultAvatarIconFileNameAtIndex(icon_index));
   fetcher_.reset(new chrome::BitmapFetcher(url, this));
@@ -48,13 +49,20 @@ void ProfileAvatarDownloader::Start() {
 // BitmapFetcherDelegate overrides.
 void ProfileAvatarDownloader::OnFetchComplete(const GURL& url,
                                               const SkBitmap* bitmap) {
-  if (!bitmap || !cache_)
+  if (!bitmap)
     return;
 
-  // Decode the downloaded bitmap. Ownership of the image is taken by |cache_|.
+  ProfileAttributesEntry* entry;
+  if (storage_->GetProfileAttributesWithPath(profile_path_, &entry)) {

[Mike Lerman, 2016/03/18 17:10:44]

just confirming... we don't use this flow for the Guest Profile's avatar?

[lwchkg, 2016/03/19 18:48:46]

Currently no, but it DOES make sense for guests to get an high-res avatar in the
future.

Now only ProfileInfoCache::DownloadHighResAvatar creates an instance of this
class. And according to code search, the call stack is
[1] ProfileInfoCache::DownloadHighResAvatarIfNeeded
[2] ProfileInfoCache::AddProfileToCache or
    ProfileInfoCache::SetAvatarIconOfProfileAtIndex or
    ProfileInfoCache::MigrateLegacyProfileNamesAndDownloadAvatars
and all functions in [2] requires the entry to be present.

Anyway, calling ProfileInfoCache::SaveAvatarImageAtPath by an object owned by
ProfileInfoCache is a bad design. That explicit function call should be made a
callback function to reduce coupling.

Therefore, now I prefer to leave the whole file untouched, and do the
refactoring in a separate CL or when we refactor the profile_info_cache
internal. WDYT?

BTW, do we use these high-res avatar images outside the desktop?

[Mike Lerman, 2016/03/30 01:18:10]

Sure, leave it as is for now, then. I like reducing the coupling.

Since Guest never appears in UIs along with other profiles as an option to
select, we shouldn't ever need the large-size avatar for it. I'd prefer to still
not download the avatar for Guest/System.

It's Desktop only. I believe there are appropriate #idefs somewhere along the
way to enforce that - look at the top of DownloadHighResAvatarIfNeeded.

[lwchkg, 2016/03/31 19:32:26]

We have those funny anonymous animals in Google Docs/Sheet. Not sure if the UI
guys will ask for a similar humor in the future. :-)

Anyway, profile_path is already refactored out of ProfileAvatarDownloader, so
the discussion is now moot.
Thanks.

+    // If the profile is deleted during the async operation, we cannot find the
+    // profile attributes entry. In this case, discard the downloaded bitmap.
+    return;
+  }
+
+  // Decode the downloaded bitmap. Ownership of the image is taken by profile
+  // attributes storage.
   gfx::Image image = gfx::Image::CreateFrom1xBitmap(*bitmap);
-  cache_->SaveAvatarImageAtPath(&image,
+  entry->SaveAvatarImage(&image,
       profiles::GetDefaultAvatarIconFileNameAtIndex(icon_index_),
-      profiles::GetPathOfHighResAvatarAtIndex(icon_index_),
-      profile_path_);
+      profiles::GetPathOfHighResAvatarAtIndex(icon_index_));
 }