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

components/autofill/content/renderer/password_form_conversion_utils.cc

Index: components/autofill/content/renderer/password_form_conversion_utils.cc
diff --git a/components/autofill/content/renderer/password_form_conversion_utils.cc b/components/autofill/content/renderer/password_form_conversion_utils.cc
index 529b972a2eee0ff467d89f5e16da0e85ffc62404..8f237f5442aa084fa0cf82a5b38c6db39b956029 100644
--- a/components/autofill/content/renderer/password_form_conversion_utils.cc
+++ b/components/autofill/content/renderer/password_form_conversion_utils.cc
@@ -34,11 +34,11 @@ bool HasAutocompleteAttributeValue(const WebInputElement* element,
                               value_in_lowercase);
 }
 
-// Helper to determine which password is the main one, and which is
-// an old password (e.g on a "make new password" form), if any.
+// Helper to determine which password is the main (current) one, and which is
+// the new password (e.g., on a sign-up or change password form), if any.
 bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
                              WebInputElement* password,
-                             WebInputElement* old_password) {
+                             WebInputElement* new_password) {
   switch (passwords.size()) {
     case 1:
       // Single password, easy.
@@ -46,26 +46,33 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
       break;
     case 2:
       if (passwords[0].value() == passwords[1].value()) {
-        // Treat two identical passwords as a single password.
-        *password = passwords[0];
+        // Two identical passwords: assume we are seeing a new password with a
+        // confirmation. This can be either a sign-up form or a password change
+        // form that does not ask for the old password.
+        *new_password = passwords[0];
       } else {
         // Assume first is old password, second is new (no choice but to guess).
-        *old_password = passwords[0];
-        *password = passwords[1];
+        *password = passwords[0];
+        *new_password = passwords[1];
       }
       break;
     case 3:
-      if (passwords[0].value() == passwords[1].value() &&
-          passwords[0].value() == passwords[2].value()) {
-        // All three passwords the same? Just treat as one and hope.
+      if (!passwords[0].value().isEmpty() &&

[engedy, 2014/07/03 13:34:19]

We have found a problem here that the set of elements that are marked as
|password_element| and/or |new_password_element| is dependent on whether the
password fields are filled with values or not.

Given that PasswordFormManager's |observed_form_| is parsed before the user
fills in the values, but the |form| received in
PasswordManager::ProvisionallySavePassword is parsed after, they might have a
different layout in terms of |password_element| and |new_password_element|
fields; and we might have the following problems:

 0.) In theory, the dependent code will make different conclusions, in the two
cases, as to whether or not we are dealing with a "login form" or "sign-up or
change password" form -- as this is based on whether or not we are seeing a
|new_password_element|.

 1.) More concretely, PasswordFormManager::DoesManage() will return false if the
|password_element|s differ, therefore the PasswordManager will not find the
PasswordFormManager originally created for |observed_form_|, and will not save
the password in the end.

 2.) Or, if we are lucky enough to get as far as
PasswordFormManager::UpdateLogin(), we might try to update missing element names
in the stored credentials based on an |observed_form_| that is not really a
"login form" -- although we could have told this already once we have seen
|form| with the filled in password values.

After having had a long discussion with Vaclav, we decided the change behavior
here somewhat to address the issues.

In a nutshell, when there is a chance that we might identify a form as a having
a |new_password_element| once we we have seen the password values, we wanted to
make sure that we will always speculatively select a |new_password| field
(perhaps the wrong one) even before the values are input into the password
fields.

We hope that the proposed changes address the most common scenarios, and that
they will not cause breakages with any other important use case.

+          (passwords[0].value() == passwords[1].value() &&
+           passwords[0].value() == passwords[2].value())) {
+        // All three passwords are the same and non-empty? This does not make
+        // any sense, give up.
+        return false;
+      } else if (passwords[1].value() == passwords[2].value()) {
+        // New password is the duplicated one, and comes second; or empty form
+        // with 3 password fields, in which case we will assume this layout.
         *password = passwords[0];
+        *new_password = passwords[1];
       } else if (passwords[0].value() == passwords[1].value()) {

[engedy, 2014/07/03 13:34:19]

Note that this is also a behavior change.

-        // Two the same and one different -> old password is duplicated one.
-        *old_password = passwords[0];
+        // It is strange that the new password comes first, but trust more which
+        // fields are duplicated than the ordering of fields.
         *password = passwords[2];
-      } else if (passwords[1].value() == passwords[2].value()) {
-        *old_password = passwords[0];
-        *password = passwords[1];
+        *new_password = passwords[0];
       } else {
         // Three different passwords, or first and last match with middle
         // different. No idea which is which, so no luck.
@@ -78,8 +85,7 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
   return true;
 }
 
-// Get information about a login form that encapsulated in the
-// PasswordForm struct.
+// Get information about a login form encapsulated in a PasswordForm struct.
 void GetPasswordForm(const WebFormElement& form, PasswordForm* password_form) {
   WebInputElement latest_input_element;
   WebInputElement username_element;
@@ -168,6 +174,11 @@ void GetPasswordForm(const WebFormElement& form, PasswordForm* password_form) {
     password_form->username_value = username_element.value();
   }
 
+  if (!username_element.isNull()) {
+    password_form->username_element = username_element.nameForAutofill();
+    password_form->username_value = username_element.value();
+  }
+
   // Get the document URL
   GURL full_origin(form.document().url());
 
@@ -180,8 +191,8 @@ void GetPasswordForm(const WebFormElement& form, PasswordForm* password_form) {
     return;
 
   WebInputElement password;
-  WebInputElement old_password;
-  if (!LocateSpecificPasswords(passwords, &password, &old_password))
+  WebInputElement new_password;
+  if (!LocateSpecificPasswords(passwords, &password, &new_password))
     return;
 
   // We want to keep the path but strip any authentication data, as well as
@@ -204,9 +215,9 @@ void GetPasswordForm(const WebFormElement& form, PasswordForm* password_form) {
     password_form->password_value = password.value();
     password_form->password_autocomplete_set = password.autoComplete();
   }
-  if (!old_password.isNull()) {
-    password_form->old_password_element = old_password.nameForAutofill();
-    password_form->old_password_value = old_password.value();
+  if (!new_password.isNull()) {
+    password_form->new_password_element = new_password.nameForAutofill();
+    password_form->new_password_value = new_password.value();
   }
 
   password_form->scheme = PasswordForm::SCHEME_HTML;