From 3a0204e7028c023d7ce81ef1d74a4d9ba8f3b3ab Mon Sep 17 00:00:00 2001 From: "georgey@chromium.org" Date: Sat, 7 Jan 2012 03:45:14 +0000 Subject: Added multi-valued field sync to Autofill (client) (Issue: Autofill profiles with multi-valued...) BUG=77444 TEST=unit-tests Review URL: http://codereview.chromium.org/8969006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@116812 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/autofill/autofill_profile.h | 5 +- .../sync/profile_sync_service_autofill_unittest.cc | 95 ++++++++- .../browser/sync/protocol/autofill_specifics.proto | 15 +- .../sync/protocol/proto_value_conversions.cc | 19 +- .../webdata/autofill_profile_syncable_service.cc | 227 ++++++++++++++------- .../webdata/autofill_profile_syncable_service.h | 33 ++- .../autofill_profile_syncable_service_unittest.cc | 190 +++++++++++++++-- 7 files changed, 473 insertions(+), 111 deletions(-) diff --git a/chrome/browser/autofill/autofill_profile.h b/chrome/browser/autofill/autofill_profile.h index df6fe30..c58d084 100644 --- a/chrome/browser/autofill/autofill_profile.h +++ b/chrome/browser/autofill/autofill_profile.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -109,7 +109,8 @@ class AutofillProfile : public FormGroup { // multi-valued items. int Compare(const AutofillProfile& profile) const; - // Comparison for Sync. Same as |Compare| but includes multi-valued fields. + // Full profile comparision, use instead of |Compare|. Same as |Compare| but + // includes multi-valued fields. int CompareMulti(const AutofillProfile& profile) const; // Equality operators compare GUIDs and the contents in the comparison. diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc index de786b5..be71eae 100644 --- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -114,7 +114,7 @@ class AutofillTableMock : public AutofillTable { bool(const std::vector&)); // NOLINT MOCK_METHOD1(GetAutofillProfiles, bool(std::vector*)); // NOLINT - MOCK_METHOD1(UpdateAutofillProfile, + MOCK_METHOD1(UpdateAutofillProfileMulti, bool(const AutofillProfile&)); // NOLINT MOCK_METHOD1(AddAutofillProfile, bool(const AutofillProfile&)); // NOLINT @@ -122,6 +122,11 @@ class AutofillTableMock : public AutofillTable { bool(const std::string&)); // NOLINT }; +MATCHER_P(MatchProfiles, profile, "") { + return (profile.CompareMulti(arg) == 0); +} + + class WebDatabaseFake : public WebDatabase { public: explicit WebDatabaseFake(AutofillTable* autofill_table) @@ -359,6 +364,10 @@ class ProfileSyncServiceAutofillTest : public AbstractProfileSyncServiceTest { personal_data_manager_ = static_cast( PersonalDataManagerFactory::GetInstance()->SetTestingFactoryAndUse( &profile_, PersonalDataManagerMock::Build)); + // GetHistoryService() gets called indirectly, but the result is ignored, so + // it is safe to return NULL. + EXPECT_CALL(profile_, GetHistoryService(_)). + WillRepeatedly(Return(static_cast(NULL))); EXPECT_CALL(*personal_data_manager_, LoadProfiles()).Times(1); EXPECT_CALL(*personal_data_manager_, LoadCreditCards()).Times(1); EXPECT_CALL(profile_, GetWebDataService(_)). @@ -723,6 +732,29 @@ class FakeServerUpdater : public base::RefCountedThreadSafe { syncable::Id parent_id_; }; +namespace { + +// Checks if the field of type |field_type| in |profile1| includes all values +// of the field in |profile2|. +bool IncludesField(const AutofillProfile& profile1, + const AutofillProfile& profile2, + AutofillFieldType field_type) { + std::vector values1; + profile1.GetMultiInfo(field_type, &values1); + std::vector values2; + profile2.GetMultiInfo(field_type, &values2); + + std::set values_set; + for (size_t i = 0; i < values1.size(); ++i) + values_set.insert(values1[i]); + for (size_t i = 0; i < values2.size(); ++i) + if (values_set.find(values2[i]) == values_set.end()) + return false; + return true; +} + +}; + // TODO(skrul): Test abort startup. // TODO(skrul): Test processing of cloud changes. // TODO(tim): Add autofill data type controller test, and a case to cover @@ -897,7 +929,8 @@ TEST_F(ProfileSyncServiceAutofillTest, HasNativeHasSyncMergeProfile) { sync_profiles.push_back(sync_profile); AddAutofillHelper add_autofill(this, sync_profiles); - EXPECT_CALL(autofill_table_, UpdateAutofillProfile(_)). + EXPECT_CALL(autofill_table_, + UpdateAutofillProfileMulti(MatchProfiles(sync_profile))). WillOnce(Return(true)); EXPECT_CALL(*personal_data_manager_, Refresh()); StartSyncService(add_autofill.callback(), false, syncable::AUTOFILL_PROFILE); @@ -907,7 +940,57 @@ TEST_F(ProfileSyncServiceAutofillTest, HasNativeHasSyncMergeProfile) { ASSERT_TRUE(GetAutofillProfilesFromSyncDBUnderProfileNode( &new_sync_profiles)); ASSERT_EQ(1U, new_sync_profiles.size()); - EXPECT_EQ(0, sync_profile.Compare(new_sync_profiles[0])); + EXPECT_EQ(0, sync_profile.CompareMulti(new_sync_profiles[0])); +} + +TEST_F(ProfileSyncServiceAutofillTest, HasNativeHasSyncMergeProfileCombine) { + AutofillProfile sync_profile; + autofill_test::SetProfileInfoWithGuid(&sync_profile, + "23355099-1170-4B71-8ED4-144470CC9EBE", "Billing", + "Mitchell", "Morrison", + "johnwayne@me.xyz", "Fox", "123 Zoo St.", "unit 5", "Hollywood", "CA", + "91601", "US", "12345678910"); + + AutofillProfile* native_profile = new AutofillProfile; + // Same address, but different names, phones and e-mails. + autofill_test::SetProfileInfoWithGuid(native_profile, + "23355099-1170-4B71-8ED4-144470CC9EBF", "Billing", "Alicia", "Saenz", + "joewayne@me.xyz", "Fox", "123 Zoo St.", "unit 5", "Hollywood", "CA", + "91601", "US", "19482937549"); + + AutofillProfile expected_profile(sync_profile); + expected_profile.OverwriteWithOrAddTo(*native_profile); + + std::vector native_profiles; + native_profiles.push_back(native_profile); + EXPECT_CALL(autofill_table_, GetAutofillProfiles(_)). + WillOnce(DoAll(SetArgumentPointee<0>(native_profiles), Return(true))); + EXPECT_CALL(autofill_table_, + AddAutofillProfile(MatchProfiles(expected_profile))). + WillOnce(Return(true)); + EXPECT_CALL(autofill_table_, + RemoveAutofillProfile("23355099-1170-4B71-8ED4-144470CC9EBF")). + WillOnce(Return(true)); + std::vector sync_profiles; + sync_profiles.push_back(sync_profile); + AddAutofillHelper add_autofill(this, sync_profiles); + + EXPECT_CALL(*personal_data_manager_, Refresh()); + StartSyncService(add_autofill.callback(), false, syncable::AUTOFILL_PROFILE); + ASSERT_TRUE(add_autofill.success()); + + std::vector new_sync_profiles; + ASSERT_TRUE(GetAutofillProfilesFromSyncDBUnderProfileNode( + &new_sync_profiles)); + ASSERT_EQ(1U, new_sync_profiles.size()); + // Check that key fields are the same. + EXPECT_TRUE(new_sync_profiles[0].IsSubsetOf(sync_profile)); + // Check that multivalued fields of the synced back data include original + // data. + EXPECT_TRUE(IncludesField(new_sync_profiles[0], sync_profile, NAME_FULL)); + EXPECT_TRUE(IncludesField(new_sync_profiles[0], sync_profile, EMAIL_ADDRESS)); + EXPECT_TRUE(IncludesField(new_sync_profiles[0], sync_profile, + PHONE_HOME_WHOLE_NUMBER)); } TEST_F(ProfileSyncServiceAutofillTest, MergeProfileWithDifferentGuid) { @@ -1114,10 +1197,6 @@ TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeRemoveProfile) { } TEST_F(ProfileSyncServiceAutofillTest, FLAKY_ServerChangeRace) { - // GetHistoryService() gets called indirectly, but the result is ignored, so - // it is safe to return NULL. - EXPECT_CALL(profile_, GetHistoryService(_)). - WillRepeatedly(Return(static_cast(NULL))); // Once for MergeDataAndStartSyncing() and twice for ProcessSyncChanges(), via // LoadAutofillData(). EXPECT_CALL(autofill_table_, GetAllAutofillEntries(_)). diff --git a/chrome/browser/sync/protocol/autofill_specifics.proto b/chrome/browser/sync/protocol/autofill_specifics.proto index d9026b1..0e9aded 100644 --- a/chrome/browser/sync/protocol/autofill_specifics.proto +++ b/chrome/browser/sync/protocol/autofill_specifics.proto @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // @@ -26,10 +26,10 @@ message AutofillProfileSpecifics { optional string guid = 15; // Contact info. - optional string name_first = 2; - optional string name_middle = 3; - optional string name_last = 4; - optional string email_address = 5; + repeated string name_first = 2; + repeated string name_middle = 3; + repeated string name_last = 4; + repeated string email_address = 5; optional string company_name = 6; // Home address. @@ -41,11 +41,10 @@ message AutofillProfileSpecifics { optional string address_home_country = 12; // Phone. - optional string phone_home_whole_number = 13; + repeated string phone_home_whole_number = 13; // Deprecated. - // TODO(jhawkins): Determine if this is safe to remove. - optional string phone_fax_whole_number = 14; + optional string phone_fax_whole_number = 14 [deprecated=true]; } message AutofillSpecifics { diff --git a/chrome/browser/sync/protocol/proto_value_conversions.cc b/chrome/browser/sync/protocol/proto_value_conversions.cc index 58b4c1d..3885fc2 100644 --- a/chrome/browser/sync/protocol/proto_value_conversions.cc +++ b/chrome/browser/sync/protocol/proto_value_conversions.cc @@ -84,6 +84,13 @@ ListValue* MakeRepeatedValue(const F& fields, V* (*converter_fn)(T)) { #define SET_INT64(field) SET(field, MakeInt64Value) #define SET_INT64_REP(field) SET_REP(field, MakeInt64Value) #define SET_STR(field) SET(field, Value::CreateStringValue) +#define SET_STR_REP(field) \ + value->Set(#field, \ + MakeRepeatedValue, \ + StringValue>(proto.field(), \ + Value::CreateStringValue)) #define SET_EXTENSION(ns, field, fn) \ do { \ @@ -232,10 +239,10 @@ DictionaryValue* AutofillProfileSpecificsToValue( SET_STR(label); SET_STR(guid); - SET_STR(name_first); - SET_STR(name_middle); - SET_STR(name_last); - SET_STR(email_address); + SET_STR_REP(name_first); + SET_STR_REP(name_middle); + SET_STR_REP(name_last); + SET_STR_REP(email_address); SET_STR(company_name); SET_STR(address_home_line1); @@ -245,8 +252,7 @@ DictionaryValue* AutofillProfileSpecificsToValue( SET_STR(address_home_zip); SET_STR(address_home_country); - SET_STR(phone_home_whole_number); - SET_STR(phone_fax_whole_number); + SET_STR_REP(phone_home_whole_number); return value; } @@ -400,6 +406,7 @@ DictionaryValue* EntitySpecificsToValue( #undef SET_INT64 #undef SET_INT64_REP #undef SET_STR +#undef SET_STR_REP #undef SET_EXTENSION diff --git a/chrome/browser/webdata/autofill_profile_syncable_service.cc b/chrome/browser/webdata/autofill_profile_syncable_service.cc index 028044c..ee2fc49 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service.cc +++ b/chrome/browser/webdata/autofill_profile_syncable_service.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -24,17 +24,6 @@ using content::BrowserThread; namespace { -// Helper to compare the local value and cloud value of a field, merge into -// the local value if they differ, and return whether the merge happened. -bool MergeField(FormGroup* form_group, - AutofillFieldType field_type, - const std::string& specifics_field) { - if (UTF16ToUTF8(form_group->GetInfo(field_type)) == specifics_field) - return false; - form_group->SetInfo(field_type, UTF8ToUTF16(specifics_field)); - return true; -} - std::string LimitData(const std::string& data) { std::string sanitized_value(data); if (sanitized_value.length() > AutofillTable::kMaxDataLength) @@ -72,7 +61,7 @@ SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( SyncChangeProcessor* sync_processor) { DCHECK(CalledOnValidThread()); DCHECK(!sync_processor_.get()); - VLOG(1) << "Associating Autofill: MergeDataAndStartSyncing"; + DVLOG(1) << "Associating Autofill: MergeDataAndStartSyncing"; if (!LoadAutofillData(&profiles_.get())) { return SyncError( @@ -80,17 +69,17 @@ SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( model_type()); } - if (VLOG_IS_ON(2)) { - VLOG(2) << "[AUTOFILL MIGRATION]" - << "Printing profiles from web db"; + if (DLOG_IS_ON(INFO)) { + DVLOG(2) << "[AUTOFILL MIGRATION]" + << "Printing profiles from web db"; for (ScopedVector::const_iterator ix = - profiles_.begin(); ix != profiles_.end(); ++ix) { + profiles_.begin(); ix != profiles_.end(); ++ix) { AutofillProfile* p = *ix; - VLOG(2) << "[AUTOFILL MIGRATION] " - << p->GetInfo(NAME_FIRST) - << p->GetInfo(NAME_LAST) - << p->guid(); + DVLOG(2) << "[AUTOFILL MIGRATION] " + << p->GetInfo(NAME_FIRST) + << p->GetInfo(NAME_LAST) + << p->guid(); } } @@ -106,10 +95,35 @@ SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( ++sync_iter) { GUIDToProfileMap::iterator it = CreateOrUpdateProfile(*sync_iter, &remaining_profiles, &bundle); + // |it| points to created/updated profile. Add it to the |profiles_map_| and + // then remove it from |remaining_profiles|. After this loop is completed + // |remaining_profiles| will have only those profiles that are not in the + // sync. profiles_map_[it->first] = it->second; remaining_profiles.erase(it); } + // Check for similar unmatched profiles - they are created independently on + // two systems, so merge them. + for (GUIDToProfileMap::iterator it = bundle.candidates_to_merge.begin(); + it != bundle.candidates_to_merge.end(); ++it) { + GUIDToProfileMap::iterator profile_to_merge = + remaining_profiles.find(it->first); + if (profile_to_merge != remaining_profiles.end()) { + bundle.profiles_to_delete.push_back(profile_to_merge->second->guid()); + if (MergeProfile(*(profile_to_merge->second), it->second)) + bundle.profiles_to_sync_back.push_back(it->second); + DVLOG(2) << "[AUTOFILL SYNC]" + << "Found similar profile in sync db but with a different guid: " + << UTF16ToUTF8(it->second->GetInfo(NAME_FIRST)) + << UTF16ToUTF8(it->second->GetInfo(NAME_LAST)) + << "New guid " << it->second->guid() + << ". Profile to be deleted " + << profile_to_merge->second->guid(); + remaining_profiles.erase(profile_to_merge); + } + } + if (!SaveChangesToWebData(bundle)) return SyncError(FROM_HERE, "Failed to update webdata.", model_type()); @@ -121,6 +135,12 @@ SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( profiles_map_[i->first] = i->second; } + for (size_t i = 0; i < bundle.profiles_to_sync_back.size(); ++i) { + new_changes.push_back( + SyncChange(SyncChange::ACTION_UPDATE, + CreateData(*(bundle.profiles_to_sync_back[i])))); + } + SyncError error; if (!new_changes.empty()) error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes); @@ -222,24 +242,25 @@ bool AutofillProfileSyncableService::SaveChangesToWebData( const DataBundle& bundle) { DCHECK(CalledOnValidThread()); + bool success = true; + for (size_t i = 0; i< bundle.profiles_to_delete.size(); ++i) { + if (!GetAutofillTable()->RemoveAutofillProfile( + bundle.profiles_to_delete[i])) + success = false; + } + for (size_t i = 0; i < bundle.profiles_to_add.size(); i++) { if (!GetAutofillTable()->AddAutofillProfile( *bundle.profiles_to_add[i])) - return false; + success = false; } for (size_t i = 0; i < bundle.profiles_to_update.size(); i++) { - if (!GetAutofillTable()->UpdateAutofillProfile( + if (!GetAutofillTable()->UpdateAutofillProfileMulti( *bundle.profiles_to_update[i])) - return false; + success = false; } - - for (size_t i = 0; i< bundle.profiles_to_delete.size(); ++i) { - if (!GetAutofillTable()->RemoveAutofillProfile( - bundle.profiles_to_delete[i])) - return false; - } - return true; + return success; } // static @@ -247,25 +268,30 @@ bool AutofillProfileSyncableService::OverwriteProfileWithServerData( const sync_pb::AutofillProfileSpecifics& specifics, AutofillProfile* profile) { bool diff = false; - diff = MergeField(profile, NAME_FIRST, specifics.name_first()) || diff; - diff = MergeField(profile, NAME_LAST, specifics.name_last()) || diff; - diff = MergeField(profile, NAME_MIDDLE, specifics.name_middle()) || diff; - diff = MergeField(profile, ADDRESS_HOME_LINE1, - specifics.address_home_line1()) || diff; - diff = MergeField(profile, ADDRESS_HOME_LINE2, - specifics.address_home_line2()) || diff; - diff = MergeField(profile, ADDRESS_HOME_CITY, - specifics.address_home_city()) || diff; - diff = MergeField(profile, ADDRESS_HOME_STATE, - specifics.address_home_state()) || diff; - diff = MergeField(profile, ADDRESS_HOME_COUNTRY, - specifics.address_home_country()) || diff; - diff = MergeField(profile, ADDRESS_HOME_ZIP, - specifics.address_home_zip()) || diff; - diff = MergeField(profile, EMAIL_ADDRESS, specifics.email_address()) || diff; - diff = MergeField(profile, COMPANY_NAME, specifics.company_name()) || diff; - diff = MergeField(profile, PHONE_HOME_WHOLE_NUMBER, - specifics.phone_home_whole_number()) || diff; + diff = UpdateMultivaluedField(NAME_FIRST, + specifics.name_first(), profile) || diff; + diff = UpdateMultivaluedField(NAME_MIDDLE, + specifics.name_middle(), profile) || diff; + diff = UpdateMultivaluedField(NAME_LAST, + specifics.name_last(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_LINE1, + specifics.address_home_line1(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_LINE2, + specifics.address_home_line2(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_CITY, + specifics.address_home_city(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_STATE, + specifics.address_home_state(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_COUNTRY, + specifics.address_home_country(), profile) || diff; + diff = UpdateField(ADDRESS_HOME_ZIP, + specifics.address_home_zip(), profile) || diff; + diff = UpdateMultivaluedField(EMAIL_ADDRESS, + specifics.email_address(), profile) || diff; + diff = UpdateField(COMPANY_NAME, specifics.company_name(), profile) || diff; + diff = UpdateMultivaluedField(PHONE_HOME_WHOLE_NUMBER, + specifics.phone_home_whole_number(), + profile) || diff; return diff; } @@ -278,12 +304,24 @@ void AutofillProfileSyncableService::WriteAutofillProfile( DCHECK(guid::IsValidGUID(profile.guid())); + // Reset all multi-valued fields in the protobuf. + specifics->clear_name_first(); + specifics->clear_name_middle(); + specifics->clear_name_last(); + specifics->clear_email_address(); + specifics->clear_phone_home_whole_number(); + specifics->set_guid(profile.guid()); - specifics->set_name_first( - LimitData(UTF16ToUTF8(profile.GetInfo(NAME_FIRST)))); - specifics->set_name_middle( - LimitData(UTF16ToUTF8(profile.GetInfo(NAME_MIDDLE)))); - specifics->set_name_last(LimitData(UTF16ToUTF8(profile.GetInfo(NAME_LAST)))); + std::vector values; + profile.GetMultiInfo(NAME_FIRST, &values); + for (size_t i = 0; i < values.size(); ++i) + specifics->add_name_first(LimitData(UTF16ToUTF8(values[i]))); + profile.GetMultiInfo(NAME_MIDDLE, &values); + for (size_t i = 0; i < values.size(); ++i) + specifics->add_name_middle(LimitData(UTF16ToUTF8(values[i]))); + profile.GetMultiInfo(NAME_LAST, &values); + for (size_t i = 0; i < values.size(); ++i) + specifics->add_name_last(LimitData(UTF16ToUTF8(values[i]))); specifics->set_address_home_line1( LimitData(UTF16ToUTF8(profile.GetInfo(ADDRESS_HOME_LINE1)))); specifics->set_address_home_line2( @@ -296,12 +334,14 @@ void AutofillProfileSyncableService::WriteAutofillProfile( LimitData(UTF16ToUTF8(profile.GetInfo(ADDRESS_HOME_COUNTRY)))); specifics->set_address_home_zip( LimitData(UTF16ToUTF8(profile.GetInfo(ADDRESS_HOME_ZIP)))); - specifics->set_email_address( - LimitData(UTF16ToUTF8(profile.GetInfo(EMAIL_ADDRESS)))); + profile.GetMultiInfo(EMAIL_ADDRESS, &values); + for (size_t i = 0; i < values.size(); ++i) + specifics->add_email_address(LimitData(UTF16ToUTF8(values[i]))); specifics->set_company_name( LimitData(UTF16ToUTF8(profile.GetInfo(COMPANY_NAME)))); - specifics->set_phone_home_whole_number( - LimitData(UTF16ToUTF8(profile.GetInfo(PHONE_HOME_WHOLE_NUMBER)))); + profile.GetMultiInfo(PHONE_HOME_WHOLE_NUMBER, &values); + for (size_t i = 0; i < values.size(); ++i) + specifics->add_phone_home_whole_number(LimitData(UTF16ToUTF8(values[i]))); } void AutofillProfileSyncableService::CreateGUIDToProfileMap( @@ -340,16 +380,21 @@ AutofillProfileSyncableService::CreateOrUpdateProfile( // Check if profile appears under a different guid. for (GUIDToProfileMap::iterator i = profile_map->begin(); i != profile_map->end(); ++i) { - if (i->second->Compare(*new_profile) == 0) { + if (i->second->CompareMulti(*new_profile) == 0) { bundle->profiles_to_delete.push_back(i->second->guid()); - VLOG(2) << "[AUTOFILL SYNC]" - << "Found in sync db but with a different guid: " - << UTF16ToUTF8(it->second->GetInfo(NAME_FIRST)) - << UTF16ToUTF8(it->second->GetInfo(NAME_LAST)) - << "New guid " << new_profile->guid() - << ". Profile to be deleted " << it->second->guid(); + DVLOG(2) << "[AUTOFILL SYNC]" + << "Found in sync db but with a different guid: " + << UTF16ToUTF8(i->second->GetInfo(NAME_FIRST)) + << UTF16ToUTF8(i->second->GetInfo(NAME_LAST)) + << "New guid " << new_profile->guid() + << ". Profile to be deleted " << i->second->guid(); profile_map->erase(i); break; + } else if (i->second->PrimaryValue() == new_profile->PrimaryValue()) { + // Add it to candidates for merge - if there is no profile with this + // guid we will merge them. + bundle->candidates_to_merge.insert(std::make_pair(i->second->guid(), + new_profile)); } } profiles_.push_back(new_profile); @@ -399,10 +444,10 @@ void AutofillProfileSyncableService::ActOnChange( } SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes); if (error.IsSet()) { - LOG(WARNING) << "[AUTOFILL SYNC]" - << " Failed processing change:" - << " Error:" << error.message() - << " Guid:" << change.key(); + DLOG(WARNING) << "[AUTOFILL SYNC]" + << " Failed processing change:" + << " Error:" << error.message() + << " Guid:" << change.key(); } } @@ -413,6 +458,48 @@ SyncData AutofillProfileSyncableService::CreateData( return SyncData::CreateLocalData(profile.guid(), profile.guid(), specifics); } +bool AutofillProfileSyncableService::UpdateField( + AutofillFieldType field_type, + const std::string& new_value, + AutofillProfile* autofill_profile) { + if (UTF16ToUTF8(autofill_profile->GetInfo(field_type)) == new_value) + return false; + autofill_profile->SetInfo(field_type, UTF8ToUTF16(new_value)); + return true; +} + +bool AutofillProfileSyncableService::UpdateMultivaluedField( + AutofillFieldType field_type, + const ::google::protobuf::RepeatedPtrField& new_values, + AutofillProfile* autofill_profile) { + std::vector values; + autofill_profile->GetMultiInfo(field_type, &values); + bool changed = false; + if (static_cast(new_values.size()) != values.size()) { + values.clear(); + values.resize(static_cast(new_values.size())); + changed = true; + } + for (size_t i = 0; i < values.size(); ++i) { + string16 synced_value( + UTF8ToUTF16(new_values.Get(static_cast(i)))); + if (values[i] != synced_value) { + values[i] = synced_value; + changed = true; + } + } + if (changed) + autofill_profile->SetMultiInfo(field_type, values); + return changed; +} + +bool AutofillProfileSyncableService::MergeProfile( + const AutofillProfile& merge_from, + AutofillProfile* merge_into) { + merge_into->OverwriteWithOrAddTo(merge_from); + return (merge_into->CompareMulti(merge_from) != 0); +} + AutofillTable* AutofillProfileSyncableService::GetAutofillTable() const { return web_data_service_->GetDatabase()->GetAutofillTable(); } diff --git a/chrome/browser/webdata/autofill_profile_syncable_service.h b/chrome/browser/webdata/autofill_profile_syncable_service.h index bab8d78..b797f06 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service.h +++ b/chrome/browser/webdata/autofill_profile_syncable_service.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #ifndef CHROME_BROWSER_WEBDATA_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_ @@ -13,6 +13,7 @@ #include "base/memory/scoped_vector.h" #include "base/synchronization/lock.h" #include "base/threading/non_thread_safe.h" +#include "chrome/browser/autofill/autofill_type.h" #include "chrome/browser/sync/api/sync_change.h" #include "chrome/browser/sync/api/sync_data.h" #include "chrome/browser/sync/api/sync_error.h" @@ -26,6 +27,7 @@ class AutofillProfile; class AutofillTable; +class FormGroup; class ProfileSyncServiceAutofillTest; class WebDataService; @@ -88,6 +90,12 @@ class AutofillProfileSyncableService ProcessSyncChanges); FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, ActOnChange); + FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, + UpdateField); + FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, + UpdateMultivaluedField); + FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, + MergeProfile); // The map of the guid to profiles owned by the |profiles_| vector. typedef std::map GUIDToProfileMap; @@ -123,6 +131,23 @@ class AutofillProfileSyncableService AutofillTable* GetAutofillTable() const; + // Helper to compare the local value and cloud value of a field, copy into + // the local value if they differ, and return whether the change happened. + static bool UpdateField(AutofillFieldType field_type, + const std::string& new_value, + AutofillProfile* autofill_profile); + // The same as |UpdateField|, but for multi-valued fields. + static bool UpdateMultivaluedField( + AutofillFieldType field_type, + const ::google::protobuf::RepeatedPtrField& new_value, + AutofillProfile* autofill_profile); + + // Calls merge_into->OverwriteWithOrAddTo() and then checks if the + // |merge_into| has extra data. Returns |true| if |merge_into| posseses some + // multi-valued field values that are not in |merge_from|, false otherwise. + static bool MergeProfile(const AutofillProfile& merge_from, + AutofillProfile* merge_into); + // For unit-tests. AutofillProfileSyncableService(); void set_sync_processor(SyncChangeProcessor* sync_processor) { @@ -150,6 +175,12 @@ struct AutofillProfileSyncableService::DataBundle { std::vector profiles_to_delete; std::vector profiles_to_update; std::vector profiles_to_add; + + // When we go through sync we find profiles that are similar but unmatched. + // Merge such profiles. + GUIDToProfileMap candidates_to_merge; + // Profiles that have multi-valued fields that are not in sync. + std::vector profiles_to_sync_back; }; #endif // CHROME_BROWSER_WEBDATA_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_ diff --git a/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc b/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc index ef2f5d8..f839d09 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc +++ b/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -24,6 +24,12 @@ using ::testing::Property; using content::BrowserThread; class AutofillProfile; +// Some guids for testing. +std::string kGuid1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44B"; +std::string kGuid2 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; +std::string kGuid3 = "EDC609ED-7EEE-4F27-B00C-423242A9C44D"; +std::string kGuid4 = "EDC609ED-7EEE-4F27-B00C-423242A9C44E"; + class MockAutofillProfileSyncableService : public AutofillProfileSyncableService { public: @@ -97,7 +103,8 @@ class MockSyncChangeProcessor : public SyncChangeProcessor { class AutofillProfileSyncableServiceTest : public testing::Test { public: AutofillProfileSyncableServiceTest() - : db_thread_(BrowserThread::DB, &message_loop_) {} + : ui_thread_(BrowserThread::UI, &message_loop_), + db_thread_(BrowserThread::DB, &message_loop_) {} virtual void SetUp() OVERRIDE { sync_processor_.reset(new MockSyncChangeProcessor); @@ -113,6 +120,7 @@ class AutofillProfileSyncableServiceTest : public testing::Test { } protected: MessageLoop message_loop_; + content::TestBrowserThread ui_thread_; content::TestBrowserThread db_thread_; MockAutofillProfileSyncableService autofill_syncable_service_; scoped_ptr sync_processor_; @@ -120,15 +128,19 @@ class AutofillProfileSyncableServiceTest : public testing::Test { TEST_F(AutofillProfileSyncableServiceTest, MergeDataAndStartSyncing) { std::vector profiles_from_web_db; - std::string guid_present1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44B"; - std::string guid_present2 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; - std::string guid_synced1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44D"; - std::string guid_synced2 = "EDC609ED-7EEE-4F27-B00C-423242A9C44E"; + std::string guid_present1 = kGuid1; + std::string guid_present2 = kGuid2; + std::string guid_synced1 = kGuid3; + std::string guid_synced2 = kGuid4; profiles_from_web_db.push_back(new AutofillProfile(guid_present1)); profiles_from_web_db.back()->SetInfo(NAME_FIRST, UTF8ToUTF16("John")); + profiles_from_web_db.back()->SetInfo(ADDRESS_HOME_LINE1, + UTF8ToUTF16("1 1st st")); profiles_from_web_db.push_back(new AutofillProfile(guid_present2)); profiles_from_web_db.back()->SetInfo(NAME_FIRST, UTF8ToUTF16("Tom")); + profiles_from_web_db.back()->SetInfo(ADDRESS_HOME_LINE1, + UTF8ToUTF16("2 2nd st")); SyncDataList data_list; AutofillProfile profile1(guid_synced1); @@ -175,8 +187,8 @@ TEST_F(AutofillProfileSyncableServiceTest, MergeDataAndStartSyncing) { TEST_F(AutofillProfileSyncableServiceTest, GetAllSyncData) { std::vector profiles_from_web_db; - std::string guid_present1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44B"; - std::string guid_present2 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; + std::string guid_present1 = kGuid1; + std::string guid_present2 = kGuid2; profiles_from_web_db.push_back(new AutofillProfile(guid_present1)); profiles_from_web_db.back()->SetInfo(NAME_FIRST, UTF8ToUTF16("John")); @@ -213,8 +225,8 @@ TEST_F(AutofillProfileSyncableServiceTest, GetAllSyncData) { TEST_F(AutofillProfileSyncableServiceTest, ProcessSyncChanges) { std::vector profiles_from_web_db; - std::string guid_present = "EDC609ED-7EEE-4F27-B00C-423242A9C44B"; - std::string guid_synced = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; + std::string guid_present = kGuid1; + std::string guid_synced = kGuid2; SyncChangeList change_list; AutofillProfile profile(guid_synced); @@ -244,18 +256,164 @@ TEST_F(AutofillProfileSyncableServiceTest, ProcessSyncChanges) { } TEST_F(AutofillProfileSyncableServiceTest, ActOnChange) { - std::string guid1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44B"; - std::string guid2 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; - - AutofillProfile profile(guid1); + AutofillProfile profile(kGuid1); profile.SetInfo(NAME_FIRST, UTF8ToUTF16("Jane")); - AutofillProfileChange change1(AutofillProfileChange::ADD, guid1, &profile); - AutofillProfileChange change2(AutofillProfileChange::REMOVE, guid2, NULL); + AutofillProfileChange change1(AutofillProfileChange::ADD, kGuid1, &profile); + AutofillProfileChange change2(AutofillProfileChange::REMOVE, kGuid2, NULL); ON_CALL(*sync_processor_, ProcessSyncChanges(_, _)) .WillByDefault(Return(SyncError(FROM_HERE, std::string("an error"), syncable::AUTOFILL_PROFILE))); + EXPECT_CALL(*sync_processor_, ProcessSyncChanges(_, _)).Times(2); autofill_syncable_service_.set_sync_processor(sync_processor_.get()); autofill_syncable_service_.ActOnChange(change1); autofill_syncable_service_.ActOnChange(change2); } + +TEST_F(AutofillProfileSyncableServiceTest, UpdateField) { + AutofillProfile profile(kGuid1); + std::string company1 = "A Company"; + std::string company2 = "Another Company"; + profile.SetInfo(COMPANY_NAME, UTF8ToUTF16(company1)); + EXPECT_FALSE(AutofillProfileSyncableService::UpdateField( + COMPANY_NAME, company1, &profile)); + EXPECT_EQ(profile.GetInfo(COMPANY_NAME), UTF8ToUTF16(company1)); + EXPECT_TRUE(AutofillProfileSyncableService::UpdateField( + COMPANY_NAME, company2, &profile)); + EXPECT_EQ(profile.GetInfo(COMPANY_NAME), UTF8ToUTF16(company2)); + EXPECT_FALSE(AutofillProfileSyncableService::UpdateField( + COMPANY_NAME, company2, &profile)); + EXPECT_EQ(profile.GetInfo(COMPANY_NAME), UTF8ToUTF16(company2)); +} + +TEST_F(AutofillProfileSyncableServiceTest, UpdateMultivaluedField) { + AutofillProfile profile(kGuid1); + + std::vector values; + values.push_back(UTF8ToUTF16("1@1.com")); + values.push_back(UTF8ToUTF16("2@1.com")); + profile.SetMultiInfo(EMAIL_ADDRESS, values); + + ::google::protobuf::RepeatedPtrField specifics_fields; + specifics_fields.AddAllocated(new std::string("2@1.com")); + specifics_fields.AddAllocated(new std::string("3@1.com")); + + EXPECT_TRUE(AutofillProfileSyncableService::UpdateMultivaluedField( + EMAIL_ADDRESS, specifics_fields, &profile)); + profile.GetMultiInfo(EMAIL_ADDRESS, &values); + ASSERT_TRUE(values.size() == 2); + EXPECT_EQ(values[0], UTF8ToUTF16("2@1.com")); + EXPECT_EQ(values[1], UTF8ToUTF16("3@1.com")); + + EXPECT_FALSE(AutofillProfileSyncableService::UpdateMultivaluedField( + EMAIL_ADDRESS, specifics_fields, &profile)); + profile.GetMultiInfo(EMAIL_ADDRESS, &values); + ASSERT_EQ(values.size(), 2U); + EXPECT_EQ(values[0], UTF8ToUTF16("2@1.com")); + EXPECT_EQ(values[1], UTF8ToUTF16("3@1.com")); + EXPECT_TRUE(AutofillProfileSyncableService::UpdateMultivaluedField( + EMAIL_ADDRESS, ::google::protobuf::RepeatedPtrField(), + &profile)); + profile.GetMultiInfo(EMAIL_ADDRESS, &values); + ASSERT_EQ(values.size(), 1U); // Always have at least an empty string. + EXPECT_EQ(values[0], UTF8ToUTF16("")); +} + +TEST_F(AutofillProfileSyncableServiceTest, MergeProfile) { + AutofillProfile profile1(kGuid1); + profile1.SetInfo(ADDRESS_HOME_LINE1, UTF8ToUTF16("111 First St.")); + + std::vector values; + values.push_back(UTF8ToUTF16("1@1.com")); + values.push_back(UTF8ToUTF16("2@1.com")); + profile1.SetMultiInfo(EMAIL_ADDRESS, values); + + AutofillProfile profile2(kGuid2); + profile2.SetInfo(ADDRESS_HOME_LINE1, UTF8ToUTF16("111 First St.")); + + // |values| now is [ "1@1.com", "2@1.com", "3@1.com" ]. + values.push_back(UTF8ToUTF16("3@1.com")); + profile2.SetMultiInfo(EMAIL_ADDRESS, values); + + values.clear(); + values.push_back(UTF8ToUTF16("John")); + profile1.SetMultiInfo(NAME_FIRST, values); + values.push_back(UTF8ToUTF16("Jane")); + profile2.SetMultiInfo(NAME_FIRST, values); + + values.clear(); + values.push_back(UTF8ToUTF16("Doe")); + profile1.SetMultiInfo(NAME_LAST, values); + values.push_back(UTF8ToUTF16("Other")); + profile2.SetMultiInfo(NAME_LAST, values); + + values.clear(); + values.push_back(UTF8ToUTF16("650234567")); + profile2.SetMultiInfo(PHONE_HOME_WHOLE_NUMBER, values); + + EXPECT_FALSE(AutofillProfileSyncableService::MergeProfile(profile2, + &profile1)); + + profile1.GetMultiInfo(NAME_FIRST, &values); + ASSERT_EQ(values.size(), 2U); + EXPECT_EQ(values[0], UTF8ToUTF16("John")); + EXPECT_EQ(values[1], UTF8ToUTF16("Jane")); + + profile1.GetMultiInfo(NAME_LAST, &values); + ASSERT_EQ(values.size(), 2U); + EXPECT_EQ(values[0], UTF8ToUTF16("Doe")); + EXPECT_EQ(values[1], UTF8ToUTF16("Other")); + + profile1.GetMultiInfo(EMAIL_ADDRESS, &values); + ASSERT_EQ(values.size(), 3U); + EXPECT_EQ(values[0], UTF8ToUTF16("1@1.com")); + EXPECT_EQ(values[1], UTF8ToUTF16("2@1.com")); + EXPECT_EQ(values[2], UTF8ToUTF16("3@1.com")); + + profile1.GetMultiInfo(PHONE_HOME_WHOLE_NUMBER, &values); + ASSERT_EQ(values.size(), 1U); + EXPECT_EQ(values[0], UTF8ToUTF16("650234567")); + + AutofillProfile profile3(kGuid3); + profile3.SetInfo(ADDRESS_HOME_LINE1, UTF8ToUTF16("111 First St.")); + + values.clear(); + values.push_back(UTF8ToUTF16("Jane")); + profile3.SetMultiInfo(NAME_FIRST, values); + + values.clear(); + values.push_back(UTF8ToUTF16("Doe")); + profile3.SetMultiInfo(NAME_LAST, values); + + EXPECT_TRUE(AutofillProfileSyncableService::MergeProfile(profile3, + &profile1)); + + profile1.GetMultiInfo(NAME_FIRST, &values); + ASSERT_EQ(values.size(), 3U); + EXPECT_EQ(values[0], UTF8ToUTF16("John")); + EXPECT_EQ(values[1], UTF8ToUTF16("Jane")); + EXPECT_EQ(values[2], UTF8ToUTF16("Jane")); + + profile1.GetMultiInfo(NAME_LAST, &values); + ASSERT_EQ(values.size(), 3U); + EXPECT_EQ(values[0], UTF8ToUTF16("Doe")); + EXPECT_EQ(values[1], UTF8ToUTF16("Other")); + EXPECT_EQ(values[2], UTF8ToUTF16("Doe")); + + // Middle name should have three entries as well. + profile1.GetMultiInfo(NAME_MIDDLE, &values); + ASSERT_EQ(values.size(), 3U); + EXPECT_TRUE(values[0].empty()); + EXPECT_TRUE(values[1].empty()); + EXPECT_TRUE(values[2].empty()); + + profile1.GetMultiInfo(EMAIL_ADDRESS, &values); + ASSERT_EQ(values.size(), 3U); + EXPECT_EQ(values[0], UTF8ToUTF16("1@1.com")); + EXPECT_EQ(values[1], UTF8ToUTF16("2@1.com")); + EXPECT_EQ(values[2], UTF8ToUTF16("3@1.com")); + + profile1.GetMultiInfo(PHONE_HOME_WHOLE_NUMBER, &values); + ASSERT_EQ(values.size(), 1U); + EXPECT_EQ(values[0], UTF8ToUTF16("650234567")); +} -- cgit v1.1