[Autofill] Make PersonalDataManager::OverwriteWith case sensitive.

That way Autofill always keeps the latest "syntax" that the user used for each field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest

Committed: https://crrev.com/2bc61dd282133e1c333e3998b726621455f509dd
Cr-Commit-Position: refs/heads/master@{#391496}

[sebsg, 2016-04-28 23:04:17]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-04-28 23:05:55]

Description was changed from

==========
[Autofill] Make PersonalDataManager::OverwriteWith case sensitive.

BUG=
==========

to

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

[sebsg, 2016-04-29 02:16:37]

Description was changed from

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

to

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

[sebsg, 2016-04-29 02:16:37]

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

[sebsg, 2016-04-29 02:29:29]

Patchset #1 (id:20001) has been deleted

[sebsg, 2016-04-29 02:31:39]

Hi math, could you please take a look?

[Mathieu, 2016-04-29 12:18:43]

Thanks for the cleanup!

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:528: ServerFieldTypeSet
field_types;
add a comment about |field_types|. Perhaps rename "types_to_override"?

// |field_types| is initially populated with all types that have non-empty data
in the incoming |profile|. After adjustment, all data from |profile|
corresponding to types in |field_types| is overwritten in |this| profile.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:562: if (group == NAME) {
nit: inline AutofillType(*iter).group() here

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:563: did_overwrite =
OverwriteName(profile.name_, app_locale) || did_overwrite;
seems like we are going to run OverwriteName for all types in group NAME, right?
possible future optimization?

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:563: did_overwrite =
OverwriteName(profile.name_, app_locale) || did_overwrite;
optional nit: did_overwrite |= OverwriteName(...) is clearer at least to me.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:567: base::string16
new_value = profile.GetRawInfo(*iter);
Add a comment: // Overwrite the data in |this| profile for the field type. Set
|did_overwrite| if previous data was different.

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:117: // Overwrites the field
data in this Profile with the non-empty ones in
nit: ones -> fields

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:118: // |profile|. Returns
|true| if at least one field was overwritten.
Should we add a note about how origin, language_code, use_count and use_date are
always updated and don't affect the return value?

[Mathieu, 2016-04-29 12:23:03]

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:40: bool
NameInfo::ParsedNamesAreEqual(const NameInfo& info) const {
update comment in .h

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:41:
l10n::CaseInsensitiveCompare compare;
no need for compare

[sebsg, 2016-04-29 15:01:57]

Patchset #2 (id:60001) has been deleted

[sebsg, 2016-04-29 15:04:49]

Thanks for the comments!

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:528: ServerFieldTypeSet
field_types;
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> add a comment about |field_types|. Perhaps rename "types_to_override"?
> 
> // |field_types| is initially populated with all types that have non-empty
data
> in the incoming |profile|. After adjustment, all data from |profile|
> corresponding to types in |field_types| is overwritten in |this| profile.

Done.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:562: if (group == NAME) {
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> nit: inline AutofillType(*iter).group() here

Done.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:563: did_overwrite =
OverwriteName(profile.name_, app_locale) || did_overwrite;
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> optional nit: did_overwrite |= OverwriteName(...) is clearer at least to me.

Done.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:563: did_overwrite =
OverwriteName(profile.name_, app_locale) || did_overwrite;
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> seems like we are going to run OverwriteName for all types in group NAME,
right?
> possible future optimization?

CollapseCompoundFieldTypes makes sure we only overwrite full types so it should
only run once.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:567: base::string16
new_value = profile.GetRawInfo(*iter);
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> Add a comment: // Overwrite the data in |this| profile for the field type. Set
> |did_overwrite| if previous data was different.

Done.

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:117: // Overwrites the field
data in this Profile with the non-empty ones in
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> nit: ones -> fields

Done.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:118: // |profile|. Returns
|true| if at least one field was overwritten.
On 2016/04/29 12:18:43, Mathieu Perreault wrote:
> Should we add a note about how origin, language_code, use_count and use_date
are
> always updated and don't affect the return value?

Done.

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

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:40: bool
NameInfo::ParsedNamesAreEqual(const NameInfo& info) const {
On 2016/04/29 12:23:03, Mathieu Perreault wrote:
> update comment in .h

Done.

https://codereview.chromium.org/1931123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:41:
l10n::CaseInsensitiveCompare compare;
On 2016/04/29 12:23:03, Mathieu Perreault wrote:
> no need for compare

Done.

[Mathieu, 2016-04-29 15:52:06]

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

https://codereview.chromium.org/1931123002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:490:
name_.SetRawInfo(NAME_FULL, imported_name.GetRawInfo(NAME_FULL));
let's make sure that if name_'s NAME_FULL is empty we fill it, below?

https://codereview.chromium.org/1931123002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:561: for
(ServerFieldTypeSet::const_iterator iter = types_to_overwrite.begin();
nit: can we use a range for loop?

for (const ServerFieldType& type : types_to_overwrite) ?

[sebsg, 2016-05-03 19:49:03]

Patchset #3 (id:100001) has been deleted

[sebsg, 2016-05-03 19:53:05]

Could you please take another look ? Thanks!

[Mathieu, 2016-05-03 19:58:34]

lgtm

[sebsg, 2016-05-03 20:02:18]

Thanks!

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

https://codereview.chromium.org/1931123002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:490:
name_.SetRawInfo(NAME_FULL, imported_name.GetRawInfo(NAME_FULL));
On 2016/04/29 15:52:06, Mathieu Perreault wrote:
> let's make sure that if name_'s NAME_FULL is empty we fill it, below?

Done.

https://codereview.chromium.org/1931123002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:561: for
(ServerFieldTypeSet::const_iterator iter = types_to_overwrite.begin();
On 2016/04/29 15:52:06, Mathieu Perreault wrote:
> nit: can we use a range for loop?
> 
> for (const ServerFieldType& type : types_to_overwrite) ?

Done.

[sebsg, 2016-05-03 20:02:34]

Patchset #3 (id:120001) has been deleted

[sebsg, 2016-05-03 20:02:41]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-05-03 20:02:41]

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/1931123002/#ps140001
(title: "Addressed comments and added tests")

[commit-bot: I haz the power, 2016-05-03 20:02:56]

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

[commit-bot: I haz the power, 2016-05-03 20:06:00]

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

[commit-bot: I haz the power, 2016-05-03 20:06:01]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sebsg, 2016-05-03 22:09:00]

Patchset #4 (id:160001) has been deleted

[sebsg, 2016-05-03 23:24:09]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-05-03 23:24:09]

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/1931123002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-03 23:25:09]

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

[commit-bot: I haz the power, 2016-05-04 00:53:02]

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

[commit-bot: I haz the power, 2016-05-04 00:53:05]

Failed to apply the patch.

[sebsg, 2016-05-04 13:38:57]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-05-04 13:39:14]

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

[commit-bot: I haz the power, 2016-05-04 13:43:43]

Description was changed from

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

to

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

[commit-bot: I haz the power, 2016-05-04 13:43:44]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2016-05-04 13:44:48]

Description was changed from

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest
==========

to

==========
That way Autofill always keeps the latest "syntax" that the user used for each
field.

Also added a couple of other merge tests.

BUG=607738
TEST=ContactInfoTest, AutofillMergeTest

Committed: https://crrev.com/2bc61dd282133e1c333e3998b726621455f509dd
Cr-Commit-Position: refs/heads/master@{#391496}
==========

[commit-bot: I haz the power, 2016-05-04 13:44:49]

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