[Autofill] Improve the merging of two profiles' names.

Make sure a profile with a complete middle name gets merged with a profile with a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest

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

[sebsg, 2016-05-12 03:15:09]

Description was changed from

==========
[Autofill] Improve the merging of two profiles' names.

BUG=
TEST=NameInfoTest, PersonalDataManagerTest
==========

to

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=
TEST=NameInfoTest, PersonalDataManagerTest
==========

[sebsg, 2016-05-12 03:20:28]

Description was changed from

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=
TEST=NameInfoTest, PersonalDataManagerTest
==========

to

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest
==========

[sebsg, 2016-05-12 22:52:35]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-05-13 02:21:37]

Patchset #1 (id:20001) has been deleted

[sebsg, 2016-05-13 02:25:16]

Patchset #1 (id:40001) has been deleted

[sebsg, 2016-05-13 02:42:34]

Patchset #1 (id:60001) has been deleted

[sebsg, 2016-05-13 04:16:48]

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

[sebsg, 2016-05-13 04:16:48]

Hi Math could you please take a look? Thanks!

[Mathieu, 2016-05-13 13:32:52]

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

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:675: // If one of the two
middle names is an initial that match the first
matches

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:677: if
((middle_name.size() == 1 || profile_middle_name.size() == 1) &&
what if exactly one of |middle_name| or |profile_middle_name| is empty string?
wouldn't checking for index [0] below be invalid?

Or is it a precondition of being in this loop? If so, perhaps a dcheck would be
good, above.

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_profile_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile_unittest.cc:1199:
TEST(AutofillProfileTest, SaveAdditionalInfo_Name_DifferentCaseNoNameFull) {
Do we have cases where one of the middle names is empty?

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

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/contact_info.h:30: // more information.
// For every non-empty NameInfo part in |new_name|, the corresponding NameInfo
part in |this| is overwritten. Special logic so that a middle initial may not
overwrite a full middle name.

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/contact_info_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/contact_info_unittest.cc:271: 
nit: feel free to remove empty lines between test cases, here and in the next
test

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4067: // Test
that saving a identical profile except with the name parts set
Can we add a test where it's clear that the full name != name parts? Like "John"
"M" "Smith" "" and "" "" "" "Marion Mitchell Morisson". These would be merged
under the new rules, right? Worth putting a TODO so that it's documented that
it's not ideal.

[sebsg, 2016-05-13 17:20:03]

Thanks for the comments. Could you take another look?

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

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:675: // If one of the two
middle names is an initial that match the first
On 2016/05/13 13:32:51, Mathieu Perreault wrote:
> matches

Done.

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:677: if
((middle_name.size() == 1 || profile_middle_name.size() == 1) &&
On 2016/05/13 13:32:51, Mathieu Perreault wrote:
> what if exactly one of |middle_name| or |profile_middle_name| is empty string?
> wouldn't checking for index [0] below be invalid?
> 
> Or is it a precondition of being in this loop? If so, perhaps a dcheck would
be
> good, above.

Yes, it's a precondition in the loop. We get the non-empty types from profile.
Then there is a if in the loop that checks if that the type is not empty in
|this|. Dcheck sounds like a super idea, thanks!

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_profile_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile_unittest.cc:1199:
TEST(AutofillProfileTest, SaveAdditionalInfo_Name_DifferentCaseNoNameFull) {
On 2016/05/13 13:32:51, Mathieu Perreault wrote:
> Do we have cases where one of the middle names is empty?

Yes, in PersonalDataManagerTest.SaveImportedProfile

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

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/contact_info.h:30: // more information.
On 2016/05/13 13:32:51, Mathieu Perreault wrote:
> // For every non-empty NameInfo part in |new_name|, the corresponding NameInfo
> part in |this| is overwritten. Special logic so that a middle initial may not
> overwrite a full middle name.

Done.

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/contact_info_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/contact_info_unittest.cc:271: 
On 2016/05/13 13:32:52, Mathieu Perreault wrote:
> nit: feel free to remove empty lines between test cases, here and in the next
> test

Done.

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1973873002/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4067: // Test
that saving a identical profile except with the name parts set
On 2016/05/13 13:32:52, Mathieu Perreault wrote:
> Can we add a test where it's clear that the full name != name parts? Like
"John"
> "M" "Smith" "" and "" "" "" "Marion Mitchell Morisson". These would be merged
> under the new rules, right? Worth putting a TODO so that it's documented that
> it's not ideal.

Done.

[Mathieu, 2016-05-16 21:01:47]

lgtm

[sebsg, 2016-05-16 21:22:41]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-05-16 21:23:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973873002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973873002/120001

[commit-bot: I haz the power, 2016-05-16 22:53:32]

Description was changed from

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest
==========

to

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest
==========

[commit-bot: I haz the power, 2016-05-16 22:53:33]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-05-16 22:56:03]

Description was changed from

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest
==========

to

==========
Make sure a profile with a complete middle name gets merged with a profile with
a corresponding middle name initial.

Make sure no loss of information is possible when merging names.

BUG=611282
TEST=NameInfoTest, PersonalDataManagerTest

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

[commit-bot: I haz the power, 2016-05-16 22:56:04]

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