Translate: Order language names in ASCII for locales ICU doesn't support

components/translate/core/browser/translate_ui_delegate.cc

Index: components/translate/core/browser/translate_ui_delegate.cc
diff --git a/components/translate/core/browser/translate_ui_delegate.cc b/components/translate/core/browser/translate_ui_delegate.cc
index fb33f7a755190a32d49f905fcfcafbfcac70ab9e..7c8bfb27ae50ec7c20fa909c99bc2c46dd5c6fa6 100644
--- a/components/translate/core/browser/translate_ui_delegate.cc
+++ b/components/translate/core/browser/translate_ui_delegate.cc
@@ -29,6 +29,18 @@ const char kModifyTargetLang[] = "Translate.ModifyTargetLang";
 const char kDeclineTranslateDismissUI[] = "Translate.DeclineTranslateDismissUI";
 const char kShowErrorUI[] = "Translate.ShowErrorUI";
 
+// TODO(hajimehoshi): Write a test for icu::Collator::createInstance.
+scoped_ptr<icu::Collator> CreateCollator(const std::string& locale) {

[Miguel Garcia, 2015/11/04 09:14:58]

document the return value

[hajimehoshi, 2015/11/04 09:48:48]

Done.

+  UErrorCode error = U_ZERO_ERROR;
+  icu::Locale loc(locale.c_str());
+  icu::Collator* collator_instance = icu::Collator::createInstance(loc, error);
+  if (!collator_instance)

[Miguel Garcia, 2015/11/04 09:14:58]

I think this should be if(!collator_instance || !U_SUCCESS(error)) so we account
for the case where you get a "degraded" collator

[hajimehoshi, 2015/11/04 09:48:48]

Ah, that's much better. Done.

+    return nullptr;
+  scoped_ptr<icu::Collator> collator(collator_instance);

[Miguel Garcia, 2015/11/04 09:14:58]

can you make this atomic? i.e. just do
scoped_ptr<icu::Collator> collator(icu::Collator::createInstance(loc, error)) in
line 36. 

[hajimehoshi, 2015/11/04 09:48:48]

Done.

+  collator->setStrength(icu::Collator::PRIMARY);
+  return collator.Pass();
+}
+
 }  // namespace
 
 namespace translate {
@@ -50,12 +62,9 @@ TranslateUIDelegate::TranslateUIDelegate(
   TranslateDownloadManager::GetSupportedLanguages(&language_codes);
 
   // Preparing for the alphabetical order in the locale.
-  UErrorCode error = U_ZERO_ERROR;
   std::string locale =
       TranslateDownloadManager::GetInstance()->application_locale();
-  icu::Locale loc(locale.c_str());
-  scoped_ptr<icu::Collator> collator(icu::Collator::createInstance(loc, error));
-  collator->setStrength(icu::Collator::PRIMARY);
+  scoped_ptr<icu::Collator> collator = CreateCollator(locale);
 
   languages_.reserve(language_codes.size());
   for (std::vector<std::string>::const_iterator iter = language_codes.begin();
@@ -67,10 +76,20 @@ TranslateUIDelegate::TranslateUIDelegate(
         l10n_util::GetDisplayNameForLocale(language_code, locale, true);
     // Insert the language in languages_ in alphabetical order.
     std::vector<LanguageNamePair>::iterator iter2;
-    for (iter2 = languages_.begin(); iter2 != languages_.end(); ++iter2) {
-      if (base::i18n::CompareString16WithCollator(*collator, language_name,
-                                                  iter2->second) == UCOL_LESS) {
-        break;
+    if (collator) {
+      for (iter2 = languages_.begin(); iter2 != languages_.end(); ++iter2) {
+        int result = base::i18n::CompareString16WithCollator(*collator,
+                                                             language_name,
+                                                             iter2->second);
+        if (result == UCOL_LESS)
+          break;
+      }
+    } else {
+      // |locale| may not be supported by ICU collator (crbug/54833). In this
+      // case, let's order the languages in ASCII.

[Miguel Garcia, 2015/11/04 09:14:58]

I guess this is actually ordering in UTF-8 which as a fallback is still fine but
let's update the comment.

+      for (iter2 = languages_.begin(); iter2 != languages_.end(); ++iter2) {
+        if (language_name.compare(iter2->second) < 0)
+          break;
       }
     }
     languages_.insert(iter2, LanguageNamePair(language_code, language_name));