Autofill form submission creates wrong addresses.

Adds logic to filter out address submissions that are redundant, or merge addresses that match existing addresses already stored in Autofill settings.

BUG=57975
TEST=PersonalDataManagerTest.Aggregate*

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

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

4 files changed, 262 insertions, 75 deletions

diff --git a/chrome/browser/autofill/autofill_profile.cc b/chrome/browser/autofill/autofill_profile.cc
index 7bc473d..efcdffd 100644
--- a/chrome/browser/autofill/autofill_profile.cc
+++ b/chrome/browser/autofill/autofill_profile.cc
@@ -432,6 +432,11 @@ bool AutoFillProfile::operator!=(const AutoFillProfile& profile) const {
   return !operator==(profile);
 }
 
+const string16 AutoFillProfile::PrimaryValue() const {
+  return GetFieldText(AutoFillType(NAME_FULL)) +
+         GetFieldText(AutoFillType(ADDRESS_HOME_LINE1));
+}
+
 Address* AutoFillProfile::GetHomeAddress() {
   return static_cast<Address*>(personal_info_[AutoFillType::ADDRESS_HOME]);
 }
diff --git a/chrome/browser/autofill/autofill_profile.h b/chrome/browser/autofill/autofill_profile.h
index 7244641..d201294 100644
--- a/chrome/browser/autofill/autofill_profile.h
+++ b/chrome/browser/autofill/autofill_profile.h
@@ -101,6 +101,11 @@ class AutoFillProfile : public FormGroup {
   virtual bool operator!=(const AutoFillProfile& profile) const;
   void set_label(const string16& label) { label_ = label; }
 
+  // Returns concatenation of full name and address line 1.  This acts as the
+  // basis of comparison for new values that are submitted through forms to
+  // aid with correct aggregation of new data.
+  const string16 PrimaryValue() const;
+
  private:
   Address* GetHomeAddress();
 
diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index 4264baf..578bb6a 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -177,7 +177,6 @@ bool PersonalDataManager::ImportFormData(
   // TODO(jhawkins): Use a hash of the CC# instead of a list of unique IDs?
   imported_credit_card_.reset(new CreditCard);
 
-  bool billing_address_info = false;
   std::vector<FormStructure*>::const_iterator iter;
   for (iter = form_structures.begin(); iter != form_structures.end(); ++iter) {
     const FormStructure* form = *iter;
@@ -232,12 +231,6 @@ bool PersonalDataManager::ImportFormData(
 
         imported_profile_->SetInfo(AutoFillType(field_type.field_type()),
                                    value);
-
-        // If we found any billing address information, then set the profile to
-        // use a separate billing address.
-        if (group == AutoFillType::ADDRESS_BILLING)
-          billing_address_info = true;
-
         ++importable_fields;
       }
     }
@@ -390,9 +383,6 @@ void PersonalDataManager::SetCreditCards(
 
 // TODO(jhawkins): Refactor SetProfiles so this isn't so hacky.
 void PersonalDataManager::AddProfile(const AutoFillProfile& profile) {
-  // Set to true if |profile| is merged into the profile list.
-  bool merged = false;
-
   // Don't save a web profile if the data in the profile is a subset of an
   // auxiliary profile.
   for (std::vector<AutoFillProfile*>::const_iterator iter =
@@ -402,24 +392,50 @@ void PersonalDataManager::AddProfile(const AutoFillProfile& profile) {
       return;
   }
 
+  // Set to true if |profile| is merged into the profile list.
+  bool merged = false;
+
+  // First preference is to add missing values to an existing profile.
+  // Only merge with the first match.
   std::vector<AutoFillProfile> profiles;
   for (std::vector<AutoFillProfile*>::const_iterator iter =
            web_profiles_.begin();
        iter != web_profiles_.end(); ++iter) {
-    if (profile.IsSubsetOf(**iter)) {
-      // In this case, the existing profile already contains all of the data
-      // in |profile|, so consider the profiles already merged.
-      merged = true;
-    } else if ((*iter)->IntersectionOfTypesHasEqualValues(profile)) {
-      // |profile| contains all of the data in this profile, plus
-      // more.
-      merged = true;
-      (*iter)->MergeWith(profile);
+    if (!merged) {
+      if (profile.IsSubsetOf(**iter)) {
+        // In this case, the existing profile already contains all of the data
+        // in |profile|, so consider the profiles already merged.
+        merged = true;
+      } else if ((*iter)->IntersectionOfTypesHasEqualValues(profile)) {
+        // |profile| contains all of the data in this profile, plus more.
+        merged = true;
+        (*iter)->MergeWith(profile);
+      }
     }
-
     profiles.push_back(**iter);
   }
 
+  // The second preference, if not merged above, is to alter non-primary values
+  // where the primary values match.
+  // Again, only merge with the first match.
+  if (!merged) {
+    profiles.clear();
+    for (std::vector<AutoFillProfile*>::const_iterator iter =
+             web_profiles_.begin();
+         iter != web_profiles_.end(); ++iter) {
+      if (!merged) {
+        if (!profile.PrimaryValue().empty() &&
+            (*iter)->PrimaryValue() == profile.PrimaryValue()) {
+          merged = true;
+          (*iter)->OverwriteWith(profile);
+        }
+      }
+      profiles.push_back(**iter);
+    }
+  }
+
+  // Finally, if the new profile was not merged with an existing profile then
+  // add the new profile to the list.
   if (!merged)
     profiles.push_back(profile);
 
diff --git a/chrome/browser/autofill/personal_data_manager_unittest.cc b/chrome/browser/autofill/personal_data_manager_unittest.cc
index cf83af9..8911a0a 100644
--- a/chrome/browser/autofill/personal_data_manager_unittest.cc
+++ b/chrome/browser/autofill/personal_data_manager_unittest.cc
@@ -604,25 +604,23 @@ TEST_F(PersonalDataManagerTest, SetUniqueCreditCardLabels) {
   EXPECT_EQ(ASCIIToUTF16("Work2"), results[5]->Label());
 }
 
-TEST_F(PersonalDataManagerTest, AggregateProfileData) {
-  scoped_ptr<FormData> form(new FormData);
-
+TEST_F(PersonalDataManagerTest, AggregateTwoDifferentProfiles) {
+  FormData form1;
   webkit_glue::FormField field;
   autofill_test::CreateTestFormField(
       "First name:", "first_name", "George", "text", &field);
-  form->fields.push_back(field);
+  form1.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Last name:", "last_name", "Washington", "text", &field);
-  form->fields.push_back(field);
+  form1.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Email:", "email", "theprez@gmail.com", "text", &field);
-  form->fields.push_back(field);
+  form1.fields.push_back(field);
 
-  scoped_ptr<FormStructure> form_structure(new FormStructure(*form));
-  scoped_ptr<std::vector<FormStructure*> > forms(
-      new std::vector<FormStructure*>);
-  forms->push_back(form_structure.get());
-  personal_data_->ImportFormData(*forms);
+  FormStructure form_structure1(form1);
+  std::vector<FormStructure*> forms;
+  forms.push_back(&form_structure1);
+  personal_data_->ImportFormData(forms);
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -630,31 +628,30 @@ TEST_F(PersonalDataManagerTest, AggregateProfileData) {
 
   MessageLoop::current()->Run();
 
-  scoped_ptr<AutoFillProfile> expected(
-      new AutoFillProfile);
-  autofill_test::SetProfileInfo(expected.get(), NULL, "George", NULL,
+  AutoFillProfile expected;
+  autofill_test::SetProfileInfo(&expected, NULL, "George", NULL,
       "Washington", "theprez@gmail.com", NULL, NULL, NULL, NULL, NULL, NULL,
       NULL, NULL, NULL);
-  const std::vector<AutoFillProfile*>& results = personal_data_->profiles();
-  ASSERT_EQ(1U, results.size());
-  EXPECT_EQ(0, expected->Compare(*results[0]));
+  const std::vector<AutoFillProfile*>& results1 = personal_data_->profiles();
+  ASSERT_EQ(1U, results1.size());
+  EXPECT_EQ(0, expected.Compare(*results1[0]));
 
   // Now create a completely different profile.
-  form.reset(new FormData);
+  FormData form2;
   autofill_test::CreateTestFormField(
       "First name:", "first_name", "John", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Last name:", "last_name", "Adams", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Email:", "email", "second@gmail.com", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
 
-  form_structure.reset(new FormStructure(*form));
-  forms.reset(new std::vector<FormStructure*>);
-  forms->push_back(form_structure.get());
-  personal_data_->ImportFormData(*forms);
+  FormStructure form_structure2(form2);
+  forms.clear();
+  forms.push_back(&form_structure2);
+  personal_data_->ImportFormData(forms);
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -663,45 +660,149 @@ TEST_F(PersonalDataManagerTest, AggregateProfileData) {
   MessageLoop::current()->Run();
 
   const std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
+
+  AutoFillProfile expected2;
+  autofill_test::SetProfileInfo(&expected2, NULL, "John", NULL,
+      "Adams", "second@gmail.com", NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+      NULL, NULL);
   ASSERT_EQ(2U, results2.size());
+  EXPECT_EQ(0, expected.Compare(*results2[0]));
+  EXPECT_EQ(0, expected2.Compare(*results2[1]));
+}
+
+TEST_F(PersonalDataManagerTest, AggregateSameProfileWithConflict) {
+  FormData form1;
+  webkit_glue::FormField field;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Address:", "address", "1600 Pennsylvania Avenue", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Email:", "email", "theprez@gmail.com", "text", &field);
+  form1.fields.push_back(field);
+
+  FormStructure form_structure1(form1);
+  std::vector<FormStructure*> forms;
+  forms.push_back(&form_structure1);
+  personal_data_->ImportFormData(forms);
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  MessageLoop::current()->Run();
+
+  AutoFillProfile expected;
+  autofill_test::SetProfileInfo(&expected, NULL, "George", NULL,
+      "Washington", "theprez@gmail.com", NULL, "1600 Pennsylvania Avenue", NULL,
+      NULL, NULL, NULL, NULL, NULL, NULL);
+  const std::vector<AutoFillProfile*>& results1 = personal_data_->profiles();
+  ASSERT_EQ(1U, results1.size());
+  EXPECT_EQ(0, expected.Compare(*results1[0]));
+
+  // Now create an updated profile.
+  FormData form2;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form2.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form2.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Address:", "address", "1600 Pennsylvania Avenue", "text", &field);
+  form2.fields.push_back(field);
+  // Country gets added.
+  autofill_test::CreateTestFormField(
+      "Country:", "country", "USA", "text", &field);
+  form2.fields.push_back(field);
+  // Email gets updated.
+  autofill_test::CreateTestFormField(
+      "Email:", "email", "new_email@gmail.com", "text", &field);
+  form2.fields.push_back(field);
+
+  FormStructure form_structure2(form2);
+  forms.clear();
+  forms.push_back(&form_structure2);
+  personal_data_->ImportFormData(forms);
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  MessageLoop::current()->Run();
+
+  const std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
+
+  AutoFillProfile expected2;
+  autofill_test::SetProfileInfo(&expected2, NULL, "George", NULL,
+      "Washington", "new_email@gmail.com", NULL, "1600 Pennsylvania Avenue",
+      NULL, NULL, NULL, NULL, "USA", NULL, NULL);
+  ASSERT_EQ(1U, results2.size());
+  EXPECT_EQ(0, expected2.Compare(*results2[0]));
+}
+
+TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInOld) {
+  FormData form1;
+  webkit_glue::FormField field;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Email:", "email", "theprez@gmail.com", "text", &field);
+  form1.fields.push_back(field);
+
+  FormStructure form_structure1(form1);
+  std::vector<FormStructure*> forms;
+  forms.push_back(&form_structure1);
+  personal_data_->ImportFormData(forms);
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  MessageLoop::current()->Run();
 
-  expected.reset(new AutoFillProfile);
-  autofill_test::SetProfileInfo(expected.get(), NULL, "George", NULL,
+  AutoFillProfile expected;
+  autofill_test::SetProfileInfo(&expected, NULL, "George", NULL,
       "Washington", "theprez@gmail.com", NULL, NULL, NULL, NULL, NULL, NULL,
       NULL, NULL, NULL);
-  EXPECT_EQ(0, expected->Compare(*results2[0]));
-
-  expected.reset(new AutoFillProfile);
-  autofill_test::SetProfileInfo(expected.get(), NULL, "John", NULL,
-      "Adams", "second@gmail.com", NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-      NULL, NULL);
-  EXPECT_EQ(0, expected->Compare(*results2[1]));
+  const std::vector<AutoFillProfile*>& results1 = personal_data_->profiles();
+  ASSERT_EQ(1U, results1.size());
+  EXPECT_EQ(0, expected.Compare(*results1[0]));
 
   // Submit a form with new data for the first profile.
-  form.reset(new FormData);
+  FormData form2;
   autofill_test::CreateTestFormField(
       "First name:", "first_name", "George", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Last name:", "last_name", "Washington", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Address Line 1:", "address", "190 High Street", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "City:", "city", "Philadelphia", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "State:", "state", "Pennsylvania", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
   autofill_test::CreateTestFormField(
       "Zip:", "zipcode", "19106", "text", &field);
-  form->fields.push_back(field);
+  form2.fields.push_back(field);
 
-  form_structure.reset(new FormStructure(*form));
-  forms.reset(new std::vector<FormStructure*>);
-  forms->push_back(form_structure.get());
-  personal_data_->ImportFormData(*forms);
+  FormStructure form_structure2(form2);
+  forms.clear();
+  forms.push_back(&form_structure2);
+  personal_data_->ImportFormData(forms);
 
   // Wait for the refresh.
   EXPECT_CALL(personal_data_observer_,
@@ -709,20 +810,80 @@ TEST_F(PersonalDataManagerTest, AggregateProfileData) {
 
   MessageLoop::current()->Run();
 
-  const std::vector<AutoFillProfile*>& results3 = personal_data_->profiles();
-  ASSERT_EQ(2U, results3.size());
+  const std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
 
-  expected.reset(new AutoFillProfile);
-  autofill_test::SetProfileInfo(expected.get(), NULL, "George", NULL,
+  AutoFillProfile expected2;
+  autofill_test::SetProfileInfo(&expected2, NULL, "George", NULL,
       "Washington", "theprez@gmail.com", NULL, "190 High Street", NULL,
       "Philadelphia", "Pennsylvania", "19106", NULL, NULL, NULL);
-  EXPECT_EQ(0, expected->Compare(*results3[0]));
+  ASSERT_EQ(1U, results2.size());
+  EXPECT_EQ(0, expected2.Compare(*results2[0]));
+}
 
-  expected.reset(new AutoFillProfile);
-  autofill_test::SetProfileInfo(expected.get(), NULL, "John", NULL,
-      "Adams", "second@gmail.com", NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-      NULL, NULL);
-  EXPECT_EQ(0, expected->Compare(*results3[1]));
+TEST_F(PersonalDataManagerTest, AggregateProfileWithMissingInfoInNew) {
+  FormData form1;
+  webkit_glue::FormField field;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Company:", "company", "Government", "text", &field);
+  form1.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Email:", "email", "theprez@gmail.com", "text", &field);
+  form1.fields.push_back(field);
+
+  FormStructure form_structure1(form1);
+  std::vector<FormStructure*> forms;
+  forms.push_back(&form_structure1);
+  personal_data_->ImportFormData(forms);
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  MessageLoop::current()->Run();
+
+  AutoFillProfile expected;
+  autofill_test::SetProfileInfo(&expected, NULL, "George", NULL,
+      "Washington", "theprez@gmail.com", "Government", NULL, NULL, NULL, NULL,
+      NULL, NULL, NULL, NULL);
+  const std::vector<AutoFillProfile*>& results1 = personal_data_->profiles();
+  ASSERT_EQ(1U, results1.size());
+  EXPECT_EQ(0, expected.Compare(*results1[0]));
+
+  // Submit a form with new data for the first profile.
+  FormData form2;
+  autofill_test::CreateTestFormField(
+      "First name:", "first_name", "George", "text", &field);
+  form2.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Last name:", "last_name", "Washington", "text", &field);
+  form2.fields.push_back(field);
+  // Note missing Company field.
+  autofill_test::CreateTestFormField(
+      "Email:", "email", "theprez@gmail.com", "text", &field);
+  form2.fields.push_back(field);
+
+  FormStructure form_structure2(form2);
+  forms.clear();
+  forms.push_back(&form_structure2);
+  personal_data_->ImportFormData(forms);
+
+  // Wait for the refresh.
+  EXPECT_CALL(personal_data_observer_,
+      OnPersonalDataLoaded()).WillOnce(QuitUIMessageLoop());
+
+  MessageLoop::current()->Run();
+
+  const std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
+
+  // Expect no change.
+  ASSERT_EQ(1U, results2.size());
+  EXPECT_EQ(0, expected.Compare(*results2[0]));
 }
 
 TEST_F(PersonalDataManagerTest, AggregateTwoDifferentCreditCards) {