These changes *are* for review :)

Autofill phone number enhancements and integration of Phone Number Util Library: part 3
BUG=71443
TEST=unit-tested
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86305

[GeorgeY, 2011-05-13 00:22:05]

ready for review

[dhollowa, 2011-05-13 18:55:35]

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:135: // Phones need to
be rebuild.
nit: s/rebuild/rebuilt/

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:152: switch
(it->second) {
This is getting pretty hard to read with |SetInfo| at line 174 and then below at
185 and 192.  It is not immediately clear how to make this more readable though.
 Do you have any ideas on how we might split out a helper function or something
to deal with phone specific logic?

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:1882: string16 saved_phone
= work_profile->GetInfo(PHONE_HOME_WHOLE_NUMBER);
It looks like this |save_phone| is unneeded.  It is used again at line 1912, but
never checked.  And |work_profile| is reset for the next test in the |SetUp|
call.  Let's remove this unused logic.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:181: // Set the type of the phones -
it is set on first SetInfo() call.
Better to use the vector initializer to set the phone/fax types here, i.e.:

AutofillProfile::AutofillProfile(const std::string& guid)
    : guid_(guid), name_(1), email_(1),
      home_number_(1, PhoneNumber(AutofillType::PHONE_HOME)),
      fax_number_(1, PhoneNumber(AutofillType::PHONE_FAX)) {

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:192: // Set the type of the phones -
it is set on first SetInfo() call.
Better to use the vector initializer to set the phone/fax types here, i.e.:

AutofillProfile::AutofillProfile(const std::string& guid)
    : guid_(guid), name_(1), email_(1),
      home_number_(1, PhoneNumber(AutofillType::PHONE_HOME)),
      fax_number_(1, PhoneNumber(AutofillType::PHONE_FAX)) {

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:261: CopyValuesToItems(type, values,
&home_number_);
Let's keep the initialization logic cohesive within |CopyValuesToItems|.  Like
this:

Here:
      CopyValuesToItems(type, values, &home_number_,
                        PhoneNumber(AutofillType::PHONE_HOME));

And add a |prototype| parameter to |CopyValuesToItems| like this:

void CopyValuesToItems(AutofillFieldType type,
                       const std::vector<string16>& values,
                       std::vector<T>* form_group_items,
                       const T& prototype) {
...
  if (form_group_items->empty())
    form_group_items->resize(1, prototype);
}

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:513: if (group ==
AutofillType::PHONE_HOME ||
A few things here:
 (1) Let's split this logic out into a helper function to keep things readable. 
Maybe something like |bool AddPhoneOrFax(...)|

 (2) We should use |autofill_i18n::PhoneNumbersMatch| here I believe,

 (3) I to keep consistent with existing use of |std::find| let's use
|std::find_if| in conjunction with |autofill_i18n::PhoneNumbersMatch|.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:122: // Normalizes the phone and
numbers.
nit: "Normalizes the home phone and fax numbers."

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
personal_data_manager_mac.cc needs to be fixed.  It is not currently compiling.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:206: // We only set complete
phone, so aggregate phone parts in these vars and set
The phone/fax specific logic in this function is getting overly large.  I'm
wondering if we can package up the intermediate components of the phone numbers
(home_number, home_city_code, etc...) into a utility class, and then move the
assembly of the final |constructed_number| below off into that utility class.

Thoughts?

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:15: #include
"third_party/libphonenumber/cpp/src/phonenumberutil.h"
This breaks encapsulation of our phone_number_i18n.h interface.

Only phone_number_i18n.h should include phonenumberutil.h.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_unittest.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_unittest.cc:21:
EXPECT_EQ(matching_types.size(), 0);
For these tests the constants should use "UL" suffix, and the expected value
should be on the left.  I.e.

EXPECT_EQ(0UL, matching_types.size());

[Ilya Sherman, 2011-05-14 04:38:51]

Couple of minor comments:

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:335: if
(!imported_profile->NormalizePhones())
It looks like this is the only place where NormalizePhones() is used -- perhaps
that logic should be local to the PersonalDataManager implementation?

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:46: // Setting it to "", actually sets it
to default locale - "US".
Should we use the app locale rather than always defaulting to "US"?

[GeorgeY, 2011-05-18 17:41:45]

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:135: // Phones need to
be rebuild.
On 2011/05/13 18:55:35, dhollowa wrote:
> nit: s/rebuild/rebuilt/

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:152: switch
(it->second) {
On 2011/05/13 18:55:35, dhollowa wrote:
> This is getting pretty hard to read with |SetInfo| at line 174 and then below
at
> 185 and 192.  It is not immediately clear how to make this more readable
though.
>  Do you have any ideas on how we might split out a helper function or
something
> to deal with phone specific logic?

Simplified it

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:1882: string16 saved_phone
= work_profile->GetInfo(PHONE_HOME_WHOLE_NUMBER);
On 2011/05/13 18:55:35, dhollowa wrote:
> It looks like this |save_phone| is unneeded.  It is used again at line 1912,
but
> never checked.  And |work_profile| is reset for the next test in the |SetUp|
> call.  Let's remove this unused logic.

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:181: // Set the type of the phones -
it is set on first SetInfo() call.
On 2011/05/13 18:55:35, dhollowa wrote:
> Better to use the vector initializer to set the phone/fax types here, i.e.:
> 
> AutofillProfile::AutofillProfile(const std::string& guid)
>     : guid_(guid), name_(1), email_(1),
>       home_number_(1, PhoneNumber(AutofillType::PHONE_HOME)),
>       fax_number_(1, PhoneNumber(AutofillType::PHONE_FAX)) {

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:192: // Set the type of the phones -
it is set on first SetInfo() call.
On 2011/05/13 18:55:35, dhollowa wrote:
> Better to use the vector initializer to set the phone/fax types here, i.e.:
> 
> AutofillProfile::AutofillProfile(const std::string& guid)
>     : guid_(guid), name_(1), email_(1),
>       home_number_(1, PhoneNumber(AutofillType::PHONE_HOME)),
>       fax_number_(1, PhoneNumber(AutofillType::PHONE_FAX)) {

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:261: CopyValuesToItems(type, values,
&home_number_);
On 2011/05/13 18:55:35, dhollowa wrote:
> Let's keep the initialization logic cohesive within |CopyValuesToItems|.  Like
> this:
> 
> Here:
>       CopyValuesToItems(type, values, &home_number_,
>                         PhoneNumber(AutofillType::PHONE_HOME));
> 
> And add a |prototype| parameter to |CopyValuesToItems| like this:
> 
> void CopyValuesToItems(AutofillFieldType type,
>                        const std::vector<string16>& values,
>                        std::vector<T>* form_group_items,
>                        const T& prototype) {
> ...
>   if (form_group_items->empty())
>     form_group_items->resize(1, prototype);
> }

OK

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.cc:513: if (group ==
AutofillType::PHONE_HOME ||
On 2011/05/13 18:55:35, dhollowa wrote:
> A few things here:
>  (1) Let's split this logic out into a helper function to keep things
readable. 
> Maybe something like |bool AddPhoneOrFax(...)|
> 
>  (2) We should use |autofill_i18n::PhoneNumbersMatch| here I believe,
> 
>  (3) I to keep consistent with existing use of |std::find| let's use
> |std::find_if| in conjunction with |autofill_i18n::PhoneNumbersMatch|.

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:122: // Normalizes the phone and
numbers.
On 2011/05/13 18:55:35, dhollowa wrote:
> nit: "Normalizes the home phone and fax numbers."

Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/05/13 18:55:35, dhollowa wrote:
> personal_data_manager_mac.cc needs to be fixed.  It is not currently
compiling.

Commented it out for now, could you, please, fix it the same way it is changed
in .cc?

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:206: // We only set complete
phone, so aggregate phone parts in these vars and set
On 2011/05/13 18:55:35, dhollowa wrote:
> The phone/fax specific logic in this function is getting overly large.  I'm
> wondering if we can package up the intermediate components of the phone
numbers
> (home_number, home_city_code, etc...) into a utility class, and then move the
> assembly of the final |constructed_number| below off into that utility class.
> 
> Thoughts?

simplified it (see code)

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/per...
chrome/browser/autofill/personal_data_manager.cc:335: if
(!imported_profile->NormalizePhones())
On 2011/05/14 04:38:51, Ilya Sherman wrote:
> It looks like this is the only place where NormalizePhones() is used --
perhaps
> that logic should be local to the PersonalDataManager implementation?

home_number_, fax_number_ are encapsulated in profile and are local to that

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:15: #include
"third_party/libphonenumber/cpp/src/phonenumberutil.h"
On 2011/05/13 18:55:35, dhollowa wrote:
> This breaks encapsulation of our phone_number_i18n.h interface.
> 
> Only phone_number_i18n.h should include phonenumberutil.h.

Forgot to remove it. Done.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:46: // Setting it to "", actually sets it
to default locale - "US".
On 2011/05/14 04:38:51, Ilya Sherman wrote:
> Should we use the app locale rather than always defaulting to "US"?

It is actually used in one place - NormalizePhones (and the unit-test) and in
that place we *do* know the locale.

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_unittest.cc (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_unittest.cc:21:
EXPECT_EQ(matching_types.size(), 0);
On 2011/05/13 18:55:35, dhollowa wrote:
> For these tests the constants should use "UL" suffix, and the expected value
> should be on the left.  I.e.
> 
> EXPECT_EQ(0UL, matching_types.size());

Done.

[isherman%chromium.org_gtempaccount.com, 2011-05-18 18:06:09]

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/6877130/diff/20001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:46: // Setting it to "", actually sets it
to default locale - "US".
On 2011/05/18 17:41:45, GeorgeY wrote:
> On 2011/05/14 04:38:51, Ilya Sherman wrote:
> > Should we use the app locale rather than always defaulting to "US"?
> 
> It is actually used in one place - NormalizePhones (and the unit-test) and in
> that place we *do* know the locale.

In that case, perhaps we should have a DCHECK here instead?

[isherman%chromium.org_gtempaccount.com, 2011-05-18 18:21:44]

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:176: locale_ = "US";  // US is a default
locale.
Here also: Why default to "US" rather than the app locale?

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:273: } else {
What about, e.g., PHONE_HOME_NUMBER -- which could be either a full number, a
prefix, or a suffix?

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:62: }
nit: Please move this curly brace onto the previous line.

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:72: }
nit: Please condense this function prototype + body to one line.

[GeorgeY, 2011-05-19 00:30:27]

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:176: locale_ = "US";  // US is a default
locale.
On 2011/05/18 18:21:45, isherman%chromium.org wrote:
> Here also: Why default to "US" rather than the app locale?

Done.

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:273: } else {
On 2011/05/18 18:21:45, isherman%chromium.org wrote:
> What about, e.g., PHONE_HOME_NUMBER -- which could be either a full number, a
> prefix, or a suffix?

Bug - apparently I need to add unit-test :).

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:62: }
On 2011/05/18 18:21:45, isherman%chromium.org wrote:
> nit: Please move this curly brace onto the previous line.

Sure, less readable, but sure.

http://codereview.chromium.org/6877130/diff/27001/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.h:72: }
On 2011/05/18 18:21:45, isherman%chromium.org wrote:
> nit: Please condense this function prototype + body to one line.

Sure, less readable, but sure.

[Ilya Sherman, 2011-05-19 05:52:25]

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:20: return
std::string(icu::Locale::getDefault().getCountry());
You might want to use AutofillCountry::ApplicationLocale() (which is just a
wrapper around g_browser_process->GetApplicationLocale()) here instead.  I think
Chrome has a better idea of the default locale than ICU does.

[GeorgeY, 2011-05-19 19:00:17]

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:20: return
std::string(icu::Locale::getDefault().getCountry());
On 2011/05/19 05:52:25, Ilya Sherman wrote:
> You might want to use AutofillCountry::ApplicationLocale() (which is just a
> wrapper around g_browser_process->GetApplicationLocale()) here instead.  I
think
> Chrome has a better idea of the default locale than ICU does.

Done.

[GeorgeY, 2011-05-19 20:45:53]

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:20: return
std::string(icu::Locale::getDefault().getCountry());
On 2011/05/19 19:00:18, GeorgeY wrote:
> On 2011/05/19 05:52:25, Ilya Sherman wrote:
> > You might want to use AutofillCountry::ApplicationLocale() (which is just a
> > wrapper around g_browser_process->GetApplicationLocale()) here instead.  I
> think
> > Chrome has a better idea of the default locale than ICU does.
> 
> Done.

Changed before I looked up what it was actually doing: it returns *language*
(like "en" or "pt-br"), and not a country ("US" or "GB" or "BR").
Changed back to icu.

[GeorgeY, 2011-05-19 20:48:03]

On 2011/05/19 20:45:53, GeorgeY wrote:
>
http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
> File chrome/browser/autofill/phone_number_i18n.cc (right):
> 
>
http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
> chrome/browser/autofill/phone_number_i18n.cc:20: return
> std::string(icu::Locale::getDefault().getCountry());
> On 2011/05/19 19:00:18, GeorgeY wrote:
> > On 2011/05/19 05:52:25, Ilya Sherman wrote:
> > > You might want to use AutofillCountry::ApplicationLocale() (which is just
a
> > > wrapper around g_browser_process->GetApplicationLocale()) here instead.  I
> > think
> > > Chrome has a better idea of the default locale than ICU does.
> > 
> > Done.
> 
> Changed before I looked up what it was actually doing: it returns *language*
> (like "en" or "pt-br"), and not a country ("US" or "GB" or "BR").
> Changed back to icu.

Oh, and language is *very* loosely related to the country - I could have a
Russian language setup in USA

[Ilya Sherman, 2011-05-19 20:57:47]

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:20: return
std::string(icu::Locale::getDefault().getCountry());
On 2011/05/19 20:45:54, GeorgeY wrote:
> On 2011/05/19 19:00:18, GeorgeY wrote:
> > On 2011/05/19 05:52:25, Ilya Sherman wrote:
> > > You might want to use AutofillCountry::ApplicationLocale() (which is just
a
> > > wrapper around g_browser_process->GetApplicationLocale()) here instead.  I
> > think
> > > Chrome has a better idea of the default locale than ICU does.
> > 
> > Done.
> 
> Changed before I looked up what it was actually doing: it returns *language*
> (like "en" or "pt-br"), and not a country ("US" or "GB" or "BR").
> Changed back to icu.

Based on """ git gs 'SetDefault(' """ we never set the default locale.  so using
icu::Locale::getDefault() here is deceptive -- it will always return the same
value ("US", I guess).

You're right that ApplicationLocale() returns the wrong thing -- I meant to
instead suggest:
""" AutofillCountry::CountryCodeForLocale(AutofillCountry::ApplicationLocale())
""".  Maybe that should actually be the exposed functionality in that class.

I think it's better for our behavior to match between the guessed country and
the guessed phone number locale.  If you have ideas on how to improve that guess
in general, that would be great :)

[GeorgeY, 2011-05-19 21:29:37]

On 2011/05/19 20:57:47, Ilya Sherman wrote:
>
http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
> File chrome/browser/autofill/phone_number_i18n.cc (right):
> 
>
http://codereview.chromium.org/6877130/diff/34003/chrome/browser/autofill/pho...
> chrome/browser/autofill/phone_number_i18n.cc:20: return
> std::string(icu::Locale::getDefault().getCountry());
> On 2011/05/19 20:45:54, GeorgeY wrote:
> > On 2011/05/19 19:00:18, GeorgeY wrote:
> > > On 2011/05/19 05:52:25, Ilya Sherman wrote:
> > > > You might want to use AutofillCountry::ApplicationLocale() (which is
just
> a
> > > > wrapper around g_browser_process->GetApplicationLocale()) here instead. 
I
> > > think
> > > > Chrome has a better idea of the default locale than ICU does.
> > > 
> > > Done.
> > 
> > Changed before I looked up what it was actually doing: it returns *language*
> > (like "en" or "pt-br"), and not a country ("US" or "GB" or "BR").
> > Changed back to icu.
> 
> Based on """ git gs 'SetDefault(' """ we never set the default locale.  so
using
> icu::Locale::getDefault() here is deceptive -- it will always return the same
> value ("US", I guess).
> 
> You're right that ApplicationLocale() returns the wrong thing -- I meant to
> instead suggest:
> """
AutofillCountry::CountryCodeForLocale(AutofillCountry::ApplicationLocale())
> """.  Maybe that should actually be the exposed functionality in that class.
> 
> I think it's better for our behavior to match between the guessed country and
> the guessed phone number locale.  If you have ideas on how to improve that
guess
> in general, that would be great :)

AFAIK, it does parse the correct locale, for it to return US we need to set
compiler flag which we do not set (at least on Windows), but sure I can use this
one instead. AFAIK, *both* make some guesses - the question is who is the best
one guessing :).

[dhollowa, 2011-05-19 23:35:02]

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:96: if (number().empty())
Let's get rid of |number()|, |set_number()|, |extension()|, and
|set_extension()| while we're in here.  They're not helpful.

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:145: 
nit: remove extra line.

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:291: // TODO(georgey): This will need to
be updated once we add support for
I'm confused by this comment.  What further work is needed?  I would have
thought this could take |locale_| into account and only use the prefix/suffix
logic if we're dealing with "US".  No?

[GeorgeY, 2011-05-20 00:41:49]

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number.cc (right):

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:96: if (number().empty())
On 2011/05/19 23:35:02, dhollowa wrote:
> Let's get rid of |number()|, |set_number()|, |extension()|, and
> |set_extension()| while we're in here.  They're not helpful.

Yep, never used. Removed

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:145: 
On 2011/05/19 23:35:02, dhollowa wrote:
> nit: remove extra line.

Done.

http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number.cc:291: // TODO(georgey): This will need to
be updated once we add support for
On 2011/05/19 23:35:02, dhollowa wrote:
> I'm confused by this comment.  What further work is needed?  I would have
> thought this could take |locale_| into account and only use the prefix/suffix
> logic if we're dealing with "US".  No?

Not only, some other countries support the splitting of the 7 digits number into
3-4 parts (Russia, for example), though I *never* saw any non-US form doing it.

[dhollowa, 2011-05-20 16:00:51]

Did the changes not get uploaded?

On 2011/05/20 00:41:49, GeorgeY wrote:
>
http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
> File chrome/browser/autofill/phone_number.cc (right):
> 
>
http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
> chrome/browser/autofill/phone_number.cc:96: if (number().empty())
> On 2011/05/19 23:35:02, dhollowa wrote:
> > Let's get rid of |number()|, |set_number()|, |extension()|, and
> > |set_extension()| while we're in here.  They're not helpful.
> 
> Yep, never used. Removed
> 
>
http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
> chrome/browser/autofill/phone_number.cc:145: 
> On 2011/05/19 23:35:02, dhollowa wrote:
> > nit: remove extra line.
> 
> Done.
> 
>
http://codereview.chromium.org/6877130/diff/38006/chrome/browser/autofill/pho...
> chrome/browser/autofill/phone_number.cc:291: // TODO(georgey): This will need
to
> be updated once we add support for
> On 2011/05/19 23:35:02, dhollowa wrote:
> > I'm confused by this comment.  What further work is needed?  I would have
> > thought this could take |locale_| into account and only use the
prefix/suffix
> > logic if we're dealing with "US".  No?
> 
> Not only, some other countries support the splitting of the 7 digits number
into
> 3-4 parts (Russia, for example), though I *never* saw any non-US form doing
it.

[dhollowa, 2011-05-20 17:48:01]

LGTM.  Thanks.

[Ilya Sherman, 2011-05-20 22:00:06]

LGTM, thanks