Save password without an associated username.

chrome/browser/password_manager/password_form_manager_unittest.cc

Index: chrome/browser/password_manager/password_form_manager_unittest.cc
===================================================================
--- chrome/browser/password_manager/password_form_manager_unittest.cc	(revision 130897)
+++ chrome/browser/password_manager/password_form_manager_unittest.cc	(working copy)
@@ -35,6 +35,26 @@
     profile_ = new TestingProfile();
   }
 
+  // Remove the username and regererate the profile.

[Ilya Sherman, 2012/04/19 00:27:37]

nit: "regererate" -> "regenerate"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  // This function change the observed_form_ and saved_match_.

[Ilya Sherman, 2012/04/19 00:27:37]

nit: "change" -> "changes"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  // This function should be called at the beginning of tests
+  // that there is no username.

[Ilya Sherman, 2012/04/19 00:27:37]

nit: "that there is no username" -> "for which there is no username field"

+  void SetUpForNoUsernameTests() {
+    observed_form_.origin = GURL("http://www.google.com/b/LoginAuth");
+    observed_form_.action = GURL("http://www.google.com/b/Login");
+    observed_form_.username_element = ASCIIToUTF16("");
+    observed_form_.password_element = ASCIIToUTF16("Passwd");
+    observed_form_.submit_element = ASCIIToUTF16("signIn");
+    observed_form_.signon_realm = "http://www.google.com";
+
+    saved_match_ = observed_form_;
+    saved_match_.origin = GURL("http://www.google.com/b/ServiceLoginAuth");
+    saved_match_.action = GURL("http://www.google.com/b/ServiceLogin");
+    saved_match_.preferred = true;
+    saved_match_.username_value = ASCIIToUTF16("");
+    saved_match_.password_value = ASCIIToUTF16("test2");
+  }
+
   virtual void TearDown() {
     delete profile_;
   }
@@ -59,6 +79,110 @@
     return p->IgnoreResult(*form);
   }
 
+  void VerifyNewLogin(PasswordFormManager* manager) {
+    // Successful login. The PasswordManager would instruct PasswordFormManager
+    // to save, which should know this is a new login.
+    EXPECT_TRUE(manager->IsNewLogin());
+    // Make sure the credentials that would be submitted on successful login
+    // are going to match the stored entry in the db.
+    EXPECT_EQ(observed_form()->origin.spec(),
+      GetPendingCredentials(manager)->origin.spec());

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please indent to the parenthesis after "EQ"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    EXPECT_EQ(observed_form()->signon_realm,
+      GetPendingCredentials(manager)->signon_realm);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please indent to the parenthesis after "EQ"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    EXPECT_TRUE(GetPendingCredentials(manager)->preferred);
+    EXPECT_EQ(saved_match()->password_value,
+      GetPendingCredentials(manager)->password_value);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please indent to the parenthesis after "EQ"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    EXPECT_EQ(saved_match()->username_value,
+      GetPendingCredentials(manager)->username_value);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please indent to the parenthesis after "EQ"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    return;

[Ilya Sherman, 2012/04/19 00:27:37]

nit: This return statement is redundant; please remove it.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  }
+
+  void EmptyAction() {

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Perhaps "TestEmptyAction()"?  Method names typically should have verbs.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    scoped_ptr<PasswordFormManager> manager(new PasswordFormManager(
+      profile(), NULL, *observed_form(), false));

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please preserve the previous indentation, i.e. indent this four extra
spaces rather than two.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+
+    saved_match()->action = GURL();
+    SimulateMatchingPhase(manager.get(), true);
+    // User logs in with the autofilled username / password from saved_match.
+    PasswordForm login = *observed_form();
+    login.username_value = saved_match()->username_value;
+    login.password_value = saved_match()->password_value;
+    manager->ProvisionallySave(login);
+    EXPECT_FALSE(manager->IsNewLogin());
+    // We bless our saved PasswordForm entry with the action URL of the
+    // observed form.
+    EXPECT_EQ(observed_form()->action,
+      GetPendingCredentials(manager.get())->action);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please indent to the parenthesis after "EQ"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  }
+
+  void ValidForms() {

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Perhaps "TestValidForms()"?  Method names typically should have verbs.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    // User submits credentials for the observed form.
+    PasswordForm credentials = *observed_form();
+    credentials.scheme = PasswordForm::SCHEME_HTML;
+    credentials.username_value = saved_match()->username_value;
+    credentials.password_value = saved_match()->password_value;
+
+    // Form with both username_element and password_element.
+    PasswordFormManager manager1(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager1, false);
+    EXPECT_TRUE(manager1.HasValidPasswordForm());
+
+    // Form without a username_element but with a password_element.
+    // It should be true, because we should save and autofill a password
+    // if there isn't any username.
+    credentials.username_element.clear();
+    PasswordFormManager manager2(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager2, false);
+    EXPECT_TRUE(manager2.HasValidPasswordForm());
+
+    // Form without a password_element but with a username_element.
+    credentials.username_element = saved_match()->username_element;
+    credentials.password_element.clear();
+    PasswordFormManager manager3(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager3, false);
+    EXPECT_FALSE(manager3.HasValidPasswordForm());
+
+    // Form with neither a password_element nor a username_element.
+    credentials.username_element.clear();
+    credentials.password_element.clear();
+    PasswordFormManager manager4(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager4, false);
+    EXPECT_FALSE(manager4.HasValidPasswordForm());
+  }
+
+  void ValidFormsBasic() {

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Perhaps "TestValidFormsBasic()"?  Method names typically should have verbs.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+    // User submits credentials for the observed form.
+    PasswordForm credentials = *observed_form();
+    credentials.scheme = PasswordForm::SCHEME_BASIC;
+    credentials.username_value = saved_match()->username_value;
+    credentials.password_value = saved_match()->password_value;
+
+    // Form with both username_element and password_element.
+    PasswordFormManager manager1(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager1, false);
+    EXPECT_TRUE(manager1.HasValidPasswordForm());
+
+    // Form without a username_element but with a password_element.
+    credentials.username_element.clear();
+    PasswordFormManager manager2(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager2, false);
+    EXPECT_TRUE(manager2.HasValidPasswordForm());
+
+    // Form without a password_element but with a username_element.
+    credentials.username_element = saved_match()->username_element;
+    credentials.password_element.clear();
+    PasswordFormManager manager3(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager3, false);
+    EXPECT_TRUE(manager3.HasValidPasswordForm());
+
+    // Form with neither a password_element nor a username_element.
+    credentials.username_element.clear();
+    credentials.password_element.clear();
+    PasswordFormManager manager4(profile(), NULL, credentials, false);
+    SimulateMatchingPhase(&manager4, false);
+    EXPECT_TRUE(manager4.HasValidPasswordForm());
+  }
+
   Profile* profile() { return profile_; }
 
   PasswordForm* observed_form() { return &observed_form_; }
@@ -80,22 +204,8 @@
   credentials.password_value = saved_match()->password_value;
   credentials.preferred = true;
   manager->ProvisionallySave(credentials);
+  VerifyNewLogin(manager);
 
-  // Successful login. The PasswordManager would instruct PasswordFormManager
-  // to save, which should know this is a new login.
-  EXPECT_TRUE(manager->IsNewLogin());
-  // Make sure the credentials that would be submitted on successful login
-  // are going to match the stored entry in the db.
-  EXPECT_EQ(observed_form()->origin.spec(),
-            GetPendingCredentials(manager)->origin.spec());
-  EXPECT_EQ(observed_form()->signon_realm,
-            GetPendingCredentials(manager)->signon_realm);
-  EXPECT_TRUE(GetPendingCredentials(manager)->preferred);
-  EXPECT_EQ(saved_match()->password_value,
-            GetPendingCredentials(manager)->password_value);
-  EXPECT_EQ(saved_match()->username_value,
-            GetPendingCredentials(manager)->username_value);
-
   // Now, suppose the user re-visits the site and wants to save an additional
   // login for the site with a new username. In this case, the matching phase
   // will yield the previously saved login.
@@ -123,6 +233,47 @@
   delete manager;
 }
 
+TEST_F(PasswordFormManagerTest, TestNewLoginAndUpdateWithoutUsername) {
+  SetUpForNoUsernameTests();
+  PasswordFormManager* manager = new PasswordFormManager(
+      profile(), NULL, *observed_form(), false);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Can this be allocated on the stack, rather than on the heap, so that we
don't need to |delete| it later?

+  SimulateMatchingPhase(manager, false);
+
+  // User submits credentials for the observed form.
+  PasswordForm credentials = *observed_form();
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value = saved_match()->password_value;
+  credentials.preferred = true;
+  manager->ProvisionallySave(credentials);
+  VerifyNewLogin(manager);
+
+  // Now, suppose the user re-visits the site and wants to save an additional
+  // login for the site with a new password. In this case, the matching phase
+  // will yield the previously saved login.
+  SimulateMatchingPhase(manager, true);
+
+  // Set up the update login.
+  string16 new_pass = ASCIIToUTF16("newpass2");
+  credentials.password_value = new_pass;
+  manager->ProvisionallySave(credentials);
+
+  // If there is no username, we supporse it's an update.

[Ilya Sherman, 2012/04/19 00:27:37]

nit: "supporse" -> "assume"

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  EXPECT_FALSE(manager->IsNewLogin());
+
+  // And make sure everything squares up again.
+  EXPECT_EQ(GetPendingCredentials(manager)->origin.spec(),
+            saved_match()->origin.spec());

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please preserve the parameter order that was used before, i.e.
"saved_match()->origin.spec()" should be the first parameter.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  EXPECT_EQ(GetPendingCredentials(manager)->signon_realm,
+            saved_match()->signon_realm);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: Please preserve the parameter order that was used before, i.e.
"saved_match()->signon_realm" should be the first parameter.

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Done.

+  EXPECT_TRUE(GetPendingCredentials(manager)->preferred);
+  EXPECT_EQ(new_pass,
+            GetPendingCredentials(manager)->password_value);
+
+  // Done.
+  delete manager;
+}
+
+
 TEST_F(PasswordFormManagerTest, TestUpdatePassword) {
   // Create a PasswordFormManager with observed_form, as if we just
   // saw this form and need to find matching logins.
@@ -180,85 +331,52 @@
 }
 
 TEST_F(PasswordFormManagerTest, TestEmptyAction) {
-  scoped_ptr<PasswordFormManager> manager(new PasswordFormManager(
-      profile(), NULL, *observed_form(), false));
+  EmptyAction();
+}
 
-  saved_match()->action = GURL();
-  SimulateMatchingPhase(manager.get(), true);
-  // User logs in with the autofilled username / password from saved_match.
-  PasswordForm login = *observed_form();
-  login.username_value = saved_match()->username_value;
-  login.password_value = saved_match()->password_value;
-  manager->ProvisionallySave(login);
-  EXPECT_FALSE(manager->IsNewLogin());
-  // We bless our saved PasswordForm entry with the action URL of the
-  // observed form.
-  EXPECT_EQ(observed_form()->action,
-            GetPendingCredentials(manager.get())->action);
+TEST_F(PasswordFormManagerTest, TestEmptyActionWithoutUsername) {
+  SetUpForNoUsernameTests();
+  EmptyAction();
 }
 
 TEST_F(PasswordFormManagerTest, TestValidForms) {
-  // User submits credentials for the observed form.
-  PasswordForm credentials = *observed_form();
-  credentials.scheme = PasswordForm::SCHEME_HTML;
-  credentials.username_value = saved_match()->username_value;
-  credentials.password_value = saved_match()->password_value;
+  ValidForms();
+}
 
-  // Form with both username_element and password_element.
-  PasswordFormManager manager1(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager1, false);
-  EXPECT_TRUE(manager1.HasValidPasswordForm());
+TEST_F(PasswordFormManagerTest, TestValidFormsWithNoUsername) {
+  SetUpForNoUsernameTests();
+  ValidForms();
+}
 
-  // Form without a username_element but with a password_element.
-  credentials.username_element.clear();
-  PasswordFormManager manager2(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager2, false);
-  EXPECT_FALSE(manager2.HasValidPasswordForm());
+TEST_F(PasswordFormManagerTest, TestValidFormsBasic) {
+  ValidFormsBasic();
+}
 
-  // Form without a password_element but with a username_element.
-  credentials.username_element = saved_match()->username_element;
-  credentials.password_element.clear();
-  PasswordFormManager manager3(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager3, false);
-  EXPECT_FALSE(manager3.HasValidPasswordForm());
-
-  // Form with neither a password_element nor a username_element.
-  credentials.username_element.clear();
-  credentials.password_element.clear();
-  PasswordFormManager manager4(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager4, false);
-  EXPECT_FALSE(manager4.HasValidPasswordForm());
+TEST_F(PasswordFormManagerTest, TestValidFormsBasicWithoutUsername) {
+  SetUpForNoUsernameTests();
+  ValidFormsBasic();
 }
 
-TEST_F(PasswordFormManagerTest, TestValidFormsBasic) {
-  // User submits credentials for the observed form.
+TEST_F(PasswordFormManagerTest, TestVisitOtherForm) {
+  // Create forms haven't username element.
+  SetUpForNoUsernameTests();
+
+  // Save the password for a form with no username element
+  // on http://www.google.com/b/LoginAuth
+  PasswordFormManager* manager = new PasswordFormManager(
+      profile(), NULL, *observed_form(), false);

[Ilya Sherman, 2012/04/19 00:27:37]

nit: It looks like this will leak.  Can it be allocated on the stack rather than
on the heap?  If not, you should probably use a scoped_ptr<>

+  SimulateMatchingPhase(manager, false);
   PasswordForm credentials = *observed_form();
-  credentials.scheme = PasswordForm::SCHEME_BASIC;
   credentials.username_value = saved_match()->username_value;
   credentials.password_value = saved_match()->password_value;
+  credentials.preferred = true;
+  manager->ProvisionallySave(credentials);
+  VerifyNewLogin(manager);
 
-  // Form with both username_element and password_element.
-  PasswordFormManager manager1(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager1, false);
-  EXPECT_TRUE(manager1.HasValidPasswordForm());
+  // the user visits foo.com/a, which has a password form
+  // *with* a username element.

[Ilya Sherman, 2012/04/19 00:27:37]

Should there be code below this comment?

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Oh, Thank you for pointing out it.
I forgot to delete the comment.
What I'd like to test in this area is already does in TestNewLogin().

 
-  // Form without a username_element but with a password_element.
-  credentials.username_element.clear();
-  PasswordFormManager manager2(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager2, false);
-  EXPECT_TRUE(manager2.HasValidPasswordForm());
+  // the user visits foo.com/a, which has a password form
+  // *without* a username element.

[Ilya Sherman, 2012/04/19 00:27:37]

Should there be code below this comment?

[Yumikiyo Osanai, 2012/04/26 23:33:22]

Oh, I'll remove this comment.
What I'd like to test in this area is already does in
TestNewLoginAndUpdateWithoutUsername.

 
-  // Form without a password_element but with a username_element.
-  credentials.username_element = saved_match()->username_element;
-  credentials.password_element.clear();
-  PasswordFormManager manager3(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager3, false);
-  EXPECT_TRUE(manager3.HasValidPasswordForm());
-
-  // Form with neither a password_element nor a username_element.
-  credentials.username_element.clear();
-  credentials.password_element.clear();
-  PasswordFormManager manager4(profile(), NULL, credentials, false);
-  SimulateMatchingPhase(&manager4, false);
-  EXPECT_TRUE(manager4.HasValidPasswordForm());
 }