Componentize several Autofill unit tests.

components/autofill/core/browser/personal_data_manager_unittest.cc

Index: components/autofill/core/browser/personal_data_manager_unittest.cc
diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc
index 7d8194ce19a7103d2a346ff27a26e0f474d6656b..81f5672c59edd273277c6a438b7e95e2f9bd9dcc 100644
--- a/components/autofill/core/browser/personal_data_manager_unittest.cc
+++ b/components/autofill/core/browser/personal_data_manager_unittest.cc
@@ -5,13 +5,13 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/guid.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
-#include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
 #include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/autofill_test_utils.h"
@@ -24,19 +24,15 @@
 #include "components/autofill/core/common/form_data.h"
 #include "components/webdata/common/web_data_service_base.h"
 #include "components/webdata/common/web_database_service.h"
-#include "content/public/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
 
-using content::BrowserThread;
-
 namespace autofill {
 namespace {
 
 ACTION(QuitUIMessageLoop) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[blundell, 2014/03/05 15:55:01]

Is it important to preserve this DCHECK in some way here?

[Ilya Sherman, 2014/03/05 23:59:44]

Nah, though it might be worth renaming the action to something like
"QuitMainMessageLoop"

[blundell, 2014/03/06 09:32:32]

Done.

   base::MessageLoop::current()->Quit();
 }
 
@@ -61,65 +57,45 @@ class TestAutofillMetrics : public AutofillMetrics {
 
 class PersonalDataManagerTest : public testing::Test {
  protected:
-  PersonalDataManagerTest()
-      : ui_thread_(BrowserThread::UI, &message_loop_),
-        db_thread_(BrowserThread::DB) {
-  }
+  PersonalDataManagerTest() {}
 
   virtual void SetUp() {
-    db_thread_.Start();
 
+    prefs_ = test::PrefServiceForTesting();
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     base::FilePath path = temp_dir_.path().AppendASCII("TestWebDB");
-    web_database_ = new WebDatabaseService(
-        path,
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB));
+    web_database_ = new WebDatabaseService(path,
+                                           base::MessageLoopProxy::current(),
+                                           base::MessageLoopProxy::current());
     web_database_->AddTable(
         scoped_ptr<WebDatabaseTable>(new AutofillTable("en-US")));
     web_database_->LoadDatabase();
-    autofill_database_service_ = new AutofillWebDataService(
-        web_database_,
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB),
-        WebDataServiceBase::ProfileErrorCallback());
+    autofill_database_service_ =
+        new AutofillWebDataService(web_database_,
+                                   base::MessageLoopProxy::current(),
+                                   base::MessageLoopProxy::current(),
+                                   WebDataServiceBase::ProfileErrorCallback());
     autofill_database_service_->Init();
 
-    profile_.reset(new TestingProfile);
-    profile_->CreateWebDataService();
-
-    test::DisableSystemServices(profile_.get());
-    ResetPersonalDataManager();
+    ResetPersonalDataManager(false);
   }
 
   virtual void TearDown() {
     // Destruction order is imposed explicitly here.
     personal_data_.reset(NULL);
-    profile_.reset(NULL);
 
     autofill_database_service_->ShutdownOnUIThread();
     web_database_->ShutdownDatabase();
     autofill_database_service_ = NULL;
     web_database_ = NULL;
-
-    // Schedule another task on the DB thread to notify us that it's safe to

[blundell, 2014/03/05 15:55:01]

Related to the question that I have below, this code seems to indicate that
db_thread_ was using a different MessageLoop before. Is it important to preserve
that?

[Ilya Sherman, 2014/03/05 23:59:44]

No, this is a test pattern that's discouraged these days, so it's great that
you're cleaning it up :)

-    // stop the thread.
-    base::WaitableEvent done(false, false);
-    BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
-        base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done)));
-    done.Wait();
-    base::MessageLoop::current()->PostTask(FROM_HERE,
-                                           base::MessageLoop::QuitClosure());
-    base::MessageLoop::current()->Run();
-    db_thread_.Stop();
   }
 
-  void ResetPersonalDataManager() {
+  void ResetPersonalDataManager(bool is_incognito) {

[Ilya Sherman, 2014/03/05 23:59:44]

nit: Please define an enum for this parameter, rather than passing a boolean. 
An enum makes the semantics of call sites easier to understand.

[blundell, 2014/03/06 09:32:32]

Done.

     personal_data_.reset(new PersonalDataManager("en-US"));
     personal_data_->Init(
         scoped_refptr<AutofillWebDataService>(autofill_database_service_),
-        profile_->GetPrefs(),
-        profile_->IsOffTheRecord());
+        prefs_.get(),
+        is_incognito);
     personal_data_->AddObserver(&personal_data_observer_);
 
     // Verify that the web database has been updated and the notification sent.
@@ -128,16 +104,8 @@ class PersonalDataManagerTest : public testing::Test {
     base::MessageLoop::current()->Run();
   }
 
-  void MakeProfileIncognito() {
-    // Switch to an incognito profile.
-    profile_->ForceIncognito(true);
-    DCHECK(profile_->IsOffTheRecord());
-  }
-
   base::MessageLoopForUI message_loop_;
-  content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread db_thread_;

[blundell, 2014/03/05 15:55:01]

I can't remember how TestBrowserThread works off the top of my head -- were
these using the same or different message loops before? If they were different,
is it important to preserve that?

[Ilya Sherman, 2014/03/05 23:59:44]

These were previously different message loops, but it's good that you're
replacing them with just a single loop instead.  It might be appropriate to add
a TestBrowserThreadBundle to this test, though I suppose if things work without
it, then even better :)

[blundell, 2014/03/06 09:32:32]

I can't use TestBrowserThreadBundle here in any case as it's in //content.

On 2014/03/05 23:59:44, Ilya Sherman wrote:

-  scoped_ptr<TestingProfile> profile_;
+  scoped_ptr<PrefService> prefs_;
   scoped_refptr<AutofillWebDataService> autofill_database_service_;
   scoped_refptr<WebDatabaseService> web_database_;
   base::ScopedTempDir temp_dir_;
@@ -152,7 +120,7 @@ TEST_F(PersonalDataManagerTest, AddProfile) {
   personal_data_->AddProfile(profile0);
 
   // Reload the database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify the addition.
   const std::vector<AutofillProfile*>& results1 = personal_data_->GetProfiles();
@@ -165,7 +133,7 @@ TEST_F(PersonalDataManagerTest, AddProfile) {
   personal_data_->AddProfile(profile0a);
 
   // Reload the database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify the non-addition.
   const std::vector<AutofillProfile*>& results2 = personal_data_->GetProfiles();
@@ -183,7 +151,7 @@ TEST_F(PersonalDataManagerTest, AddProfile) {
   personal_data_->AddProfile(profile1);
 
   // Reload the database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify the addition.
   const std::vector<AutofillProfile*>& results3 = personal_data_->GetProfiles();
@@ -244,7 +212,7 @@ TEST_F(PersonalDataManagerTest, AddUpdateRemoveProfiles) {
   // Reset the PersonalDataManager.  This tests that the personal data was saved
   // to the web database, and that we can load the profiles from the web
   // database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify that we've loaded the profiles from the web database.
   const std::vector<AutofillProfile*>& results3 = personal_data_->GetProfiles();
@@ -299,7 +267,7 @@ TEST_F(PersonalDataManagerTest, AddUpdateRemoveCreditCards) {
   // Reset the PersonalDataManager.  This tests that the personal data was saved
   // to the web database, and that we can load the credit cards from the web
   // database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify that we've loaded the credit cards from the web database.
   const std::vector<CreditCard*>& results3 = personal_data_->GetCreditCards();
@@ -497,7 +465,7 @@ TEST_F(PersonalDataManagerTest, SetEmptyProfile) {
   // Reset the PersonalDataManager.  This tests that the personal data was saved
   // to the web database, and that we can load the profiles from the web
   // database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify that we've loaded the profiles from the web database.
   const std::vector<AutofillProfile*>& results2 = personal_data_->GetProfiles();
@@ -516,7 +484,7 @@ TEST_F(PersonalDataManagerTest, SetEmptyCreditCard) {
   // Reset the PersonalDataManager.  This tests that the personal data was saved
   // to the web database, and that we can load the credit cards from the web
   // database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Verify that we've loaded the credit cards from the web database.
   const std::vector<CreditCard*>& results2 = personal_data_->GetCreditCards();
@@ -954,7 +922,7 @@ TEST_F(PersonalDataManagerTest, SetUniqueCreditCardLabels) {
   // Reset the PersonalDataManager.  This tests that the personal data was saved
   // to the web database, and that we can load the credit cards from the web
   // database.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   const std::vector<CreditCard*>& results = personal_data_->GetCreditCards();
   ASSERT_EQ(6U, results.size());
@@ -1436,7 +1404,7 @@ TEST_F(PersonalDataManagerTest, AggregateProfileWithInsufficientAddress) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   const std::vector<AutofillProfile*>& profiles = personal_data_->GetProfiles();
   ASSERT_EQ(0U, profiles.size());
@@ -1624,7 +1592,7 @@ TEST_F(PersonalDataManagerTest, AggregateInvalidCreditCard) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   const std::vector<CreditCard*>& results2 = personal_data_->GetCreditCards();
   ASSERT_EQ(1U, results2.size());
@@ -1756,7 +1724,7 @@ TEST_F(PersonalDataManagerTest, AggregateEmptyCreditCardWithConflict) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // No change is expected.
   CreditCard expected2(base::GenerateGUID(), "https://www.example.com");
@@ -1823,7 +1791,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInNew) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // No change is expected.
   CreditCard expected2(base::GenerateGUID(), "https://www.example.com");
@@ -1851,7 +1819,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInNew) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // No change is expected.
   CreditCard expected3(base::GenerateGUID(), "https://www.example.com");
@@ -1959,7 +1927,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameCreditCardWithSeparators) {
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Expect that no new card is saved.
   const std::vector<CreditCard*>& results2 = personal_data_->GetCreditCards();
@@ -2067,7 +2035,7 @@ TEST_F(PersonalDataManagerTest,
 
   // Since no refresh is expected, reload the data from the database to make
   // sure no changes were written out.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
 
   // Expect that the saved credit card is not modified.
   const std::vector<CreditCard*>& results = personal_data_->GetCreditCards();
@@ -2426,11 +2394,9 @@ TEST_F(PersonalDataManagerTest, IncognitoReadOnly) {
       &bill_gates, "William H. Gates", "5555555555554444", "1", "2020");
   personal_data_->AddCreditCard(bill_gates);
 
-  MakeProfileIncognito();
-
   // The personal data manager should be able to read existing profiles in an
   // off-the-record context.
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(true);
   ASSERT_EQ(1U, personal_data_->GetProfiles().size());
   ASSERT_EQ(1U, personal_data_->GetCreditCards().size());
 
@@ -2445,7 +2411,7 @@ TEST_F(PersonalDataManagerTest, IncognitoReadOnly) {
       &larry_page, "Lawrence Page", "4111111111111111", "10", "2025");
   personal_data_->AddCreditCard(larry_page);
 
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(true);
   EXPECT_EQ(1U, personal_data_->GetProfiles().size());
   EXPECT_EQ(1U, personal_data_->GetCreditCards().size());
 
@@ -2456,7 +2422,7 @@ TEST_F(PersonalDataManagerTest, IncognitoReadOnly) {
   bill_gates.SetRawInfo(CREDIT_CARD_NAME, ASCIIToUTF16("Bill Gates"));
   personal_data_->SaveImportedCreditCard(bill_gates);
 
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(true);
   EXPECT_EQ(ASCIIToUTF16("Steven"),
             personal_data_->GetProfiles()[0]->GetRawInfo(NAME_FIRST));
   EXPECT_EQ(ASCIIToUTF16("William H. Gates"),
@@ -2469,7 +2435,7 @@ TEST_F(PersonalDataManagerTest, IncognitoReadOnly) {
   bill_gates.SetRawInfo(CREDIT_CARD_NAME, ASCIIToUTF16("Bill Gates"));
   personal_data_->UpdateCreditCard(bill_gates);
 
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(true);
   EXPECT_EQ(ASCIIToUTF16("Steven"),
             personal_data_->GetProfiles()[0]->GetRawInfo(NAME_FIRST));
   EXPECT_EQ(ASCIIToUTF16("William H. Gates"),
@@ -2479,7 +2445,7 @@ TEST_F(PersonalDataManagerTest, IncognitoReadOnly) {
   personal_data_->RemoveByGUID(steve_jobs.guid());
   personal_data_->RemoveByGUID(bill_gates.guid());
 
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(true);
   EXPECT_EQ(1U, personal_data_->GetProfiles().size());
   EXPECT_EQ(1U, personal_data_->GetCreditCards().size());
 }
@@ -2506,13 +2472,13 @@ TEST_F(PersonalDataManagerTest, DefaultCountryCodeIsCached) {
 
   // Disabling Autofill blows away this cache and shouldn't account for Autofill
   // profiles.
-  profile_->GetPrefs()->SetBoolean(prefs::kAutofillEnabled, false);
+  prefs_->SetBoolean(prefs::kAutofillEnabled, false);
   EXPECT_EQ(default_country,
             personal_data_->GetDefaultCountryCodeForNewAddress());
 
   // Enabling Autofill blows away the cached value and should reflect the new
   // value (accounting for profiles).
-  profile_->GetPrefs()->SetBoolean(prefs::kAutofillEnabled, true);
+  prefs_->SetBoolean(prefs::kAutofillEnabled, true);
   EXPECT_EQ(base::UTF16ToUTF8(moose.GetRawInfo(ADDRESS_HOME_COUNTRY)),
             personal_data_->GetDefaultCountryCodeForNewAddress());
 }
@@ -2523,7 +2489,7 @@ TEST_F(PersonalDataManagerTest, DefaultCountryCodeComesFromProfiles) {
       "", "1 Taiga TKTR", "", "Calgary", "AB", "T2B 2K2",
       "CA", "(800) 555-9000");
   personal_data_->AddProfile(moose);
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   EXPECT_EQ("CA", personal_data_->GetDefaultCountryCodeForNewAddress());
 
   // Multiple profiles cast votes.
@@ -2537,26 +2503,26 @@ TEST_F(PersonalDataManagerTest, DefaultCountryCodeComesFromProfiles) {
       "MX", "(800) 555-9000");
   personal_data_->AddProfile(armadillo);
   personal_data_->AddProfile(armadillo2);
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   EXPECT_EQ("MX", personal_data_->GetDefaultCountryCodeForNewAddress());
 
   personal_data_->RemoveByGUID(armadillo.guid());
   personal_data_->RemoveByGUID(armadillo2.guid());
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   // Verified profiles count more.
   armadillo.set_origin("http://randomwebsite.com");
   armadillo2.set_origin("http://randomwebsite.com");
   personal_data_->AddProfile(armadillo);
   personal_data_->AddProfile(armadillo2);
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   EXPECT_EQ("CA", personal_data_->GetDefaultCountryCodeForNewAddress());
 
   personal_data_->RemoveByGUID(armadillo.guid());
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   // But unverified profiles can be a tie breaker.
   armadillo.set_origin("Chrome settings");
   personal_data_->AddProfile(armadillo);
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   EXPECT_EQ("MX", personal_data_->GetDefaultCountryCodeForNewAddress());
 
   // Invalid country codes are ignored.
@@ -2567,7 +2533,7 @@ TEST_F(PersonalDataManagerTest, DefaultCountryCodeComesFromProfiles) {
       "mm@example.com", "", "1 Flying Object", "", "Valles Marineris", "",
       "", "XX", "");
   personal_data_->AddProfile(moose);
-  ResetPersonalDataManager();
+  ResetPersonalDataManager(false);
   EXPECT_EQ("MX", personal_data_->GetDefaultCountryCodeForNewAddress());
 }