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

components/translate/core/browser/translate_ui_delegate.cc

 // Copyright 2014 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.
 
 #include "components/translate/core/browser/translate_ui_delegate.h"
 
 #include "base/i18n/string_compare.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/translate/core/browser/language_state.h"
 #include "components/translate/core/browser/translate_client.h"
@@ skipping 11 matching lines @@
 const char kRevertTranslation[] = "Translate.RevertTranslation";
 const char kPerformTranslate[] = "Translate.Translate";
 const char kNeverTranslateLang[] = "Translate.NeverTranslateLang";
 const char kNeverTranslateSite[] = "Translate.NeverTranslateSite";
 const char kAlwaysTranslateLang[] = "Translate.AlwaysTranslateLang";
 const char kModifyOriginalLang[] = "Translate.ModifyOriginalLang";
 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 {
 
 TranslateUIDelegate::TranslateUIDelegate(
     const base::WeakPtr<TranslateManager>& translate_manager,
     const std::string& original_language,
     const std::string& target_language)
     : translate_driver_(
           translate_manager->translate_client()->GetTranslateDriver()),
       translate_manager_(translate_manager),
       original_language_index_(kNoIndex),
       initial_original_language_index_(kNoIndex),
       target_language_index_(kNoIndex) {
   DCHECK(translate_driver_);
   DCHECK(translate_manager_);
 
   std::vector<std::string> language_codes;
   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();
        iter != language_codes.end();
        ++iter) {
     std::string language_code = *iter;
 
     base::string16 language_name =
         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));
   }
   for (std::vector<LanguageNamePair>::const_iterator iter = languages_.begin();
        iter != languages_.end();
        ++iter) {
     std::string language_code = iter->first;
     if (language_code == original_language) {
       original_language_index_ = iter - languages_.begin();
@@ skipping 171 matching lines @@
   UMA_HISTOGRAM_BOOLEAN(kAlwaysTranslateLang, true);
 }
 
 std::string TranslateUIDelegate::GetPageHost() {
   if (!translate_driver_->HasCurrentPage())
     return std::string();
   return translate_driver_->GetLastCommittedURL().HostNoBrackets();
 }
 
 }  // namespace translate