Fix the settings page for es-419 locale: Spanish (Latin America). (issue6626070)

[sat...@chromium.org]

Reviewers: Ilya Sherman, James Hawkins,

Description:
Fix the settings page for es-419 locale: Spanish (Latin America).

Before the fix, the settings page for es-419 was half broken as there
was an JavaScript error like below while loading the page.

Uncaught TypeError: Cannot read property 'name' of undefined

The error was caused by the fact that "419" isn't a valid country code.

BUG=chromium-os:12857
TEST=open the "Languages and Input" settings on Chromium OS, and confirmed
that
it's shown correctly.

Please review this at http://codereview.chromium.org/6626070/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/autofill/autofill_country.cc
M chrome/browser/autofill/autofill_country_unittest.cc

Index: chrome/browser/autofill/autofill_country.cc
diff --git a/chrome/browser/autofill/autofill_country.cc
b/chrome/browser/autofill/autofill_country.cc
index
7cc8a45b6a949e03be8aed391c997a63bcc7225b..44982e32130cedb89d7953d4e7461f24e281a421
100644
--- a/chrome/browser/autofill/autofill_country.cc
+++ b/chrome/browser/autofill/autofill_country.cc
@@ -386,8 +386,14 @@ const std::string
AutofillCountry::CountryCodeForLocale(
// Extract the country code.
std::string country_code =
icu::Locale(likely_locale.c_str()).getCountry();

- // Default to the United States if we have no better guess.
- return !country_code.empty() ? country_code : "US";
+ // Default to the United States if we have no better guess. Consider the
+ // country code invalid if it is empty or contains any character other
+ // than A-Z.
+ if (country_code.empty() ||
+ country_code.find_first_not_of("ABCDEFGHIJKLMNOPQRSTUVWXYZ") !=
+ std::string::npos)
+ return "US";
+ return country_code;
}

// static
Index: chrome/browser/autofill/autofill_country_unittest.cc
diff --git a/chrome/browser/autofill/autofill_country_unittest.cc
b/chrome/browser/autofill/autofill_country_unittest.cc
index
f0dd81480cf63a8c1f191653cb9187d019caf9c6..c052def48c9da10b29899edcc40362680859c68a
100644
--- a/chrome/browser/autofill/autofill_country_unittest.cc
+++ b/chrome/browser/autofill/autofill_country_unittest.cc
@@ -38,6 +38,9 @@ TEST(AutofillCountryTest, CountryCodeForLocale) {
EXPECT_EQ("CA", AutofillCountry::CountryCodeForLocale("fr_CA"));
EXPECT_EQ("FR", AutofillCountry::CountryCodeForLocale("fr"));
EXPECT_EQ("US", AutofillCountry::CountryCodeForLocale("Unknown"));
+ // "es-419" isn't associated with a country. See base/l10n/l10n_util.cc
+ // for details about this locale. Default to US.
+ EXPECT_EQ("US", AutofillCountry::CountryCodeForLocale("es-419"));
}

// Test mapping of localized country names to country codes.

[sat...@chromium.org]

I haven't tested but this issue probably reproduces on platforms other than
Chrome OS. This issue may be P1, thus the change may be worth merging to
M11...

http://codereview.chromium.org/6626070/

[ishe...@chromium.org]

Thanks for tracking this down! I agree that we should probably merge this
to
M11.

http://codereview.chromium.org/6626070/diff/1/chrome/browser/autofill/autofill_country.cc
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6626070/diff/1/chrome/browser/autofill/autofill_country.cc#newcode394
chrome/browser/autofill/autofill_country.cc:394: std::string::npos)
Probably the right check here is actually to see if the country code is
in the map of known countries:

if (!AutofillCountries::countries().count(country_code))
return "US";

http://codereview.chromium.org/6626070/

[sat...@chromium.org]

http://codereview.chromium.org/6626070/diff/1/chrome/browser/autofill/autofill_country.cc
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6626070/diff/1/chrome/browser/autofill/autofill_country.cc#newcode394
chrome/browser/autofill/autofill_country.cc:394: std::string::npos)

On 2011/03/08 10:35:18, Ilya Sherman wrote:
> Probably the right check here is actually to see if the country code
is in the
> map of known countries:

> if (!AutofillCountries::countries().count(country_code))
> return "US";

Done.

http://codereview.chromium.org/6626070/

[ishe...@chromium.org]

LGTM, thanks!

http://codereview.chromium.org/6626070/