New credential chooser bubble view.

New credential chooser bubble view.

The mock is https://docs.google.com/a/google.com/presentation/d/1MXib5nh5RFcFU5lraK3EBJmU4fr92qiSh4kK4Xbr7OM/edit#slide=id.g4dd87e181_396 The bubble is supposed to be triggered by the Credential Managment javascript API.
The new classes aren't instantiated yet. They use fake data.

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

[vasilii, 2014-11-12 16:16:03]

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

[vasilii, 2014-11-12 16:18:50]

Hi Mike,

please review the CL. The new class isn't instantiated yet.

[Mike West, 2014-11-13 10:06:27]

Looking good as a first pass, thanks! A few comments.

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:25: const gfx::Image&
image = cache.GetAvatarIconOfProfileAtIndex(0);
Can you add some comment to the effect that this is totally temporary and just
stubs out the Avatar functionality by grabbing the profile image?

Even better would be to grab the avatar data somewhere else and pass it into
this method, as that's what you're going to be doing in the future.

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:179: // and a
rejection combobox. -------------------------------------
Nit: drop the ----- from this line.

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:214:
l10n_util::GetStringFUTF16Int(IDS_NUMBERED_PROFILE_NAME, i);
This doesn't seem robust. Is this going to crash if a user only has one profile?

More generally, I'm fine with using the profiles as a placeholder for data, but
please:

1. Move the enumeration of profiles to a method on either the
ManagePasswordsBubbleView or the Model. This view shouldn't be generating it's
own data, even as a stub.

2. Add comments so that it's clear that you're just stubbing out the
functionality.

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:218:
layout->AddView(credential_view);
Where do you plan to handle clicks on individual credentials? Will the subviews
bubble clicks up to this view, or will you handle everything in the subview?

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:56: class
AccountChooserView;
We might be getting to the point where splitting these out into separate files
makes sense, just from a code-manageability perspective.

(Not in this CL, of course)

[Mike West, 2014-11-13 10:11:07]

(Also, please update the CL description with a little more detail about what
you're doing (possibly with a link to the UI section of the spec), and the fact
that it's just a stub that isn't yet wired up to anything)

[vasilii, 2014-11-13 12:00:19]

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/credentials_item_view.cc:25: const gfx::Image&
image = cache.GetAvatarIconOfProfileAtIndex(0);
On 2014/11/13 10:06:26, Mike West wrote:
> Can you add some comment to the effect that this is totally temporary and just
> stubs out the Avatar functionality by grabbing the profile image?
> 
> Even better would be to grab the avatar data somewhere else and pass it into
> this method, as that's what you're going to be doing in the future.

I'm not sure what happens in the future. CredentialsItemView may get "const
autofill::PasswordForm&" as ManagePasswordItemView does and load everything
itself.

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:179: // and a
rejection combobox. -------------------------------------
On 2014/11/13 10:06:26, Mike West wrote:
> Nit: drop the ----- from this line.

The whole comment was pure copy-paste.

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:214:
l10n_util::GetStringFUTF16Int(IDS_NUMBERED_PROFILE_NAME, i);
On 2014/11/13 10:06:26, Mike West wrote:
> This doesn't seem robust. Is this going to crash if a user only has one
profile?
> 
> More generally, I'm fine with using the profiles as a placeholder for data,
but
> please:
> 
> 1. Move the enumeration of profiles to a method on either the
> ManagePasswordsBubbleView or the Model. This view shouldn't be generating it's
> own data, even as a stub.
> 
> 2. Add comments so that it's clear that you're just stubbing out the
> functionality.

It just reads the string "User $1" from the resource and changes the parameter
to 0 or 1. It has nothing to do with the actual profiles.

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:218:
layout->AddView(credential_view);
On 2014/11/13 10:06:26, Mike West wrote:
> Where do you plan to handle clicks on individual credentials? Will the
subviews
> bubble clicks up to this view, or will you handle everything in the subview?

That's a good question. For consistency with other classes I'll handle them in
AccountChooserView. I don't know if it's the best strategy.

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

https://codereview.chromium.org/720573003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:56: class
AccountChooserView;
On 2014/11/13 10:06:26, Mike West wrote:
> We might be getting to the point where splitting these out into separate files
> makes sense, just from a code-manageability perspective.
> 
> (Not in this CL, of course)

Maybe. Though I don't expect AccountChooserView to grow by the final version.

[vasilii, 2014-11-14 10:00:23]

ping

[Mike West, 2014-11-14 10:15:15]

LGTM, looking forward to the next patches!

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

https://codereview.chromium.org/720573003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/credentials_item_view.cc:23: // Temporary code
below shows the built-in profile icon instead of avatar.
Nit: TODO(vasilii)

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

https://codereview.chromium.org/720573003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:212: // This
is a stub instead of actual data. We temporary show 2 credentials.
Nit: TODO

[vasilii, 2014-11-14 11:01:54]

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

https://codereview.chromium.org/720573003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/credentials_item_view.cc:23: // Temporary code
below shows the built-in profile icon instead of avatar.
On 2014/11/14 10:15:14, Mike West wrote:
> Nit: TODO(vasilii)

Done.

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

https://codereview.chromium.org/720573003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:212: // This
is a stub instead of actual data. We temporary show 2 credentials.
On 2014/11/14 10:15:14, Mike West wrote:
> Nit: TODO

Done.

[vasilii, 2014-11-14 11:03:01]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-11-14 11:03:08]

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

[commit-bot: I haz the power, 2014-11-14 11:53:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-14 11:54:00]

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