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

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

BUG=400674
Committed: https://crrev.com/cbb747e50de6b4a16e37dab2cae8f87a2da026bd
Cr-Commit-Position: refs/heads/master@{#312597}

[vasilii, 2015-01-20 17:12:32]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org

[vasilii, 2015-01-20 17:13:13]

Hi Mike,

please review.

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

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:66: void
Start(net::URLRequestContextGetter* request_context);
Nit: Newline after.

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:100:
delegate_->UpdateAvatar(gfx::Image::CreateFrom1xBitmap(*bitmap));
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.

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:117:
ResourceBundle::GetSharedInstance().GetImageNamed(IDR_PROFILE_AVATAR_26);
This isn't the same image you were displaying before. I assume that's
intentional?

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:118:
image_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));
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. :)

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:190:
image_view_->SetImage(image.ToImageSkia());
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.

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:213: new
CredentialsItemView(this, *form, parent_->model()->GetProfile());
It looks like you only use the Profile to get the ResourceContext. Perhaps you
could pass that in instead? Or will you need the Profile for something in the
future?

[Mike West, 2015-01-20 17:55:10]

(Also no tests; but I figured this was just a first pass sort of thing :) )

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

What tests do you want to see?

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:66: void
Start(net::URLRequestContextGetter* request_context);
On 2015/01/20 17:48:55, Mike West wrote:
> Nit: Newline after.

Done.

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:100:
delegate_->UpdateAvatar(gfx::Image::CreateFrom1xBitmap(*bitmap));
On 2015/01/20 17:48:55, Mike West wrote:
> 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.

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

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:117:
ResourceBundle::GetSharedInstance().GetImageNamed(IDR_PROFILE_AVATAR_26);
On 2015/01/20 17:48:55, Mike West wrote:
> This isn't the same image you were displaying before. I assume that's
> intentional?

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.

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:118:
image_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));
On 2015/01/20 17:48:55, Mike West wrote:
> 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. :)

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

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:190:
image_view_->SetImage(image.ToImageSkia());
On 2015/01/20 17:48:55, Mike West wrote:
> 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.

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

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/854173003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:213: new
CredentialsItemView(this, *form, parent_->model()->GetProfile());
On 2015/01/20 17:48:55, Mike West wrote:
> It looks like you only use the Profile to get the ResourceContext. Perhaps you
> could pass that in instead? Or will you need the Profile for something in the
> future?

Done.

[vasilii, 2015-01-21 16:36:43]

I'm resizing the avatars now.

[Mike West, 2015-01-21 16:51:41]

As we discussed, this seems like a fine first-pass. I think it would be _better_
to have a service off to the side which fetched and cached images without
waiting for a bubble to kick off the process, but it's a good-enough solution
for the moment. We're certainly going to have to look at resizing vs cropping vs
mandating sizes in the future, for example, but whatever. First walk, then run.

On 2015/01/20 19:43:03, vasilii wrote:
> What tests do you want to see?

Regarding tests, the usual: we should ensure that creating a CredentialItemView
triggers requests for the relevant avatar images if there's an avatarURL, and
doesn't if there isn't. One way to do that would be to expose AvatarFetcher as a
public thing, subclass it for tests, and inject it when creating the ItemView.
Another might be to expose an error callback on CredentialItemView, and verify
that either the error callback or ::UpdateAvatar is called after creation.

[Mike West, 2015-01-21 16:51:52]

https://codereview.chromium.org/854173003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/854173003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/credentials_item_view.cc:35: gfx::ImageSkia
skia_image = image.AsImageSkia();
Can we shortcut this by checking whether the image happens to magically already
be the right size? That would save an image creation.

[vasilii, 2015-01-21 19:41:23]

Added a test.

https://codereview.chromium.org/854173003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/854173003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/credentials_item_view.cc:35: gfx::ImageSkia
skia_image = image.AsImageSkia();
On 2015/01/21 16:51:52, Mike West wrote:
> Can we shortcut this by checking whether the image happens to magically
already
> be the right size? That would save an image creation.

I've rewritten the code so we use more gfx::ImageSkia when possible. I believe
ImageSkiaOperations::CreateResizedImage() is already optimized for this case.

[Mike West, 2015-01-22 06:59:13]

LGTM, thanks!

[vasilii, 2015-01-22 09:12:34]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-01-22 09:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/854173003/60001

[commit-bot: I haz the power, 2015-01-22 09:56:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-22 09:58:15]

Patchset 4 (id:??) landed as
https://crrev.com/cbb747e50de6b4a16e37dab2cae8f87a2da026bd
Cr-Commit-Position: refs/heads/master@{#312597}