Don't show "Save password" prompt for a failed login

components/password_manager/core/browser/password_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_manager.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 322 matching lines @@
 }
 
 bool PasswordManager::ShouldPromptUserToSavePassword() const {
   return !client_->IsAutomaticPasswordSavingEnabled() &&
          provisional_save_manager_->IsNewLogin() &&
          !provisional_save_manager_->HasGeneratedPassword() &&
          !provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch();
 }
 
 void PasswordManager::OnPasswordFormsRendered(
-    const std::vector<PasswordForm>& visible_forms) {
+    const std::vector<PasswordForm>& visible_forms,
+    bool did_stop_loading) {
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_ON_PASSWORD_FORMS_RENDERED_METHOD);
   }
 
   if (!provisional_save_manager_.get()) {
     if (logger) {
       logger->LogMessage(Logger::STRING_NO_PROVISIONAL_SAVE_MANAGER);
       logger->LogMessage(Logger::STRING_DECISION_DROP);
     }
     return;
   }
 
   DCHECK(IsSavingEnabled());
 
   if (logger) {
     logger->LogNumber(Logger::STRING_NUMBER_OF_VISIBLE_FORMS,
                       visible_forms.size());
   }
 
+  // Stores all visible forms from the frame.

[vabr (Chromium), 2014/06/11 08:22:12]

optional nit: Stores -> Store
We usually use imperative to describe what the coming line does, see, e.g., the
"Clear..." comments below.

[vabr (Chromium), 2014/06/11 08:22:12]

optional nit: It's up to you if you say "record" or "store", but please be
consistent. Currently you say "record" in the header, and "store" in the .cc
file. That's confusing, as it sounds like you distinguish between them on
purpose, because they are two different things.

+  for (size_t i = 0; i < visible_forms.size(); ++i)
+    all_visible_forms_.insert(all_visible_forms_.end(), visible_forms[i]);

[vabr (Chromium), 2014/06/11 08:22:12]

No, this is not what I meant. :)
I meant: "Use insert to spare yourself the for-cycle", in other words this:

all_visible_forms_.insert(all_visible_forms_.end(),
                          visible_forms.begin(),
                          visible_forms.end());

+
   // If we see the login form again, then the login failed.
-  for (size_t i = 0; i < visible_forms.size(); ++i) {
-    // TODO(vabr): The similarity check is just action equality for now. If it
-    // becomes more complex, it may make sense to consider modifying and using
-    // PasswordFormManager::DoesManage for it.
-    if (visible_forms[i].action.is_valid() &&
-        provisional_save_manager_->pending_credentials().action ==
-            visible_forms[i].action) {
-      if (logger) {
-        logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
-                                visible_forms[i]);
-        logger->LogMessage(Logger::STRING_DECISION_DROP);
+  if (did_stop_loading) {
+    for (size_t i = 0; i < all_visible_forms_.size(); ++i) {
+      // TODO(vabr): The similarity check is just action equality for now. If it
+      // becomes more complex, it may make sense to consider modifying and using
+      // PasswordFormManager::DoesManage for it.
+      if (all_visible_forms_[i].action.is_valid() &&
+          provisional_save_manager_->pending_credentials().action ==
+              all_visible_forms_[i].action) {
+        if (logger) {
+          logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
+                                  visible_forms[i]);
+          logger->LogMessage(Logger::STRING_DECISION_DROP);
+        }
+        provisional_save_manager_->SubmitFailed();
+        provisional_save_manager_.reset();
+        // Clear all_visible_forms_ once we found the match.
+        all_visible_forms_.clear();
+        return;
       }
-      provisional_save_manager_->SubmitFailed();
+    }
+
+    // Clear all_visible_forms_ after checking all the visible forms.
+    all_visible_forms_.clear();
+
+    // Looks like a successful login attempt. Either show an infobar or
+    // automatically save the login data. We prompt when the user hasn't
+    // already given consent, either through previously accepting the infobar
+    // or by having the browser generate the password.
+    provisional_save_manager_->SubmitPassed();
+
+    if (ShouldPromptUserToSavePassword()) {
+      if (logger)
+        logger->LogMessage(Logger::STRING_DECISION_ASK);
+      client_->PromptUserToSavePassword(provisional_save_manager_.release());
+    } else {
+      if (logger)
+        logger->LogMessage(Logger::STRING_DECISION_SAVE);
+      provisional_save_manager_->Save();
       provisional_save_manager_.reset();
-      return;
     }
   }
-
-  // Looks like a successful login attempt. Either show an infobar or
-  // automatically save the login data. We prompt when the user hasn't already
-  // given consent, either through previously accepting the infobar or by having
-  // the browser generate the password.
-  provisional_save_manager_->SubmitPassed();
-
-  if (ShouldPromptUserToSavePassword()) {
-    if (logger)
-      logger->LogMessage(Logger::STRING_DECISION_ASK);
-    client_->PromptUserToSavePassword(provisional_save_manager_.release());
-  } else {
-    if (logger)
-      logger->LogMessage(Logger::STRING_DECISION_SAVE);
-    provisional_save_manager_->Save();
-    provisional_save_manager_.reset();
-  }
 }
 
 void PasswordManager::PossiblyInitializeUsernamesExperiment(
     const PasswordFormMap& best_matches) const {
   if (base::FieldTrialList::Find(kOtherPossibleUsernamesExperiment))
     return;
 
   bool other_possible_usernames_exist = false;
   for (autofill::PasswordFormMap::const_iterator it = best_matches.begin();
        it != best_matches.end();
@@ skipping 67 matching lines @@
           observers_,
           OnAutofillDataAvailable(preferred_match.username_value,
                                   preferred_match.password_value));
       break;
   }
 
   client_->PasswordWasAutofilled(best_matches);
 }
 
 }  // namespace password_manager