[Profiles] Download high-res avatars using the --new-profile-management flag

chrome/browser/profiles/profiles_state.cc

Index: chrome/browser/profiles/profiles_state.cc
diff --git a/chrome/browser/profiles/profiles_state.cc b/chrome/browser/profiles/profiles_state.cc
index 282ffd3f3544b25312ce4518a7be51cf4b0faacf..2ce7489880a4a80f381e1ef450871a69355d166a 100644
--- a/chrome/browser/profiles/profiles_state.cc
+++ b/chrome/browser/profiles/profiles_state.cc
@@ -10,6 +10,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_avatar_downloader.h"
+#include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
@@ -113,4 +115,22 @@ bool IsRegularOrGuestSession(Browser* browser) {
   return profile->IsGuestSession() || !profile->IsOffTheRecord();
 }
 
+void DownloadHighResAvatarIfNeeded(size_t icon_index) {
+  // TODO(noms): We should check whether the file already exists on disk

[msw, 2014/04/28 20:46:37]

This seems like a fairly straightforward duty of ProfileInfoCache, is it really
important to land this CL before it can avoid re-downloading high-res resources?

[noms (inactive), 2014/04/29 19:12:27]

We'd like to test it for a bit, given that it's such a giant change (it's also
been in review for a month, so I was trying to get the smallest CL possible
out).

On 2014/04/28 20:46:37, msw wrote:

+  // 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

[msw, 2014/04/28 20:46:37]

What is the plan going forward if the resources change? Do we plan to always
re-download the resource, use versions/timestamps, or naively expected resources
to never change and allow some clients to go out of date if they do? How is this
handled for other downloaded resources?

[noms (inactive), 2014/04/29 19:12:27]

I don't think we plan to do any versioning. The idea is that the resources will
not change once this feature goes out (much like the avatars haven't change
since they got introduced). If the avatars need to be redesigned any time in the
future, we will rename the files, and get the new ones like that.

On 2014/04/28 20:46:37, msw wrote:

+  // avatar changes, to make sure we have the latest copy.
+  ProfileInfoCache* cache =
+      &g_browser_process->profile_manager()->GetProfileInfoCache();
+
+  // 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_dowloader = new ProfileAvatarDownloader(

[msw, 2014/04/28 20:46:37]

Is this object leaked if DownloadHighResAvatar returns early because a download
of the same file is already in progress? Perhaps pass a scoped_ptr, or even
better, have DownloadHighResAvatar construct its own ProfileAvatarDownloader
internally, only when needed.

[noms (inactive), 2014/04/29 19:12:27]

Ah yes, this is much better. This didn't work before because it relied on the
BitmapFetcher refactor, but I should've fixed it after it landed. Sorry & done!

On 2014/04/28 20:46:37, msw wrote:

+      g_browser_process->system_request_context(),

[msw, 2014/04/28 20:46:37]

Can't ProfileAvatarDownloader obtain this value internally?
Instead of passing values associated with |g_browser_process| or
TestingBrowserProcess, ProfileInfoCache should have a SetBrowserProcessForTest
helper that will override it's internal use of either value, or similar.

[noms (inactive), 2014/04/29 19:12:27]

Done.

+      icon_index,
+      cache);
+  cache->DownloadHighResAvatar(avatar_dowloader, icon_index);
+}
+
 }  // namespace profiles