Autofill DOMUI Prefs should work with i18n phone numbers

Autofill DOMUI Prefs should work with i18n phone numbers

Adds validation to the phone and fax lists in WebUI based prefs for Autofill.  Also adds the start of phone_number_i18n.cc/h module.

BUG=80101
TEST=PhoneNumberI18NTest.PhoneNumbersMatch

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84320

[GeorgeY, 2011-05-05 18:24:25]

LGTM

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:33: match ==
PhoneNumberUtil::EXACT_MATCH;
Don't we want to have SHORT_NSN_MATCH here as well?

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
country_code);
In my utils I have

enum PhoneMatch {
  PHONES_NOT_EQUAL,
  PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
  PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for +1(650)2345678.
};

// Compares two phones, normalizing them before comparision.
PhoneMatch ComparePhones(const string16& phone1, const string16& phone2,
                         const char* locale);

[dhollowa, 2011-05-05 18:31:01]

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:33: match ==
PhoneNumberUtil::EXACT_MATCH;
No, the short match would equate "8002223333" and "2223333".  This is different
enough from a user perspective that we should allow it when hand entered.

On 2011/05/05 18:24:25, GeorgeY wrote:
> Don't we want to have SHORT_NSN_MATCH here as well?

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
country_code);
Do you make use of "PHONES_SUBMATCH" ?  If so, for what?

On 2011/05/05 18:24:25, GeorgeY wrote:
> In my utils I have
> 
> enum PhoneMatch {
>   PHONES_NOT_EQUAL,
>   PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
>   PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for
+1(650)2345678.
> };
> 
> // Compares two phones, normalizing them before comparision.
> PhoneMatch ComparePhones(const string16& phone1, const string16& phone2,
>                          const char* locale);

[dhollowa, 2011-05-05 19:27:04]

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
country_code);
Let's generalize this once we know we need it.  I'll leave as-is for now. 
Thanks.

On 2011/05/05 18:31:01, dhollowa wrote:
> Do you make use of "PHONES_SUBMATCH" ?  If so, for what?
> 
> On 2011/05/05 18:24:25, GeorgeY wrote:
> > In my utils I have
> > 
> > enum PhoneMatch {
> >   PHONES_NOT_EQUAL,
> >   PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
> >   PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for
> +1(650)2345678.
> > };
> > 
> > // Compares two phones, normalizing them before comparision.
> > PhoneMatch ComparePhones(const string16& phone1, const string16& phone2,
> >                          const char* locale);
>

[James Hawkins, 2011-05-05 22:35:43]

On 2011/05/05 19:27:04, dhollowa wrote:
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> File chrome/browser/autofill/phone_number_i18n.h (right):
> 
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
> country_code);
> Let's generalize this once we know we need it.  I'll leave as-is for now. 
> Thanks.
> 
> On 2011/05/05 18:31:01, dhollowa wrote:
> > Do you make use of "PHONES_SUBMATCH" ?  If so, for what?
> > 
> > On 2011/05/05 18:24:25, GeorgeY wrote:
> > > In my utils I have
> > > 
> > > enum PhoneMatch {
> > >   PHONES_NOT_EQUAL,
> > >   PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
> > >   PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for
> > +1(650)2345678.
> > > };
> > > 
> > > // Compares two phones, normalizing them before comparision.
> > > PhoneMatch ComparePhones(const string16& phone1, const string16& phone2,
> > >                          const char* locale);
> >

I'm not sure how the OWNERs check didn't kick in here, but I should have been on
the review for this change.

[dhollowa, 2011-05-05 23:16:55]

James, I've reverted due to GYP issues.  I'll be sure to include you on these in
the future.  Please feel free to review this one now too... and I'll address any
issues before relanding.  Thanks.

On 2011/05/05 22:35:43, James Hawkins wrote:
> On 2011/05/05 19:27:04, dhollowa wrote:
> >
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> > File chrome/browser/autofill/phone_number_i18n.h (right):
> > 
> >
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> > chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
> > country_code);
> > Let's generalize this once we know we need it.  I'll leave as-is for now. 
> > Thanks.
> > 
> > On 2011/05/05 18:31:01, dhollowa wrote:
> > > Do you make use of "PHONES_SUBMATCH" ?  If so, for what?
> > > 
> > > On 2011/05/05 18:24:25, GeorgeY wrote:
> > > > In my utils I have
> > > > 
> > > > enum PhoneMatch {
> > > >   PHONES_NOT_EQUAL,
> > > >   PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
> > > >   PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for
> > > +1(650)2345678.
> > > > };
> > > > 
> > > > // Compares two phones, normalizing them before comparision.
> > > > PhoneMatch ComparePhones(const string16& phone1, const string16& phone2,
> > > >                          const char* locale);
> > >
> 
> I'm not sure how the OWNERs check didn't kick in here, but I should have been
on
> the review for this change.

[dhollowa, 2011-05-06 01:05:22]

Please see http://codereview.chromium.org/6935033 for new patch.

On 2011/05/05 23:16:55, dhollowa wrote:
> James, I've reverted due to GYP issues.  I'll be sure to include you on these
in
> the future.  Please feel free to review this one now too... and I'll address
any
> issues before relanding.  Thanks.
> 
> On 2011/05/05 22:35:43, James Hawkins wrote:
> > On 2011/05/05 19:27:04, dhollowa wrote:
> > >
> >
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> > > File chrome/browser/autofill/phone_number_i18n.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6930037/diff/1010/chrome/browser/autofill/phon...
> > > chrome/browser/autofill/phone_number_i18n.h:17: const std::string&
> > > country_code);
> > > Let's generalize this once we know we need it.  I'll leave as-is for now. 
> > > Thanks.
> > > 
> > > On 2011/05/05 18:31:01, dhollowa wrote:
> > > > Do you make use of "PHONES_SUBMATCH" ?  If so, for what?
> > > > 
> > > > On 2011/05/05 18:24:25, GeorgeY wrote:
> > > > > In my utils I have
> > > > > 
> > > > > enum PhoneMatch {
> > > > >   PHONES_NOT_EQUAL,
> > > > >   PHONES_EQUAL,  // +1(650)2345678 is equal to 1-650-234-56-78.
> > > > >   PHONES_SUBMATCH, // 650-234-56-78 or 2345678 is a submatch for
> > > > +1(650)2345678.
> > > > > };
> > > > > 
> > > > > // Compares two phones, normalizing them before comparision.
> > > > > PhoneMatch ComparePhones(const string16& phone1, const string16&
phone2,
> > > > >                          const char* locale);
> > > >
> > 
> > I'm not sure how the OWNERs check didn't kick in here, but I should have
been
> on
> > the review for this change.