Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891903002:
[Autofill] Set basic information when adding a new profiles and credit cards. (Closed)
|
Created:
4 years, 8 months ago by sebsg Modified:
4 years, 8 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, Justin Donnelly Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the use count to one and the use date and modification date to "now".
Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440.
Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles.
BUG=603640, 248440
TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest
Committed: https://crrev.com/43391bda1708665a069ca796e59c60b15720b37c
Cr-Commit-Position: refs/heads/master@{#389545}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed sync merge return value #
Total comments: 2
Patch Set 3 : Nit #Patch Set 4 : Further changes to the sync logic #
Total comments: 40
Patch Set 5 : Addressed comments #
Total comments: 4
Patch Set 6 : Rebase #Patch Set 7 : Fixed test that was added in rebase and removed unnecessary test. #Messages
Total messages: 38 (22 generated)
Description was changed from ========== [Autofill] Set basic information when adding a new profiles and credit cards. Set the use count to one and the use date and modification date to "now". BUG=603640 TEST=PersonalDataManagerTest ========== to ========== Set the use count to one and the use date and modification date to "now". BUG=603640 TEST=PersonalDataManagerTest ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Could you please take a look? Thanks!
Thanks Seb, some questions about tests https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:4585: personal_data_.ClearAutofillProfiles(); unclear why we need to call this at the beginning of the test? Is there bleeding from one test to the other? If so, can it be in AutofillManagerTest::SetUp? https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: if (profile.use_count() > use_count()) as discussed, let's keep the + here. https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1152: TEST_F(AutofillProfileSyncableServiceTest, SyncUpdatesEmptyUsageStats) { Can we have a test where Local has some use counts and Sync has some use counts? I don't see it here and it would be good to document what is happening in that case. https://codereview.chromium.org/1891903002/diff/80001/components/browser_sync... File components/browser_sync/browser/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/browser_sync... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:988: sync_profile.set_use_date(base::Time()); can we have non zero use dates here and see what happens?
Description was changed from ========== Set the use count to one and the use date and modification date to "now". BUG=603640 TEST=PersonalDataManagerTest ========== to ========== Set the use count to one and the use date and modification date to "now". BUG=603640, 248440 TEST=PersonalDataManagerTest ==========
Patchset #2 (id:100001) has been deleted
Could you take another look? Thanks! https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:4585: personal_data_.ClearAutofillProfiles(); On 2016/04/19 14:29:35, Mathieu Perreault wrote: > unclear why we need to call this at the beginning of the test? Is there bleeding > from one test to the other? If so, can it be in AutofillManagerTest::SetUp? It's because test profiles and credit cards are created in the constructor of TestPersonalDataManager. Since almost all the test use those, I thought it would be simpler to remove them for the specific tests. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: if (profile.use_count() > use_count()) On 2016/04/19 14:29:35, Mathieu Perreault wrote: > as discussed, let's keep the + here. Done. https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1152: TEST_F(AutofillProfileSyncableServiceTest, SyncUpdatesEmptyUsageStats) { On 2016/04/19 14:29:35, Mathieu Perreault wrote: > Can we have a test where Local has some use counts and Sync has some use counts? > I don't see it here and it would be good to document what is happening in that > case. Done. https://codereview.chromium.org/1891903002/diff/80001/components/browser_sync... File components/browser_sync/browser/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/80001/components/browser_sync... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:988: sync_profile.set_use_date(base::Time()); On 2016/04/19 14:29:35, Mathieu Perreault wrote: > can we have non zero use dates here and see what happens? Done.
lgtm https://codereview.chromium.org/1891903002/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1178: // Local autofill profile has 0 for use_count/use_date. remove?
sebsg@chromium.org changed reviewers: + zea@chromium.org
zea@chromium.org: Could you please review the changes in components/browser_sync/browser/profile_sync_service_autofill_unittest.cc ? Thanks. https://codereview.chromium.org/1891903002/diff/120001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/120001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1178: // Local autofill profile has 0 for use_count/use_date. On 2016/04/20 20:48:39, Mathieu Perreault wrote: > remove? Done.
QQ: will this trigger updating all autofill profile sync data on upgrade? Or will this only take effect for modified profiles?
Patchset #4 (id:160001) has been deleted
Description was changed from ========== Set the use count to one and the use date and modification date to "now". BUG=603640, 248440 TEST=PersonalDataManagerTest ========== to ========== Set the use count to one and the use date and modification date to "now". Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440. Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles. BUG=603640, 248440 TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest ==========
Hi, I made some further changes to the sync logic and modified the CL description to detail those changes. zea@: To answer your question, it should not trigger an update of all the existing profiles. It should change how merges between similar profiles are done. It should also set new default usage stats to new profiles. Thanks!
LGTM with some comment clarification nits https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... File components/browser_sync/browser/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:984: // deleted and that the sync profile gets merged down as a local profile. I find it tough to follow this comment. Are you saying that the newly added profile gets deleted? And what does it mean for the sync profile to be "merged down as a local profile"? If by "merged down" you mean merged into the native model, I think it's clearer to state that explicitly. https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:1032: // Make sure the new information was merged down. Same as above. "merged down" -> "merged into the native model" to be explicit (here and below)
Mostly comments and formatting changes, so that it's clear what's going on. Thanks https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:419: bool AutofillProfile::EqualsForSyncPurposes( nit: let's revert those lines to previous formatting since it's only a formatting change now and it would create an unnecessary blame. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:1034: language_code() == profile.language_code() && Compare(profile) == 0; leave previous formatting? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:123: GUIDToProfileMap remaining_profiles; nit: rename to remaining_local_profiles? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:126: // Go through and check for all the profiles that sync already knows about. I would prefer // For every incoming profile from sync, attempt to update a local profile or otherwise create a new one. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:135: remaining_profiles.erase(it); Mention that this may be a no-op since |it| is sometimes an entirely new profile that came from sync. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:140: for (const auto& it : bundle.candidates_to_merge) { rename "it" -> sync_profile_it ? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:144: bundle.profiles_to_delete.push_back(profile_to_merge->second->guid()); Let's add a comment: // For similar profile pairs, the local profile is always removed and its content merged (if applicable) in the profile that came from sync. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:145: if (MergeSimilarProfile(*(profile_to_merge->second), it.second, Let's add a comment: if new changes were merged into |it.second| from |profile_to_merge|, they will be synced back. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:635: AutofillProfile old_merge_into = *merge_into; const? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.h (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:166: // |merge_into| has extra data. Returns true the merge has made a change to *if the merge https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:169: static bool MergeSimilarProfile(const AutofillProfile& merge_from, nit: MergeSimilarProfiles? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:406: 2); // Merging two profile add their user count. nit: weird format. Let's move the comment to its own line https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:601: Mention what is tested here and possibly have a more descriptive test name, perhaps MergeSimilarProfiles_AdditionalInfoInBothProfiles https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:603: AutofillProfile profile1(kGuid1, kHttpOrigin); rename to |from_profile| ? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:606: AutofillProfile profile2(kGuid2, kHttpsOrigin); rename to |into_profile|? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:625: EXPECT_TRUE(AutofillProfileSyncableService::MergeSimilarProfile( // We expect true because there is additional information in |from_profile| that was transferred to |into_profile|. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:627: can you add some expectations for the content of |into_profile|? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:631: EXPECT_TRUE(AutofillProfileSyncableService::MergeSimilarProfile( this should probably be in a different test because it assumes profile1 and profile2 are in a certain state (if you leave it this way, how can you be sure that it returns true here due to the different use dates?)
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
Thanks, lgtm with a question https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:157: << "Found similar profile in sync db but wsync_profile_ith a " batch rename? :) https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:692: // Tests that MergeSimilarProfiles replaces different names from |from_profile| I thought we changed PrimaryValue() so that John and Jane are no longer considered similar?
Patchset #6 (id:280001) has been deleted
Patchset #7 (id:320001) has been deleted
Thanks for the comments. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:419: bool AutofillProfile::EqualsForSyncPurposes( On 2016/04/22 20:54:27, Mathieu Perreault wrote: > nit: let's revert those lines to previous formatting since it's only a > formatting change now and it would create an unnecessary blame. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:1034: language_code() == profile.language_code() && Compare(profile) == 0; On 2016/04/22 20:54:27, Mathieu Perreault wrote: > leave previous formatting? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:123: GUIDToProfileMap remaining_profiles; On 2016/04/22 20:54:28, Mathieu Perreault wrote: > nit: rename to remaining_local_profiles? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:126: // Go through and check for all the profiles that sync already knows about. On 2016/04/22 20:54:28, Mathieu Perreault wrote: > I would prefer > > // For every incoming profile from sync, attempt to update a local profile or > otherwise create a new one. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:135: remaining_profiles.erase(it); On 2016/04/22 20:54:28, Mathieu Perreault wrote: > Mention that this may be a no-op since |it| is sometimes an entirely new profile > that came from sync. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:140: for (const auto& it : bundle.candidates_to_merge) { On 2016/04/22 20:54:28, Mathieu Perreault wrote: > rename "it" -> sync_profile_it ? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:144: bundle.profiles_to_delete.push_back(profile_to_merge->second->guid()); On 2016/04/22 20:54:28, Mathieu Perreault wrote: > Let's add a comment: // For similar profile pairs, the local profile is always > removed and its content merged (if applicable) in the profile that came from > sync. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:145: if (MergeSimilarProfile(*(profile_to_merge->second), it.second, On 2016/04/22 20:54:28, Mathieu Perreault wrote: > Let's add a comment: if new changes were merged into |it.second| from > |profile_to_merge|, they will be synced back. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:635: AutofillProfile old_merge_into = *merge_into; On 2016/04/22 20:54:28, Mathieu Perreault wrote: > const? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.h (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:166: // |merge_into| has extra data. Returns true the merge has made a change to On 2016/04/22 20:54:28, Mathieu Perreault wrote: > *if the merge Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:169: static bool MergeSimilarProfile(const AutofillProfile& merge_from, On 2016/04/22 20:54:28, Mathieu Perreault wrote: > nit: MergeSimilarProfiles? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:406: 2); // Merging two profile add their user count. On 2016/04/22 20:54:28, Mathieu Perreault wrote: > nit: weird format. Let's move the comment to its own line Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:601: On 2016/04/22 20:54:28, Mathieu Perreault wrote: > Mention what is tested here and possibly have a more descriptive test name, > perhaps MergeSimilarProfiles_AdditionalInfoInBothProfiles Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:603: AutofillProfile profile1(kGuid1, kHttpOrigin); On 2016/04/22 20:54:28, Mathieu Perreault wrote: > rename to |from_profile| ? Isn't it the other way around? https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:606: AutofillProfile profile2(kGuid2, kHttpsOrigin); On 2016/04/22 20:54:28, Mathieu Perreault wrote: > rename to |into_profile|? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:625: EXPECT_TRUE(AutofillProfileSyncableService::MergeSimilarProfile( On 2016/04/22 20:54:28, Mathieu Perreault wrote: > // We expect true because there is additional information in |from_profile| that > was transferred to |into_profile|. Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:627: On 2016/04/22 20:54:28, Mathieu Perreault wrote: > can you add some expectations for the content of |into_profile|? Done. https://codereview.chromium.org/1891903002/diff/180001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:631: EXPECT_TRUE(AutofillProfileSyncableService::MergeSimilarProfile( On 2016/04/22 20:54:28, Mathieu Perreault wrote: > this should probably be in a different test because it assumes profile1 and > profile2 are in a certain state (if you leave it this way, how can you be sure > that it returns true here due to the different use dates?) Done. https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... File components/browser_sync/browser/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:984: // deleted and that the sync profile gets merged down as a local profile. On 2016/04/22 20:15:22, Nicolas Zea wrote: > I find it tough to follow this comment. Are you saying that the newly added > profile gets deleted? And what does it mean for the sync profile to be "merged > down as a local profile"? If by "merged down" you mean merged into the native > model, I think it's clearer to state that explicitly. Is it more clear now? https://codereview.chromium.org/1891903002/diff/180001/components/browser_syn... components/browser_sync/browser/profile_sync_service_autofill_unittest.cc:1032: // Make sure the new information was merged down. On 2016/04/22 20:15:22, Nicolas Zea wrote: > Same as above. "merged down" -> "merged into the native model" to be explicit > (here and below) Done. https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:157: << "Found similar profile in sync db but wsync_profile_ith a " On 2016/04/25 13:41:16, Mathieu Perreault wrote: > batch rename? :) Exactly! I thought I had found them all thanks. https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc (right): https://codereview.chromium.org/1891903002/diff/260001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:692: // Tests that MergeSimilarProfiles replaces different names from |from_profile| On 2016/04/25 13:41:16, Mathieu Perreault wrote: > I thought we changed PrimaryValue() so that John and Jane are no longer > considered similar? Yes you are right, testing this doesn't really make sense so I will remove the test. Thanks.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1891903002/#ps340001 (title: "Fixed test that was added in rebase and removed unnecessary test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891903002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891903002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by sebsg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891903002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891903002/340001
Message was sent while issue was closed.
Description was changed from ========== Set the use count to one and the use date and modification date to "now". Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440. Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles. BUG=603640, 248440 TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest ========== to ========== Set the use count to one and the use date and modification date to "now". Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440. Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles. BUG=603640, 248440 TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest ==========
Message was sent while issue was closed.
Committed patchset #7 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Set the use count to one and the use date and modification date to "now". Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440. Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles. BUG=603640, 248440 TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest ========== to ========== Set the use count to one and the use date and modification date to "now". Changed AutofillProfileSyncableService::MergeProfile to MergeSimilarProfiles. It now compared the merge_into before and after the merge to determine if the merge had any effect. It fixes bug 248440. Also changed AutofillProfile::PrimaryValue(). Its comment said it included the name but it did not. The name was added because a profile otherwise two people living at the same address could have the profile overwrite one another instead of creating two distinct profiles. BUG=603640, 248440 TEST=PersonalDataManagerTest, AutofillProfileSyncableServiceTest, ProfileSyncServiceAutofillTest Committed: https://crrev.com/43391bda1708665a069ca796e59c60b15720b37c Cr-Commit-Position: refs/heads/master@{#389545} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/43391bda1708665a069ca796e59c60b15720b37c Cr-Commit-Position: refs/heads/master@{#389545} |
