[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/password_form_manager.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/metrics/histogram.h"
@@ skipping 358 matching lines @@
     PasswordStore::AuthorizationPromptPolicy prompt_policy) {
   DCHECK_EQ(state_, PRE_MATCHING_PHASE);
   state_ = MATCHING_PHASE;
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_FETCH_LOGINS_METHOD);
   }
 
-  // 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()) {
-    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)
       logger->LogMessage(Logger::STRING_NO_STORE);
     NOTREACHED();
     return;
   }
   password_store->GetLogins(observed_form_, prompt_policy, this);
 }
 
 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;
+bool PasswordFormManager::IsIgnorableChangePasswordForm(
+    const autofill::PasswordForm& candidate) const {
+  bool is_change_password_form = !candidate.new_password_element.empty() &&
+                                 !candidate.password_element.empty();
+  return is_change_password_form && !candidate.username_marked_by_site &&
+         !HasMatchingCredentialsInStore(candidate);
 }
 
 void PasswordFormManager::OnRequestDone(
     const std::vector<PasswordForm*>& logins_result) {
   // Note that the result gets deleted after this call completes, but we own
   // the PasswordForm objects pointed to by the result vector, thus we keep
   // copies to a minimum here.
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
@@ skipping 123 matching lines @@
   // (1) we are in Incognito mode, (2) the ACTION paths don't match,
   // or (3) if it matched using public suffix domain matching.
   bool wait_for_username = client_->IsOffTheRecord() ||
                            observed_form_.action.GetWithEmptyPath() !=
                                preferred_match_->action.GetWithEmptyPath() ||
                            preferred_match_->IsPublicSuffixMatch();
   if (wait_for_username)
     manager_action_ = kManagerActionNone;
   else
     manager_action_ = kManagerActionAutofilled;
+
+  // Do not autofill on sign-up or change password forms (until we have some

[vabr (Chromium), 2015/02/10 18:54:56]

Could you move the whole added block above line 530, which says "Proceed to
autofill"?

[Pritam Nikam, 2015/02/19 11:18:47]

Done.

+  // working change password functionality).
+  if (!observed_form_.new_password_element.empty()) {
+    scoped_ptr<BrowserSavePasswordProgressLogger> logger;

[vabr (Chromium), 2015/02/10 18:54:56]

You only need the |logger| in the if-block below, so don't use scoped_ptr,
create it simply as a stack variable.
Also, please add a log message identifying the current method (see the *_METHOD
constants in components/autofill/core/common/save_password_progress_logger.cc
for the pattern to use). In other words, please do this:
if (...IsLoggingActive...) {
  BrowserSavePasswordProgressLogger logger(client_);
  logger.LogMessage(Logger::PROCESS_FRAME_METHOD);
  logger.LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED);
}

[Pritam Nikam, 2015/02/19 11:18:47]

Done.

+    if (client_->IsLoggingActive()) {
+      logger.reset(new BrowserSavePasswordProgressLogger(client_));
+      logger->LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED);
+    }
+    client_->AutofillResultsComputed();

[vabr (Chromium), 2015/02/10 18:54:57]

Remove this line. AutofillResultsComputed has already been called when the
password store results were received.

[Pritam Nikam, 2015/02/19 11:18:47]

Done.

+    return;
+  }
+
   password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
                               *preferred_match_, wait_for_username);
 }
 
 void PasswordFormManager::OnGetPasswordStoreResults(
     const std::vector<autofill::PasswordForm*>& results) {
   DCHECK_EQ(state_, MATCHING_PHASE);
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
@@ skipping 297 matching lines @@
   if (has_generated_password_)
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMITTED);
 }
 
 void PasswordFormManager::SubmitFailed() {
   submit_result_ = kSubmitResultFailed;
   if (has_generated_password_)
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMISSION_FAILED);
 }
 
+bool PasswordFormManager::HasMatchingCredentialsInStore(
+    const autofill::PasswordForm& candidate) const {
+  for (auto match : best_matches_) {
+    if (match.second->username_value == candidate.username_value &&
+        match.second->password_value == candidate.password_value)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace password_manager