Add locale to cache key for set of fallback fonts

The fallback cache was originally introduced in issue 603314 to avoid
extra IPC overhead when finding fallback fonts for an identical base
family. It needs to be investigated in how much this cache actually
helps, but for now, avoid CJK fallback cache priming inconsistencies by
adding the locale to the cache key.

When an ideograph character was requested with a Chinese locale before
requesting it with a Japanese locale, the Japanese request would
retrieve a Chinese font and vice versa. Add the locale to the key to
avoid ignoring the locale, which is very relevant for CJK fallback.

I was not able to add a test with reasonable effort: as the current code
tries the user preferences fonts first for CJK characters and does not
necessarily reach fallback. On the other hand, testing this is in a more
localized unittest is not possible at it requires a Mojo IPC connection
to the browser process and DWriteFontProxy to be running on the other
side. In the existing FontFallback unittests, a DWriteFakeSender is
initialized which simulates a font collection with only one font and
does not allow access to system fonts.

Bug: 992936
Change-Id: I4635bf2d3f7d598e89357dbd3269510667e2f961
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1751246
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686405}

content/child/dwrite_font_proxy/font_fallback_win.cc

@@ -38,6 +38,12 @@ void LogFallbackResult(DirectWriteFontFallbackResult fallback_result) {
                             fallback_result, FONT_FALLBACK_RESULT_MAX_VALUE);
 }
 
+base::string16 MakeCacheKey(const wchar_t* base_family_name,
+                            const wchar_t* locale) {
+  base::string16 cache_key(base_family_name);
+  return cache_key + L"_" + locale;
+}
+
 }  // namespace
 
 HRESULT FontFallback::Create(FontFallback** font_fallback_out,
@@ -79,16 +85,6 @@ HRESULT FontFallback::MapCharacters(IDWriteTextAnalysisSource* source,
 
   base_family_name = base_family_name ? base_family_name : L"";
 
-  if (GetCachedFont(text_chunk, base_family_name, base_weight, base_style,
-                    base_stretch, mapped_font, mapped_length)) {
-    DCHECK(*mapped_font);
-    DCHECK_GT(*mapped_length, 0u);
-    LogFallbackResult(SUCCESS_CACHE);
-    return S_OK;
-  }
-
-  TRACE_EVENT0("dwrite,fonts", "FontFallback::MapCharacters (IPC)");
-
   const WCHAR* locale = nullptr;
   // |locale_text_length| is actually the length of text with the locale, not
   // the length of the locale string itself.
@@ -98,6 +94,16 @@ HRESULT FontFallback::MapCharacters(IDWriteTextAnalysisSource* source,
 
   locale = locale ? locale : L"";
 
+  if (GetCachedFont(text_chunk, base_family_name, locale, base_weight,
+                    base_style, base_stretch, mapped_font, mapped_length)) {
+    DCHECK(*mapped_font);
+    DCHECK_GT(*mapped_length, 0u);
+    LogFallbackResult(SUCCESS_CACHE);
+    return S_OK;
+  }
+
+  TRACE_EVENT0("dwrite,fonts", "FontFallback::MapCharacters (IPC)");
+
   blink::mojom::MapCharactersResultPtr result;
 
   if (!GetFontProxy().MapCharacters(text_chunk,
@@ -145,7 +151,7 @@ HRESULT FontFallback::MapCharacters(IDWriteTextAnalysisSource* source,
   }
 
   DCHECK(*mapped_font);
-  AddCachedFamily(std::move(family), base_family_name);
+  AddCachedFamily(std::move(family), base_family_name, locale);
   LogFallbackResult(SUCCESS_IPC);
   return S_OK;
 }
@@ -158,13 +164,14 @@ FontFallback::RuntimeClassInitialize(DWriteFontCollectionProxy* collection) {
 
 bool FontFallback::GetCachedFont(const base::string16& text,
                                  const wchar_t* base_family_name,
+                                 const wchar_t* locale,
                                  DWRITE_FONT_WEIGHT base_weight,
                                  DWRITE_FONT_STYLE base_style,
                                  DWRITE_FONT_STRETCH base_stretch,
                                  IDWriteFont** font,
                                  uint32_t* mapped_length) {
   std::map<base::string16, std::list<mswr::ComPtr<IDWriteFontFamily>>>::iterator
-      it = fallback_family_cache_.find(base_family_name);
+      it = fallback_family_cache_.find(MakeCacheKey(base_family_name, locale));
   if (it == fallback_family_cache_.end())
     return false;
 
@@ -211,9 +218,10 @@ bool FontFallback::GetCachedFont(const base::string16& text,
 
 void FontFallback::AddCachedFamily(
     Microsoft::WRL::ComPtr<IDWriteFontFamily> family,
-    const wchar_t* base_family_name) {
+    const wchar_t* base_family_name,
+    const wchar_t* locale) {
   std::list<mswr::ComPtr<IDWriteFontFamily>>& family_list =
-      fallback_family_cache_[base_family_name];
+      fallback_family_cache_[MakeCacheKey(base_family_name, locale)];
   family_list.push_front(std::move(family));
 
   UMA_HISTOGRAM_COUNTS_100("DirectWrite.Fonts.Proxy.Fallback.CacheSize",

content/child/dwrite_font_proxy/font_fallback_win.h

@@ -55,6 +55,7 @@ class FontFallback
 
   bool GetCachedFont(const base::string16& text,
                      const wchar_t* base_family_name,
+                     const wchar_t* locale,
                      DWRITE_FONT_WEIGHT base_weight,
                      DWRITE_FONT_STYLE base_style,
                      DWRITE_FONT_STRETCH base_stretch,
@@ -62,7 +63,8 @@ class FontFallback
                      uint32_t* mapped_length);
 
   void AddCachedFamily(Microsoft::WRL::ComPtr<IDWriteFontFamily> family,
-                       const wchar_t* base_family_name);
+                       const wchar_t* base_family_name,
+                       const wchar_t* locale);
 
  private:
   blink::mojom::DWriteFontProxy& GetFontProxy();