Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling. r=jfkthame
authorZibi Braniecki <zbraniecki@mozilla.com>
Tue, 06 Mar 2018 18:54:57 -0800
changeset 464871 675d641249f12f878e363e5bcf3db99237621199
parent 464870 ace4ace29452c3e41d3e1df08a6ff1c1fa49c598
child 464872 8d24dfbdc98af6e64b66394c073109e169fd0b67
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjfkthame
bugs1345957
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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));
+}