Make autofill's Address store country using the country code so that app locale isn't needed for th…

Make autofill's Address store country using the country code so that app locale isn't needed for the raw methods.

This is in preparation for removing content::GetContentClient calls outside of content.

BUG=227047
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192588

[jam, 2013-04-05 02:27:25]

Ilia, please take a look. I'm not familiar with this code, so not sure if there
are better ways or conventions of doing this (perhaps write migration code)?

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
File components/webdata/autofill/autofill_table.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:94: // Old versions put the
localized country name in column 7.
This is the first time I look at the webdata code. should this instead be a
database migration to a new version?

[Ilya Sherman, 2013-04-05 05:18:01]

Thanks for taking this on, John!

Two more files that I think we'll need to update:
autofill_dialog_controller_impl.cc [1] (2 callsites) and autocheckout_manager.cc
[2] (1 callsite).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
chrome/browser/autofill/android/personal_data_manager_android.cc:54:
profile.GetRawInfo(ADDRESS_HOME_COUNTRY)).obj(),
I think this might need to be updated to be GetInfo().

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
chrome/browser/autofill/android/personal_data_manager_android.cc:97:
profile->SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
Please use SetInfo() rather than SetRawInfo() for this.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_browsertest.cc (left):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_browsertest.cc:1020:
profile1.SetInfo(ADDRESS_HOME_COUNTRY, WideToUTF16(L"India"), "en-US");
Please preserve this; it should continue to work.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_browsertest.cc:1087:
profile7.SetInfo(ADDRESS_HOME_COUNTRY, WideToUTF16(L"United States"),
Ditto.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_browsertest.cc:1159:
without_invalid.SetInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("United States"),
Ditto.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/autofi...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/autofi...
chrome/browser/webdata/autofill_profile_syncable_service.cc:323:
country_name_or_code, g_browser_process->GetApplicationLocale());
Again, this happens on the DB thread (I think), so it's not safe to call into
g_browser_process.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_database_migration_unittest.cc:64:
AutofillCountry::GetCountryCode(s.ColumnString16(12), "en-US")));
Please use SetInfo() for this.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_database_migration_unittest.cc:87:
AutofillCountry::GetCountryCode(s.ColumnString16(7), "en-US")));
Please use SetInfo() for this.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address.cc:108:
ASCIIToUTF16(AutofillCountry::GetCountryCode(value, app_locale));
We should return false from this case if the conversion fails.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address_unittest.cc (left):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address_unittest.cc:115: }
Hmm, why did you remove this test coverage?

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address_unittest.cc:101:
address.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(""));
nit: string16() rather than ASCIIToUTF16("")

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/android/auxiliary_profile_unittest_android.cc
(right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/android/auxiliary_profile_unittest_android.cc:112:
string16 country = ASCIIToUTF16("GB");
Please preserve the string "England" here -- this should continue to work.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/android/auxiliary_profiles_android.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/android/auxiliary_profiles_android.cc:95:
profile->SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
Please call SetInfo with the app_locale rather than doing the conversion within
this file.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_profile_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_profile_unittest.cc:735:
EXPECT_EQ(ASCIIToUTF16("CA"), field.value);
This should remain "Canada".  "CA" isn't actually one of the elements in the
<select> dropdown.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_profile_unittest.cc:753:
profile.SetInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("CA"), "en-US");
nit: Might as well use SetRawInfo now.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/f...
File components/autofill/browser/field_types.h (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/f...
components/autofill/browser/field_types.h:65: ADDRESS_BILLING_COUNTRY = 43,  //
This is the 2-letter country code.
This annotation is not accurate.  We will now always *store* countries as their
2-letter country codes, but the field type covers spelled-out names as well.

I'd suggest moving this annotation to describe the field country_code_ in
address.cc.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:672: if
(UTF16ToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY)) == "US" &&
nit: It's more typical to convert the "US" constant using ASCIIToUTF16().

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:679: if
(UTF16ToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY)) == "US" &&
Ditto.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
File components/autofill/browser/phone_number.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/phone_number.cc:39: return
WideToASCII(country_code);
I think you want UTF16ToASCII here.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/w...
File components/autofill/browser/wallet/wallet_address.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/w...
components/autofill/browser/wallet/wallet_address.cc:85: :
country_name_code_(WideToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))),
I think you want UTF16ToASCII here... though actually, it might make more sense
to just change the type of country_name_code_ to be string16 and skip the string
conversion entirely.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
File components/webdata/autofill/autofill_table.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:94: // Old versions put the
localized country name in column 7.
On 2013/04/05 02:27:26, jam wrote:
> This is the first time I look at the webdata code. should this instead be a
> database migration to a new version?

We should be able to just keep this code as it was.  The migration code in
MigrateToVersion34ProfilesBasedOnCountryCode() should have already taken care of
ensuring that this field is populated correctly.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1771: "country_code VARCHAR, "
You shouldn't change this migration code.  The semantics of the migration code
are expected to be frozen.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1781: "zipcode, country, country,
phone, date_modified "
Ditto.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1842: "country_code VARCHAR, "
Ditto.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1871:
profile.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
Please call SetInfo() here instead.

[jam, 2013-04-05 06:45:53]

Thanks for the detailed review.

On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Thanks for taking this on, John!
> 
> Two more files that I think we'll need to update:
> autofill_dialog_controller_impl.cc [1] (2 callsites) and
autocheckout_manager.cc

Done. They were also helpfully caught by tests and the dcheck that only 2 letter
codes are set.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
chrome/browser/autofill/android/personal_data_manager_android.cc:54:
profile.GetRawInfo(ADDRESS_HOME_COUNTRY)).obj(),
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> I think this might need to be updated to be GetInfo().

I had traced through the java code to
http://developer.android.com/reference/android/provider/ContactsContract.Comm...
which says this could be name or country code.

but probably best not to test that assumption and keep as before, done

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/andro...
chrome/browser/autofill/android/personal_data_manager_android.cc:97:
profile->SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please use SetInfo() rather than SetRawInfo() for this.

Done.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_browsertest.cc (left):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_browsertest.cc:1020:
profile1.SetInfo(ADDRESS_HOME_COUNTRY, WideToUTF16(L"India"), "en-US");
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please preserve this; it should continue to work.

Done. i don't know what i was thinking when I changed that.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_browsertest.cc:1159:
without_invalid.SetInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("United States"),
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/autofi...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/autofi...
chrome/browser/webdata/autofill_profile_syncable_service.cc:323:
country_name_or_code, g_browser_process->GetApplicationLocale());
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Again, this happens on the DB thread (I think), so it's not safe to call into
> g_browser_process.

Done.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/autofi...
chrome/browser/webdata/autofill_profile_syncable_service.cc:323:
country_name_or_code, g_browser_process->GetApplicationLocale());
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Again, this happens on the DB thread (I think), so it's not safe to call into
> g_browser_process.

Done.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_database_migration_unittest.cc:64:
AutofillCountry::GetCountryCode(s.ColumnString16(12), "en-US")));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please use SetInfo() for this.

Done.

https://codereview.chromium.org/13697002/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_database_migration_unittest.cc:87:
AutofillCountry::GetCountryCode(s.ColumnString16(7), "en-US")));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please use SetInfo() for this.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address.cc:108:
ASCIIToUTF16(AutofillCountry::GetCountryCode(value, app_locale));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> We should return false from this case if the conversion fails.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address_unittest.cc (left):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address_unittest.cc:115: }
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Hmm, why did you remove this test coverage?

it seemed that that test coverate was for Address::GetMatchingTypes which I
deleted in this patch since that was used for when the country could be either
code or full name. now that ADDRESS_HOME_COUNTRY always returns countrycode, it
didn't seem like it's needed?

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address_unittest.cc:101:
address.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(""));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> nit: string16() rather than ASCIIToUTF16("")

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/android/auxiliary_profile_unittest_android.cc
(right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/android/auxiliary_profile_unittest_android.cc:112:
string16 country = ASCIIToUTF16("GB");
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please preserve the string "England" here -- this should continue to work.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/android/auxiliary_profiles_android.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/android/auxiliary_profiles_android.cc:95:
profile->SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please call SetInfo with the app_locale rather than doing the conversion
within
> this file.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_profile_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_profile_unittest.cc:735:
EXPECT_EQ(ASCIIToUTF16("CA"), field.value);
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> This should remain "Canada".  "CA" isn't actually one of the elements in the
> <select> dropdown.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_profile_unittest.cc:753:
profile.SetInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("CA"), "en-US");
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> nit: Might as well use SetRawInfo now.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/f...
File components/autofill/browser/field_types.h (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/f...
components/autofill/browser/field_types.h:65: ADDRESS_BILLING_COUNTRY = 43,  //
This is the 2-letter country code.
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> This annotation is not accurate.  We will now always *store* countries as
their
> 2-letter country codes, but the field type covers spelled-out names as well.

ah, you're right. removed.
> 
> I'd suggest moving this annotation to describe the field country_code_ in
> address.cc.

the variable there is pretty well-named, so i think a comment would be redundant

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:672: if
(UTF16ToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY)) == "US" &&
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> nit: It's more typical to convert the "US" constant using ASCIIToUTF16().

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:679: if
(UTF16ToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY)) == "US" &&
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
File components/autofill/browser/phone_number.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/p...
components/autofill/browser/phone_number.cc:39: return
WideToASCII(country_code);
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> I think you want UTF16ToASCII here.

yeah that was a mistake, fixed already

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/w...
File components/autofill/browser/wallet/wallet_address.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/w...
components/autofill/browser/wallet/wallet_address.cc:85: :
country_name_code_(WideToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))),
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> I think you want UTF16ToASCII here...

Done

> though actually, it might make more sense
> to just change the type of country_name_code_ to be string16 and skip the
string
> conversion entirely.

I've left that for the original authors to change if they want

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
File components/webdata/autofill/autofill_table.cc (right):

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:94: // Old versions put the
localized country name in column 7.
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> On 2013/04/05 02:27:26, jam wrote:
> > This is the first time I look at the webdata code. should this instead be a
> > database migration to a new version?
> 
> We should be able to just keep this code as it was.  The migration code in
> MigrateToVersion34ProfilesBasedOnCountryCode() should have already taken care
of
> ensuring that this field is populated correctly.

Done.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1771: "country_code VARCHAR, "
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> You shouldn't change this migration code.  The semantics of the migration code
> are expected to be frozen.

Done.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1781: "zipcode, country, country,
phone, date_modified "
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1842: "country_code VARCHAR, "
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/13697002/diff/1/components/webdata/autofill/a...
components/webdata/autofill/autofill_table.cc:1871:
profile.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16(country_code));
On 2013/04/05 05:18:01, Ilya Sherman wrote:
> Please call SetInfo() here instead.

Done.

[Ilya Sherman, 2013-04-05 07:18:41]

https://chromiumcodereview.appspot.com/13697002/diff/1/components/autofill/br...
File components/autofill/browser/address_unittest.cc (left):

https://chromiumcodereview.appspot.com/13697002/diff/1/components/autofill/br...
components/autofill/browser/address_unittest.cc:115: }
On 2013/04/05 06:45:54, jam wrote:
> On 2013/04/05 05:18:01, Ilya Sherman wrote:
> > Hmm, why did you remove this test coverage?
> 
> it seemed that that test coverate was for Address::GetMatchingTypes which I
> deleted in this patch since that was used for when the country could be either
> code or full name. now that ADDRESS_HOME_COUNTRY always returns countrycode,
it
> didn't seem like it's needed?

Ah, I missed that you'd removed that code.  I'm pretty sure that code should
remain -- see my other comment for the reason.

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/aut...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/aut...
chrome/browser/autofill/android/personal_data_manager_android.cc:14: #include
"components/autofill/browser/autofill_country.h"
nit: No longer needed.

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/aut...
chrome/browser/autofill/android/personal_data_manager_android.cc:99:
g_browser_process->GetApplicationLocale());
nit: Looks like this line should be indented four fewer spaces.

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:184: } else if
(it->first->type == ADDRESS_HOME_COUNTRY) {
I think you need to handle ADDRESS_BILLING_COUNTRY here as well.  Ditto for the
AutocheckoutManager code.  This is one of the few places in the Autofill
codebase that ever uses that field type.

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/web...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/web...
chrome/browser/webdata/autofill_profile_syncable_service.h:174: std::string
app_locale_;
nit: Can this be const?

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/chrome/browser/web...
chrome/browser/webdata/web_database_migration_unittest.cc:23: #include
"components/autofill/browser/autofill_country.h"
nit: Not needed?

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/autofil...
File components/autofill/browser/address.cc (left):

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/autofil...
components/autofill/browser/address.cc:106: }
Hmm, I missed that you removed this.  I'm pretty sure it should stay.  The
purpose of this method is to determine whether the raw text that a user typed
matches existing Autofill data, for the sake of uploading votes to the
crowdsourcing server.  For this purpose, all of "US", "USA", "United States",
and "United States of America" should be considered equivalent.  In contrast,
the code in FormGroup::GetMatchingTypes would only accept "United States".

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/autofil...
File components/autofill/browser/autocheckout_manager.cc (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/autofil...
components/autofill/browser/autocheckout_manager.cc:293: } else if
(result->field(i)->type() == ADDRESS_HOME_COUNTRY) {
I think you need to handle ADDRESS_BILLING_COUNTRY here as well.

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/webdata...
File components/webdata/autofill/autofill_table.cc (right):

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/webdata...
components/webdata/autofill/autofill_table.cc:20: #include
"base/utf_string_conversions.h"
nit: Not needed now?

https://chromiumcodereview.appspot.com/13697002/diff/47005/components/webdata...
components/webdata/autofill/autofill_table.cc:92: string16 country_code =
s.ColumnString16(8);
nit: Probably no need for this expression to have a named variable

[jam, 2013-04-05 07:35:34]

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
File components/autofill/browser/address_unittest.cc (left):

https://codereview.chromium.org/13697002/diff/1/components/autofill/browser/a...
components/autofill/browser/address_unittest.cc:115: }
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> On 2013/04/05 06:45:54, jam wrote:
> > On 2013/04/05 05:18:01, Ilya Sherman wrote:
> > > Hmm, why did you remove this test coverage?
> > 
> > it seemed that that test coverate was for Address::GetMatchingTypes which I
> > deleted in this patch since that was used for when the country could be
either
> > code or full name. now that ADDRESS_HOME_COUNTRY always returns countrycode,
> it
> > didn't seem like it's needed?
> 
> Ah, I missed that you'd removed that code.  I'm pretty sure that code should
> remain -- see my other comment for the reason.

Done.

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/autofill/a...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/autofill/a...
chrome/browser/autofill/android/personal_data_manager_android.cc:14: #include
"components/autofill/browser/autofill_country.h"
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: No longer needed.

Done.

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/autofill/a...
chrome/browser/autofill/android/personal_data_manager_android.cc:99:
g_browser_process->GetApplicationLocale());
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Looks like this line should be indented four fewer spaces.

Done.

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:184: } else if
(it->first->type == ADDRESS_HOME_COUNTRY) {
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> I think you need to handle ADDRESS_BILLING_COUNTRY here as well.  Ditto for
the
> AutocheckoutManager code.  This is one of the few places in the Autofill
> codebase that ever uses that field type.

Done.

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_profile_syncable_service.h:174: std::string
app_locale_;
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Can this be const?

yes it can, but i'm personally not a fan of consting member variables.i
understand the argument in both sides, and i lean on this one. since it's not in
styl eguide, and note autofilltable already has it this way.

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/13697002/diff/47005/chrome/browser/webdata/we...
chrome/browser/webdata/web_database_migration_unittest.cc:23: #include
"components/autofill/browser/autofill_country.h"
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/13697002/diff/47005/components/autofill/brows...
File components/autofill/browser/address.cc (left):

https://codereview.chromium.org/13697002/diff/47005/components/autofill/brows...
components/autofill/browser/address.cc:106: }
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> Hmm, I missed that you removed this.  I'm pretty sure it should stay.  The
> purpose of this method is to determine whether the raw text that a user typed
> matches existing Autofill data, for the sake of uploading votes to the
> crowdsourcing server.  For this purpose, all of "US", "USA", "United States",
> and "United States of America" should be considered equivalent.  In contrast,
> the code in FormGroup::GetMatchingTypes would only accept "United States".

Done. I didn't see that this code was used for user input. that also fixes
AutofillMetricsTest.QualityMetrics* failures :)

https://codereview.chromium.org/13697002/diff/47005/components/autofill/brows...
File components/autofill/browser/autocheckout_manager.cc (right):

https://codereview.chromium.org/13697002/diff/47005/components/autofill/brows...
components/autofill/browser/autocheckout_manager.cc:293: } else if
(result->field(i)->type() == ADDRESS_HOME_COUNTRY) {
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> I think you need to handle ADDRESS_BILLING_COUNTRY here as well.

Done.

https://codereview.chromium.org/13697002/diff/47005/components/webdata/autofi...
File components/webdata/autofill/autofill_table.cc (right):

https://codereview.chromium.org/13697002/diff/47005/components/webdata/autofi...
components/webdata/autofill/autofill_table.cc:20: #include
"base/utf_string_conversions.h"
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Not needed now?

Done.

https://codereview.chromium.org/13697002/diff/47005/components/webdata/autofi...
components/webdata/autofill/autofill_table.cc:20: #include
"base/utf_string_conversions.h"
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Not needed now?

Done.

https://codereview.chromium.org/13697002/diff/47005/components/webdata/autofi...
components/webdata/autofill/autofill_table.cc:92: string16 country_code =
s.ColumnString16(8);
On 2013/04/05 07:18:41, Ilya Sherman wrote:
> nit: Probably no need for this expression to have a named variable

Done.

[Ilya Sherman, 2013-04-05 09:02:11]

LGTM

https://chromiumcodereview.appspot.com/13697002/diff/50001/components/autofil...
File components/autofill/browser/address.h (right):

https://chromiumcodereview.appspot.com/13697002/diff/50001/components/autofil...
components/autofill/browser/address.h:33: const std::string& app_locale)
OVERRIDE;
nit: Trailing whitespace

[Ilya Sherman, 2013-04-05 09:02:12]

LGTM