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..2d416e6300340ec3eb910f3724392ae7fabc972c 100644
--- a/components/translate/core/browser/translate_ui_delegate.cc
+++ b/components/translate/core/browser/translate_ui_delegate.cc
@@ -54,8 +54,13 @@ TranslateUIDelegate::TranslateUIDelegate(
   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();
@@ -68,10 +73,18 @@ TranslateUIDelegate::TranslateUIDelegate(
     // 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) {

[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));
   }