GetLoginsRequest: Use ScopedVector to express ownership of forms

chrome/browser/password_manager/password_store_mac_unittest.cc

Index: chrome/browser/password_manager/password_store_mac_unittest.cc
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 29a3f90f6c8b93e0ad1f891d6ce2f87c928d97a8..4e90d3ed2dff45d4dbe2351136d82bee778e62c1 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 #include "chrome/common/chrome_paths.h"
 #include "components/password_manager/core/browser/login_database.h"
+#include "components/password_manager/core/browser/password_form_data.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "content/public/test/test_browser_thread.h"
 #include "crypto/mock_apple_keychain.h"
@@ -32,15 +33,13 @@ using password_manager::PasswordStore;
 using password_manager::PasswordStoreConsumer;
 using testing::_;
 using testing::DoAll;
+using testing::ElementsAre;
 using testing::Invoke;
+using testing::SizeIs;
 using testing::WithArg;
 
 namespace {
 
-ACTION(STLDeleteElements0) {
-  STLDeleteContainerPointers(arg0.begin(), arg0.end());
-}
-
 ACTION(QuitUIMessageLoop) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   base::MessageLoop::current()->Quit();
@@ -48,28 +47,20 @@ ACTION(QuitUIMessageLoop) {
 
 class MockPasswordStoreConsumer : public PasswordStoreConsumer {
  public:
-  MOCK_METHOD1(OnGetPasswordStoreResults,
-               void(const std::vector<autofill::PasswordForm*>&));
+  MOCK_METHOD1(OnGetPasswordStoreResultsConstRef,
+               void(const std::vector<PasswordForm*>&));
 
-  void CopyElements(const std::vector<autofill::PasswordForm*>& forms) {
-    last_result.clear();
-    for (size_t i = 0; i < forms.size(); ++i) {
-      last_result.push_back(*forms[i]);
-    }
+  // GMock cannot mock methods with move-only args.
+  void OnGetPasswordStoreResults(ScopedVector<PasswordForm> results) override {
+    OnGetPasswordStoreResultsConstRef(results.get());
   }
 
-  // Runs the current thread's message loop until OnGetPasswordStoreResults()
-  // is posted to it. This method should be called immediately after GetLogins,
-  // without pumping the message loop in-between.
-  void WaitOnGetPasswordStoreResults() {
-    EXPECT_CALL(*this, OnGetPasswordStoreResults(_)).WillOnce(DoAll(
-        WithArg<0>(Invoke(this, &MockPasswordStoreConsumer::CopyElements)),
-        WithArg<0>(STLDeleteElements0()),
-        QuitUIMessageLoop()));
-    base::MessageLoop::current()->Run();
+  void SaveACopyOfFirstForm(const std::vector<PasswordForm*>& result) {

[vasilii, 2015/02/05 19:23:27]

Turn it to the action taking an argument (PasswordForm* or
std::vector<PasswordForm>*) and copying the input to this argument. Then you can
drop |first_form|. Example:

PasswordForm returned_form;
EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
    .WillOnce(Invoke(SaveACopyOfFirstForm(&returned_form)));
// Do something with returned_form

[vabr (Chromium), 2015/02/06 14:16:05]

Done.

+    DCHECK(!result.empty());
+    first_form = *result[0];
   }
 
-  std::vector<PasswordForm> last_result;
+  PasswordForm first_form;  // First form of the returned result, if any.
 };
 
 class MockPasswordStoreObserver : public PasswordStore::Observer {
@@ -230,11 +221,10 @@ struct PasswordFormData {
   const double creation_time;
 };
 
-// Creates and returns a new PasswordForm built from form_data. Caller is
-// responsible for deleting the object when finished with it.
-static PasswordForm* CreatePasswordFormFromData(
+// Creates and returns a new PasswordForm built from form_data.
+static scoped_ptr<PasswordForm> CreatePasswordFormFromData(

[vasilii, 2015/02/05 19:23:26]

I'd put this method and it's neighbors to the namespace {} above. I don't like
'static' keyword.

[vabr (Chromium), 2015/02/06 14:16:05]

Done.
BTW., this method and the struct will be removed in
https://codereview.chromium.org/896903003/.

     const PasswordFormData& form_data) {
-  PasswordForm* form = new PasswordForm();
+  scoped_ptr<PasswordForm> form(new PasswordForm());
   form->scheme = form_data.scheme;
   form->preferred = form_data.preferred;
   form->ssl_valid = form_data.ssl_valid;
@@ -263,7 +253,7 @@ static PasswordForm* CreatePasswordFormFromData(
   }
   form->avatar_url = GURL("https://accounts.google.com/Avatar");
   form->federation_url = GURL("https://accounts.google.com/login");
-  return form;
+  return form.Pass();
 }
 
 // Macro to simplify calling CheckFormsAgainstExpectations with a useful label.
@@ -477,8 +467,8 @@ TEST_F(PasswordStoreMacInternalsTest, TestKeychainSearch) {
   MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
   for (unsigned int i = 0; i < arraysize(test_data); ++i) {
-    scoped_ptr<PasswordForm> query_form(
-        CreatePasswordFormFromData(test_data[i].data));
+    scoped_ptr<PasswordForm> query_form =
+        CreatePasswordFormFromData(test_data[i].data);
 
     // Check matches treating the form as a fill target.
     ScopedVector<autofill::PasswordForm> matching_items =
@@ -557,8 +547,8 @@ TEST_F(PasswordStoreMacInternalsTest, TestKeychainExactSearch) {
 
   for (unsigned int i = 0; i < arraysize(base_form_data); ++i) {
     // Create a base form and make sure we find a match.
-    scoped_ptr<PasswordForm> base_form(CreatePasswordFormFromData(
-        base_form_data[i]));
+    scoped_ptr<PasswordForm> base_form =
+        CreatePasswordFormFromData(base_form_data[i]);
     EXPECT_TRUE(keychain_adapter.HasPasswordsMergeableWithForm(*base_form));
     EXPECT_TRUE(keychain_adapter.HasPasswordExactlyMatchingForm(*base_form));
 
@@ -631,8 +621,8 @@ TEST_F(PasswordStoreMacInternalsTest, TestKeychainAdd) {
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
 
   for (unsigned int i = 0; i < arraysize(test_data); ++i) {
-    scoped_ptr<PasswordForm> in_form(
-        CreatePasswordFormFromData(test_data[i].data));
+    scoped_ptr<PasswordForm> in_form =
+        CreatePasswordFormFromData(test_data[i].data);
     bool add_succeeded = owned_keychain_adapter.AddPassword(*in_form);
     EXPECT_EQ(test_data[i].should_succeed, add_succeeded);
     if (add_succeeded) {
@@ -650,7 +640,7 @@ TEST_F(PasswordStoreMacInternalsTest, TestKeychainAdd) {
       "http://some.domain.com/insecure.html", NULL,
       NULL, NULL, NULL, L"joe_user", L"updated_password", false, false, 0
     };
-    scoped_ptr<PasswordForm> update_form(CreatePasswordFormFromData(data));
+    scoped_ptr<PasswordForm> update_form = CreatePasswordFormFromData(data);
     MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
     EXPECT_TRUE(keychain_adapter.AddPassword(*update_form));
     SecKeychainItemRef keychain_item = reinterpret_cast<SecKeychainItemRef>(2);
@@ -683,13 +673,13 @@ TEST_F(PasswordStoreMacInternalsTest, TestKeychainRemove) {
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
 
   // Add our test item so that we can delete it.
-  PasswordForm* add_form = CreatePasswordFormFromData(test_data[0].data);
+  scoped_ptr<PasswordForm> add_form =
+      CreatePasswordFormFromData(test_data[0].data);
   EXPECT_TRUE(owned_keychain_adapter.AddPassword(*add_form));
-  delete add_form;
 
   for (unsigned int i = 0; i < arraysize(test_data); ++i) {
-    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
-        test_data[i].data));
+    scoped_ptr<PasswordForm> form =
+        CreatePasswordFormFromData(test_data[i].data);
     EXPECT_EQ(test_data[i].should_succeed,
               owned_keychain_adapter.RemovePassword(*form));
 
@@ -890,13 +880,13 @@ TEST_F(PasswordStoreMacInternalsTest, TestFormMerge) {
     for (std::vector<PasswordFormData*>::iterator i =
              test_data[KEYCHAIN_INPUT][test_case].begin();
          i != test_data[KEYCHAIN_INPUT][test_case].end(); ++i) {
-      keychain_forms.push_back(CreatePasswordFormFromData(*(*i)));
+      keychain_forms.push_back(CreatePasswordFormFromData(*(*i)).release());
     }
     ScopedVector<autofill::PasswordForm> database_forms;
     for (std::vector<PasswordFormData*>::iterator i =
              test_data[DATABASE_INPUT][test_case].begin();
          i != test_data[DATABASE_INPUT][test_case].end(); ++i) {
-      database_forms.push_back(CreatePasswordFormFromData(*(*i)));
+      database_forms.push_back(CreatePasswordFormFromData(*(*i)).release());
     }
 
     ScopedVector<autofill::PasswordForm> merged_forms;
@@ -941,7 +931,7 @@ TEST_F(PasswordStoreMacInternalsTest, TestPasswordBulkLookup) {
   };
   ScopedVector<autofill::PasswordForm> database_forms;
   for (unsigned int i = 0; i < arraysize(db_data); ++i) {
-    database_forms.push_back(CreatePasswordFormFromData(db_data[i]));
+    database_forms.push_back(CreatePasswordFormFromData(db_data[i]).release());
   }
   ScopedVector<autofill::PasswordForm> merged_forms;
   internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
@@ -968,7 +958,7 @@ TEST_F(PasswordStoreMacInternalsTest, TestBlacklistedFiltering) {
   };
   ScopedVector<autofill::PasswordForm> database_forms;
   for (unsigned int i = 0; i < arraysize(db_data); ++i) {
-    database_forms.push_back(CreatePasswordFormFromData(db_data[i]));
+    database_forms.push_back(CreatePasswordFormFromData(db_data[i]).release());
   }
   ScopedVector<autofill::PasswordForm> merged_forms;
   internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
@@ -1069,8 +1059,8 @@ TEST_F(PasswordStoreMacInternalsTest, TestPasswordGetAll) {
       L"testname", L"testpass", false, false, 0 },
   };
   for (unsigned int i = 0; i < arraysize(owned_password_data); ++i) {
-    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
-        owned_password_data[i]));
+    scoped_ptr<PasswordForm> form =
+        CreatePasswordFormFromData(owned_password_data[i]);
     owned_keychain_adapter.AddPassword(*form);
   }
 
@@ -1137,7 +1127,9 @@ class PasswordStoreMacTest : public testing::Test {
     // to finish.
     MockPasswordStoreConsumer consumer;
     store_->GetLogins(PasswordForm(), PasswordStore::ALLOW_PROMPT, &consumer);
-    consumer.WaitOnGetPasswordStoreResults();
+    EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(_))
+        .WillOnce(QuitUIMessageLoop());
+    base::MessageLoop::current()->Run();
   }
 
   TestPasswordStoreMac* store() { return store_.get(); }
@@ -1161,7 +1153,7 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     "http://some.domain.com/insecure.html", "login.cgi",
     L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
   };
-  scoped_ptr<PasswordForm> joint_form(CreatePasswordFormFromData(joint_data));
+  scoped_ptr<PasswordForm> joint_form = CreatePasswordFormFromData(joint_data);
   login_db()->AddLogin(*joint_form);
   MockAppleKeychain::KeychainTestData joint_keychain_data = {
     kSecAuthenticationTypeHTMLForm, "some.domain.com",
@@ -1210,8 +1202,8 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     },
   };
   for (unsigned int i = 0; i < arraysize(updates); ++i) {
-    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
-        updates[i].form_data));
+    scoped_ptr<PasswordForm> form =
+        CreatePasswordFormFromData(updates[i].form_data);
     store_->UpdateLogin(*form);
   }
 
@@ -1219,8 +1211,8 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
 
   MacKeychainPasswordFormAdapter keychain_adapter(keychain());
   for (unsigned int i = 0; i < arraysize(updates); ++i) {
-    scoped_ptr<PasswordForm> query_form(
-        CreatePasswordFormFromData(updates[i].form_data));
+    scoped_ptr<PasswordForm> query_form =
+        CreatePasswordFormFromData(updates[i].form_data);
 
     ScopedVector<autofill::PasswordForm> matching_items =
         keychain_adapter.PasswordsFillingForm(query_form->signon_realm,
@@ -1263,7 +1255,7 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
     "http://www.facebook.com/index.html", "login",
     L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
   };
-  scoped_ptr<PasswordForm> www_form(CreatePasswordFormFromData(www_form_data));
+  scoped_ptr<PasswordForm> www_form = CreatePasswordFormFromData(www_form_data);
   login_db()->AddLogin(*www_form);
   MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain());
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
@@ -1276,11 +1268,15 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
 
   MockPasswordStoreConsumer consumer;
   store_->GetLogins(m_form, PasswordStore::ALLOW_PROMPT, &consumer);
-  consumer.WaitOnGetPasswordStoreResults();
-  EXPECT_EQ(1u, consumer.last_result.size());
+  PasswordForm returned_form;

[vasilii, 2015/02/05 19:23:26]

Unused now, but to be used in your next PatchSet :-)

[vabr (Chromium), 2015/02/06 14:16:05]

Acknowledged. :)

+  EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
+      .WillOnce(DoAll(
+          Invoke(&consumer, &MockPasswordStoreConsumer::SaveACopyOfFirstForm),
+          QuitUIMessageLoop()));
+  base::MessageLoop::current()->Run();
 
   // 3. Add the returned password for m.facebook.com.
-  login_db()->AddLogin(consumer.last_result[0]);
+  login_db()->AddLogin(consumer.first_form);
   owned_keychain_adapter.AddPassword(m_form);
 
   // 4. Remove both passwords.
@@ -1349,12 +1345,12 @@ void CheckRemoveLoginsBetween(PasswordStoreMacTest* test, bool check_created) {
       PasswordForm::SCHEME_HTML, "http://different.com/",
       "http://different.com/index.html", "login", L"submit", L"username",
       L"password", L"different_joe_user", L"sekrit", true, false, 0 };
-  scoped_ptr<PasswordForm> form_facebook(
-      CreatePasswordFormFromData(www_form_data_facebook));
-  scoped_ptr<PasswordForm> form_facebook_old(
-      CreatePasswordFormFromData(www_form_data_facebook_old));
-  scoped_ptr<PasswordForm> form_other(
-      CreatePasswordFormFromData(www_form_data_other));
+  scoped_ptr<PasswordForm> form_facebook =
+      CreatePasswordFormFromData(www_form_data_facebook);
+  scoped_ptr<PasswordForm> form_facebook_old =
+      CreatePasswordFormFromData(www_form_data_facebook_old);
+  scoped_ptr<PasswordForm> form_other =
+      CreatePasswordFormFromData(www_form_data_other);
   base::Time now = base::Time::Now();
   // TODO(vasilii): remove the next line once crbug/374132 is fixed.
   now = base::Time::FromTimeT(now.ToTimeT());
@@ -1453,7 +1449,8 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
       PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
       "http://www.facebook.com/index.html", "login", L"username", L"password",
       L"submit", L"joe_user", L"sekrit", true, false, 1 };
-  scoped_ptr<PasswordForm> www_form(CreatePasswordFormFromData(www_form_data1));
+  scoped_ptr<PasswordForm> www_form =
+      CreatePasswordFormFromData(www_form_data1);
   EXPECT_TRUE(owned_keychain_adapter.AddPassword(*www_form));
 
   // Add a password from the current profile.
@@ -1461,7 +1458,7 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
       PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
       "http://www.facebook.com/index.html", "login", L"username", L"password",
       L"submit", L"not_joe_user", L"12345", true, false, 1 };
-  www_form.reset(CreatePasswordFormFromData(www_form_data2));
+  www_form = CreatePasswordFormFromData(www_form_data2);
   store_->AddLogin(*www_form);
   FinishAsyncProcessing();
 
@@ -1506,7 +1503,7 @@ TEST_F(PasswordStoreMacTest, StoreIsUsableImmediatelyAfterConstruction) {
       PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
       "http://www.facebook.com/index.html", "login", L"username", L"password",
       L"submit", L"not_joe_user", L"12345", true, false, 1};
-  scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(www_form_data));
+  scoped_ptr<PasswordForm> form = CreatePasswordFormFromData(www_form_data);
   store()->AddLogin(*form);
 
   MockPasswordStoreConsumer mock_consumer;
@@ -1515,8 +1512,9 @@ TEST_F(PasswordStoreMacTest, StoreIsUsableImmediatelyAfterConstruction) {
   // Now the read/write tasks are scheduled, let the DB initialization proceed.
   event.Signal();
 
-  mock_consumer.WaitOnGetPasswordStoreResults();
-  EXPECT_EQ(1u, mock_consumer.last_result.size());
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
+      .WillOnce(QuitUIMessageLoop());
+  base::MessageLoop::current()->Run();
   EXPECT_TRUE(login_db());
 }
 
@@ -1538,7 +1536,7 @@ TEST_F(PasswordStoreMacTest, OperationsOnABadDatabaseSilentlyFail) {
       PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
       "http://www.facebook.com/index.html", "login", L"username", L"password",
       L"submit", L"not_joe_user", L"12345", true, false, 1};
-  scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(www_form_data));
+  scoped_ptr<PasswordForm> form = CreatePasswordFormFromData(www_form_data);
   scoped_ptr<PasswordForm> blacklisted_form(new PasswordForm(*form));
   blacklisted_form->signon_realm = "http://foo.example.com";
   blacklisted_form->origin = GURL("http://foo.example.com/origin");
@@ -1551,14 +1549,19 @@ TEST_F(PasswordStoreMacTest, OperationsOnABadDatabaseSilentlyFail) {
   // Get all logins; autofillable logins; blacklisted logins.
   MockPasswordStoreConsumer mock_consumer;
   store()->GetLogins(*form, PasswordStore::DISALLOW_PROMPT, &mock_consumer);
-  mock_consumer.WaitOnGetPasswordStoreResults();
-  EXPECT_TRUE(mock_consumer.last_result.empty());
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))

[vasilii, 2015/02/05 19:23:26]

IsEmpty()

[vabr (Chromium), 2015/02/06 14:16:04]

Done.

+      .WillOnce(QuitUIMessageLoop());
+  base::MessageLoop::current()->Run();
+
   store()->GetAutofillableLogins(&mock_consumer);
-  mock_consumer.WaitOnGetPasswordStoreResults();
-  EXPECT_TRUE(mock_consumer.last_result.empty());
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
+      .WillOnce(QuitUIMessageLoop());
+  base::MessageLoop::current()->Run();
+
   store()->GetBlacklistLogins(&mock_consumer);
-  mock_consumer.WaitOnGetPasswordStoreResults();
-  EXPECT_TRUE(mock_consumer.last_result.empty());
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
+      .WillOnce(QuitUIMessageLoop());

[vasilii, 2015/02/05 19:23:26]

This is repeated many times here. Sounds like a candidate for
ON_CALL().WillByDefault

[vabr (Chromium), 2015/02/06 14:16:04]

Done.

+  base::MessageLoop::current()->Run();
 
   // Report metrics.
   store()->ReportMetrics("Test Username", true);