PasswordForm: move from current/old password scheme to current/new.

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 1ddefdebcd70ece716647816ff4d6dd0e343c4b4..c1e18a9855fe7a4f95cfea001d254bc386177e3b 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -225,7 +225,8 @@ bool PasswordFormManager::HasValidPasswordForm() {
   if (observed_form_.scheme != PasswordForm::SCHEME_HTML)
     return true;
   return !observed_form_.username_element.empty() &&
-      !observed_form_.password_element.empty();
+         (!observed_form_.password_element.empty() ||
+          !observed_form_.new_password_element.empty());
 }
 
 void PasswordFormManager::ProvisionallySave(
@@ -234,6 +235,13 @@ void PasswordFormManager::ProvisionallySave(
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials));
 
+  // If this was a sign-up or change password form, we want to persist the new
+  // password; if this was a login form, then the current password (which might
+  // be "new" in the sense that we see these credentials for the first time, or
+  // when the stored password was long out-of-date).
+  base::string16 password_to_save(credentials.new_password_element.empty() ?
+      credentials.password_value : credentials.new_password_value);
+
   // Make sure the important fields stay the same as the initially observed or
   // autofilled ones, as they may have changed if the user experienced a login
   // failure.
@@ -251,7 +259,7 @@ void PasswordFormManager::ProvisionallySave(
       user_action_ = kUserActionChoosePslMatch;
 
     // Check to see if we're using a known username but a new password.
-    if (pending_credentials_.password_value != credentials.password_value)
+    if (pending_credentials_.password_value != password_to_save)
       user_action_ = kUserActionOverridePassword;
   } else if (action == ALLOW_OTHER_POSSIBLE_USERNAMES &&
              UpdatePendingCredentialsIfOtherPossibleUsername(
@@ -269,6 +277,20 @@ void PasswordFormManager::ProvisionallySave(
     pending_credentials_.username_value = credentials.username_value;
     pending_credentials_.other_possible_usernames =
         credentials.other_possible_usernames;
+
+    // The password value will be filled in later, remove any garbage for now.
+    pending_credentials_.password_value.clear();
+    pending_credentials_.new_password_value.clear();
+
+    // If this was a sign-up or change password form, the names of the elements
+    // are likely different than those on a login form, so do not bother saving
+    // them. We will fill them with meaningful values in UpdateLogin() when the
+    // user goes onto a real login form for the first time.
+    if (!credentials.new_password_element.empty()) {
+      pending_credentials_.username_element.clear();
+      pending_credentials_.password_element.clear();
+      pending_credentials_.new_password_element.clear();
+    }
   }
 
   pending_credentials_.action = credentials.action;
@@ -278,7 +300,7 @@ void PasswordFormManager::ProvisionallySave(
   if (pending_credentials_.action.is_empty())
     pending_credentials_.action = observed_form_.action;
 
-  pending_credentials_.password_value = credentials.password_value;
+  pending_credentials_.password_value = password_to_save;
   pending_credentials_.preferred = credentials.preferred;
 
   if (user_action_ == kUserActionOverridePassword &&
@@ -451,13 +473,12 @@ void PasswordFormManager::OnGetPasswordStoreResults(
 }
 
 bool PasswordFormManager::IgnoreResult(const PasswordForm& form) const {
-  // Ignore change password forms until we have some change password
-  // functionality
-  if (observed_form_.old_password_element.length() != 0) {
+  // Ignore change passwords until we have some change password functionality.

[Garrett Casto, 2014/07/07 21:57:24]

I don't think that we need this, either earlier or now. Since we don't serialize
new_password_element it will never be non-empty after loading from the
PasswordStore. Also I think the comment is misleading. If we were to handle
change password forms better, it would be during saving not during loading.

[engedy, 2014/07/07 22:07:56]

Note that this is actually even more misleading: we are checking
|observed_form_| here, not the |form| coming from the PasswordStore.

I think the original intention was to prevent autofill when we are seeing a
change password form. I was also hesitant whether or not this is actually
needed, but did not want to change behavior unrelated to this refactoring step.

I am happy to remove this in one step if you prefer.

[Garrett Casto, 2014/07/09 22:24:18]

Ah, yeah misread that. The current check seems too restrictive in that case
given that change password forms don't necessarily need to have password_element
set. This would be a change in behavior from before though as a form with two
password fields would not autofill (since initially both fields will be empty).
I think that this is probably okay since we are moving in this direction anyway.
This probably doesn't affect that many forms anyway, since it's pretty rare to
have a username field in change password forms, and we won't autofill in any
case without one.

So I think that switching this to just
!observed_form_.new_password_element.empty() should be fine for now, and we can
work out any other changes we want to make later. 

[engedy, 2014/07/10 09:23:03]

Done.

+  if (!observed_form_.password_element.empty() &&
+      !observed_form_.new_password_element.empty()) {
     return true;
   }
-  // Don't match an invalid SSL form with one saved under secure
-  // circumstances.
+  // Don't match an invalid SSL form with one saved under secure circumstances.
   if (form.ssl_valid && !observed_form_.ssl_valid) {
     return true;
   }
@@ -581,12 +602,16 @@ void PasswordFormManager::UpdateLogin() {
     copy.origin = observed_form_.origin;
     copy.action = observed_form_.action;
     password_store->AddLogin(copy);
-  } else if (pending_credentials_.password_element.empty() ||
-             pending_credentials_.username_element.empty() ||
-             pending_credentials_.submit_element.empty()) {
-    // password_element and username_element can't be updated because they are
-    // part of Sync and PasswordStore primary key. Thus, we must delete the old
-    // one and add again.
+  } else if (observed_form_.new_password_element.empty() &&
+             (pending_credentials_.password_element.empty() ||
+              pending_credentials_.username_element.empty() ||
+              pending_credentials_.submit_element.empty())) {
+    // If |observed_form_| was a sign-up or change password form, there is no
+    // point in trying to update element names: they are likely going to be
+    // different than those on a login form.
+    // Otherwise, given that |password_element| and |username_element| can't be
+    // updated because they are part of Sync and PasswordStore primary key, we
+    // must delete the old credentials altogether and then add the new ones.
     password_store->RemoveLogin(pending_credentials_);
     pending_credentials_.password_element = observed_form_.password_element;
     pending_credentials_.username_element = observed_form_.username_element;