Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords

components/password_manager/core/browser/password_manager.cc

Index: components/password_manager/core/browser/password_manager.cc
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
index 0ca48b3d5aba57c8d604ba4d14c71c2ae0d9acaf..74dd6ec386a0cced638b7f0e567ca7fb2842c72f 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -149,11 +149,19 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   scoped_ptr<PasswordFormManager> manager;
   ScopedVector<PasswordFormManager>::iterator matched_manager_it =
       pending_login_managers_.end();
+  // A "successful candidate" is a PasswordFormManager which DoesManage(form).
+  // We keep track of them for UMA.
+  bool successful_candidates_found = false;

[engedy, 2014/07/11 17:05:36]

nit: I would rename this to |has_found_matching_managers|, so as to prevent
introducing a new concept.

[vabr (Chromium), 2014/07/11 18:15:48]

Done.

   for (ScopedVector<PasswordFormManager>::iterator iter =
            pending_login_managers_.begin();
        iter != pending_login_managers_.end();
        ++iter) {
     PasswordFormManager::MatchResultMask result = (*iter)->DoesManage(form);
+    if (result != PasswordFormManager::RESULT_NO_MATCH) {

[engedy, 2014/07/11 17:05:36]

I wonder if it would make the control flow more readable if we swapped the two
nested conditionals (and update contents accordingly). Just an idea, I would
leave it up to you.

[vabr (Chromium), 2014/07/11 18:15:48]

Done.

+      successful_candidates_found = true;
+      if (!(*iter)->HasCompletedMatching())
+        continue;
+    }
     if (result == PasswordFormManager::RESULT_COMPLETE_MATCH) {
       // If we find a manager that exactly matches the submitted form including
       // the action URL, exit the loop.
@@ -180,20 +188,18 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
     // |manager|.
     manager.reset(*matched_manager_it);
     pending_login_managers_.weak_erase(matched_manager_it);
+  } else if (successful_candidates_found) {
+    // We found some managers, but none finished matching yet. The user has
+    // tried to submit credentials before we had time to even find matching
+    // results for the given form and autofill. If this is the case, we just
+    // give up.
+    RecordFailure(MATCHING_NOT_COMPLETE, form.origin.host(), logger.get());
+    return;
   } else {
     RecordFailure(NO_MATCHING_FORM, form.origin.host(), logger.get());
     return;
   }
 
-  // If we found a manager but it didn't finish matching yet, the user has
-  // tried to submit credentials before we had time to even find matching
-  // results for the given form and autofill. If this is the case, we just
-  // give up.
-  if (!manager->HasCompletedMatching()) {
-    RecordFailure(MATCHING_NOT_COMPLETE, form.origin.host(), logger.get());
-    return;
-  }
-
   // Also get out of here if the user told us to 'never remember' passwords for
   // this form.
   if (manager->IsBlacklisted()) {
@@ -262,7 +268,7 @@ void PasswordManager::RecordFailure(ProvisionalSaveFailure failure,
         logger->LogMessage(Logger::STRING_EMPTY_PASSWORD);
         break;
       case MATCHING_NOT_COMPLETE:
-        logger->LogMessage(Logger::STRING_NO_FORM_MANAGER);
+        logger->LogMessage(Logger::STRING_MATCHING_NOT_COMPLETE);
         break;
       case NO_MATCHING_FORM:
         logger->LogMessage(Logger::STRING_NO_MATCHING_FORM);