Use AutofillProfileComparator in place of ad-hoc merge logic.

Use AutofillProfileComparator in place of ad-hoc merge logic.

This CL integrates the AutofillProfileComparator into the profile
comparison and merge logic (used during profile creation/update
as well as sync).

 - Use the AutofileProfileComparator::AreMergeabable(p1, p2) API

 - Use the AutofillProfileComparator::Merge* APIs

 - Add (in)equality operators to the autofill profile component
   types (NameInfo, Address, EmailInfo, CompanyInfo, PhoneNumber)

 - Updates the parser for the data driven autofill tests to be
   less sensitive to whitespace.

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

Committed: https://crrev.com/b457c9197023162148f861d3fb88df2e91b8b3d8
Cr-Commit-Position: refs/heads/master@{#403021}

[Roger McFarlane (Chromium), 2016-06-28 19:04:12]

Patchset #1 (id:1) has been deleted

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

Patchset #1 (id:20001) has been deleted

[Roger McFarlane (Chromium), 2016-06-28 19:04:34]

Patchset #1 (id:40001) has been deleted

[Roger McFarlane (Chromium), 2016-06-28 19:05:33]

rogerm@chromium.org changed reviewers:
+ sebsg@chromium.org, zea@chromium.org

[Roger McFarlane (Chromium), 2016-06-28 19:05:34]

Please take a look?

[Nicolas Zea, 2016-06-28 19:46:11]

sync LGTM

[sebsg, 2016-06-29 08:57:04]

Great work. Much cleaner that before too. Thanks!

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_merge_unittest.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:108: 
nit: remove second whitespace.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:549:
DCHECK(comparator.AreMergeable(*this, profile));
Would it be better to return false here instead? It would be more in line with
the previous behaviour where we returned false if we did not find the profiles
to be mergeable. What do you think?

I'm also thinking this method should be renamed but I can do that in another CL.
There are a bunch a method names changes and small refactoring enabled by your
CL.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:115: // have no effect on
the return value.
Add a note that the two profiles should be considered mergeable before calling
this method.

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

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator_unittest.cc:552:
AutofillProfile profile1(base::GenerateGUID(), "https://www.example.com/");
This seems different that the previous test cases where you had mergeable and
non mergeable profiles. I would either merge it into p1 and etc by adding
punctuation there or I would add a AreMergeable_CaseAndPunctuation test and put
it there.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1664:
comparator.AreMergeable(*existing_profile, *profile_to_merge)) {
If you made the change to SaveAdditionnalInfo (dcheck to return false) this line
can be removed.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/phone_number.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/phone_number.cc:59: return number_ ==
other.number_ && profile_ == other.profile_;
I think profile_ == other.profiles_ results in a comparison of profiles
pointers?

If we use the object instead, the whole profile including it's guid and all of
it's content will be compared, which seems to be a lot more that what I would
expect from comparing phone numbers.

Since the only way profile_ is used in this class is to GetRegion(...), let's
add a TODO to check whether we could just keep the region instead of a pointer
to the profile.

In the meantime maybe we could check that 
GetRegion(*profile_, app_locale) == GetRegion(*(other.profile_), app_locale) or
something like that?

What do you think?

[sebsg, 2016-06-29 15:35:24]

Added another small comment to remove even more useless code!

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:542:
GetNonEmptyTypes(app_locale, &field_types);
We don't need those 3 lines either now :)

[Mathieu, 2016-06-29 15:50:57]

Thanks, lgtm but I'll defer to seb because he knows the code much more than I do

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_merge_unittest.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:98: if
(!value.empty()) {
You will probably need to update
https://cs.chromium.org/chromium/src/components/test/data/autofill/merge/tool....


Something like:
lines.append("%s:%s" % (field_type, " " + value if value else ""))

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:426: if
(!compare->StringsEqual(value, profile.GetRawInfo(type)))
I wonder if this code also needs changing. I'm wary of all "string equals" now.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:487: // other profile in
this position to prefer accepting updates instead of
possibly change "other" -> "incoming"

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:505: bool modified = false;
Please add an obvious comment above this block. Something like "now that the new
values have been obtained, overwrite them if applicable".

[tmartino, 2016-06-29 18:21:30]

lgtm

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_unittest.cc:4179: // and
that the longer address is retained..
nit: two periods

[Roger McFarlane (Chromium), 2016-06-29 18:21:37]

Thanks!

Another look?

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_merge_unittest.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:98: if
(!value.empty()) {
On 2016/06/29 15:50:56, Mathieu Perreault wrote:
> You will probably need to update
>
https://cs.chromium.org/chromium/src/components/test/data/autofill/merge/tool....
> 
> 
> Something like:
> lines.append("%s:%s" % (field_type, " " + value if value else ""))

Done.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:108: 
On 2016/06/29 08:57:03, sebsg wrote:
> nit: remove second whitespace.

Done.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:426: if
(!compare->StringsEqual(value, profile.GetRawInfo(type)))
On 2016/06/29 15:50:57, Mathieu Perreault wrote:
> I wonder if this code also needs changing. I'm wary of all "string equals"
now. 
> 

Done.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:487: // other profile in
this position to prefer accepting updates instead of
On 2016/06/29 15:50:57, Mathieu Perreault wrote:
> possibly change "other" -> "incoming"

Done.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:505: bool modified = false;
On 2016/06/29 15:50:57, Mathieu Perreault wrote:
> Please add an obvious comment above this block. Something like "now that the
new
> values have been obtained, overwrite them if applicable". 

Done.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:542:
GetNonEmptyTypes(app_locale, &field_types);
On 2016/06/29 15:35:24, sebsg wrote:
> We don't need those 3 lines either now :)

Removed.

Also removed OverwriteName() which is also no longer used.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:549:
DCHECK(comparator.AreMergeable(*this, profile));
On 2016/06/29 08:57:03, sebsg wrote:
> Would it be better to return false here instead? It would be more in line with
> the previous behaviour where we returned false if we did not find the profiles
> to be mergeable. What do you think?
> 
> I'm also thinking this method should be renamed but I can do that in another
CL.
> There are a bunch a method names changes and small refactoring enabled by your
> CL.

This dcheck firing indicates a programming error, not an expected possible
condition. The relevant test that would return false from this method is the
IsVerified !! profile.IsVerified test at the top. Otherwise the do
something/nothing logic is the same modulo the bail out in the middle initial
handling of the old version.

I'd actually refactor this a bit to simplify the extra layer that
SaveAdditionalInfo adds. I'm just not sure yet what I want.

I don't really like the logic of "yes they're mergeable, so merge them and maybe
it'll be a NOP but return true anyway".

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:115: // have no effect on
the return value.
On 2016/06/29 08:57:03, sebsg wrote:
> Add a note that the two profiles should be considered mergeable before calling
> this method.

Done.

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

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_comparator_unittest.cc:552:
AutofillProfile profile1(base::GenerateGUID(), "https://www.example.com/");
On 2016/06/29 08:57:03, sebsg wrote:
> This seems different that the previous test cases where you had mergeable and
> non mergeable profiles. I would either merge it into p1 and etc by adding
> punctuation there or I would add a AreMergeable_CaseAndPunctuation test and
put
> it there.

Yeah, I moved this test case from
AutofillProfileTest.PrimaryValue_SimilarProfiles

I'll merge the test and add punctuation, case and diacritic variants.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1664:
comparator.AreMergeable(*existing_profile, *profile_to_merge)) {
On 2016/06/29 08:57:04, sebsg wrote:
> If you made the change to SaveAdditionnalInfo (dcheck to return false) this
line
> can be removed.

That's true, but I think that would make this code less clear.

Maybe if we rename SaveAdditionalInfo to AttemptMerge or something, then it
would make sense?

---- Start Chat ----

rogerm: SaveAdditionalInfo seems like it could use a rename (or a deletion)

rogerm: it's called in two places MergeProfiles and DedupProfiles and really
only serves to abort if the IsVerified() status is not right and/or to
degenerate to "successful" NOP.

sebsg: Yes exactly, it used to do what AreMeargeable now does with a few more
things like you mentioned. So I'm thinking about what the best course of action
is, renaming and keeping it super small or move the code into OverwriteWith or
something else

rogerm: So, the issue to me is that the IsVerified logic isn't accounted for in
AreMergeable and OverwriteWith (which I'm renaming to MergeDataFrom, btw)

sebsg: yeah, what I'd do for now is leave it as is but with the return false
instead. also line 541-543 can probably be removed
That way the behavior of SaveAdditionalInfo is pretty much the same as before

rogerm: Ok, I'm going to do a quick layout and comment update at the
SaveAdditionalInfo call sites, but leave the logic as it is in this CL and we
can revisit this later.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/phone_number.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/phone_number.cc:59: return number_ ==
other.number_ && profile_ == other.profile_;
On 2016/06/29 08:57:04, sebsg wrote:
> I think profile_ == other.profiles_ results in a comparison of profiles
> pointers?
> 
> If we use the object instead, the whole profile including it's
> guid and all of it's content will be compared, which seems to
> be a lot more that what I would expect from comparing phone
> numbers.

Hmmm...no, I don't think that's what we want.

We really just want to know if this phone number is the same as the other, so
that we know whether or not there really was a change. The profile is actually
somewhat inconsequential because the way the phone number is constructed when
this operator is used ensures that the phone_number being compared points to the
same profile.

We're might be better off just dropping the profile from the comparison, but the
assignment operators take care to handle it so I included it here as well.

> Since the only way profile_ is used in this class is to
> GetRegion(...), let's add a TODO to check whether we could
> just keep the region instead of a pointer to the profile.

The region and the phone number are not necessarily related. If the phone number
is international, it could refer to any region.

> In the meantime maybe we could check that
>   GetRegion(*profile_, app_locale) ==
>     GetRegion(*(other.profile_), app_locale)
> or something like that?
> 
> What do you think?

Alas, we don't have app_locale_ here.

I'm comfortable just doing the pointer comparison here since it's actually a
more-restrictive subset of the instances where operator== should return true
(i.e., it can never do the wrong thing), whereas, I think we need to think a bit
more about what it should return if the profiles are different.

https://codereview.chromium.org/2110563002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/2110563002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:122: bool
SaveAdditionalInfo(const AutofillProfile& profile,

Maybe (in a follow-on CL?) rename MergeDataFrom() above to MergeDataFromImpl()
and rename this to MergeDataFrom() and have it:

  Perform the mergeability check
  Enforce the Verified vs !Verified constraints.
  Call MergeDataFromImpl
  Update the stats.

MergeDataFromImpl can then be made private (?)

[Roger McFarlane (Chromium), 2016-06-29 18:25:49]

Patchset #2 (id:80001) has been deleted

[Roger McFarlane (Chromium), 2016-06-29 18:26:19]

https://codereview.chromium.org/2110563002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/2110563002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.h:122: bool
SaveAdditionalInfo(const AutofillProfile& profile,
Maybe (in a follow-on CL?) rename MergeDataFrom() above to MergeDataFromImpl()
and rename this to MergeDataFrom() and have it:

  Perform the mergeability check
  Enforce the Verified vs !Verified constraints.
  Call MergeDataFromImpl
  Update the stats.

MergeDataFromImpl can then be made private (?)

[sebsg, 2016-06-29 19:04:50]

LGTM!

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:549:
DCHECK(comparator.AreMergeable(*this, profile));
On 2016/06/29 18:21:37, Roger McFarlane (Chromium) wrote:
> On 2016/06/29 08:57:03, sebsg wrote:
> > Would it be better to return false here instead? It would be more in line
with
> > the previous behaviour where we returned false if we did not find the
profiles
> > to be mergeable. What do you think?
> > 
> > I'm also thinking this method should be renamed but I can do that in another
> CL.
> > There are a bunch a method names changes and small refactoring enabled by
your
> > CL.
> 
> This dcheck firing indicates a programming error, not an expected possible
> condition. The relevant test that would return false from this method is the
> IsVerified !! profile.IsVerified test at the top. Otherwise the do
> something/nothing logic is the same modulo the bail out in the middle initial
> handling of the old version.
> 
> I'd actually refactor this a bit to simplify the extra layer that
> SaveAdditionalInfo adds. I'm just not sure yet what I want.
> 
> I don't really like the logic of "yes they're mergeable, so merge them and
maybe
> it'll be a NOP but return true anyway".

Yes, let's discuss it and fix it in a subsequent CL. This looks good.

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2110563002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1664:
comparator.AreMergeable(*existing_profile, *profile_to_merge)) {
On 2016/06/29 18:21:37, Roger McFarlane (Chromium) wrote:
> On 2016/06/29 08:57:04, sebsg wrote:
> > If you made the change to SaveAdditionnalInfo (dcheck to return false) this
> line
> > can be removed.
> 
> That's true, but I think that would make this code less clear.
> 
> Maybe if we rename SaveAdditionalInfo to AttemptMerge or something, then it
> would make sense?
> 
> ---- Start Chat ----
> 
> rogerm: SaveAdditionalInfo seems like it could use a rename (or a deletion)
> 
> rogerm: it's called in two places MergeProfiles and DedupProfiles and really
> only serves to abort if the IsVerified() status is not right and/or to
> degenerate to "successful" NOP.
> 
> sebsg: Yes exactly, it used to do what AreMeargeable now does with a few more
> things like you mentioned. So I'm thinking about what the best course of
action
> is, renaming and keeping it super small or move the code into OverwriteWith or
> something else
> 
> rogerm: So, the issue to me is that the IsVerified logic isn't accounted for
in
> AreMergeable and OverwriteWith (which I'm renaming to MergeDataFrom, btw)
> 
> sebsg: yeah, what I'd do for now is leave it as is but with the return false
> instead. also line 541-543 can probably be removed
> That way the behavior of SaveAdditionalInfo is pretty much the same as before
> 
> rogerm: Ok, I'm going to do a quick layout and comment update at the
> SaveAdditionalInfo call sites, but leave the logic as it is in this CL and we
> can revisit this later.

Yes, AttemptMerge would be a better name for sure. But let's do that in another
CL as refactoring. This works well for the moment.

https://codereview.chromium.org/2110563002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/2110563002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.h:122: bool
SaveAdditionalInfo(const AutofillProfile& profile,
On 2016/06/29 18:26:19, Roger McFarlane (Chromium) wrote:
> Maybe (in a follow-on CL?) rename MergeDataFrom() above to MergeDataFromImpl()
> and rename this to MergeDataFrom() and have it:
> 
>   Perform the mergeability check
>   Enforce the Verified vs !Verified constraints.
>   Call MergeDataFromImpl
>   Update the stats.
> 
> MergeDataFromImpl can then be made private (?)

I agree, let's do that.

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

The CQ bit was checked by rogerm@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
zea@chromium.org, tmartino@chromium.org, sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2110563002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-29 19:23:53]

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

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

The CQ bit was checked by rogerm@chromium.org

[commit-bot: I haz the power, 2016-06-29 19:24:52]

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-30 00:24:10]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-06-30 00:24:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 00:25:36]

Description was changed from

==========
Use AutofillProfileComparator in place of ad-hoc merge logic.

This CL integrates the AutofillProfileComparator into the profile
comparison and merge logic (used during profile creation/update
as well as sync).

 - Use the AutofileProfileComparator::AreMergeabable(p1, p2) API

 - Use the AutofillProfileComparator::Merge* APIs

 - Add (in)equality operators to the autofill profile component
   types (NameInfo, Address, EmailInfo, CompanyInfo, PhoneNumber)

 - Updates the parser for the data driven autofill tests to be
   less sensitive to whitespace.

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

to

==========
Use AutofillProfileComparator in place of ad-hoc merge logic.

This CL integrates the AutofillProfileComparator into the profile
comparison and merge logic (used during profile creation/update
as well as sync).

 - Use the AutofileProfileComparator::AreMergeabable(p1, p2) API

 - Use the AutofillProfileComparator::Merge* APIs

 - Add (in)equality operators to the autofill profile component
   types (NameInfo, Address, EmailInfo, CompanyInfo, PhoneNumber)

 - Updates the parser for the data driven autofill tests to be
   less sensitive to whitespace.

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

Committed: https://crrev.com/b457c9197023162148f861d3fb88df2e91b8b3d8
Cr-Commit-Position: refs/heads/master@{#403021}
==========

[commit-bot: I haz the power, 2016-06-30 00:25:38]

Patchset 3 (id:??) landed as
https://crrev.com/b457c9197023162148f861d3fb88df2e91b8b3d8
Cr-Commit-Position: refs/heads/master@{#403021}