Password Login Database: report correct changes from AddLogin().

components/password_manager/core/browser/login_database_unittest.cc

Index: components/password_manager/core/browser/login_database_unittest.cc
diff --git a/components/password_manager/core/browser/login_database_unittest.cc b/components/password_manager/core/browser/login_database_unittest.cc
index 7a4840e1e87c57dfc93ef12848aff30b574365da..a1d77fbc179c7477887c043a1be13787345b91dd 100644
--- a/components/password_manager/core/browser/login_database_unittest.cc
+++ b/components/password_manager/core/browser/login_database_unittest.cc
@@ -22,6 +22,15 @@ using base::ASCIIToUTF16;
 using ::testing::Eq;
 
 namespace password_manager {
+namespace {
+
+PasswordStoreChangeList ChangeListFromForm(const PasswordForm& form) {

[Garrett Casto, 2014/05/14 07:46:35]

nit: Doesn't seem like the name quite captures exactly what this function does,
since you could imagine multiple types of change lists from this form ({},
{ADD}, {DELETE, ADD} at least). AddChangeForForm maybe?

[vasilii, 2014/05/14 11:20:18]

Done.

+  return PasswordStoreChangeList(1,
+                                 PasswordStoreChange(PasswordStoreChange::ADD,
+                                                     form));
+}
+
+}  // namespace
 
 class LoginDatabaseTest : public testing::Test {
  protected:
@@ -50,7 +59,7 @@ class LoginDatabaseTest : public testing::Test {
   }
 
   void TestNonHTMLFormPSLMatching(const PasswordForm::Scheme& scheme) {
-    std::vector<PasswordForm*> result;
+    ScopedVector<PasswordForm> result;
 
     base::Time now = base::Time::Now();
 
@@ -61,6 +70,7 @@ class LoginDatabaseTest : public testing::Test {
     non_html_auth.password_value = ASCIIToUTF16("test");
     non_html_auth.signon_realm = "http://example.com/Realm";
     non_html_auth.scheme = scheme;
+    non_html_auth.date_created = now;

[Garrett Casto, 2014/05/14 07:46:35]

Was the lack of date_created causing test failures?

[vasilii, 2014/05/14 11:20:18]

Yes. RemoveLoginsCreatedBetween did nothing.

 
     // Simple password form.
     PasswordForm html_form(non_html_auth);
@@ -71,14 +81,13 @@ class LoginDatabaseTest : public testing::Test {
     html_form.submit_element = ASCIIToUTF16("");
     html_form.signon_realm = "http://example.com/";
     html_form.scheme = PasswordForm::SCHEME_HTML;
+    html_form.date_created = now;
 
     // Add them and make sure it is there.
-    EXPECT_TRUE(db_.AddLogin(non_html_auth));
-    EXPECT_TRUE(db_.AddLogin(html_form));
-    EXPECT_TRUE(db_.GetAutofillableLogins(&result));
+    EXPECT_EQ(ChangeListFromForm(non_html_auth), db_.AddLogin(non_html_auth));
+    EXPECT_EQ(ChangeListFromForm(html_form), db_.AddLogin(html_form));
+    EXPECT_TRUE(db_.GetAutofillableLogins(&result.get()));
     EXPECT_EQ(2U, result.size());
-    delete result[0];
-    delete result[1];
     result.clear();
 
     PasswordForm second_non_html_auth(non_html_auth);
@@ -86,7 +95,7 @@ class LoginDatabaseTest : public testing::Test {
     second_non_html_auth.signon_realm = "http://second.example.com/Realm";
 
     // This shouldn't match anything.
-    EXPECT_TRUE(db_.GetLogins(second_non_html_auth, &result));
+    EXPECT_TRUE(db_.GetLogins(second_non_html_auth, &result.get()));
     EXPECT_EQ(0U, result.size());
 
     // Clear state.
@@ -124,7 +133,7 @@ TEST_F(LoginDatabaseTest, Logins) {
 
   // Add it and make sure it is there and that all the fields were retrieved
   // correctly.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(1U, result.size());
   FormsAreEqual(form, *result[0]);
@@ -168,7 +177,7 @@ TEST_F(LoginDatabaseTest, Logins) {
   EXPECT_EQ(0U, result.size());
 
   // Let's imagine the user logs into the secure site.
-  EXPECT_TRUE(db_.AddLogin(form4));
+  EXPECT_EQ(ChangeListFromForm(form4), db_.AddLogin(form4));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(2U, result.size());
   delete result[0];
@@ -262,7 +271,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatching) {
   form.scheme = PasswordForm::SCHEME_HTML;
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(1U, result.size());
   delete result[0];
@@ -320,7 +329,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingShouldMatchingApply) {
   form.scheme = PasswordForm::SCHEME_HTML;
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(1U, result.size());
   delete result[0];
@@ -370,7 +379,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingDifferentSites) {
   form.scheme = PasswordForm::SCHEME_HTML;
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(1U, result.size());
   delete result[0];
@@ -410,7 +419,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingDifferentSites) {
   form.scheme = PasswordForm::SCHEME_HTML;
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(2U, result.size());
   delete result[0];
@@ -464,7 +473,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingRegexp) {
   form.scheme = PasswordForm::SCHEME_HTML;
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(1U, result.size());
   delete result[0];
@@ -475,7 +484,7 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingRegexp) {
       GetFormWithNewSignonRealm(form, "http://www.foo-bar.com/");
 
   // Add it and make sure it is there.
-  EXPECT_TRUE(db_.AddLogin(form_dash));
+  EXPECT_EQ(ChangeListFromForm(form_dash), db_.AddLogin(form_dash));
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
   EXPECT_EQ(2U, result.size());
   delete result[0];
@@ -575,7 +584,7 @@ static bool AddTimestampedLogin(LoginDatabase* db, std::string url,
   form.submit_element = ASCIIToUTF16("signIn");
   form.signon_realm = url;
   form.date_created = time;
-  return db->AddLogin(form);
+  return db->AddLogin(form) == ChangeListFromForm(form);
 }
 
 static void ClearResults(std::vector<PasswordForm*>* results) {
@@ -647,7 +656,7 @@ TEST_F(LoginDatabaseTest, BlacklistedLogins) {
   form.preferred = true;
   form.blacklisted_by_user = true;
   form.scheme = PasswordForm::SCHEME_HTML;
-  EXPECT_TRUE(db_.AddLogin(form));
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
 
   // Get all non-blacklisted logins (should be none).
   EXPECT_TRUE(db_.GetAutofillableLogins(&result));
@@ -700,7 +709,7 @@ TEST_F(LoginDatabaseTest, UpdateIncompleteCredentials) {
   incomplete_form.preferred = true;
   incomplete_form.blacklisted_by_user = false;
   incomplete_form.scheme = PasswordForm::SCHEME_HTML;
-  EXPECT_TRUE(db_.AddLogin(incomplete_form));
+  EXPECT_EQ(ChangeListFromForm(incomplete_form), db_.AddLogin(incomplete_form));
 
   // A form on some website. It should trigger a match with the stored one.
   PasswordForm encountered_form;
@@ -773,7 +782,7 @@ TEST_F(LoginDatabaseTest, UpdateOverlappingCredentials) {
   incomplete_form.preferred = true;
   incomplete_form.blacklisted_by_user = false;
   incomplete_form.scheme = PasswordForm::SCHEME_HTML;
-  EXPECT_TRUE(db_.AddLogin(incomplete_form));
+  EXPECT_EQ(ChangeListFromForm(incomplete_form), db_.AddLogin(incomplete_form));
 
   // Save a complete version of the previous form. Both forms could exist if
   // the user created the complete version before importing the incomplete
@@ -783,7 +792,7 @@ TEST_F(LoginDatabaseTest, UpdateOverlappingCredentials) {
   complete_form.username_element = ASCIIToUTF16("username_element");
   complete_form.password_element = ASCIIToUTF16("password_element");
   complete_form.submit_element = ASCIIToUTF16("submit");
-  EXPECT_TRUE(db_.AddLogin(complete_form));
+  EXPECT_EQ(ChangeListFromForm(complete_form), db_.AddLogin(complete_form));
 
   // Make sure both passwords exist.
   ScopedVector<autofill::PasswordForm> result;
@@ -808,6 +817,26 @@ TEST_F(LoginDatabaseTest, UpdateOverlappingCredentials) {
   EXPECT_EQ(expected_form, *result[0]);
 }
 
+TEST_F(LoginDatabaseTest, DoubleAdd) {
+  PasswordForm form;
+  form.origin = GURL("http://accounts.google.com/LoginAuth");
+  form.signon_realm = "http://accounts.google.com/";
+  form.username_value = ASCIIToUTF16("my_username");
+  form.password_value = ASCIIToUTF16("my_password");
+  form.ssl_valid = false;
+  form.preferred = true;
+  form.blacklisted_by_user = false;
+  form.scheme = PasswordForm::SCHEME_HTML;
+  EXPECT_EQ(ChangeListFromForm(form), db_.AddLogin(form));
+
+  // Add almost the same form again.
+  form.times_used++;
+  PasswordStoreChangeList list;
+  list.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE, form));
+  list.push_back(PasswordStoreChange(PasswordStoreChange::ADD, form));
+  EXPECT_EQ(list, db_.AddLogin(form));
+}
+
 #if defined(OS_POSIX)
 // Only the current user has permission to read the database.
 //