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 36 matching lines @@
   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);
+  icu::Collator* collator_instance = icu::Collator::createInstance(loc, error);

[Miguel Garcia, 2015/11/02 12:09:26]

this code is starting to be far from trivial and it happens in a constructor
which is not great. Any chance we can move it to a static (or at least non
virtual) method and write a test for it?

[hajimehoshi, 2015/11/04 05:50:30]

createInstance's behavior depends on platforms, so it seems hard to write a
test. I left TODO comment. Done.

+  scoped_ptr<icu::Collator> collator;
+  if (U_SUCCESS(error)) {
+    DCHECK(collator_instance);
+    collator = scoped_ptr<icu::Collator>(collator_instance);
+    collator->setStrength(icu::Collator::PRIMARY);
+  }

[Miguel Garcia, 2015/11/02 12:09:26]

It seems you can get a collator object and still have an error code other than
U_SUCCESS which will break the logic in line 76 and below.

[hajimehoshi, 2015/11/04 05:50:30]

Thanks. Changed to check if |collator_instance| is null or not instead. Done.

 
   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) {
+      if (collator) {

[Miguel Garcia, 2015/11/02 12:09:26]

can you perhaps check if we should do ascii or icu sorting before the loop sort
appropriately in the right method instead of using continue?

[hajimehoshi, 2015/11/04 05:50:30]

Done.

+        int result = base::i18n::CompareString16WithCollator(*collator,
+                                                             language_name,
+                                                             iter2->second);
+        if (result == UCOL_LESS)
+          break;
+        continue;
+      }
+      // |locale| may not be supported by ICU collator (crbug/54833). In this
+      // case, let's order the languages in ASCII.
+      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();
       initial_original_language_index_ = original_language_index_;
@@ skipping 170 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