[Password Manager] Fix password saving on Macys registration page

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 0ed1987d18093b7dab2cc0535f72a1107729d4b9..7d20d9ad3894b770bd8f1710ad7c94f8c4276f20 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -107,6 +107,25 @@ void RecordWhetherTargetDomainDiffers(const GURL& src, const GURL& target) {
                         target_domain_differs);
 }
 
+// Returns true if the observed form has both the current and new password
+// fields, and the username field was not explicitly marked with
+// autocomplete=username. In these cases it is not clear whether the username
+// field is the right guess (often such change password forms do not contain
+// the username at all), and the user should not be bothered with saving a
+// potentially malformed credential. Once we handle change password forms
+// correctly, or http://crbug.com/448351 gets implemented, this method should
+// be replaced accordingly.
+bool IsIgnorableChangePasswordForm(const PasswordForm& form) {

[vabr (Chromium), 2015/02/25 10:21:15]

Heads-up: Your base files are out of date. There has been
https://codereview.chromium.org/870513002 in the mean time, which introduced
checking the saved form credentials against the content of the password store.
Please make sure to not break what that fixed.

[Garrett Casto, 2015/02/26 07:11:01]

Rebased and moved this back into PasswordFormManager to incorporate both
changes.

+  bool is_change_password_form =
+      !form.new_password_element.empty() && !form.password_element.empty();
+  bool is_username_certainly_correct = form.username_marked_by_site;
+  return is_change_password_form && !is_username_certainly_correct;
+}
+
+bool IsSignupForm(const PasswordForm& form) {
+  return !form.new_password_element.empty() && form.password_element.empty();
+}
+
 }  // namespace
 
 const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
@@ -221,6 +240,11 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
     return;
   }
 
+  if (IsIgnorableChangePasswordForm(form)) {
+    RecordFailure(CHANGE_PASSWORD_FORM, form.origin, logger.get());
+    return;
+  }
+
   scoped_ptr<PasswordFormManager> manager;
   ScopedVector<PasswordFormManager>::iterator matched_manager_it =
       pending_login_managers_.end();
@@ -236,12 +260,6 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
     if (result == PasswordFormManager::RESULT_NO_MATCH)
       continue;
 
-    if ((*iter)->IsIgnorableChangePasswordForm()) {
-      if (logger)
-        logger->LogMessage(Logger::STRING_CHANGE_PASSWORD_FORM);
-      continue;
-    }
-
     if (!(*iter)->HasCompletedMatching()) {
       has_found_matching_managers_which_were_not_ready = true;
       continue;
@@ -263,6 +281,18 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
       if (logger)
         logger->LogMessage(Logger::STRING_MATCH_WITHOUT_ACTION);
       matched_manager_it = iter;
+    } else if (IsSignupForm(form) &&
+               (result & PasswordFormManager::RESULT_ORIGINS_MATCH)) {
+      // Signup forms don't require HTML attributes to match because we don't
+      // need to fill these saved passwords on the same form in the future.
+      // Don't break in case there exists a better match.
+      // TODO(gcasto): Matching in this way is very inprecise and will take

[vabr (Chromium), 2015/02/25 10:21:15]

typo: imprecise

[Garrett Casto, 2015/02/26 07:11:01]

Done.

+      // the last form that exists on the page. Having some better way to
+      // match the same form when the HTML elements change (e.g. text element
+      // changed to password element) would be useful.
+      if (logger)
+        logger->LogMessage(Logger::STRING_ORIGINS_MATCH);
+      matched_manager_it = iter;

[vabr (Chromium), 2015/02/25 10:21:15]

Should we at least give precedence to matches coming from line 283? Something
like storing the last result, and if matched_manager_it is assigned, and the
last result better than just RESULT_ORIGINS_MATCH, we would not execute the
current line 295?

[Garrett Casto, 2015/02/26 07:11:01]

That's a good idea. The change I made also will prefer action + origin matches
to just origin matches.

     }
   }
   // If we didn't find a manager, this means a form was submitted without
@@ -362,6 +392,9 @@ void PasswordManager::RecordFailure(ProvisionalSaveFailure failure,
       case EMPTY_PASSWORD:
         logger->LogMessage(Logger::STRING_EMPTY_PASSWORD);
         break;
+      case CHANGE_PASSWORD_FORM:
+        logger->LogMessage(Logger::STRING_CHANGE_PASSWORD_FORM);
+        break;
       case MATCHING_NOT_COMPLETE:
         logger->LogMessage(Logger::STRING_MATCHING_NOT_COMPLETE);
         break;