Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling. r?jfkthame draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Tue, 06 Mar 2018 18:54:57 -0800
changeset 764084 c2e50fb251c9bf2bd4adc016a6795304938fa5ef
parent 763853 efe1f380d0279d7786b0c6340a60aaf6c76eabbe
child 764085 7fa2466b11d0f2bc06ffe67e749f8598d3c38242
push id101659
push userbmo:gandalf@aviary.pl
push dateWed, 07 Mar 2018 05:11:28 +0000
reviewersjfkthame
bugs1345957
milestone60.0a1
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling. r?jfkthame MozReview-Commit-ID: 9ZxHI0RpYpi
intl/locale/LocaleService.cpp
intl/locale/LocaleService.h
intl/locale/MozLocale.cpp
intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
--- a/intl/locale/LocaleService.cpp
+++ b/intl/locale/LocaleService.cpp
@@ -427,16 +427,17 @@ LocaleService::FilterMatches(const nsTAr
   // no available locale will be found more than once.
   AutoTArray<Locale, 100> availLocales;
   for (auto& avail : aAvailable) {
     availLocales.AppendElement(Locale(avail));
   }
 
   for (auto& requested : aRequested) {
     if (requested.IsEmpty()) {
+      MOZ_ASSERT(!requested.IsEmpty(), "Locale string cannot be empty.");
       continue;
     }
 
     // 1) Try to find a simple (case-insensitive) string match for the request.
     auto matchesExactly = [&](Locale& aLoc) {
       return requested.Equals(aLoc.AsString(),
                               nsCaseInsensitiveCStringComparator());
     };
@@ -499,42 +500,51 @@ LocaleService::FilterMatches(const nsTAr
     // 6) Try to match against a region as a range
     requestedLocale.ClearRegion();
     if (findRangeMatches(requestedLocale, true, true)) {
       HANDLE_STRATEGY;
     }
   }
 }
 
-bool
+void
 LocaleService::NegotiateLanguages(const nsTArray<nsCString>& aRequested,
                                   const nsTArray<nsCString>& aAvailable,
                                   const nsACString& aDefaultLocale,
                                   LangNegStrategy aStrategy,
                                   nsTArray<nsCString>& aRetVal)
 {
-  // If the strategy is Lookup, we require the defaultLocale to be set.
+  MOZ_ASSERT(aDefaultLocale.IsEmpty() || Locale(aDefaultLocale).IsValid(),
+    "If specified, default locale must be a valid BCP47 language tag.");
+
   if (aStrategy == LangNegStrategy::Lookup && aDefaultLocale.IsEmpty()) {
-    return false;
+    NS_WARNING("Default locale should be specified when using lookup strategy.");
   }
 
   FilterMatches(aRequested, aAvailable, aStrategy, aRetVal);
 
   if (aStrategy == LangNegStrategy::Lookup) {
+    // If the strategy is Lookup and Filtering returned no matches, use
+    // the default locale.
     if (aRetVal.Length() == 0) {
-      // If the strategy is Lookup and Filtering returned no matches, use
-      // the default locale.
-      aRetVal.AppendElement(aDefaultLocale);
+      // If the default locale is empty, we already issued a warning, so
+      // now we will just pick up the LocaleService's defaultLocale.
+      if (aDefaultLocale.IsEmpty()) {
+        nsAutoCString defaultLocale;
+        GetDefaultLocale(defaultLocale);
+        aRetVal.AppendElement(defaultLocale);
+      } else {
+        aRetVal.AppendElement(aDefaultLocale);
+      }
     }
   } else if (!aDefaultLocale.IsEmpty() && !aRetVal.Contains(aDefaultLocale)) {
     // If it's not a Lookup strategy, add the default locale only if it's
     // set and it's not in the results already.
     aRetVal.AppendElement(aDefaultLocale);
   }
-  return true;
 }
 
 bool
 LocaleService::IsAppLocaleRTL()
 {
   nsAutoCString locale;
   GetAppLocaleAsBCP47(locale);
 
@@ -830,22 +840,18 @@ LocaleService::NegotiateLanguages(const 
     availableLocales.AppendElement(aAvailable[i]);
   }
 
   nsAutoCString defaultLocale(aDefaultLocale);
 
   LangNegStrategy strategy = ToLangNegStrategy(aStrategy);
 
   AutoTArray<nsCString, 100> supportedLocales;
-  bool result = NegotiateLanguages(requestedLocales, availableLocales,
-                                   defaultLocale, strategy, supportedLocales);
-
-  if (!result) {
-    return NS_ERROR_INVALID_ARG;
-  }
+  NegotiateLanguages(requestedLocales, availableLocales,
+                     defaultLocale, strategy, supportedLocales);
 
   *aRetVal =
     static_cast<char**>(moz_xmalloc(sizeof(char*) * supportedLocales.Length()));
 
   *aCount = 0;
   for (const auto& supported : supportedLocales) {
     (*aRetVal)[(*aCount)++] = moz_xstrdup(supported.get());
   }
--- a/intl/locale/LocaleService.h
+++ b/intl/locale/LocaleService.h
@@ -238,17 +238,17 @@ public:
    *
    * Strategy is one of the three strategies described at the top of this file.
    *
    * The result list is canonicalized and ordered according to the order
    * of the requested locales.
    *
    * (See mozILocaleService.idl for a JS-callable version of this.)
    */
-  bool NegotiateLanguages(const nsTArray<nsCString>& aRequested,
+  void NegotiateLanguages(const nsTArray<nsCString>& aRequested,
                           const nsTArray<nsCString>& aAvailable,
                           const nsACString& aDefaultLocale,
                           LangNegStrategy aLangNegStrategy,
                           nsTArray<nsCString>& aRetVal);
 
   /**
    * Returns whether the current app locale is RTL.
    */
--- a/intl/locale/MozLocale.cpp
+++ b/intl/locale/MozLocale.cpp
@@ -13,16 +13,18 @@
 using namespace mozilla::intl;
 
 /**
  * Note: The file name is `MozLocale` to avoid compilation problems on case-insensitive
  * Windows. The class name is `Locale`.
  */
 Locale::Locale(const nsACString& aLocale)
 {
+  MOZ_ASSERT(!aLocale.IsEmpty(), "Locale string cannot be empty");
+
   int32_t position = 0;
 
   if (!IsASCII(aLocale)) {
     mIsValid = false;
     return;
   }
 
   nsAutoCString normLocale(aLocale);
--- a/intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
+++ b/intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
@@ -25,8 +25,29 @@ TEST(Intl_Locale_LocaleService, Negotiat
 
   LocaleService::GetInstance()->NegotiateLanguages(
       requestedLocales, availableLocales, defaultLocale, strategy, supportedLocales);
 
   ASSERT_TRUE(supportedLocales.Length() == 2);
   ASSERT_TRUE(supportedLocales[0].EqualsLiteral("sr-Cyrl"));
   ASSERT_TRUE(supportedLocales[1].EqualsLiteral("en-US"));
 }
+
+TEST(Intl_Locale_LocaleService, UseLSDefaultLocale) {
+  nsTArray<nsCString> requestedLocales;
+  nsTArray<nsCString> availableLocales;
+  nsTArray<nsCString> supportedLocales;
+  nsAutoCString defaultLocale("");
+  LocaleService::LangNegStrategy strategy =
+    LocaleService::LangNegStrategy::Lookup;
+
+  requestedLocales.AppendElement(NS_LITERAL_CSTRING("sr"));
+
+  availableLocales.AppendElement(NS_LITERAL_CSTRING("de"));
+
+  LocaleService::GetInstance()->NegotiateLanguages(
+      requestedLocales, availableLocales, defaultLocale, strategy, supportedLocales);
+
+  nsAutoCString lsDefaultLocale;
+  LocaleService::GetInstance()->GetDefaultLocale(lsDefaultLocale);
+  ASSERT_TRUE(supportedLocales.Length() == 1);
+  ASSERT_TRUE(supportedLocales[0].Equals(lsDefaultLocale));
+}