Remove diacritics when normalizing autofill profile strings for comparison.

Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

Committed: https://crrev.com/96e2312ade0d985d9b2c4877f54dfacd7939bc9f
Cr-Commit-Position: refs/heads/master@{#396748}

[Roger McFarlane (Chromium), 2016-05-26 15:12:07]

Patchset #1 (id:1) has been deleted

[Roger McFarlane (Chromium), 2016-05-26 15:12:52]

Description was changed from

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

BUG=612043
R=mathp,sebsg
==========

to

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

[Roger McFarlane (Chromium), 2016-05-26 15:12:53]

rogerm@chromium.org changed reviewers:
+ mathp@chromium.org, sebsg@chromium.or
- mathp@google.com, sebsg@chromium.org

[Roger McFarlane (Chromium), 2016-05-26 15:14:56]

Description was changed from

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

to

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

[Roger McFarlane (Chromium), 2016-05-26 15:15:12]

rogerm@chromium.org changed reviewers:
+ sebsg@chromium.org
- sebsg@chromium.or

[Roger McFarlane (Chromium), 2016-05-26 15:30:20]

PTAL

[sebsg, 2016-05-26 16:26:07]

On 2016/05/26 15:30:20, Roger McFarlane (Chromium) wrote:
> PTAL

The code looks but could you please add some tests to make sure we merge
profiles and upload the correct types to the server? 

I'm thinking using some diacritics in a name or something else and making sure
the profile merges with another one with no diacritics. For example Mitchell <->
Mïtchéll.
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Also making sure it works when determining possible field type for upload. Here
you can probably use Ile de France -> Île de France or something like that.
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Thanks, this is going to improve Autofill a lot!

[Roger McFarlane (Chromium), 2016-05-27 15:50:07]

Thanks! Another look?

[sebsg, 2016-05-27 16:58:44]

On 2016/05/27 15:50:07, Roger McFarlane (Chromium) wrote:
> Thanks! Another look?

LGTM with nit. Thanks!

[sebsg, 2016-05-27 16:59:14]

LGTM with nit. Thanks!

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_unittest.cc:1221: //
uppercase.
nit: ... in uppercase and have diacritics.

[Mathieu, 2016-05-27 17:47:13]

Thanks, comments

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3563: {"Ile de
France", ADDRESS_HOME_STATE},  // Note: missing accent on Île.
Could we expand the test cases to verify other things are normalized? Is it
worth adding a test case for Ile-de-France, for example?

Also can we add a test case for Île de France

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:244: // Remove diacritics
and fold case.
extra space before comment

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:247: "NFD; [:Nonspacing
Mark:] Remove; Lower; NFC", UTRANS_FORWARD, status));
Can we put an explanation (or a link to the explanation) for the magic string?
The more details the better, so that the reader (which could be us in 6 months)
know what's happening.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:250: LOG(ERROR) <<
"normalization failed: " << u_errorName(status);
Let's capitalize the comment and make it more explicit so that it's clear to
someone reading the logs.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:255:
transliterator->transliterate(value);
can we check "if (transliterator)" here? ICU has a bad habit of returning null
instances sometimes.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:854: case
U_TITLECASE_LETTER:
Will this now be done by the normalizer?

[Roger McFarlane (Chromium), 2016-05-27 20:20:43]

Thanks. Another look?

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3563: {"Ile de
France", ADDRESS_HOME_STATE},  // Note: missing accent on Île.
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> Could we expand the test cases to verify other things are normalized? Is it
> worth adding a test case for Ile-de-France, for example?
> 
> Also can we add a test case for Île de France

Done.

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:244: // Remove diacritics
and fold case.
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> extra space before comment

Done.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:247: "NFD; [:Nonspacing
Mark:] Remove; Lower; NFC", UTRANS_FORWARD, status));
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> Can we put an explanation (or a link to the explanation) for the magic string?
> The more details the better, so that the reader (which could be us in 6
months)
> know what's happening.

Done.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:250: LOG(ERROR) <<
"normalization failed: " << u_errorName(status);
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> Let's capitalize the comment and make it more explicit so that it's clear to
> someone reading the logs.

Done.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:255:
transliterator->transliterate(value);
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> can we check "if (transliterator)" here? ICU has a bad habit of returning null
> instances sometimes.

Done.

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:854: case
U_TITLECASE_LETTER:
On 2016/05/27 17:47:13, Mathieu Perreault wrote:
> Will this now be done by the normalizer?

Yes! Done.

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

https://codereview.chromium.org/2013063002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_profile_unittest.cc:1221: //
uppercase.
On 2016/05/27 16:59:14, sebsg wrote:
> nit: ... in uppercase and have diacritics.

Done.

[Roger McFarlane (Chromium), 2016-05-27 20:21:24]

Patchset #3 (id:60001) has been deleted

[Mathieu, 2016-05-27 20:26:49]

lgtm thanks

[Roger McFarlane (Chromium), 2016-05-30 16:50:03]

The CQ bit was checked by rogerm@chromium.org

[Roger McFarlane (Chromium), 2016-05-30 16:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2013063002/#ps80001
(title: "Simplify normalization loop and expand test cases.")

[commit-bot: I haz the power, 2016-05-30 16:50:13]

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

[commit-bot: I haz the power, 2016-05-30 18:17:09]

Description was changed from

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

to

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

[commit-bot: I haz the power, 2016-05-30 18:17:11]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-05-30 18:18:18]

Description was changed from

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

to

==========
Remove diacritics when normalizing autofill profile strings for comparison.

Uses the ICU Transliterator to remove accents (and other non-spacing marks)
from characters that have an ASCII equivalents and transforms uppercase chars
to lower case, while leaving other charcters unchanged.

See: http://userguide.icu-project.org/transforms/general

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

Committed: https://crrev.com/96e2312ade0d985d9b2c4877f54dfacd7939bc9f
Cr-Commit-Position: refs/heads/master@{#396748}
==========

[commit-bot: I haz the power, 2016-05-30 18:18:19]

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