Implement autocomplete='current-password' and 'new-password'.

components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc

Index: components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
diff --git a/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc b/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
index c38ff86a3fbdba7b71be36986466b9fef49efa81..9fb466c19c80caa8d276a9e30c3e46197512ee25 100644
--- a/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
+++ b/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
@@ -52,12 +52,17 @@ class PasswordFormBuilder {
   }
 
   // Appends a new password-type field at the end of the form, having the
-  // specified |name_and_id| and |value| attributes.
-  void AddPasswordField(const char* name_and_id, const char* value) {
+  // specified |name_and_id|, |value|, and |autocomplete| attributes. Special
+  // values for |autocomplete| are the same as in AddUsernameField.
+  void AddPasswordField(const char* name_and_id,
+                        const char* value,
+                        const char* autocomplete) {
+    std::string autocomplete_attribute(autocomplete != NULL ?
+        base::StringPrintf("autocomplete=\"%s\"", autocomplete): "");
     base::StringAppendF(
         &html_,
-        "<INPUT type=\"password\" name=\"%s\" id=\"%s\" value=\"%s\"/>",
-        name_and_id, name_and_id, value);
+        "<INPUT type=\"password\" name=\"%s\" id=\"%s\" value=\"%s\" %s/>",
+        name_and_id, name_and_id, value, autocomplete_attribute.c_str());
   }
 
   // Appends a new submit-type field at the end of the form.
@@ -111,7 +116,7 @@ TEST_F(PasswordFormConversionUtilsTest, ValidWebFormElementToPasswordForm) {
   PasswordFormBuilder builder(kTestFormActionURL);
   builder.AddUsernameField("username", "johnsmith", NULL);
   builder.AddSubmitButton();
-  builder.AddPasswordField("password", "secret");
+  builder.AddPasswordField("password", "secret", NULL);
   std::string html = builder.ProduceHTML();
 
   scoped_ptr<PasswordForm> password_form;
@@ -135,7 +140,7 @@ TEST_F(PasswordFormConversionUtilsTest, InvalidWebFormElementToPasswordForm) {
   PasswordFormBuilder builder("invalid_target");
   builder.AddUsernameField("username", "johnsmith", NULL);
   builder.AddSubmitButton();
-  builder.AddPasswordField("password", "secret");
+  builder.AddPasswordField("password", "secret", NULL);
   std::string html = builder.ProduceHTML();
 
   scoped_ptr<PasswordForm> password_form;
@@ -147,11 +152,11 @@ TEST_F(PasswordFormConversionUtilsTest,
        WebFormWithMultipleUseNameAndPassWordFieldsToPasswordForm) {
   PasswordFormBuilder builder(kTestFormActionURL);
   builder.AddUsernameField("username1", "John", NULL);
-  builder.AddPasswordField("password1", "oldsecret");
+  builder.AddPasswordField("password1", "oldsecret", NULL);
   builder.AddUsernameField("username2", "William", NULL);
-  builder.AddPasswordField("password2", "secret");
+  builder.AddPasswordField("password2", "secret", NULL);
   builder.AddUsernameField("username3", "Smith", NULL);
-  builder.AddPasswordField("password3", "secret");
+  builder.AddPasswordField("password3", "secret", NULL);
   builder.AddSubmitButton();
   std::string html = builder.ProduceHTML();
 
@@ -185,9 +190,9 @@ TEST_F(PasswordFormConversionUtilsTest,
        WebFormwithThreeDifferentPasswordsToPasswordForm) {
   PasswordFormBuilder builder(kTestFormActionURL);
   builder.AddUsernameField("username1", "John", NULL);
-  builder.AddPasswordField("password1", "alpha");
-  builder.AddPasswordField("password2", "beta");
-  builder.AddPasswordField("password3", "gamma");
+  builder.AddPasswordField("password1", "alpha", NULL);
+  builder.AddPasswordField("password2", "beta", NULL);
+  builder.AddPasswordField("password3", "gamma", NULL);
   builder.AddSubmitButton();
   std::string html = builder.ProduceHTML();
 
@@ -223,7 +228,7 @@ TEST_F(PasswordFormConversionUtilsTest,
       {{"", "", ""}, "username2", "William", "John+Smith"},
       {{"", "", "username"}, "username3", "Smith", ""},
       {{"username", "", "username"}, "username1", "John", "Smith"},
-      // Whether attribute values are upper or mixed case, it should not matter.
+      // It should not matter if attribute values are upper or mixed case.
       {{"USERNAME", NULL, "uSeRNaMe"}, "username1", "John", "Smith"},
       {{"uSeRNaMe", NULL, "USERNAME"}, "username1", "John", "Smith"}};
 
@@ -233,7 +238,7 @@ TEST_F(PasswordFormConversionUtilsTest,
     PasswordFormBuilder builder(kTestFormActionURL);
     builder.AddUsernameField("username1", "John", cases[i].autocomplete[0]);
     builder.AddUsernameField("username2", "William", cases[i].autocomplete[1]);
-    builder.AddPasswordField("password", "secret");
+    builder.AddPasswordField("password", "secret", NULL);
     builder.AddUsernameField("username3", "Smith", cases[i].autocomplete[2]);
     builder.AddSubmitButton();
     std::string html = builder.ProduceHTML();
@@ -253,4 +258,125 @@ TEST_F(PasswordFormConversionUtilsTest,
   }
 }
 
+TEST_F(PasswordFormConversionUtilsTest,
+       PasswordFieldsWithAutocompleteAttributes) {
+  // Each test case consists of a set of parameters to be plugged into the
+  // PasswordFormBuilder below, plus the corresponding expectations.
+  struct TestCase {
+    const char* autocomplete[3];
+    const char* expected_password_element;
+    const char* expected_password_value;
+    const char* expected_new_password_element;
+    const char* expected_new_password_value;
+  } cases[] = {
+      // When there are elements marked with autocomplete='current-password',
+      // but no elements with 'new-password', we should treat the first of the
+      // former kind as the current password, and ignore all other password
+      // fields, assuming they are not intentionally not marked. They might be
+      // for other purposes, such as PINs, OTPs, and the like. Actual values in
+      // the password fields should be ignored in all cases below.
+      {{"current-password", NULL, NULL}, "password1", "alpha", "", ""},
+      {{NULL, "current-password", NULL}, "password2", "beta", "", ""},
+      {{NULL, NULL, "current-password"}, "password3", "gamma", "", ""},
+      {{NULL, "current-password", "current-password"},
+       "password2", "beta", "", ""},
+      {{"current-password", NULL, "current-password"},
+       "password1", "alpha", "", ""},
+      {{"current-password", "current-password", NULL},
+       "password1", "alpha", "", ""},
+      {{"current-password", "current-password", "current-password"},
+       "password1", "alpha", "", ""},
+      // The same goes vice versa for autocomplete='new-password'.
+      {{"new-password", NULL, NULL}, "", "", "password1", "alpha"},
+      {{NULL, "new-password", NULL}, "", "", "password2", "beta"},
+      {{NULL, NULL, "new-password"}, "", "", "password3", "gamma"},
+      {{NULL, "new-password", "new-password"}, "", "", "password2", "beta"},
+      {{"new-password", NULL, "new-password"}, "", "", "password1", "alpha"},
+      {{"new-password", "new-password", NULL}, "", "", "password1", "alpha"},
+      {{"new-password", "new-password", "new-password"},
+       "", "", "password1", "alpha"},
+      // When there is one element marked with autocomplete='current-password',
+      // and one with 'new-password', just comply, regardless of their order.
+      // Ignore the unmarked password field(s) for the same reason as above.
+      {{"current-password", "new-password", NULL},
+       "password1", "alpha", "password2", "beta"},
+      {{"current-password", NULL, "new-password"},
+       "password1", "alpha", "password3", "gamma"},
+      {{NULL, "current-password", "new-password"},
+       "password2", "beta", "password3", "gamma"},
+      {{"new-password", "current-password", NULL},
+       "password2", "beta", "password1", "alpha"},
+      {{"new-password", NULL, "current-password"},
+       "password3", "gamma", "password1", "alpha"},
+      {{NULL, "new-password", "current-password"},
+       "password3", "gamma", "password2", "beta"},
+      // In case of duplicated elements of either kind, go with the first one of
+      // its kind.
+      {{"current-password", "current-password", "new-password"},
+       "password1", "alpha", "password3", "gamma"},
+      {{"current-password", "new-password", "current-password"},
+       "password1", "alpha", "password2", "beta"},
+      {{"new-password", "current-password", "current-password"},
+       "password2", "beta", "password1", "alpha"},
+      {{"current-password", "new-password", "new-password"},
+       "password1", "alpha", "password2", "beta"},
+      {{"new-password", "current-password", "new-password"},
+       "password2", "beta", "password1", "alpha"},
+      {{"new-password", "new-password", "current-password"},
+       "password3", "gamma", "password1", "alpha"},
+      // When there is an empty autocomplete attribute (i.e. autocomplete=''),
+      // it should have the same effect as having no attribute whatsoever.
+      {{"current-password", "", ""},
+       "password1", "alpha", "", ""},
+      {{"", "", "new-password"},
+       "", "", "password3", "gamma"},
+      {{"", "new-password", ""},
+       "", "", "password2", "beta"},
+      {{"", "current-password", "current-password"},
+       "password2", "beta", "", ""},
+      {{"new-password", "", "new-password"}, "", "", "password1", "alpha"},
+      {{"new-password", "", "current-password"},
+       "password3", "gamma", "password1", "alpha"},
+      // It should not matter if attribute values are upper or mixed case.
+      {{NULL, "current-password", NULL},
+       "password2", "beta", "", ""},
+      {{NULL, "CURRENT-PASSWORD", NULL},
+       "password2", "beta", "", ""},
+      {{NULL, "new-password", NULL},
+       "", "", "password2", "beta"},
+      {{NULL, "nEw-PaSsWoRd", NULL},
+       "", "", "password2", "beta"}};
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); ++i) {
+    SCOPED_TRACE(testing::Message() << "Iteration " << i);
+
+    PasswordFormBuilder builder(kTestFormActionURL);
+    builder.AddPasswordField("password1", "alpha", cases[i].autocomplete[0]);
+    builder.AddUsernameField("username1", "William", NULL);
+    builder.AddPasswordField("password2", "beta", cases[i].autocomplete[1]);
+    builder.AddUsernameField("username2", "Smith", NULL);
+    builder.AddPasswordField("password3", "gamma", cases[i].autocomplete[2]);
+    builder.AddSubmitButton();
+    std::string html = builder.ProduceHTML();
+
+    scoped_ptr<PasswordForm> password_form;
+    ASSERT_NO_FATAL_FAILURE(LoadHTMLAndConvertForm(html, &password_form));
+    ASSERT_NE(static_cast<PasswordForm*>(NULL), password_form.get());

[vabr (Chromium), 2014/07/05 09:21:39]

nit:
ASSERT_TRUE(password_form)
This is more readable than the current line, because it only contains the
important stuff (assert, and the object).
Usage of (scoped) pointers as Booleans is widespread enough in Chromium for this
shortcut to make sense.
Also, there is no added value in having ASSERT_NE output the value f the pointer
in the failing case, because it will be "NULL vs. NULL".

[engedy, 2014/07/07 08:00:39]

I could not agree more. However, for consistency with the rest of the tests, let
us keep this as is, just for a day or two.

I am planning to file a follow-up CL to fix this and some other smells in this
test file.

[vabr (Chromium), 2014/07/07 10:01:46]

OK, as soon as fixing this is scheduled soon and tracked, it's fine to keep this
CL as is.

[engedy, 2014/07/07 10:49:26]

I have prepared https://codereview.chromium.org/372683003/.

+
+    EXPECT_EQ(base::UTF8ToUTF16("username1"), password_form->username_element);

[vabr (Chromium), 2014/07/05 09:21:40]

I'm a bit confused as for why this test is concerned with usernames at all. I
suggest not to include usernames in the expectations at all, because it might
hide the main purpose of the test.

Alternatively, please add a comment why it is important to know how parsing of
usernames went. (Does that influence how passwords are identified?)

[engedy, 2014/07/07 08:00:39]

Done.

+    EXPECT_EQ(base::UTF8ToUTF16("William"), password_form->username_value);
+    EXPECT_EQ(base::UTF8ToUTF16(cases[i].expected_password_element),
+              password_form->password_element);
+    EXPECT_EQ(base::UTF8ToUTF16(cases[i].expected_password_value),
+              password_form->password_value);
+    EXPECT_EQ(base::UTF8ToUTF16(cases[i].expected_new_password_element),
+              password_form->new_password_element);
+    EXPECT_EQ(base::UTF8ToUTF16(cases[i].expected_new_password_value),
+              password_form->new_password_value);
+    ASSERT_EQ(1u, password_form->other_possible_usernames.size());

[vabr (Chromium), 2014/07/05 09:21:40]

Why is this an assertion and all others are expectation?
I could not find a canonical guide to choose between those two, but my
understanding is that assertions are to make sure that the environment is sane,
whereas expectations make sure that the tested code (not its dependencies) works
as expected.
So in this case I would suggest using EXPECT_EQ. It's OK to guard the next
expectation with an if-condition, to make sure you don't go beyond the vector
size, because if that condition fails, the expectation on this current line will
already be failing.

[engedy, 2014/07/07 08:00:39]

I think it is common practice [1] in Chromium to use ASSERTs to short-circuit
the test case and prevent a crash regardless of what is being asserted. Unless
you feel strongly about this, I would prefer to keep it as is.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=ASSERT_EQ%5C(.*%5C.si...

[vabr (Chromium), 2014/07/07 10:01:46]

So here is one more for not using the ASSERT: when somebody modifies the test,
and adds more checks at the end, which will not be affected by the property you
assert on, those checks will not be done if the assert fails. It is generally
useful to do as many checks as possible even in failing tests, to make
diagnostics easier.
Also, I'm not sure I buy your link [1] completely -- there are plenty of places
where such asserts are used correctly in the sense I described in my comment.
Often, though common, the code practises found in our tests are not the best to
follow as well.

This is just a suggestion, though. I'm not forcing this change on you, it's fine
to submit the CL as it is.

[engedy, 2014/07/07 10:49:26]

I stand convinced. I have also changed this in
https://codereview.chromium.org/372683003/.

+    EXPECT_EQ(base::UTF8ToUTF16("Smith"),
+              password_form->other_possible_usernames[0]);
+  }
+}
+
 }  // namespace autofill