Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling. r?jfkthame draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Fri, 04 Aug 2017 12:41:32 -0700
changeset 642175 cc9152d2d18a02290b546972b0216d6df9efb06c
parent 642173 fde1450a4368d04e97174e2eb00fb48901179857
child 724925 6a589bc72f15e726adbe81f998c7e3b06d1b059b
push id72670
push userbmo:gandalf@aviary.pl
push dateMon, 07 Aug 2017 20:04:59 +0000
reviewersjfkthame
bugs1345957
milestone57.0a1
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling. r?jfkthame MozReview-Commit-ID: Bb2oRS7GHKV
intl/locale/LocaleService.cpp
intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
intl/locale/tests/unit/test_localeService.js
intl/locale/tests/unit/test_localeService_negotiateLanguages.js
--- a/intl/locale/LocaleService.cpp
+++ b/intl/locale/LocaleService.cpp
@@ -399,16 +399,17 @@ LocaleService::FilterMatches(const nsTAr
   // the target was the last in the array).
   auto eraseFromAvail = [&](nsTArray<Locale>::iterator aIter) {
     nsTArray<Locale>::size_type index = aIter - availLocales.begin();
     availLocales.RemoveElementAt(index);
     return availLocales.begin() + index;
   };
 
   for (auto& requested : aRequested) {
+    MOZ_ASSERT(!requested.IsEmpty(), "Locale string cannot be empty.");
 
     // 1) Try to find a simple (case-insensitive) string match for the request.
     auto matchesExactly = [&](const Locale& aLoc) {
       return requested.Equals(aLoc.AsString(),
                               nsCaseInsensitiveCStringComparator());
     };
     auto match = std::find_if(availLocales.begin(), availLocales.end(),
                               matchesExactly);
@@ -468,18 +469,18 @@ LocaleService::FilterMatches(const nsTAr
 
 bool
 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.
   if (aStrategy == LangNegStrategy::Lookup && aDefaultLocale.IsEmpty()) {
+    NS_ERROR("Default locale cannot be empty for strategy `lookup`.");
     return false;
   }
 
   FilterMatches(aRequested, aAvailable, aStrategy, aRetVal);
 
   if (aStrategy == LangNegStrategy::Lookup) {
     if (aRetVal.Length() == 0) {
       // If the strategy is Lookup and Filtering returned no matches, use
@@ -720,21 +721,24 @@ LocaleService::NegotiateLanguages(const 
   }
 
   return NS_OK;
 }
 
 LocaleService::Locale::Locale(const nsCString& aLocale, bool aRange)
   : mLocaleStr(aLocale)
 {
+  MOZ_ASSERT(!aLocale.IsEmpty(), "Locale string cannot be empty");
+
   int32_t partNum = 0;
 
   nsAutoCString normLocale(aLocale);
   normLocale.ReplaceChar('_', '-');
 
+  // We follow a subset of RFC4646 plus ranges
   for (const nsACString& part : normLocale.Split('-')) {
     switch (partNum) {
       case 0:
         if (part.EqualsLiteral("*") ||
             part.Length() == 2 || part.Length() == 3) {
           mLanguage.Assign(part);
         }
         break;
@@ -746,19 +750,24 @@ LocaleService::Locale::Locale(const nsCS
 
         // fallover to region case
         partNum++;
         MOZ_FALLTHROUGH;
       case 2:
         if (part.EqualsLiteral("*") || part.Length() == 2) {
           mRegion.Assign(part);
         }
-        break;
+
+        // fallover to region case
+        partNum++;
+        MOZ_FALLTHROUGH;
       case 3:
-        if (part.EqualsLiteral("*") || part.Length() == 3) {
+        // special case for old Mozilla convention "ja-JP-mac"
+        if (part.EqualsLiteral("*") || part.EqualsLiteral("mac") ||
+            part.Length() >= 5) {
           mVariant.Assign(part);
         }
         break;
     }
     partNum++;
   }
 
   if (aRange) {
--- a/intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
+++ b/intl/locale/tests/gtest/TestLocaleServiceNegotiate.cpp
@@ -17,16 +17,17 @@ TEST(Intl_Locale_LocaleService, Negotiat
   nsAutoCString defaultLocale("en-US");
   LocaleService::LangNegStrategy strategy =
     LocaleService::LangNegStrategy::Filtering;
 
   requestedLocales.AppendElement(NS_LITERAL_CSTRING("sr"));
 
   availableLocales.AppendElement(NS_LITERAL_CSTRING("sr-Cyrl"));
   availableLocales.AppendElement(NS_LITERAL_CSTRING("sr-Latn"));
+  availableLocales.AppendElement(NS_LITERAL_CSTRING("en-US"));
 
   LocaleService::GetInstance()->NegotiateLanguages(
       requestedLocales, availableLocales, defaultLocale, strategy, supportedLocales);
 
   ASSERT_TRUE(supportedLocales.Length() == 2);
   ASSERT_TRUE(supportedLocales[0].Equals("sr-Cyrl"));
   ASSERT_TRUE(supportedLocales[1].Equals("en-US"));
 }
--- a/intl/locale/tests/unit/test_localeService.js
+++ b/intl/locale/tests/unit/test_localeService.js
@@ -119,20 +119,20 @@ add_test(function test_getRequestedLocal
 
 add_test(function test_getRequestedLocale() {
   Services.prefs.setBoolPref(PREF_MATCH_OS_LOCALE, false);
   Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "tlh");
 
   let requestedLocale = localeService.getRequestedLocale();
   do_check_true(requestedLocale === "tlh", "requestedLocale returns the right value");
 
-  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "");
+  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "sr-Cyrl");
 
   requestedLocale = localeService.getRequestedLocale();
-  do_check_true(requestedLocale === "", "requestedLocale returns empty value value");
+  do_check_true(requestedLocale === "sr-Cyrl", "requestedLocale returns empty value value");
 
   Services.prefs.clearUserPref(PREF_MATCH_OS_LOCALE);
   Services.prefs.clearUserPref(PREF_SELECTED_LOCALE);
 
   run_next_test();
 });
 
 add_test(function test_setRequestedLocales() {
--- a/intl/locale/tests/unit/test_localeService_negotiateLanguages.js
+++ b/intl/locale/tests/unit/test_localeService_negotiateLanguages.js
@@ -44,41 +44,41 @@ const data = {
       [["en"], ["en-US"], ["en-US"]],
       [["en-US"], ["en-GB"], ["en-GB"]],
       [["en-Latn-US"], ["en-Latn-GB"], ["en-Latn-GB"]],
       // This is a cross-region check, because the requested Locale
       // is really lang: en, script: *, region: undefined
       [["en-*"], ["en-US"], ["en-US"]],
     ],
     "should match cross-variant": [
-      [["en-US-mac"], ["en-US-win"], ["en-US-win"]],
+      [["en-US-mac"], ["en-US-windows"], ["en-US-windows"]],
+      [["ja-windows"], ["ja-mac"], ["ja-mac"]],
     ],
     "should prioritize properly": [
       // exact match first
-      [["en-US"], ["en-US-mac", "en", "en-US"], ["en-US", "en", "en-US-mac"]],
+      [["en-US"], ["en-US-mac", "en", "en-US"], ["en-US", "en-US-mac", "en"]],
       // available as range second
       [["en-Latn-US"], ["en-GB", "en-US"], ["en-US", "en-GB"]],
       // likely subtags third
       [["en"], ["en-Cyrl-US", "en-Latn-US"], ["en-Latn-US"]],
       // variant range fourth
-      [["en-US-mac"], ["en-US-win", "en-GB-mac"], ["en-US-win", "en-GB-mac"]],
+      [["en-US-mac"], ["en-US-windows", "en-GB-mac"], ["en-US-windows", "en-GB-mac"]],
       // regional range fifth
-      [["en-US-mac"], ["en-GB-win"], ["en-GB-win"]],
+      [["en-US-mac"], ["en-GB-windows"], ["en-GB-windows"]],
     ],
     "should prioritize properly (extra tests)": [
       [["en-US"], ["en-GB", "en"], ["en", "en-GB"]],
     ],
     "should handle default locale properly": [
       [["fr"], ["de", "it"], []],
-      [["fr"], ["de", "it"], "en-US", ["en-US"]],
-      [["fr"], ["de", "en-US"], "en-US", ["en-US"]],
-      [["fr", "de-DE"], ["de-DE", "fr-CA"], "en-US", ["fr-CA", "de-DE", "en-US"]],
+      [["fr"], ["de", "it", "en-US"], "en-US", ["en-US"]],
+      [["fr", "de-DE"], ["de-DE", "fr-CA", "en-US"], "en-US", ["fr-CA", "de-DE", "en-US"]],
     ],
     "should handle all matches on the 1st higher than any on the 2nd": [
-      [["fr-CA-mac", "de-DE"], ["de-DE", "fr-FR-win"], ["fr-FR-win", "de-DE"]],
+      [["fr-CA-mac", "de-DE"], ["de-DE", "fr-FR-windows"], ["fr-FR-windows", "de-DE"]],
     ],
     "should handle cases and underscores": [
       [["fr_FR"], ["fr-FR"], ["fr-FR"]],
       [["fr_fr"], ["fr-fr"], ["fr-fr"]],
       [["fr_Fr"], ["fr-fR"], ["fr-fR"]],
       [["fr_lAtN_fr"], ["fr-Latn-FR"], ["fr-Latn-FR"]],
       [["fr_FR"], ["fr_FR"], ["fr_FR"]],
       [["fr-FR"], ["fr_FR"], ["fr_FR"]],