CountryNames: Separate data creation from usage

components/autofill/core/browser/country_names.cc

Index: components/autofill/core/browser/country_names.cc
diff --git a/components/autofill/core/browser/country_names.cc b/components/autofill/core/browser/country_names.cc
index ce94dbb1591661fa3b576c383c47c73ca5b5d26d..d2370803db5089e93cadc1012768c82bc72d7ac6 100644
--- a/components/autofill/core/browser/country_names.cc
+++ b/components/autofill/core/browser/country_names.cc
@@ -7,6 +7,7 @@
 #include <stdint.h>
 #include <utility>
 
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
@@ -21,6 +22,11 @@
 namespace autofill {
 namespace {
 
+// A copy of the application locale string, which should be ready for
+// CountryName's construction.
+static base::LazyInstance<std::string> g_application_locale =
+    LAZY_INSTANCE_INITIALIZER;
+
 // Returns the ICU sort key corresponding to |str| for the given |collator|.
 // Uses |buffer| as temporary storage, and might resize |buffer| as a side-
 // effect. |buffer_size| should specify the |buffer|'s size, and is updated if
@@ -74,19 +80,103 @@ std::map<std::string, std::string> GetCommonNames() {
   return common_names;
 }
 
+// Creates collator for |locale| and sets its attributes as needed.
+scoped_ptr<icu::Collator> CreateCollator(const icu::Locale& locale) {
+  scoped_ptr<icu::Collator> collator(
+      autofill::l10n::GetCollatorForLocale(locale));
+  if (!collator)
+    return nullptr;
+
+  // Compare case-insensitively and ignoring punctuation.
+  UErrorCode ignored = U_ZERO_ERROR;
+  collator->setAttribute(UCOL_STRENGTH, UCOL_SECONDARY, ignored);
+  ignored = U_ZERO_ERROR;
+  collator->setAttribute(UCOL_ALTERNATE_HANDLING, UCOL_SHIFTED, ignored);
+
+  return collator;
+}
+
+// If |locale| is different from "en_US", returns a collator for "en_US" and
+// sets its attributes as appropriate. Otherwise returns null.
+scoped_ptr<icu::Collator> CreateDefaultCollator(const icu::Locale& locale) {
+  icu::Locale default_locale("en_US");
+
+  if (default_locale != locale)
+    return CreateCollator(default_locale);
+
+  return nullptr;
+}
+
+// Returns the mapping of country names localized to |locale| to their
+// corresponding country codes. The provided |collator| should be suitable for
+// the locale. The collator being null is handled gracefully by returning an
+// empty string, to account for the very rare cases when the collator fails to

[Ilya Sherman, 2016/01/21 02:07:56]

nit: s/string/map

[vabr (Chromium), 2016/01/22 17:36:42]

Done.

+// initialize.
+std::map<std::string, std::string> GetLocalizedNames(
+    const std::string& locale,
+    const icu::Collator* collator) {
+  if (!collator)
+    return std::map<std::string, std::string>();
+
+  std::map<std::string, std::string> localized_names;
+  int32_t buffer_size = 1000;
+  scoped_ptr<uint8_t[]> buffer(new uint8_t[buffer_size]);
+
+  for (const std::string& country_code :
+       CountryDataMap::GetInstance()->country_codes()) {
+    base::string16 country_name =
+        l10n_util::GetDisplayNameForCountry(country_code, locale);
+    std::string sort_key =
+        GetSortKey(*collator, country_name, &buffer, &buffer_size);
+
+    localized_names.insert(std::make_pair(sort_key, country_code));
+  }
+  return localized_names;
+}
+
+// If |locale| represents a different locale than "en_US", returns
+// GetLocalizedNames("en_US", collator). Otherwise returns an empty map.
+std::map<std::string, std::string> GetDefaultLocalizedNames(
+    const std::string& locale,
+    const icu::Collator* collator) {
+  icu::Locale locale_object(locale.c_str());
+  icu::Locale default_locale("en_US");
+
+  if (default_locale != locale_object)

[Ilya Sherman, 2016/01/21 02:07:56]

If you pass the default_collotor_ into this method, then comparisons against
"en_US" are unnecessary.

[vabr (Chromium), 2016/01/22 17:36:41]

You are correct, thanks for catching this!
Removed the whole function.

+    return GetLocalizedNames("en_US", collator);
+
+  return std::map<std::string, std::string>();
+}
+
 }  // namespace
 
+void SetLocaleString(std::string locale) {
+  static bool initialized = false;
+  if (!initialized) {
+    initialized = true;
+    g_application_locale.Get() = std::move(locale);
+  }

[Ilya Sherman, 2016/01/21 02:07:56]

I think it would be good to DCHECK that the locale never changes during Chrome's
runtime.

[Ilya Sherman, 2016/01/21 02:07:56]

Tracking |initialized| here seems not necessary -- you could just check whether
the locale is set or not.

[vabr (Chromium), 2016/01/22 17:36:42]

I'm not sure how to do that. LazyInstance does not seem to have a way to tell
whether the instance has been already created (without creating it as a
side-effect).

So I assumed you meant treating the empty locale as a special value meaning
"initialized == false". This should work as long as nobody passes an empty
string to this method, which should not happen (e.g., on Linux, even if the
environment variable LANGUAGE is empty, the browser process reports the default
English locale). (Setting |g_application_locale| multiple times might result in
race conditions again, because at the same time |g_application_locale| is set
for the second time, CountryNames could be being constructed on another thread.)

[vabr (Chromium), 2016/01/22 17:36:42]

So apparently, Chrome might be expected to be able to change locale on some
platforms after all, looking at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

While there is the API (used by the linked test) to change the locale, it is
apparently not working on all platforms, and I could not find a way to trigger
this via UI on the platforms I checked (Linux, Win, Android).

Judging the trade-offs, for losing this flexibility:
 (+) we gain the fix for the race condition (which relies on computing the
locale-based data once and at start, thus avoiding races later),
 (-) we make autofill's heuristics slightly worse in the case the user manages
to switch locale while Chrome is running.

I see (-) dwarfing (+) because of the following:
 * The situation from (-) is unlikely to happen (I have no idea how it can be
triggered, if at all).
 * The impact of (-) is questionable -- it is perfectly normal for a user to
have, say, English locale, yet browse, e.g., Czech pages. In cases like this,
autofill is equally bad at understanding the names of the countries anyway.
 * http://crbug.com/579971 should restore this feature eventually

In the light of this, I dropped the check, and put a TODO inside instead. Does
that sound reasonable?

+}
+
 // static
 CountryNames* CountryNames::GetInstance() {
   return base::Singleton<CountryNames>::get();
 }
 
-CountryNames::CountryNames() : common_names_(GetCommonNames()) {}
+CountryNames::CountryNames(const std::string& locale_name)
+    : locale_(locale_name.c_str()),
+      collator_(CreateCollator(locale_)),
+      default_collator_(CreateDefaultCollator(locale_)),
+      common_names_(GetCommonNames()),
+      localized_names_(GetLocalizedNames(locale_name, collator_.get())),
+      default_localized_names_(
+          GetDefaultLocalizedNames(locale_name, collator_.get())) {}

[Ilya Sherman, 2016/01/21 02:07:56]

Why doesn't this use the default_collator_?

[vabr (Chromium), 2016/01/22 17:36:41]

I got myself confused with all the default stuff, but you helped me prune it. :)
Fixed now.

+
+CountryNames::CountryNames() : CountryNames(g_application_locale.Get()) {}

[Ilya Sherman, 2016/01/21 02:07:56]

Please DCHECK that g_application_locale is non-empty by this point.

[vabr (Chromium), 2016/01/22 17:36:42]

Done.

 
 CountryNames::~CountryNames() = default;
 
-const std::string CountryNames::GetCountryCode(const base::string16& country,
-                                               const std::string& locale_name) {
+const std::string CountryNames::GetCountryCode(const base::string16& country) {
   // First, check common country names, including 2- and 3-letter country codes.
   std::string country_utf8 = base::UTF16ToUTF8(base::ToUpperASCII(country));
   const auto result = common_names_.find(country_utf8);
@@ -94,66 +184,35 @@ const std::string CountryNames::GetCountryCode(const base::string16& country,
     return result->second;
 
   // Next, check country names localized to the current locale.
-  icu::Locale locale(locale_name.c_str());
-  std::string country_code = GetCountryCodeForLocalizedName(country, locale);
+  std::string country_code =
+      GetCountryCodeForLocalizedName(country, localized_names_, *collator_);
   if (!country_code.empty())
     return country_code;
 
   icu::Locale default_locale("en_US");
   // Finally, check country names localized to US English, unless done already.
-  if (default_locale != locale)
-    return GetCountryCodeForLocalizedName(country, default_locale);
-
-  return std::string();
-}
-
-void CountryNames::AddLocalizedNamesForLocale(const std::string& locale,
-                                              const icu::Collator& collator) {
-  // Nothing to do if we've previously added the localized names for the given
-  // |locale|.
-  if (locales_to_localized_names_.count(locale))
-    return;
-
-  std::map<std::string, std::string> localized_names;
-  int32_t buffer_size = 1000;
-  scoped_ptr<uint8_t[]> buffer(new uint8_t[buffer_size]);
-
-  for (const std::string& country_code :
-       CountryDataMap::GetInstance()->country_codes()) {
-    base::string16 country_name =
-        l10n_util::GetDisplayNameForCountry(country_code, locale);
-    std::string sort_key =
-        GetSortKey(collator, country_name, &buffer, &buffer_size);
-
-    localized_names.insert(std::make_pair(sort_key, country_code));
+  if (default_locale != locale_) {
+    DCHECK(default_collator_);

[Ilya Sherman, 2016/01/21 02:07:56]

nit: You could just write "if (default_collator_)" and drop the default_locale
entirely.

[vabr (Chromium), 2016/01/22 17:36:42]

Done.

+    return GetCountryCodeForLocalizedName(country, default_localized_names_,
+                                          *default_collator_);
   }
 
-  locales_to_localized_names_.insert(std::make_pair(locale, localized_names));
+  return std::string();
 }
 
 const std::string CountryNames::GetCountryCodeForLocalizedName(
     const base::string16& country_name,
-    const icu::Locale& locale) {
-  const icu::Collator* collator = GetCollatorForLocale(locale);
-  // In very rare cases, the collator fails to initialize.
-  if (!collator)
-    return std::string();
-
-  std::string locale_name = locale.getName();
-  AddLocalizedNamesForLocale(locale_name, *collator);
-
+    const std::map<std::string, std::string>& localized_names,
+    const icu::Collator& collator) {
   // As recommended[1] by ICU, initialize the buffer size to four times the
   // source string length.
   // [1] http://userguide.icu-project.org/collation/api#TOC-Examples
   int32_t buffer_size = country_name.size() * 4;
   scoped_ptr<uint8_t[]> buffer(new uint8_t[buffer_size]);
   std::string sort_key =
-      GetSortKey(*collator, country_name, &buffer, &buffer_size);
+      GetSortKey(collator, country_name, &buffer, &buffer_size);
 
-  const std::map<std::string, std::string>& localized_names =
-      locales_to_localized_names_[locale_name];
-  std::map<std::string, std::string>::const_iterator result =
-      localized_names.find(sort_key);
+  auto result = localized_names.find(sort_key);
 
   if (result != localized_names.end())
     return result->second;
@@ -161,25 +220,4 @@ const std::string CountryNames::GetCountryCodeForLocalizedName(
   return std::string();
 }
 
-const icu::Collator* CountryNames::GetCollatorForLocale(
-    const icu::Locale& locale) {
-  std::string locale_name = locale.getName();
-  if (!ContainsKey(collators_, locale_name)) {
-    scoped_ptr<icu::Collator> collator(
-        autofill::l10n::GetCollatorForLocale(locale));
-    if (!collator)
-      return nullptr;
-
-    // Compare case-insensitively and ignoring punctuation.
-    UErrorCode ignored = U_ZERO_ERROR;
-    collator->setAttribute(UCOL_STRENGTH, UCOL_SECONDARY, ignored);
-    ignored = U_ZERO_ERROR;
-    collator->setAttribute(UCOL_ALTERNATE_HANDLING, UCOL_SHIFTED, ignored);
-
-    collators_[locale_name] = std::move(collator);
-  }
-
-  return collators_[locale_name].get();
-}
-
 }  // namespace autofill