Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(461)

Issue 2041413004: Add an AutofillProfileComparator class. (Closed)

Created:
4 years, 6 months ago by Roger McFarlane (Chromium)
Modified:
4 years, 6 months ago
Reviewers:
Mathieu, sebsg
CC:
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.

Description

Add an AutofillProfileComparator class. This CL adds an AutofillProfileComparator class that applies broader heuristics to identify mergeable autofill profiles. This is the first of several CLs to more intelligently prevent and eliminate duplicate autofill profiles. Chrome Autofill often create duplicate profiles. These profiles differ lexically, but have identical meaning to the user. While we do perform some level of deduplication, it is insufficient. We can improve Chrome's ability to detect and resolve duplicate autofill profiles by adopting the following approaches: - Normalize fields before comparison to fold case, remove diacritics, remove punctuation and collapse or remove whitespace. - Smarter name comparison to account for compound names and the use of initials. For example, John Quincy Public and John Q. Public. - Broaden phone number matching to include missing extensions or area codes. - Use bag-of-words comparison to compare addresses. This accounts for different orderings of the information. For example, 23-1 Main St vs 23 Main St, Apt 1 BUG=618095, 587465 R=mathp@chromium.org, sebsg@chromium.org Committed: https://crrev.com/a7b3333ea833f2d7e961e970437408a0b346f5f0 Cr-Commit-Position: refs/heads/master@{#399452}

Patch Set 1 #

Patch Set 2 : Fix gyp files. #

Total comments: 14

Patch Set 3 : Apply review comments fixes and use GetInfo instead of GetRawInfo #

Patch Set 4 : rebase #

Total comments: 16

Patch Set 5 : Moar tests and handling of initials #

Patch Set 6 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+909 lines, -84 lines) Patch
M components/autofill.gypi View 1 1 chunk +2 lines, -0 lines 0 comments Download
M components/autofill/core/browser/BUILD.gn View 2 chunks +3 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_profile.h View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M components/autofill/core/browser/autofill_profile.cc View 1 2 3 chunks +4 lines, -79 lines 0 comments Download
A components/autofill/core/browser/autofill_profile_comparator.h View 1 2 3 4 1 chunk +142 lines, -0 lines 0 comments Download
A components/autofill/core/browser/autofill_profile_comparator.cc View 1 2 3 4 1 chunk +375 lines, -0 lines 0 comments Download
A components/autofill/core/browser/autofill_profile_comparator_unittest.cc View 1 2 3 4 1 chunk +382 lines, -0 lines 0 comments Download
M components/components_tests.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
Roger McFarlane (Chromium)
PTAL?
4 years, 6 months ago (2016-06-08 14:40:01 UTC) #1
Mathieu
Thanks, initial comments https://codereview.chromium.org/2041413004/diff/20001/components/autofill/core/browser/autofill_profile.cc File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/2041413004/diff/20001/components/autofill/core/browser/autofill_profile.cc#newcode776 components/autofill/core/browser/autofill_profile.cc:776: base::string16 AutofillProfile::CanonicalizeProfileString( I think it makes ...
4 years, 6 months ago (2016-06-08 17:56:06 UTC) #2
sebsg
I hear that you are modifing some things. Here are some nit comments I had ...
4 years, 6 months ago (2016-06-08 19:03:55 UTC) #3
Roger McFarlane (Chromium)
Thanks. Another look? https://codereview.chromium.org/2041413004/diff/20001/components/autofill/core/browser/autofill_profile.cc File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/2041413004/diff/20001/components/autofill/core/browser/autofill_profile.cc#newcode776 components/autofill/core/browser/autofill_profile.cc:776: base::string16 AutofillProfile::CanonicalizeProfileString( On 2016/06/08 17:56:06, Mathieu ...
4 years, 6 months ago (2016-06-09 20:19:11 UTC) #5
Mathieu
looks great, just few small questions https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.h File components/autofill/core/browser/autofill_profile_comparator.h (right): https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.h#newcode42 components/autofill/core/browser/autofill_profile_comparator.h:42: // are all ...
4 years, 6 months ago (2016-06-10 14:10:58 UTC) #6
sebsg
Awesome work! https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.cc File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.cc#newcode179 components/autofill/core/browser/autofill_profile_comparator.cc:179: const base::string16& family_name = name_1_parts.family; Hum, I'm ...
4 years, 6 months ago (2016-06-10 15:12:13 UTC) #7
Roger McFarlane (Chromium)
Thanks! Another look? https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.cc File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2041413004/diff/80001/components/autofill/core/browser/autofill_profile_comparator.cc#newcode179 components/autofill/core/browser/autofill_profile_comparator.cc:179: const base::string16& family_name = name_1_parts.family; On ...
4 years, 6 months ago (2016-06-10 16:19:44 UTC) #9
Mathieu
lgtm, please wait for seb's approval too :)
4 years, 6 months ago (2016-06-10 20:31:38 UTC) #10
sebsg
lgtm this is awesome!
4 years, 6 months ago (2016-06-10 20:42:55 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041413004/140001
4 years, 6 months ago (2016-06-13 14:53:02 UTC) #14
commit-bot: I haz the power
Committed patchset #6 (id:140001)
4 years, 6 months ago (2016-06-13 15:27:36 UTC) #15
commit-bot: I haz the power
CQ bit was unchecked
4 years, 6 months ago (2016-06-13 15:27:51 UTC) #16
commit-bot: I haz the power
4 years, 6 months ago (2016-06-13 15:29:18 UTC) #18
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/a7b3333ea833f2d7e961e970437408a0b346f5f0
Cr-Commit-Position: refs/heads/master@{#399452}

Powered by Google App Engine
This is Rietveld 408576698