From f1ed7278a2575b4c33eff0babb86a4b4e285352d Mon Sep 17 00:00:00 2001 From: "dhollowa@chromium.org" Date: Thu, 11 Nov 2010 21:57:07 +0000 Subject: 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 --- chrome/browser/autofill/autofill_profile.cc | 5 + chrome/browser/autofill/autofill_profile.h | 5 + chrome/browser/autofill/personal_data_manager.cc | 56 +++-- .../autofill/personal_data_manager_unittest.cc | 271 ++++++++++++++++----- 4 files changed, 262 insertions(+), 75 deletions(-) (limited to 'chrome') 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(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::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::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 profiles; for (std::vector::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::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 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 form_structure(new FormStructure(*form)); - scoped_ptr > forms( - new std::vector); - forms->push_back(form_structure.get()); - personal_data_->ImportFormData(*forms); + FormStructure form_structure1(form1); + std::vector 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 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& results = personal_data_->profiles(); - ASSERT_EQ(1U, results.size()); - EXPECT_EQ(0, expected->Compare(*results[0])); + const std::vector& 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); - 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& 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 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& 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& 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 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& 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); - 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& results3 = personal_data_->profiles(); - ASSERT_EQ(2U, results3.size()); + const std::vector& 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 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& 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& results2 = personal_data_->profiles(); + + // Expect no change. + ASSERT_EQ(1U, results2.size()); + EXPECT_EQ(0, expected.Compare(*results2[0])); } TEST_F(PersonalDataManagerTest, AggregateTwoDifferentCreditCards) { -- cgit v1.1