Autofill: Be more selective when importing autofill data.

(a) Only save a profile if there are at least 3 fillable address fields; same for credit cards.
(b) Don't upload empty profiles to the autofill server

BUG=63540
TEST=unit_tests --gtest_filter=PersonalDataManagerTest.*

Review URL: http://codereview.chromium.org/5188010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67344 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 61 insertions, 32 deletions

diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index 784d405..786a9fe 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -22,8 +22,10 @@
 namespace {
 
 // The minimum number of fields that must contain user data and have known types
-// before AutoFill will attempt to import the data into a profile.
-const int kMinImportSize = 3;
+// before AutoFill will attempt to import the data into a profile or a credit
+// card.
+const int kMinProfileImportSize = 3;
+const int kMinCreditCardImportSize = 2;
 
 template<typename T>
 class FormGroupGUIDMatchesFunctor {
@@ -208,7 +210,9 @@ bool PersonalDataManager::ImportFormData(
           string16 city_code;
           string16 country_code;
           PhoneNumber::ParsePhoneNumber(value,
-                                        &number, &city_code, &country_code);
+                                        &number,
+                                        &city_code,
+                                        &country_code);
           if (number.empty())
             continue;
 
@@ -255,13 +259,9 @@ bool PersonalDataManager::ImportFormData(
 
   // If the user did not enter enough information on the page then don't bother
   // importing the data.
-  if (importable_fields + importable_credit_card_fields < kMinImportSize)
-    return false;
-
-  if (importable_fields == 0)
+  if (importable_fields < kMinProfileImportSize)
     imported_profile_.reset();
-
-  if (importable_credit_card_fields == 0)
+  if (importable_credit_card_fields < kMinCreditCardImportSize)
     imported_credit_card_.reset();
 
   if (imported_credit_card_.get()) {
@@ -283,10 +283,12 @@ bool PersonalDataManager::ImportFormData(
     }
   }
 
-  // We always save imported profiles.
-  SaveImportedProfile();
+  if (imported_profile_.get()) {
+    // We always save imported profiles.
+    SaveImportedProfile();
+  }
 
-  return true;
+  return imported_profile_.get() || imported_credit_card_.get();
 }
 
 void PersonalDataManager::GetImportedFormData(AutoFillProfile** profile,
diff --git a/chrome/browser/autofill/personal_data_manager_unittest.cc b/chrome/browser/autofill/personal_data_manager_unittest.cc
index 97c1403..11978df 100644
--- a/chrome/browser/autofill/personal_data_manager_unittest.cc
+++ b/chrome/browser/autofill/personal_data_manager_unittest.cc
@@ -526,7 +526,7 @@ TEST_F(PersonalDataManagerTest, ImportFormData) {
   FormStructure form_structure(form);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -543,6 +543,33 @@ TEST_F(PersonalDataManagerTest, ImportFormData) {
   EXPECT_EQ(0, expected.Compare(*results[0]));
 }
 
+TEST_F(PersonalDataManagerTest, ImportFormDataNotEnoughFilledFields) {
+  FormData form;
+  webkit_glue::FormField field;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Card number:", "card_number", "4111 1111 1111 1111", "text", &field);
+  form.fields.push_back(field);
+  FormStructure form_structure(form);
+  std::vector<FormStructure*> forms;
+  forms.push_back(&form_structure);
+  EXPECT_FALSE(personal_data_->ImportFormData(forms));
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  const std::vector<AutoFillProfile*>& profiles = personal_data_->profiles();
+  ASSERT_EQ(0U, profiles.size());
+  const std::vector<CreditCard*>& credit_cards = personal_data_->credit_cards();
+  ASSERT_EQ(0U, credit_cards.size());
+}
+
 TEST_F(PersonalDataManagerTest, ImportPhoneNumberSplitAcrossMultipleFields) {
   FormData form;
   webkit_glue::FormField field;
@@ -567,7 +594,7 @@ TEST_F(PersonalDataManagerTest, ImportPhoneNumberSplitAcrossMultipleFields) {
   FormStructure form_structure(form);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -660,7 +687,7 @@ TEST_F(PersonalDataManagerTest, AggregateTwoDifferentProfiles) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -691,7 +718,7 @@ TEST_F(PersonalDataManagerTest, AggregateTwoDifferentProfiles) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -729,7 +756,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameProfileWithConflict) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -768,7 +795,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameProfileWithConflict) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -802,7 +829,7 @@ TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInOld) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -842,7 +869,7 @@ TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInOld) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -879,7 +906,7 @@ TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInNew) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -911,7 +938,7 @@ TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInNew) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -947,7 +974,7 @@ TEST_F(PersonalDataManagerTest, AggregateTwoDifferentCreditCards) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -981,7 +1008,7 @@ TEST_F(PersonalDataManagerTest, AggregateTwoDifferentCreditCards) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1020,7 +1047,7 @@ TEST_F(PersonalDataManagerTest, AggregateInvalidCreditCard) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1054,7 +1081,7 @@ TEST_F(PersonalDataManagerTest, AggregateInvalidCreditCard) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_FALSE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Note: no refresh here.
@@ -1085,7 +1112,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameCreditCardWithConflict) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1120,7 +1147,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameCreditCardWithConflict) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1160,7 +1187,7 @@ TEST_F(PersonalDataManagerTest, AggregateEmptyCreditCardWithConflict) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1226,7 +1253,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInNew) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1292,7 +1319,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInOld) {
   FormStructure form_structure1(form1);
   std::vector<FormStructure*> forms;
   forms.push_back(&form_structure1);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.
@@ -1327,7 +1354,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInOld) {
   FormStructure form_structure2(form2);
   forms.clear();
   forms.push_back(&form_structure2);
-  personal_data_->ImportFormData(forms);
+  EXPECT_TRUE(personal_data_->ImportFormData(forms));
   personal_data_->SaveImportedCreditCard();
 
   // Wait for the refresh.