Change the password blacklisting algorithm and add 'x' in the UI.

The blacklisting forms aren't scored. They are just matched against the current form.
The password save bubble should contain 'x' as specified by the UI review.

BUG=493257
TBR=groby@chromium.org,vabr@chromium.org

Review URL: https://codereview.chromium.org/1322823002

Cr-Commit-Position: refs/heads/master@{#346663}
(cherry picked from commit 3a24dad0ff93c297cb0451621e72858138cd58b6)

Review URL: https://codereview.chromium.org/1311013011 .

Cr-Commit-Position: refs/branch-heads/2490@{#182}
Cr-Branched-From: 7790a3535f2a81a03685eca31a32cf69ae0c114f-refs/heads/master@{#344925}

8 files changed, 146 insertions, 23 deletions

diff --git a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.h b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.h
index 1aefa650..70e315f 100644
--- a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.h
+++ b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.h
@@ -22,6 +22,7 @@ class ManagePasswordsBubbleModel;
   ManagePasswordsBubbleModel* model_;  // weak
   base::scoped_nsobject<NSButton> saveButton_;
   base::scoped_nsobject<NSButton> neverButton_;
+  base::scoped_nsobject<NSButton> closeButton_;
   base::scoped_nsobject<HyperlinkTextView> titleView_;
   base::scoped_nsobject<ManagePasswordItemViewController> passwordItem_;
 }
@@ -32,6 +33,7 @@ class ManagePasswordsBubbleModel;
 @interface ManagePasswordsBubblePendingViewController (Testing)
 @property(readonly) NSButton* saveButton;
 @property(readonly) NSButton* neverButton;
+@property(readonly) NSButton* closeButton;
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_PASSWORDS_MANAGE_PASSWORDS_BUBBLE_PENDING_VIEW_CONTROLLER_H_
diff --git a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm
index 32194db..d664041 100644
--- a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm
+++ b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm
@@ -8,6 +8,7 @@
 
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/ui/chrome_style.h"
+#import "chrome/browser/ui/cocoa/hover_close_button.h"
 #import "chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.h"
 #include "chrome/browser/ui/passwords/manage_passwords_bubble_model.h"
 #include "chrome/grit/generated_resources.h"
@@ -55,11 +56,21 @@ using namespace password_manager::mac::ui;
   return YES;
 }
 
+- (base::scoped_nsobject<NSButton>)newCloseButton {
+  const int dimension = chrome_style::GetCloseButtonSize();
+  NSRect frame = NSMakeRect(0, 0, dimension, dimension);
+  base::scoped_nsobject<NSButton> button(
+      [[WebUIHoverCloseButton alloc] initWithFrame:frame]);
+  [button setAction:@selector(viewShouldDismiss)];
+  [button setTarget:delegate_];
+  return button;
+}
+
 - (void)loadView {
   base::scoped_nsobject<NSView> view([[NSView alloc] initWithFrame:NSZeroRect]);
 
   // -----------------------------------
-  // |  Title                          |
+  // |  Title                        x |
   // |  username   password            |
   // |                [Never]  [Save]  |
   // -----------------------------------
@@ -75,6 +86,10 @@ using namespace password_manager::mac::ui;
 
   // Create the elements and add them to the view.
 
+  // Close button.
+  closeButton_ = [self newCloseButton];
+  [view addSubview:closeButton_];
+
   // Title.
   titleView_.reset([[HyperlinkTextView alloc] initWithFrame:NSZeroRect]);
   NSColor* textColor = [NSColor blackColor];
@@ -106,11 +121,13 @@ using namespace password_manager::mac::ui;
   }
 
   // Force the text to wrap to fit in the bubble size.
+  int titleRightPadding =
+      2 * chrome_style::kCloseButtonPadding + NSWidth([closeButton_ frame]);
+  int titleWidth = kDesiredBubbleWidth - kFramePadding - titleRightPadding;
   [titleView_ setVerticallyResizable:YES];
-  [titleView_ setFrameSize:NSMakeSize(kDesiredBubbleWidth - 2 * kFramePadding,
-                                      MAXFLOAT)];
-  [titleView_ sizeToFit];
+  [titleView_ setFrameSize:NSMakeSize(titleWidth, MAXFLOAT)];
   [[titleView_ textContainer] setLineFragmentPadding:0];
+  [titleView_ sizeToFit];
 
   [view addSubview:titleView_];
 
@@ -141,7 +158,7 @@ using namespace password_manager::mac::ui;
 
   // Compute the bubble width using the password item.
   const CGFloat contentWidth =
-      kFramePadding + NSWidth([titleView_ frame]) + kFramePadding;
+      kFramePadding + NSWidth([titleView_ frame]) + titleRightPadding;
   const CGFloat width = std::max(kDesiredBubbleWidth, contentWidth);
 
   // Layout the elements, starting at the bottom and moving up.
@@ -164,9 +181,17 @@ using namespace password_manager::mac::ui;
   // Title goes at the top after some padding.
   curY = NSMaxY([password frame]) + kUnrelatedControlVerticalPadding;
   [titleView_ setFrameOrigin:NSMakePoint(curX, curY)];
+  const CGFloat height = NSMaxY([titleView_ frame]) + kFramePadding;
+
+  // The close button is in the corner.
+  NSPoint closeButtonOrigin = NSMakePoint(
+      width - NSWidth([closeButton_ frame]) - chrome_style::kCloseButtonPadding,
+      height - NSHeight([closeButton_ frame]) -
+          chrome_style::kCloseButtonPadding);
+  [closeButton_ setFrameOrigin:closeButtonOrigin];
 
   // Update the bubble size.
-  const CGFloat height = NSMaxY([titleView_ frame]) + kFramePadding;
+
   [view setFrame:NSMakeRect(0, 0, width, height)];
 
   [self setView:view];
@@ -184,4 +209,8 @@ using namespace password_manager::mac::ui;
   return neverButton_.get();
 }
 
+- (NSButton*)closeButton {
+  return closeButton_.get();
+}
+
 @end
diff --git a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm
index 35aecfd..6ee33e9 100644
--- a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm
@@ -63,4 +63,13 @@ TEST_F(ManagePasswordsBubblePendingViewControllerTest,
   EXPECT_TRUE(ui_controller()->never_saved_password());
 }
 
+TEST_F(ManagePasswordsBubblePendingViewControllerTest,
+       ShouldDismissWhenCrossClicked) {
+  [controller().closeButton performClick:nil];
+
+  EXPECT_TRUE([delegate() dismissed]);
+  EXPECT_FALSE(ui_controller()->saved_password());
+  EXPECT_FALSE(ui_controller()->never_saved_password());
+}
+
 }  // namespace
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
index 743c754..8eeb880 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
@@ -1010,6 +1010,10 @@ void ManagePasswordsBubbleView::OnWidgetClosing(views::Widget* /*widget*/) {
     anchor_view_->SetActive(false);
 }
 
+bool ManagePasswordsBubbleView::ShouldShowCloseButton() const {
+  return model()->state() == password_manager::ui::PENDING_PASSWORD_STATE;
+}
+
 void ManagePasswordsBubbleView::Refresh() {
   RemoveAllChildViews(true);
   initially_focused_view_ = NULL;
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
index f412adb..480d490 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
@@ -76,6 +76,9 @@ class ManagePasswordsBubbleView : public ManagePasswordsBubble,
   // WidgetObserver:
   void OnWidgetClosing(views::Widget* widget) override;
 
+  // WidgetDelegate:
+  bool ShouldShowCloseButton() const override;
+
   // Refreshes the bubble's state: called to display a confirmation screen after
   // a user selects "Never for this site", for instance.
   void Refresh();
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index ba7ba88..7ba864c 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -66,6 +66,12 @@ std::vector<std::string> SplitPathToSegments(const std::string& path) {
                            base::SPLIT_WANT_ALL);
 }
 
+// Return false iff the strings are neither empty nor equal.
+bool AreStringsEqualOrEmpty(const base::string16& s1,
+                            const base::string16& s2) {
+  return s1.empty() || s2.empty() || s1 == s2;
+}
+
 }  // namespace
 
 PasswordFormManager::PasswordFormManager(
@@ -345,6 +351,18 @@ void PasswordFormManager::OnRequestDone(
   logins_result =
       client_->CreateStoreResultFilter()->FilterResults(logins_result.Pass());
 
+  // Deal with blacklisted forms.
+  auto begin_blacklisted = std::partition(
+      logins_result.begin(), logins_result.end(),
+      [](PasswordForm* form) { return !form->blacklisted_by_user; });
+  for (auto it = begin_blacklisted; it != logins_result.end(); ++it) {
+    if (IsBlacklistMatch(**it)) {
+      blacklisted_matches_.push_back(*it);
+      *it = nullptr;
+    }
+  }
+  logins_result.erase(begin_blacklisted, logins_result.end());
+
   // Now compute scores for the remaining credentials in |login_result|.
   std::vector<uint32_t> credential_scores;
   credential_scores.reserve(logins_result.size());
@@ -371,6 +389,7 @@ void PasswordFormManager::OnRequestDone(
     // Take ownership of the PasswordForm from the ScopedVector.
     scoped_ptr<PasswordForm> login(logins_result[i]);
     logins_result[i] = nullptr;
+    DCHECK(!login->blacklisted_by_user);
 
     if (credential_scores[i] < best_score) {
       // Empty path matches are most commonly imports from Firefox, and
@@ -383,7 +402,7 @@ void PasswordFormManager::OnRequestDone(
           observed_form_.scheme == PasswordForm::SCHEME_HTML &&
           base::StartsWith("/", login->origin.path(),
                            base::CompareCase::SENSITIVE) &&
-          credential_scores[i] > 0 && !login->blacklisted_by_user;
+          credential_scores[i] > 0;
       // Passwords generated on a signup form must show on a login form even if
       // there are better-matching saved credentials. TODO(gcasto): We don't
       // want to cut credentials that were saved on signup forms even if they
@@ -397,11 +416,6 @@ void PasswordFormManager::OnRequestDone(
       continue;
     }
 
-    if (login->blacklisted_by_user) {
-      blacklisted_matches_.push_back(login.Pass());
-      continue;
-    }
-
     // If there is another best-score match for the same username, replace it.
     // TODO(vabr): Spare the replacing and keep the first instead of the last
     // candidate.
@@ -436,12 +450,7 @@ void PasswordFormManager::OnRequestDone(
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsNotShown",
                        logins_result_size - best_matches_.size());
 
-  // Check to see if the user told us to ignore this site in the past.
-  if (best_matches_.empty()) {
-    DCHECK(!preferred_match_);
-    client_->PasswordAutofillWasBlocked(best_matches_);
-    manager_action_ = kManagerActionBlacklisted;
-  } else {
+  if (!best_matches_.empty()) {
     // It is possible we have at least one match but have no preferred_match_,
     // because a user may have chosen to 'Forget' the preferred match. So we
     // just pick the first one and whichever the user selects for submit will
@@ -871,6 +880,7 @@ void PasswordFormManager::CreatePendingCredentials() {
 
 uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
   DCHECK_EQ(state_, MATCHING_PHASE);
+  DCHECK(!candidate.blacklisted_by_user);
   // For scoring of candidate login data:
   // The most important element that should match is the signon_realm followed
   // by the origin, the action, the password name, the submit button name, and
@@ -931,6 +941,28 @@ uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
   return score;
 }
 
+bool PasswordFormManager::IsBlacklistMatch(
+    const autofill::PasswordForm& blacklisted_form) const {
+  DCHECK(blacklisted_form.blacklisted_by_user);
+
+  if (blacklisted_form.IsPublicSuffixMatch())
+    return false;
+  if (blacklisted_form.origin.GetOrigin() != observed_form_.origin.GetOrigin())
+    return false;
+  if (observed_form_.scheme == PasswordForm::SCHEME_HTML) {
+    if (!AreStringsEqualOrEmpty(blacklisted_form.submit_element,
+                                observed_form_.submit_element))
+      return false;
+    if (!AreStringsEqualOrEmpty(blacklisted_form.password_element,
+                                observed_form_.password_element))
+      return false;
+    if (!AreStringsEqualOrEmpty(blacklisted_form.username_element,
+                                observed_form_.username_element))
+      return false;
+  }
+  return true;
+}
+
 void PasswordFormManager::DeleteEmptyUsernameCredentials() {
   if (best_matches_.empty() || pending_credentials_.username_value.empty())
     return;
diff --git a/components/password_manager/core/browser/password_form_manager.h b/components/password_manager/core/browser/password_form_manager.h
index a96a7bb..ce5ad9b 100644
--- a/components/password_manager/core/browser/password_form_manager.h
+++ b/components/password_manager/core/browser/password_form_manager.h
@@ -294,7 +294,10 @@ class PasswordFormManager : public PasswordStoreConsumer {
 
   // Helper for OnGetPasswordStoreResults to score an individual result
   // against the observed_form_.
-  uint32_t ScoreResult(const autofill::PasswordForm& form) const;
+  uint32_t ScoreResult(const autofill::PasswordForm& candidate) const;
+
+  // For the blacklisted |form| returns true iff it blacklists |observed_form_|.
+  bool IsBlacklistMatch(const autofill::PasswordForm& form) const;
 
   // Helper for Save in the case that best_matches.size() > 0, meaning
   // we have at least one match for this form/username/password. This
@@ -375,7 +378,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // frequently require lookups by username value in IsNewLogin.
   autofill::PasswordFormMap best_matches_;
 
-  // Set of blacklisted forms from the DB that best match the current form.
+  // Set of blacklisted forms from the PasswordStore that best match the current
+  // form.
   ScopedVector<autofill::PasswordForm> blacklisted_matches_;
 
   // The PasswordForm from the page or dialog managed by |this|.
diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc
index a4e9ceb..0635f9f 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -444,6 +444,47 @@ TEST_F(PasswordFormManagerTest, TestBlacklist) {
               ElementsAre(Pointee(actual_add_form)));
 }
 
+TEST_F(PasswordFormManagerTest, TestBlacklistMatching) {
+  observed_form()->origin = GURL("http://accounts.google.com/a/LoginAuth");
+  observed_form()->action = GURL("http://accounts.google.com/a/Login");
+  observed_form()->signon_realm = "http://accounts.google.com";
+  PasswordFormManager form_manager(nullptr, client(), kNoDriver,
+                                   *observed_form(), false);
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+
+  // Doesn't match because of PSL.
+  PasswordForm blacklisted_psl = *observed_form();
+  blacklisted_psl.original_signon_realm = "http://m.accounts.google.com";
+  blacklisted_psl.blacklisted_by_user = true;
+
+  // Doesn't match because of different origin.
+  PasswordForm blacklisted_not_match = *observed_form();
+  blacklisted_not_match.origin = GURL("http://google.com/a/LoginAuth");
+  blacklisted_not_match.blacklisted_by_user = true;
+
+  // Doesn't match because of different username element.
+  PasswordForm blacklisted_not_match2 = *observed_form();
+  blacklisted_not_match2.username_element = ASCIIToUTF16("Element");
+  blacklisted_not_match2.blacklisted_by_user = true;
+
+  PasswordForm blacklisted_match = *observed_form();
+  blacklisted_match.origin = GURL("http://accounts.google.com/a/LoginAuth1234");
+  blacklisted_match.blacklisted_by_user = true;
+
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(blacklisted_psl));
+  result.push_back(new PasswordForm(blacklisted_not_match));
+  result.push_back(new PasswordForm(blacklisted_not_match2));
+  result.push_back(new PasswordForm(blacklisted_match));
+  result.push_back(new PasswordForm(*saved_match()));
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+  EXPECT_TRUE(form_manager.IsBlacklisted());
+  EXPECT_THAT(form_manager.blacklisted_matches(),
+              ElementsAre(Pointee(blacklisted_match)));
+  EXPECT_EQ(1u, form_manager.best_matches().size());
+  EXPECT_EQ(*saved_match(), *form_manager.preferred_match());
+}
+
 TEST_F(PasswordFormManagerTest, AutofillBlacklisted) {
   TestPasswordManager password_manager(client());
   PasswordFormManager form_manager(&password_manager, client(),
@@ -1001,12 +1042,11 @@ TEST_F(PasswordFormManagerTest, TestSendNotBlacklistedMessage) {
 
   // Signing up on a previously visited site. Credentials are found in the
   // password store, but they are blacklisted. AllowPasswordGenerationForForm
-  // should not be called and no "not blacklisted" message sent.
+  // is still called.
   PasswordFormManager manager_blacklisted(password_manager(), client(),
                                           client()->driver(), *observed_form(),
                                           false);
-  EXPECT_CALL(*(client()->mock_driver()), AllowPasswordGenerationForForm(_))
-      .Times(0);
+  EXPECT_CALL(*(client()->mock_driver()), AllowPasswordGenerationForForm(_));
   manager_blacklisted.SimulateFetchMatchingLoginsFromPasswordStore();
   simulated_results.push_back(CreateSavedMatch(true));
   manager_blacklisted.OnGetPasswordStoreResults(simulated_results.Pass());