Expand autofill profile merge logic.

Expand autofill profile merge logic.

The AutofillProfileComparator expands the range of "mergeable" autofill
profiles that Chrome can detect. This CL adds the corresponding logic
to determine which parts of each profile to prefer when carring out
a merge operations.

The CL adds only the merge logic and unit-tests. It does not integrate
this functionality into the main profile deduplication flow.

BUG=618095, 618095
R=mathp@chromium.org, sebsg@chromium.org

Committed: https://crrev.com/8b7aad0dfb2118f1f054634969b87372c94205d2
Cr-Commit-Position: refs/heads/master@{#402520}

[Roger McFarlane (Chromium), 2016-06-20 19:47:34]

PTAL?

[Mathieu, 2016-06-21 18:55:07]

mathp@chromium.org changed reviewers:
- sebsg@chromium.org

[Mathieu, 2016-06-21 18:55:07]

Will wait for more tests around MergeX functions

https://codereview.chromium.org/2088443002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_profile_comparator.cc (right):

https://codereview.chromium.org/2088443002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_profile_comparator.cc:233: case 0:
use enum?

[Roger McFarlane (Chromium), 2016-06-23 18:27:38]

Thanks. I pulled on more merge functions (put them in the wrong branch, sorry)
and got the phone number merge to work properly.

Another look?

https://codereview.chromium.org/2088443002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_profile_comparator.cc (right):

https://codereview.chromium.org/2088443002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_profile_comparator.cc:233: case 0:
On 2016/06/21 18:55:07, Mathieu Perreault wrote:
> use enum?

Done.

[Mathieu, 2016-06-23 18:36:22]

lgtm, pretty thorough thanks! Just one nit

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

https://codereview.chromium.org/2088443002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator_unittest.cc:531:
name1.SetRawInfo(NAME_FULL, UTF8ToUTF16("John Quincy Public"));
Would you have an example where only NAME_FULL is present? I'm pretty sure this
happens if the user submits a form that only saved their full name. Also
conversely, an example where NAME_FULL is empty but other fields are not. Could
be in a different test.

[Roger McFarlane (Chromium), 2016-06-23 20:23:18]

rogerm@chromium.org changed reviewers:
+ tmartino@chromium.org

[Roger McFarlane (Chromium), 2016-06-23 20:23:19]

Thanks!

Adding Tommy as a second set of eyes.

[tmartino, 2016-06-27 17:56:47]

lgtm with a few nits/questions

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile_comparator.cc (right):

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:294:
region.empty() ? PhoneNumberUtil::NATIONAL
It seems like region.empty() can't ever be true here, because we do the same
check and populate with a default value on lines 256-257.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:398: // tokens
from the other. Pick the non-em that is shorter. This is usually the
This says pick the shorter, but the logic all seems to be picking the longer.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:468: // the two
sets are exactly the samel.
nit: /s/samel/same

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:688: // subset
of the other.
It looks to me like we aren't handling abbreviations here? So "MN" and
"Minnesota" would be different states?

Also, does the bag-of-words subset heuristic handle a case like "West Virginia"
and "Virginia"?

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile_comparator.h (right):

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.h:52: // been found
to be mergeable.
Can you add a brief description to these docs of the logic used to select the
final value? I noticed there are differences between the two (e.g., email
addresses go by use date, whereas names do not) so it would be good to have that
written out.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.h:103: // This
method expects that the value is either the same in |p1| or |p2| or
nit: Should this read "either the same in |p1| AND |p2|" instead of "or"?

[Roger McFarlane (Chromium), 2016-06-28 17:23:35]

Patchset #4 (id:60001) has been deleted

[Roger McFarlane (Chromium), 2016-06-28 17:26:47]

thanks!

One last peek?

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile_comparator.cc (right):

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:294:
region.empty() ? PhoneNumberUtil::NATIONAL
On 2016/06/27 17:56:46, tmartino wrote:
> It seems like region.empty() can't ever be true here, because we do the same
> check and populate with a default value on lines 256-257.

I wasn't assuming that the default was guaranteed to be non-empty; but, yes,
you're right, it will probably never be empty. Unless you feel really strongly,
though, I'd prefer to leave this as is.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:398: // tokens
from the other. Pick the non-em that is shorter. This is usually the
On 2016/06/27 17:56:46, tmartino wrote:
> This says pick the shorter, but the logic all seems to be picking the longer.

Done.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:468: // the two
sets are exactly the samel.
On 2016/06/27 17:56:46, tmartino wrote:
> nit: /s/samel/same

Done.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.cc:688: // subset
of the other.
On 2016/06/27 17:56:46, tmartino wrote:
> It looks to me like we aren't handling abbreviations here? So "MN" and
> "Minnesota" would be different states?
> 
> Also, does the bag-of-words subset heuristic handle a case like "West
Virginia"
> and "Virginia"?

Added provisional support for state (US).

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile_comparator.h (right):

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.h:52: // been found
to be mergeable.
On 2016/06/27 17:56:46, tmartino wrote:
> Can you add a brief description to these docs of the logic used to select the
> final value? I noticed there are differences between the two (e.g., email
> addresses go by use date, whereas names do not) so it would be good to have
that
> written out.

Done.

https://codereview.chromium.org/2088443002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator.h:103: // This
method expects that the value is either the same in |p1| or |p2| or
On 2016/06/27 17:56:46, tmartino wrote:
> nit: Should this read "either the same in |p1| AND |p2|" instead of "or"?

Done.

[tmartino, 2016-06-28 17:32:22]

lgtm

[Roger McFarlane (Chromium), 2016-06-28 17:36:02]

The CQ bit was checked by rogerm@chromium.org

[Roger McFarlane (Chromium), 2016-06-28 17:36:02]

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/2088443002/#ps80001
(title: "Tommy's comments")

[commit-bot: I haz the power, 2016-06-28 17:36:56]

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, 2016-06-28 20:19:44]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-06-28 20:22:39]

Description was changed from

==========
Expand autofill profile merge logic.

The AutofillProfileComparator expands the range of "mergeable" autofill
profiles that Chrome can detect. This CL adds the corresponding logic
to determine which parts of each profile to prefer when carring out
a merge operations.

The CL adds only the merge logic and unit-tests. It does not integrate
this functionality into the main profile deduplication flow.

BUG=618095, 618095
R=mathp@chromium.org, sebsg@chromium.org
==========

to

==========
Expand autofill profile merge logic.

The AutofillProfileComparator expands the range of "mergeable" autofill
profiles that Chrome can detect. This CL adds the corresponding logic
to determine which parts of each profile to prefer when carring out
a merge operations.

The CL adds only the merge logic and unit-tests. It does not integrate
this functionality into the main profile deduplication flow.

BUG=618095, 618095
R=mathp@chromium.org, sebsg@chromium.org

Committed: https://crrev.com/8b7aad0dfb2118f1f054634969b87372c94205d2
Cr-Commit-Position: refs/heads/master@{#402520}
==========

[commit-bot: I haz the power, 2016-06-28 20:22:40]

Patchset 4 (id:??) landed as
https://crrev.com/8b7aad0dfb2118f1f054634969b87372c94205d2
Cr-Commit-Position: refs/heads/master@{#402520}