Credential Management API: the account chooser dialog supports custom avatars.

chrome/browser/ui/views/passwords/credentials_item_view.cc

Index: chrome/browser/ui/views/passwords/credentials_item_view.cc
diff --git a/chrome/browser/ui/views/passwords/credentials_item_view.cc b/chrome/browser/ui/views/passwords/credentials_item_view.cc
index 0c4bdbb2318388eccaa5b211754ab31db2f5270f..6fc2086a1047927dd7ed8816eb606451c13cb2e7 100644
--- a/chrome/browser/ui/views/passwords/credentials_item_view.cc
+++ b/chrome/browser/ui/views/passwords/credentials_item_view.cc
@@ -4,11 +4,10 @@
 
 #include "chrome/browser/ui/views/passwords/credentials_item_view.h"
 
-#include "chrome/browser/browser_process.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/bitmap_fetcher/bitmap_fetcher.h"
+#include "chrome/browser/profiles/profile.h"
 #include "grit/theme_resources.h"
+#include "net/base/load_flags.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image.h"
@@ -56,23 +55,74 @@ void CircularImageView::OnPaint(gfx::Canvas* canvas) {
 
 }  // namespace
 
+// Helper class to download the avatar. It deletes itself once the request is
+// done.
+class CredentialsItemView::AvatarFetcher
+    : public chrome::BitmapFetcherDelegate {
+ public:
+  AvatarFetcher(const GURL& url,
+                const base::WeakPtr<CredentialsItemView>& delegate);
+
+  void Start(net::URLRequestContextGetter* request_context);

[Mike West, 2015/01/20 17:48:55]

Nit: Newline after.

[vasilii, 2015/01/20 19:43:03]

Done.

+ private:
+  ~AvatarFetcher() override;
+
+  // chrome::BitmapFetcherDelegate:
+  void OnFetchComplete(const GURL url, const SkBitmap* bitmap) override;
+
+  chrome::BitmapFetcher fetcher_;
+  base::WeakPtr<CredentialsItemView> delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(AvatarFetcher);
+};
+
+CredentialsItemView::AvatarFetcher::AvatarFetcher(
+    const GURL& url,
+    const base::WeakPtr<CredentialsItemView>& delegate)
+    : fetcher_(url, this),
+      delegate_(delegate) {
+}
+
+CredentialsItemView::AvatarFetcher::~AvatarFetcher() = default;
+
+void CredentialsItemView::AvatarFetcher::Start(
+    net::URLRequestContextGetter* request_context) {
+  fetcher_.Start(request_context, std::string(),
+                 net::URLRequest::NEVER_CLEAR_REFERRER,
+                 net::LOAD_DO_NOT_SEND_COOKIES |
+                 net::LOAD_DO_NOT_SAVE_COOKIES |
+                 net::LOAD_MAYBE_USER_GESTURE);
+}
+
+void CredentialsItemView::AvatarFetcher::OnFetchComplete(
+    const GURL /*url*/, const SkBitmap* bitmap) {
+  if (bitmap && delegate_)
+    delegate_->UpdateAvatar(gfx::Image::CreateFrom1xBitmap(*bitmap));

[Mike West, 2015/01/20 17:48:55]

We'll probably need to do some validation here (cropping/scaling) to ensure that
the image we get maps to the size we have available.

This is also a TODO for me to add something to the spec about image sizes.

[vasilii, 2015/01/20 19:43:03]

Currently ImageView resizes it for us. Though the quality is bad.
What should we do with rectangular images?

+
+  delete this;
+}
+
 CredentialsItemView::CredentialsItemView(views::ButtonListener* button_listener,
-                                         const autofill::PasswordForm& form)
+                                         const autofill::PasswordForm& form,
+                                         Profile* profile)
     : LabelButton(button_listener, base::string16()),
-      form_(form) {
+      form_(form),
+      weak_ptr_factory_(this) {
   set_notify_enter_exit_on_child(true);
   // Create an image-view for the avatar. Make sure it ignores events so that
   // the parent can receive the events instead.
   image_view_ = new CircularImageView;
   image_view_->set_interactive(false);
-
-  // TODO(vasilii): temporary code below shows the built-in profile icon instead
-  // of avatar.
-  const ProfileInfoCache& cache =
-      g_browser_process->profile_manager()->GetProfileInfoCache();
-  const gfx::Image& image = cache.GetAvatarIconOfProfileAtIndex(0);
-  image_view_->SetImage(profiles::GetSizedAvatarIcon(
-      image, true, kIconSize, kIconSize).ToImageSkia());
+  gfx::Image image =
+      ResourceBundle::GetSharedInstance().GetImageNamed(IDR_PROFILE_AVATAR_26);

[Mike West, 2015/01/20 17:48:55]

This isn't the same image you were displaying before. I assume that's
intentional?

[vasilii, 2015/01/20 19:43:03]

It's profile_avatar_placeholder.png. Have a look at it
chrome://theme/IDR_PROFILE_AVATAR_26.
It's big enough for the size of the dialog.

+  image_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));

[Mike West, 2015/01/20 17:48:55]

Why do you need to set the image size here, when you didn't in the previous
code? Why don't you need to set the size in ::UpdateAvatar?

Note: I know nothing at all about Skia. :)

[vasilii, 2015/01/20 19:43:03]

With this new line ImageView resizes the image automatically on paint.

+  image_view_->SetImage(image.ToImageSkia());
+  if (form_.avatar_url.is_valid()) {
+    // Fetch the actual avatar.
+    AvatarFetcher* fetcher = new AvatarFetcher(form_.avatar_url,
+                                               weak_ptr_factory_.GetWeakPtr());
+    fetcher->Start(profile->GetRequestContext());
+  }
   AddChildView(image_view_);
 
   // Add a label to show the full name.
@@ -135,3 +185,7 @@ void CredentialsItemView::Layout() {
     username_label_->SetBoundsRect(gfx::Rect(label_origin, username_size));
   }
 }
+
+void CredentialsItemView::UpdateAvatar(const gfx::Image& image) {
+  image_view_->SetImage(image.ToImageSkia());

[Mike West, 2015/01/20 17:48:55]

This flow means that we'll be requesting the image when creating the ItemView
and swapping it in over the default image once the network request resolves,
right?

I think that's fine as a first pass at the problem, but I would like to see a
solution that caches the images so that a) we don't generate network requests
every time we pop open a bubble, and b) don't flash a default image that's then
overridden with a "real" image after some period of time.

I'd imagined a background service that would grab the images and stick them into
a cache when we save the credential, or when the credential is updated (from
sync or via the API); we know the URL at that point, we should be able to grab
it once, persist it as part of the user's local profile, and use it directly
rather than going out to the network.

[vasilii, 2015/01/20 19:43:03]

net::URLFetcher (used by chrome::BitmapFetcher) is smart enough to check the
cache first. With this behavior do you still have concerns?

+}