Refactor ProfileInfoCache in c/b/profiles

chrome/browser/profiles/profile_avatar_downloader.cc

 // Copyright 2014 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_avatar_downloader.h"
 
 #include "base/files/file_path.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "net/base/load_flags.h"
 
 namespace {
 const char kHighResAvatarDownloadUrlPrefix[] =
     "https://www.gstatic.com/chrome/profile_avatars/";
 }
 
 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));
 }
 
 ProfileAvatarDownloader::~ProfileAvatarDownloader() {
 }
 
 void ProfileAvatarDownloader::Start() {
   // In unit tests, the browser process can return a NULL request context.
   net::URLRequestContextGetter* request_context =
       g_browser_process->system_request_context();
   if (request_context) {
     fetcher_->Init(
         request_context,
         std::string(),
         net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,
         net::LOAD_NORMAL);
     fetcher_->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_));
 }