Bug 1960221 - Remove the aNeedsToCache parameter from nsLanguageAtomService::GetLanguageGroup, and simplify callers. r=emilio

We don't need this param; it should be safe to call the method from any thread
and just rely on its internal locking to protect the cache.

Differential Revision: https://phabricator.services.mozilla.com/D245357

gfx/thebes/gfxFcPlatformFontList.cpp

@@ -2149,8 +2149,7 @@ static void GetSystemFontList(nsTArray<nsString>& aListOfFonts,
   // add the lang to the pattern
   nsAutoCString fcLang;
   gfxFcPlatformFontList* pfl = gfxFcPlatformFontList::PlatformFontList();
-  pfl->GetSampleLangForGroup(aLangGroup, fcLang,
+  pfl->GetSampleLangForGroup(aLangGroup, fcLang);
-                             /*aForFontEnumerationThread*/ true);
   if (!fcLang.IsEmpty()) {
     FcPatternAddString(pat, FC_LANG, ToFcChar8Ptr(fcLang.get()));
   }
@@ -2771,8 +2770,7 @@ const MozLangGroupData MozLangGroups[] = {
 
 bool gfxFcPlatformFontList::TryLangForGroup(const nsACString& aOSLang,
                                             nsAtom* aLangGroup,
-                                            nsACString& aFcLang,
+                                            nsACString& aFcLang) {
-                                            bool aForFontEnumerationThread) {
   // Truncate at '.' or '@' from aOSLang, and convert '_' to '-'.
   // aOSLang is in the form "language[_territory][.codeset][@modifier]".
   // fontconfig takes languages in the form "language-territory".
@@ -2798,24 +2796,12 @@ bool gfxFcPlatformFontList::TryLangForGroup(const nsACString& aOSLang,
     ++pos;
   }
 
-  if (!aForFontEnumerationThread) {
+  nsAtom* atom = mLangService->LookupLanguage(aFcLang);
-    nsAtom* atom = mLangService->LookupLanguage(aFcLang);
+  return atom == aLangGroup;
-    return atom == aLangGroup;
-  }
-
-  // If we were called by the font enumeration thread, we can't use
-  // mLangService->LookupLanguage because it is not thread-safe.
-  // Use GetUncachedLanguageGroup to avoid unsafe access to the lang-group
-  // mapping cache hashtable.
-  nsAutoCString lowered(aFcLang);
-  ToLowerCase(lowered);
-  RefPtr<nsAtom> lang = NS_Atomize(lowered);
-  RefPtr<nsAtom> group = mLangService->GetUncachedLanguageGroup(lang);
-  return group.get() == aLangGroup;
 }
 
-void gfxFcPlatformFontList::GetSampleLangForGroup(
+void gfxFcPlatformFontList::GetSampleLangForGroup(nsAtom* aLanguage,
-    nsAtom* aLanguage, nsACString& aLangStr, bool aForFontEnumerationThread) {
+                                                  nsACString& aLangStr) {
   aLangStr.Truncate();
   if (!aLanguage) {
     return;
@@ -2850,8 +2836,7 @@ void gfxFcPlatformFontList::GetSampleLangForGroup(
     for (const char* pos = languages; true; ++pos) {
       if (*pos == '\0' || *pos == separator) {
         if (languages < pos &&
-            TryLangForGroup(Substring(languages, pos), aLanguage, aLangStr,
+            TryLangForGroup(Substring(languages, pos), aLanguage, aLangStr)) {
-                            aForFontEnumerationThread)) {
           return;
         }
 
@@ -2864,8 +2849,8 @@ void gfxFcPlatformFontList::GetSampleLangForGroup(
     }
   }
   const char* ctype = setlocale(LC_CTYPE, nullptr);
-  if (ctype && TryLangForGroup(nsDependentCString(ctype), aLanguage, aLangStr,
+  if (ctype &&
-                               aForFontEnumerationThread)) {
+      TryLangForGroup(nsDependentCString(ctype), aLanguage, aLangStr)) {
     return;
   }
 

gfx/thebes/gfxFcPlatformFontList.h

@@ -308,11 +308,7 @@ class gfxFcPlatformFontList final : public gfxPlatformFontList {
   }
 
   // map lang group ==> lang string
-  // When aForFontEnumerationThread is true, this method will avoid using
+  void GetSampleLangForGroup(nsAtom* aLanguage, nsACString& aLangStr);
-  // LanguageService::LookupLanguage, because it is not safe for off-main-
-  // thread use (except by stylo traversal, which does the necessary locking)
-  void GetSampleLangForGroup(nsAtom* aLanguage, nsACString& aLangStr,
-                             bool aForFontEnumerationThread = false);
 
  protected:
   virtual ~gfxFcPlatformFontList();
@@ -361,7 +357,7 @@ class gfxFcPlatformFontList final : public gfxPlatformFontList {
 
   // helper method for finding an appropriate lang string
   bool TryLangForGroup(const nsACString& aOSLang, nsAtom* aLangGroup,
-                       nsACString& aLang, bool aForFontEnumerationThread);
+                       nsACString& aLang);
 
 #ifdef MOZ_BUNDLED_FONTS
   void ActivateBundledFonts();

intl/locale/nsLanguageAtomService.cpp

@@ -153,15 +153,12 @@ nsAtom* nsLanguageAtomService::GetLocaleLanguage() {
   return mLocaleLanguage;
 }
 
-nsStaticAtom* nsLanguageAtomService::GetLanguageGroup(nsAtom* aLanguage,
+nsStaticAtom* nsLanguageAtomService::GetLanguageGroup(nsAtom* aLanguage) {
-                                                      bool* aNeedsToCache) {
+  {
-  if (aNeedsToCache) {
     AutoReadLock lock(mLock);
     if (nsStaticAtom* atom = mLangToGroup.Get(aLanguage)) {
       return atom;
     }
-    *aNeedsToCache = true;
-    return nullptr;
   }
 
   AutoWriteLock lock(mLock);

intl/locale/nsLanguageAtomService.h

@@ -36,26 +36,15 @@ class nsLanguageAtomService final {
   already_AddRefed<nsAtom> LookupCharSet(NotNull<const Encoding*> aCharSet);
   nsAtom* GetLocaleLanguage();
 
-  // Returns the language group that the specified language is a part of.
+  // Returns the language group that the specified language is a part of,
-  //
+  // using a cache to avoid repeatedly doing full lookups.
-  // aNeedsToCache is used for two things.  If null, it indicates that
+  nsStaticAtom* GetLanguageGroup(nsAtom* aLanguage);
-  // the nsLanguageAtomService is safe to cache the result of the
-  // language group lookup, either because we're on the main thread,
-  // or because we're on a style worker thread but the font lock has
-  // been acquired.  If non-null, it indicates that it's not safe to
-  // cache the result of the language group lookup (because we're on
-  // a style worker thread without the lock acquired).  In this case,
-  // GetLanguageGroup will store true in *aNeedsToCache true if we
-  // would have cached the result of a new lookup, and false if we
-  // were able to use an existing cached result.  Thus, callers that
-  // get a true *aNeedsToCache outparam value should make an effort
-  // to re-call GetLanguageGroup when it is safe to cache, to avoid
-  // recomputing the language group again later.
-  nsStaticAtom* GetLanguageGroup(nsAtom* aLanguage,
-                                 bool* aNeedsToCache = nullptr);
-  nsStaticAtom* GetUncachedLanguageGroup(nsAtom* aLanguage) const;
 
  private:
+  // The core implementation of lang-tag to language-group lookup. (Now used
+  // only internally by GetLanguageGroup.)
+  nsStaticAtom* GetUncachedLanguageGroup(nsAtom* aLanguage) const;
+
   static mozilla::StaticAutoPtr<nsLanguageAtomService> sLangAtomService;
 
   nsTHashMap<RefPtr<nsAtom>, nsStaticAtom*> mLangToGroup MOZ_GUARDED_BY(mLock);

layout/base/StaticPresData.cpp

@@ -199,30 +199,19 @@ void LangGroupFontPrefs::Initialize(nsStaticAtom* aLangGroupAtom) {
   }
 }
 
-nsStaticAtom* StaticPresData::GetLangGroup(nsAtom* aLanguage,
+nsStaticAtom* StaticPresData::GetLangGroup(nsAtom* aLanguage) const {
-                                           bool* aNeedsToCache) const {
+  nsStaticAtom* langGroupAtom = mLangService->GetLanguageGroup(aLanguage);
-  nsStaticAtom* langGroupAtom =
-      mLangService->GetLanguageGroup(aLanguage, aNeedsToCache);
   // Assume x-western is safe...
   return langGroupAtom ? langGroupAtom : nsGkAtoms::x_western;
 }
 
-nsStaticAtom* StaticPresData::GetUncachedLangGroup(nsAtom* aLanguage) const {
-  nsStaticAtom* langGroupAtom =
-      mLangService->GetUncachedLanguageGroup(aLanguage);
-  return langGroupAtom ? langGroupAtom : nsGkAtoms::x_western;
-}
-
 const LangGroupFontPrefs* StaticPresData::GetFontPrefsForLang(
     nsAtom* aLanguage, bool* aNeedsToCache) {
   // Get language group for aLanguage:
   MOZ_ASSERT(aLanguage);
   MOZ_ASSERT(mLangService);
 
-  nsStaticAtom* langGroupAtom = GetLangGroup(aLanguage, aNeedsToCache);
+  nsStaticAtom* langGroupAtom = GetLangGroup(aLanguage);
-  if (aNeedsToCache && *aNeedsToCache) {
-    return nullptr;
-  }
 
   if (!aNeedsToCache) {
     AssertIsMainThreadOrServoFontMetricsLocked();

layout/base/StaticPresData.h

@@ -113,28 +113,8 @@ class StaticPresData {
   /**
    * Given a language, get the language group name, which can
    * be used as an argument to LangGroupFontPrefs::Initialize()
-   *
-   * aNeedsToCache is used for two things.  If null, it indicates that
-   * the nsLanguageAtomService is safe to cache the result of the
-   * language group lookup, either because we're on the main thread,
-   * or because we're on a style worker thread but the font lock has
-   * been acquired.  If non-null, it indicates that it's not safe to
-   * cache the result of the language group lookup (because we're on
-   * a style worker thread without the lock acquired).  In this case,
-   * GetLanguageGroup will store true in *aNeedsToCache true if we
-   * would have cached the result of a new lookup, and false if we
-   * were able to use an existing cached result.  Thus, callers that
-   * get a true *aNeedsToCache outparam value should make an effort
-   * to re-call GetLanguageGroup when it is safe to cache, to avoid
-   * recomputing the language group again later.
    */
-  nsStaticAtom* GetLangGroup(nsAtom* aLanguage,
+  nsStaticAtom* GetLangGroup(nsAtom* aLanguage) const;
-                             bool* aNeedsToCache = nullptr) const;
-
-  /**
-   * Same as GetLangGroup, but will not cache the result
-   */
-  nsStaticAtom* GetUncachedLangGroup(nsAtom* aLanguage) const;
 
   /**
    * Fetch the user's font preferences for the given aLanguage's
@@ -150,7 +130,19 @@ class StaticPresData {
    * with an additional per-session cache that new callers can use if they don't
    * have a PresContext.
    *
-   * See comment on GetLangGroup for the usage of aNeedsToCache.
+   * aNeedsToCache is used for two things.  If null, it indicates that
+   * it is safe for the StaticPresData to cache the result of the
+   * prefs lookup, either because we're on the main thread,
+   * or because we're on a style worker thread but the font lock has
+   * been acquired.  If non-null, it indicates that it's not safe to
+   * cache the result of the prefs lookup (because we're on
+   * a style worker thread without the lock acquired).  In this case,
+   * GetFontPrefsForLang will store true in *aNeedsToCache true if we
+   * would have cached the result of a new lookup, and false if we
+   * were able to use an existing cached result.  Thus, callers that
+   * get a true *aNeedsToCache outparam value should make an effort
+   * to re-call GetFontPrefsForLang when it is safe to cache, to avoid
+   * recomputing the prefs again later.
    */
   const LangGroupFontPrefs* GetFontPrefsForLang(nsAtom* aLanguage,
                                                 bool* aNeedsToCache = nullptr);