Password bubble: Give users the option of unblacklisting a site.

Password bubble: Give users the option of unblacklisting a site.

This CL adds a new view to the bubble, to be displayed to the user when
she clicks on the icon for a site where she's previously elected to
"Never save passwords". From this view, she may elect to unblacklist the
site, enabling passwords to be saved going forward.

To do so, a PasswordFormMap is piped through the password manager client
when sending a BlacklistBlockedAutofill notifiation. That contains
the information we'll need to pass into the PasswordStore to remove the
blacklist.

BUG=357597
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268202

[Mike West, 2014-04-29 12:04:37]

Hi Vaclav, Markus! This patch (based on
https://codereview.chromium.org/255903006/) attempts to add the ability to
unblacklist sites. It needs a bit of cleanup and more extensive tests, but I
hope you can take some time to skim the CL now to verify that I'm on the right
path while I clean it up on the side.

Thanks!

-mike

[vabr (Chromium), 2014-04-29 13:30:12]

The current state LGTM with comments.

Cheers,
Vaclav

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:148: //
TODO(mkwst): Hrm.
Please elaborate on "Hrm.", so that if you suddenly win a billion Euro and stop
working, the rest of the team can still complete your code up to the point of
perfection. :)

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:38: void
OnPasswordAutofilled(const autofill::PasswordFormMap& password_form_map);
nit: Match the argument name (|map|) with the comment above.

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:44: const
autofill::PasswordFormMap& password_form_map);
ditto (argument name vs. comment)

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:163:
EXPECT_EQ(ManagePasswordsIcon::BLACKLISTED_STATE, mock.state());
I read the test as "after unblacklisting, the icon should indicate a blacklisted
site".
Do I read it wrong, or is that a mistake?

https://codereview.chromium.org/259153004/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/259153004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:40: const
autofill::PasswordFormMap& best_matches) const {}
nit: Please comment on the role of |best_matches|.

[Mike West, 2014-04-30 10:39:31]

Thanks Vaclav! I've cleaned this up a bit. If you'd like to take another look,
I'd appreciate it.

Ilya, would you mind taking a look at the tools/metrics change?

Thanks!

-mike

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc:148: //
TODO(mkwst): Hrm.
On 2014/04/29 13:30:12, vabr (Chromium) wrote:
> Please elaborate on "Hrm.", so that if you suddenly win a billion Euro and
stop
> working, the rest of the team can still complete your code up to the point of
> perfection. :)

That's a _much_ happier scenario than the typical "hit by a bus" concerns that I
hear about!

The "Hrm" was a placeholder while I figured out what I needed here. The
implementation here seems pretty reasonable, so I'm less hrmmy about it. :)

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:38: void
OnPasswordAutofilled(const autofill::PasswordFormMap& password_form_map);
On 2014/04/29 13:30:12, vabr (Chromium) wrote:
> nit: Match the argument name (|map|) with the comment above.

Done.

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:44: const
autofill::PasswordFormMap& password_form_map);
On 2014/04/29 13:30:12, vabr (Chromium) wrote:
> ditto (argument name vs. comment)

Done.

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
File
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/259153004/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc:163:
EXPECT_EQ(ManagePasswordsIcon::BLACKLISTED_STATE, mock.state());
On 2014/04/29 13:30:12, vabr (Chromium) wrote:
> I read the test as "after unblacklisting, the icon should indicate a
blacklisted
> site".
> Do I read it wrong, or is that a mistake?

No, I think this is a bug. Hooray for bugs! I'll fix.

https://codereview.chromium.org/259153004/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/259153004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:40: const
autofill::PasswordFormMap& best_matches) const {}
On 2014/04/29 13:30:12, vabr (Chromium) wrote:
> nit: Please comment on the role of |best_matches|.

Done.

[Mike West, 2014-04-30 10:40:39]

Note that this is based on https://codereview.chromium.org/255903006/ which is
based on https://codereview.chromium.org/254263002/, so I can't throw it to the
bots yet. Trust me, it compiles. ;)

[vabr (Chromium), 2014-04-30 11:08:46]

Hi Mike.

I'm glad you like being hit by luck more than being hit by a bus. :)

I like your code, LGTM.

Some unimportant nits below.

Cheers,
Vaclav

https://codereview.chromium.org/259153004/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/259153004/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:53: 
nit: Please reduce to a single blank line.
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...)

https://codereview.chromium.org/259153004/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:75: 
nit: Please remove this blank line.

https://codereview.chromium.org/259153004/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h
(right):

https://codereview.chromium.org/259153004/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h:102: // All
previously stored credentials for a specific site.  Set by
nit: Both 2 spaces and 1 space seem fine to separate sentences, but please be
consistent and stick with your choice.

[Ilya Sherman, 2014-05-01 00:54:53]

histograms.xml changes lgtm

[Mike West, 2014-05-05 15:01:12]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-05-05 15:01:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/259153004/60001

[commit-bot: I haz the power, 2014-05-05 16:52:14]

Change committed as 268202