CountryNames: Separate data creation from usage

components/autofill/core/browser/country_names.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_COUNTRY_NAMES_H_
 #define COMPONENTS_AUTOFILL_CORE_BROWSER_COUNTRY_NAMES_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "third_party/icu/source/common/unicode/locid.h"
 #include "third_party/icu/source/i18n/unicode/coll.h"
 
 namespace base {
 template <typename T>
 struct DefaultSingletonTraits;
 }
 
 namespace autofill {
 
+// Tells CountryNames, what is the application locale. Only the first supplied
+// value is used, further calls result in no changes.  Call this on the UI
+// thread, before first using CountryNames.
+void SetLocaleString(std::string locale);

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

nit: Pass by reference.

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

Please define this as a static method on CountryNames.

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

Good call!
Done.

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

This is by value on purpose. Looking in the SetLocaleString definition, you will
see that |locale| is moved. So if the argument to this method is a temporary,
there will be no copy. If it is not a temporary (including when it is a
refererence), the copy will happen on calling, but that's the same as if it
happened by assigining a const ref in the function body.

So in summary, this is in some cases as expensive, and in other cheaper than
using a const ref.

(This technique was one of the main takeaways of the C++11 course for me.)

[Ilya Sherman, 2016/01/23 01:41:04]

Boy, this seems like a super duper micro-optimization.  I'd recommend emailing
chromium-dev@ to ask whether this style is preferred -- it certainly surprised
me to see std::move() being used to move strings of ~5 characters in length. 
But maybe I'm just behind the times =)

[vabr (Chromium), 2016/01/25 10:36:44]

E-mailing chromium-dev seems like a good idea, I'll do that. I will still
proceed with landing the current code, because the crasher needs fixing, and
this is just a nit. If there is a conclusive feedback against pass by value +
move, I am happy to make a follow-up CL.

Back to the issue itself -- IMO this is not an optimization, it is a safe
default practice. It was not done before because there was no moving in C++, not
because it has any disadvantages. The point is that one can write this and not
think about how long the strings actually are.

[vabr (Chromium), 2016/01/26 09:13:11]

Follow-up filed in https://codereview.chromium.org/1635813003/.

+
 // A singleton class that encapsulates mappings from country names to their
 // corresponding country codes.
 class CountryNames {
  public:
   static CountryNames* GetInstance();
 
   // Returns the country code corresponding to |country|, which should be a
   // country code or country name localized to |locale_name|. This function
   // can be expensive so use judiciously.

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

Can this function still be expensive?  It's probably GetInstance() that is now
potentially expensive, right?

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

Correct, GetCountryCode is nowhere as expensive as the creation of CountryNames.
So I moved that note to GetInstance.

-  const std::string GetCountryCode(const base::string16& country,
-                                   const std::string& locale_name);
+  const std::string GetCountryCode(const base::string16& country);
+
+ protected:
+  // Create CountryNames for |locale_name|. Protected for testing.
+  explicit CountryNames(const std::string& locale_name);
+
+  // Protected for testing.
+  ~CountryNames();
 
  private:
+  // Create CountryNames for the default locale.
   CountryNames();
-  ~CountryNames();
+
   friend struct base::DefaultSingletonTraits<CountryNames>;
 
-  // Populates |locales_to_localized_names_| with the mapping of country names
-  // localized to |locale| to their corresponding country codes. Uses a
-  // |collator| which is suitable for the locale.
-  void AddLocalizedNamesForLocale(const std::string& locale,
-                                  const icu::Collator& collator);
-
-  // Interprets |country_name| as a full country name localized to the given
-  // |locale| and returns the corresponding country code stored in
-  // |locales_to_localized_names_|, or an empty string if there is none.
+  // Looks up |country_name| in |localized_names|, using |collator| andd

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

nit: s/andd/and

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

Done.

+  // returns the corresponding country code or an empty string if there is
+  // none.
   const std::string GetCountryCodeForLocalizedName(
       const base::string16& country_name,
-      const icu::Locale& locale);
+      const std::map<std::string, std::string>& localized_names,
+      const icu::Collator& collator);
 
   // Returns an ICU collator -- i.e. string comparator -- appropriate for the
   // given |locale|, or null if no collator is available.
   const icu::Collator* GetCollatorForLocale(const icu::Locale& locale);
 
+  // The locale object for the application locale string.
+  const icu::Locale locale_;
+
+  // Collator for the application locale.
+  const scoped_ptr<icu::Collator> collator_;
+
+  // Collator for the "en_US" locale, if different from the application
+  // locale, null otherwise.
+  const scoped_ptr<icu::Collator> default_collator_;
+
   // Maps from common country names, including 2- and 3-letter country codes,
   // to the corresponding 2-letter country codes. The keys are uppercase ASCII
   // strings.
   const std::map<std::string, std::string> common_names_;
 
-  // The outer map keys are ICU locale identifiers.
-  // The inner maps map from localized country names to their corresponding
-  // country codes. The inner map keys are ICU collation sort keys corresponding
-  // to the target localized country name.
-  std::map<std::string, std::map<std::string, std::string>>
-      locales_to_localized_names_;
+  // Maps from localized country names (in the application locale) to their
+  // corresponding country codes. The keys are ICU collation sort keys
+  // corresponding to the target localized country name.
+  const std::map<std::string, std::string> localized_names_;
 
-  // Maps ICU locale names to their corresponding collators.
-  std::map<std::string, scoped_ptr<icu::Collator>> collators_;
+  // The same as |localized_names_| but for the "en_US" locale. Empty if
+  // "en_US" is the application locale already.
+  const std::map<std::string, std::string> default_localized_names_;
 
   DISALLOW_COPY_AND_ASSIGN(CountryNames);
 };
 
 }  // namespace autofill
 
 #endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_COUNTRY_NAMES_H_