Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1989173005:
[Autofill] Dedupe similar profiles on insertion. (Closed)
|
Created:
4 years, 7 months ago by sebsg Modified:
4 years, 6 months ago CC:
bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOn profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s).
BUG=618356
TEST=PersonalDataManagerTest
Committed: https://crrev.com/c7fc72c36d74dd40a71d076c45558daabc7c825c
Cr-Commit-Position: refs/heads/master@{#399495}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments #
Total comments: 16
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments #
Messages
Total messages: 27 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Autofill] Dedupe similar profiles (WIP). BUG= ========== to ========== On profile insertion, goes through all the user's profiles and try to merge. Delete the old profile if the profiles were merged. BUG=618356 TEST=PersonalDataManagerTest ==========
Description was changed from ========== On profile insertion, goes through all the user's profiles and try to merge. Delete the old profile if the profiles were merged. BUG=618356 TEST=PersonalDataManagerTest ========== to ========== On profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s). BUG=618356 TEST=PersonalDataManagerTest ==========
Patchset #1 (id:40001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org, rogerm@chromium.org
Hi Math and Roger, could you please take a look? Thanks.
Exciting times https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:440: return base::UTF8ToUTF16(base::JoinString(primary_values, " ")); can't you use the string16 version of JoinString? Right now going from 16->8->16 https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1269: TEST(AutofillProfileTest, PrimaryValue) { This test is mostly testing JoinString, and it's not really useful. Would prefer amending the tests for functions where PrimaryValue is used. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:866: std::string PersonalDataManager::MergeProfile( It feels weird to remove the static property here. Can we make profile_guids_to_delete be an argument, eventually populated by MergeProfile? The caller of MergeProfile (also in PDM) could be responsible for deleting those guids. Then all your functions could be static! Would also rename MergeProfile to something else if it does a little more than merge. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1542: AutofillProfile::CanonicalizeProfileString( Using the expanded PrimaryValue feels like a hack (with effects on other users of PrimaryValue). Have a look (and review) Roger's CL which introduces IsMergeable, which is a better API.
https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:439: base::UTF16ToUTF8(GetRawInfo(ADDRESS_HOME_CITY))}; GetInfo() instead of GetRawInfo()? https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.h:421: void FindAndMergeDuplicateProfiles( a comment that this is exposed for testability.
Patchset #2 (id:80001) has been deleted
Thanks for the comments, could you please take another look? https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:439: base::UTF16ToUTF8(GetRawInfo(ADDRESS_HOME_CITY))}; On 2016/06/08 18:55:38, Roger McFarlane (Chromium) wrote: > GetInfo() instead of GetRawInfo()? Done. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:440: return base::UTF8ToUTF16(base::JoinString(primary_values, " ")); On 2016/06/08 18:32:24, Mathieu Perreault wrote: > can't you use the string16 version of JoinString? Right now going from 16->8->16 Done. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1269: TEST(AutofillProfileTest, PrimaryValue) { On 2016/06/08 18:32:24, Mathieu Perreault wrote: > This test is mostly testing JoinString, and it's not really useful. Would prefer > amending the tests for functions where PrimaryValue is used. You are right, it was not very useful. I added a test for the case that initially made me change the PrimaryValue() code to fix a bug. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:866: std::string PersonalDataManager::MergeProfile( On 2016/06/08 18:32:24, Mathieu Perreault wrote: > It feels weird to remove the static property here. Can we make > profile_guids_to_delete be an argument, eventually populated by MergeProfile? > The caller of MergeProfile (also in PDM) could be responsible for deleting those > guids. Then all your functions could be static! > > Would also rename MergeProfile to something else if it does a little more than > merge. I like the idea. Would you mind if I did it in another CL? I would probably put the delete the code near or inside SetProfiles() and this method looks like it needs a good refactoring. I would also change a bunch of method names. For example AutofillProfile::OverwriteWith should probably be call MergeWith. AutofillProfile::SaveAdditionalInfo should be renamed with somehting like AreMergeable and make use of Roger's new API. Finally, I would not call the Merging logic inside of SaveAdditionalInfo/AreMergeable. This would allow for better testing and also to make a difference between profiles that we thought were not mergeable and profiles that failed merging. https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1542: AutofillProfile::CanonicalizeProfileString( On 2016/06/08 18:32:24, Mathieu Perreault wrote: > Using the expanded PrimaryValue feels like a hack (with effects on other users > of PrimaryValue). Have a look (and review) Roger's CL which introduces > IsMergeable, which is a better API. The reason why I changed PrimaryValue was to fix a very subtle bug where the canonicalized value of the similar profile with different syntax would not be considered similar. I don't think this should affect other users of PrimaryValue, since I'm only adding spaces between the fields. The reason I use PrimaryValue here is to not call SaveAdditionalInfo on profiles that are clearly not similar (eg different names or addresses). When Roger's CL lands, I want to actually remove SaveAdditionalInfo and use his API instead. Since it's super efficient at returning false for different profiles, I would also remove the call to PrimaryValue here). https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/1989173005/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.h:421: void FindAndMergeDuplicateProfiles( On 2016/06/08 18:55:38, Roger McFarlane (Chromium) wrote: > a comment that this is exposed for testability. Done.
https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:441: return base::JoinString(primary_values, base::UTF8ToUTF16(" ")); Note that JoinString isn't smart about empty strings. Maybe normalize it here instead of at the call site? Arghhh ... the return value isn't being canonicalized before use as a comparand! :( Double arghhh ... this is call this multiple time on the same object in an if! :( https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata...
lgtm Sorry for the delay https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1546: existing_profile->PrimaryValue(app_locale_)) == if you use app_locale_, remove app_locale from arguments list. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1549: if (existing_profile->SaveAdditionalInfo(*profile_to_merge, app_locale)) { same here, let's be consistent... https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4373: // Created saved profiles. *Create? https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4374: // Create two very similar profiles that should be deduped. nit: mention the difference for the reader "(one has a company name and the other does not)" https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4388: test::SetProfileInfo(&profile3, "Homer", "Jay", "Simpson", Let's have one with no middle name. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4451: // saved profile should be kept. nit: |profile2| https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4508: Let's check that other guids are still present in |existing_profiles|
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Another look? Thanks https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:441: return base::JoinString(primary_values, base::UTF8ToUTF16(" ")); On 2016/06/09 20:37:36, Roger McFarlane (Chromium) wrote: > Note that JoinString isn't smart about empty strings. Maybe normalize it here > instead of at the call site? > > Arghhh ... the return value isn't being canonicalized before use as a comparand! > :( > > Double arghhh ... this is call this multiple time on the same object in an if! > :( > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata... Good point. It should be better now. Anyway, as soon as you CL lands, I would like to replace PrimaryValue with your new comparator. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1546: existing_profile->PrimaryValue(app_locale_)) == On 2016/06/09 20:39:28, Mathieu Perreault wrote: > if you use app_locale_, remove app_locale from arguments list. Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1549: if (existing_profile->SaveAdditionalInfo(*profile_to_merge, app_locale)) { On 2016/06/09 20:39:28, Mathieu Perreault wrote: > same here, let's be consistent... Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4373: // Created saved profiles. On 2016/06/09 20:39:28, Mathieu Perreault wrote: > *Create? Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4374: // Create two very similar profiles that should be deduped. On 2016/06/09 20:39:28, Mathieu Perreault wrote: > nit: mention the difference for the reader > > "(one has a company name and the other does not)" Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4388: test::SetProfileInfo(&profile3, "Homer", "Jay", "Simpson", On 2016/06/09 20:39:28, Mathieu Perreault wrote: > Let's have one with no middle name. Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4451: // saved profile should be kept. On 2016/06/09 20:39:28, Mathieu Perreault wrote: > nit: |profile2| Done. https://codereview.chromium.org/1989173005/diff/100001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:4508: On 2016/06/09 20:39:28, Mathieu Perreault wrote: > Let's check that other guids are still present in |existing_profiles| Done.
Unit test failures in try bots? https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:443: return base::JoinString(primary_values, base::UTF8ToUTF16(" ")); Do the join first and canonicalize after. i.e.: const std::vector<base::string16> primary_values = { GetInfo(...), GetInfo(...), GetInfo(...), ... }: return CanonicalizeProfileString( base::JoinString(primary_values, base::UTF8ToUTF16(" "))); https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1529: for (std::string profile_guid_to_delete : profile_guids_to_delete) { const ref https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:548: if (!local_profile->IsVerified() && !new_profile->IsVerified() && maybe break up this if if (!local_profile->IsVerified() && !new_profile->IsVerifier()) { const base::string16 local_profile_primary_value = local_profile->PrimaryValue(app_locale_); if (!local_profile_primary_value.empty() && ...) { bundle->... } }
Thanks, another look? https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_profile.cc:443: return base::JoinString(primary_values, base::UTF8ToUTF16(" ")); On 2016/06/13 15:12:52, Roger McFarlane (Chromium) wrote: > Do the join first and canonicalize after. > > i.e.: > > const std::vector<base::string16> primary_values = { > GetInfo(...), GetInfo(...), GetInfo(...), ... }: > return CanonicalizeProfileString( > base::JoinString(primary_values, base::UTF8ToUTF16(" "))); Done. https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1529: for (std::string profile_guid_to_delete : profile_guids_to_delete) { On 2016/06/13 15:12:52, Roger McFarlane (Chromium) wrote: > const ref Done. https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc (right): https://codereview.chromium.org/1989173005/diff/160001/components/autofill/co... components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:548: if (!local_profile->IsVerified() && !new_profile->IsVerified() && On 2016/06/13 15:12:52, Roger McFarlane (Chromium) wrote: > maybe break up this if > > if (!local_profile->IsVerified() && !new_profile->IsVerifier()) { > const base::string16 local_profile_primary_value = > local_profile->PrimaryValue(app_locale_); > if (!local_profile_primary_value.empty() && ...) { > bundle->... > } > } Done.
lgtm
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1989173005/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989173005/180001
Message was sent while issue was closed.
Description was changed from ========== On profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s). BUG=618356 TEST=PersonalDataManagerTest ========== to ========== On profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s). BUG=618356 TEST=PersonalDataManagerTest ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== On profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s). BUG=618356 TEST=PersonalDataManagerTest ========== to ========== On profile insertion, goes through all the user's profiles and try to merge. Delete all original profiles that were used in the merge, keeping only the result of the merge(s). BUG=618356 TEST=PersonalDataManagerTest Committed: https://crrev.com/c7fc72c36d74dd40a71d076c45558daabc7c825c Cr-Commit-Position: refs/heads/master@{#399495} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c7fc72c36d74dd40a71d076c45558daabc7c825c Cr-Commit-Position: refs/heads/master@{#399495} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
