Uses AutofillProfileComparator to compare profile names.

Uses AutofillProfileComparator to compare profile names.

BUG=710974
Review-Url: https://codereview.chromium.org/2864483005
Cr-Commit-Position: refs/heads/master@{#469731}
Committed: https://chromium.googlesource.com/chromium/src/+/d7995ddddb7ac652c721c532c7425de58def0147

[csashi, 2017-05-04 21:19:02]

csashi@google.com changed reviewers:
+ jsaul@google.com, rogerm@chromium.org

[csashi, 2017-05-04 21:19:02]

Hi,
Please take a look. Thanks!
-sashi.

[csashi, 2017-05-04 21:19:20]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 21:20:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jared Saul, 2017-05-04 21:58:07]

lgtm

Question for Roger: Can you explain how
AutofillProfileComparator::GetNamePartVariants() works?  Is it a strict
substring checker (Chris vs. Christopher, Jon vs. Jonathan), or does it allow
for slightly more complex cases like Joe vs. Joseph, Jenny vs. Jennifer, and
Mike vs. Michael?

[commit-bot: I haz the power, 2017-05-04 22:33:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 22:33:45]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-05-05 14:04:57]

sebsg@chromium.org changed reviewers:
+ sebsg@chromium.org

[sebsg, 2017-05-05 14:04:58]

Nice LGTM, but let's wait for Roger's comments :)

[Roger McFarlane (Chromium), 2017-05-05 16:05:30]

The name comparison is not a substring checker, although that might be
interesting.

It only checks for permutations of initials and full name parts.

i.e., John-Paul Quincy Public
        --> (J(ohn)?)? (P(aul)?)? (Q(uincy)?)? Public

Hmm... maybe it could be made simpler by constructing this regular expression?

You ever hear the joke: I have this problem. I think I'll solve it with regular
expressions! Now I have two problems.

Prefix matching is trickier. There are a lot of names where a maching prefix is
actually a different name (gendered name variants, for example).

https://codereview.chromium.org/2864483005/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2864483005/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1311: } else if
(!comparator.IsNameVariantOf(verified_name, address_name)) {
Would it be helpful to add the bidirectional name variant test  as a method on
the comparator? It's basically the last bits of HaveMergeableNames(), which
could also call that method.

There's also some CJK name handling that you might want to look at. in there as
well.

[csashi, 2017-05-05 16:22:31]

https://codereview.chromium.org/2864483005/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2864483005/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1311: } else if
(!comparator.IsNameVariantOf(verified_name, address_name)) {
On 2017/05/05 16:05:30, Roger McFarlane (Chromium) wrote:
> Would it be helpful to add the bidirectional name variant test  as a method on
> the comparator? It's basically the last bits of HaveMergeableNames(), which
> could also call that method.
> 

I prefer the current version because I would like to know if full_name_1 can be
derived from full_name_2, or is it vice versa.

> There's also some CJK name handling that you might want to look at. in there
as
> well.

AFAIK, We are not enabled in CJK countries yet (may be enabled in Japan - not
sure). So not worried about that just yet.

[Roger McFarlane (Chromium), 2017-05-05 16:46:46]

LGTM

nit: the declaration order in the comparator header no longer matches the
definition order in the comparator source file.

[csashi, 2017-05-05 17:10:37]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[csashi, 2017-05-05 17:10:59]

On 2017/05/05 16:46:46, Roger McFarlane (Chromium) wrote:
> LGTM
> 
> nit: the declaration order in the comparator header no longer matches the
> definition order in the comparator source file.

Done.

[commit-bot: I haz the power, 2017-05-05 17:11:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[csashi, 2017-05-05 17:23:51]

The CQ bit was checked by csashi@google.com

[csashi, 2017-05-05 17:23:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
sebsg@chromium.org, jsaul@google.com
Link to the patchset: https://codereview.chromium.org/2864483005/#ps60001
(title: "Reorder method definitions to match declaration order.")

[commit-bot: I haz the power, 2017-05-05 17:24:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 18:29:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494005031443110,
"parent_rev": "9a1b65feb44e32a48ad7fb51384fa868bc864008", "commit_rev":
"d7995ddddb7ac652c721c532c7425de58def0147"}

[commit-bot: I haz the power, 2017-05-05 18:36:01]

Description was changed from

==========
Uses AutofillProfileComparator to compare profile names.

BUG=710974
==========

to

==========
Uses AutofillProfileComparator to compare profile names.

BUG=710974

Review-Url: https://codereview.chromium.org/2864483005
Cr-Commit-Position: refs/heads/master@{#469731}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7995ddddb7ac652c721c532c742...
==========

[commit-bot: I haz the power, 2017-05-05 18:36:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d7995ddddb7ac652c721c532c742...