[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 096257b8d1a3114a819949087cb7350fd5c5c466..4f6e232066a258e32f619b7977716c6b6c18974c 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -232,7 +232,7 @@ bool PasswordFormManager::HasValidPasswordForm() const {
          !observed_form_.new_password_element.empty();
 }
 
-void PasswordFormManager::ProvisionallySave(
+bool PasswordFormManager::ProvisionallySave(
     const PasswordForm& credentials,
     OtherPossibleUsernamesAction action) {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
@@ -303,6 +303,13 @@ void PasswordFormManager::ProvisionallySave(
       is_new_login_ = false;
       if (password_changed)
         user_action_ = kUserActionOverridePassword;
+
+      // For ignorable change-password form, bail out early if usernames are

[vabr (Chromium), 2015/02/06 16:27:07]

Why should change password forms not be ignored in the presence of PSL matches?

[Pritam Nikam, 2015/02/09 15:48:17]

Done.

+      // matching but passwords are not matching.
+      if (IsIgnorableChangePasswordForm() &&
+          pending_credentials_.password_value != credentials.password_value) {
+        return false;
+      }
     }
   } else if (action == ALLOW_OTHER_POSSIBLE_USERNAMES &&
              UpdatePendingCredentialsIfOtherPossibleUsername(
@@ -315,6 +322,11 @@ void PasswordFormManager::ProvisionallySave(
     is_new_login_ = false;
   } else {
     // User typed in a new, unknown username.
+    // For ignorable change-password form, bail out early if usernames are not
+    // matching.
+    if (IsIgnorableChangePasswordForm())
+      return false;
+
     user_action_ = kUserActionOverrideUsernameAndPassword;
     pending_credentials_ = observed_form_;
     pending_credentials_.username_value = credentials.username_value;
@@ -353,6 +365,8 @@ void PasswordFormManager::ProvisionallySave(
 
   if (has_generated_password_)
     pending_credentials_.type = PasswordForm::TYPE_GENERATED;
+
+  return true;
 }
 
 void PasswordFormManager::Save() {
@@ -376,19 +390,6 @@ void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
     logger->LogMessage(Logger::STRING_FETCH_LOGINS_METHOD);
   }
 
-  // Do not autofill on sign-up or change password forms (until we have some

[vabr (Chromium), 2015/02/06 16:27:07]

The goal of http://crbug.com/448351 is not at all to support autofilling on
sign-up and change password forms. Revert this deletion.

[Pritam Nikam, 2015/02/09 15:48:17]

In my opinion, this is needed to fetch stored forms and identify genuine
ignorable change-password form cases. We can skip autofilling for all
change-password form in later part.

-  // working change password functionality).
-  if (!observed_form_.new_password_element.empty()) {
-    if (logger)
-      logger->LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED);
-    client_->AutofillResultsComputed();
-    // There is no point in looking for the credentials in the store when they
-    // won't be autofilled, so pretend there were none.
-    std::vector<autofill::PasswordForm*> dummy_results;
-    OnGetPasswordStoreResults(dummy_results);
-    return;
-  }
-
   PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     if (logger)
@@ -403,13 +404,6 @@ bool PasswordFormManager::HasCompletedMatching() const {
   return state_ == POST_MATCHING_PHASE;
 }
 
-bool PasswordFormManager::IsIgnorableChangePasswordForm() const {
-  bool is_change_password_form = !observed_form_.new_password_element.empty() &&
-                                 !observed_form_.password_element.empty();
-  bool is_username_certainly_correct = observed_form_.username_marked_by_site;
-  return is_change_password_form && !is_username_certainly_correct;
-}
-
 void PasswordFormManager::OnRequestDone(
     const std::vector<PasswordForm*>& logins_result) {
   // Note that the result gets deleted after this call completes, but we own
@@ -551,8 +545,13 @@ void PasswordFormManager::ProcessFrame(
     manager_action_ = kManagerActionNone;
   else
     manager_action_ = kManagerActionAutofilled;
-  password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
-                              *preferred_match_, wait_for_username);
+
+  // Do not autofill on sign-up or change password forms (until we have some
+  // working change password functionality).
+  if (observed_form_.new_password_element.empty()) {

[vabr (Chromium), 2015/02/06 16:27:07]

What's the advantage of moving the check to so late?

[Pritam Nikam, 2015/02/09 15:48:17]

We can skip autofilling for all change-password form here.

+    password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
+                                *preferred_match_, wait_for_username);
+  }
 }
 
 void PasswordFormManager::OnGetPasswordStoreResults(
@@ -868,4 +867,11 @@ void PasswordFormManager::SubmitFailed() {
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMISSION_FAILED);
 }
 
+bool PasswordFormManager::IsIgnorableChangePasswordForm() const {
+  bool is_change_password_form = !observed_form_.new_password_element.empty() &&
+                                 !observed_form_.password_element.empty();
+  bool is_username_certainly_correct = observed_form_.username_marked_by_site;
+  return is_change_password_form && !is_username_certainly_correct;
+}
+
 }  // namespace password_manager