Make AutofillField::FindValueInSelectControl() handle non-ASCII values.

Make AutofillField::FindValueInSelectControl() handle non-ASCII values.

BUG=470726
Committed: https://crrev.com/6334aa835e879d9e025679d276f48149ba97b665
Cr-Commit-Position: refs/heads/master@{#323844}

[Lei Zhang, 2015-04-03 21:23:34]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Lei Zhang, 2015-04-03 21:26:57]

BTW, we have a lot more code in autofill_field.cc using StringToLowerASCII(). I
have a CL in base/ to DCHECK(IsStringASCII(input)) for
TrimWhitespaceASCII(input), and it failed a bunch of tests, including an
AutofillFieldTest.

[Evan Stade, 2015-04-03 22:02:51]

estade@chromium.org changed reviewers:
+ jshin@chromium.org

[Evan Stade, 2015-04-03 22:02:51]

> BTW, we have a lot more code in autofill_field.cc using StringToLowerASCII().

can we just change that to tolower?

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

https://codereview.chromium.org/1059173002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_field.cc:400: return
base::i18n::CompareString16WithCollator(collator, lhs, rhs) ==
I was expecting this code to use base::CaseInsensitiveCompare. What is the
difference between that and ICU? Can you get jungshik or someone to help review?

[Lei Zhang, 2015-04-03 22:10:14]

On 2015/04/03 22:02:51, Evan Stade wrote:
> > BTW, we have a lot more code in autofill_field.cc using
StringToLowerASCII().
> 
> can we just change that to tolower?
> 
>
https://codereview.chromium.org/1059173002/diff/1/components/autofill/core/br...
> File components/autofill/core/browser/autofill_field.cc (right):
> 
>
https://codereview.chromium.org/1059173002/diff/1/components/autofill/core/br...
> components/autofill/core/browser/autofill_field.cc:400: return
> base::i18n::CompareString16WithCollator(collator, lhs, rhs) ==
> I was expecting this code to use base::CaseInsensitiveCompare. What is the
> difference between that and ICU? Can you get jungshik or someone to help
review?

I wrote the quebec test case first and tried to use tolower() via
CaseInsensitiveCompare() and it doesn't fix the test case.

Try this and you'll see tolower() is a no-op in the C, en_US.UTF-8, and
ru_RU.UTF-8 locales.

#include <ctype.h>
#include <stdio.h>

int main() {
  char foo[] = "QUÉBEC";
  printf("%s\n", foo);
  printf("%d\n", foo[2]);

  foo[2] = tolower(foo[2]);
  printf("%s\n", foo);
  printf("%d\n", foo[2]);

  return 0;
}

[Evan Stade, 2015-04-03 22:48:28]

On 2015/04/03 22:10:14, Lei Zhang wrote:
> On 2015/04/03 22:02:51, Evan Stade wrote:
> > > BTW, we have a lot more code in autofill_field.cc using
> StringToLowerASCII().
> > 
> > can we just change that to tolower?
> > 
> >
>
https://codereview.chromium.org/1059173002/diff/1/components/autofill/core/br...
> > File components/autofill/core/browser/autofill_field.cc (right):
> > 
> >
>
https://codereview.chromium.org/1059173002/diff/1/components/autofill/core/br...
> > components/autofill/core/browser/autofill_field.cc:400: return
> > base::i18n::CompareString16WithCollator(collator, lhs, rhs) ==
> > I was expecting this code to use base::CaseInsensitiveCompare. What is the
> > difference between that and ICU? Can you get jungshik or someone to help
> review?
> 
> I wrote the quebec test case first and tried to use tolower() via
> CaseInsensitiveCompare() and it doesn't fix the test case.
> 
> Try this and you'll see tolower() is a no-op in the C, en_US.UTF-8, and
> ru_RU.UTF-8 locales.
> 
> #include <ctype.h>
> #include <stdio.h>
> 
> int main() {
>   char foo[] = "QUÉBEC";
>   printf("%s\n", foo);
>   printf("%d\n", foo[2]);
> 
>   foo[2] = tolower(foo[2]);
>   printf("%s\n", foo);
>   printf("%d\n", foo[2]);
> 
>   return 0;
> }

I guess that if you chose the right locale, the test case would still pass with
CaseInsensitiveCompare? Like if the locale were en_CA or something. OK, lgtm

[Lei Zhang, 2015-04-03 22:55:15]

On 2015/04/03 22:48:28, Evan Stade wrote:
> I guess that if you chose the right locale, the test case would still pass
with
> CaseInsensitiveCompare? Like if the locale were en_CA or something. OK, lgtm

Doesn't work there either.

[Lei Zhang, 2015-04-03 22:55:18]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-04-03 22:56:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059173002/1

[commit-bot: I haz the power, 2015-04-03 23:00:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-03 23:01:08]

Patchset 1 (id:??) landed as
https://crrev.com/6334aa835e879d9e025679d276f48149ba97b665
Cr-Commit-Position: refs/heads/master@{#323844}