1224965 - add pref for limit on generic substitutions in fontconfig platform fontlist

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Comment 1

[John Daggett (:jtd)]

The patches on bug 1173260 enable multiple fonts to be mapped to generics via fontconfig under Linux. To limit the time spent checking fonts during font matching, I put in an arbitrary limit of three but this should probably be in a pref so that users and/or distros that are doing something more complicated and account for longer substitution lists.

Comment 2

[Karl Tomlinson (:karlt back Feb 10)]

I expect the only time that there would be many families before the first with support for the language (bug 1056479 comment 42) would be when there are no families that support the language.

Comment 3

[John Daggett (:jtd)]

Attachment: patch p1, add pref for max generic substitutions

Convert a constant limit into a pref value.

Comment 4

[John Daggett (:jtd)]

Attachment: patch p2, modify logic of generic font lookups

Tweak the logic for looking up generic fonts under fontconfig. Add fonts into the array until the pattern has the lang, up until the pref-defined limit (default = 3). If no pattern has the lang, only use the first font (e.g. for obscure languages that don't match).

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8688274 [details] [diff] [review]
patch p1, add pref for max generic substitutions

Review of attachment 8688274 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +375,5 @@
> +    } else {
> +        gfxPlatform::FontsPrefsChanged(aPref);
> +        return;
> +    }
> +

I think that the following is better.

    if (strcmp(GFX_PREF_MAX_GENERIC_SUBSTITUTIONS, aPref)) {
        gfxPlatform::FontsPrefsChanged(aPref);
        return;
    }

    mMaxGenericSubstitutions = UNINITIALIZED_VALUE;

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 8688278 [details] [diff] [review]
patch p2, modify logic of generic font lookups

># HG changeset patch
># User John Daggett <[e-mail]>
># Parent  220a1ac8397e1888c1fdc7544b4b5de9bc1a778c
>Bug 1224965 p2 - tweak the handling of generic substitutions. r?karlt
>
>diff --git a/gfx/thebes/gfxFcPlatformFontList.cpp b/gfx/thebes/gfxFcPlatformFontList.cpp
>--- a/gfx/thebes/gfxFcPlatformFontList.cpp
>+++ b/gfx/thebes/gfxFcPlatformFontList.cpp
>@@ -396,41 +396,47 @@ gfxFontconfigFontEntry::gfxFontconfigFon
>     mStretch = aStretch;
>     mIsLocalUserFont = true;
> }
> 
> gfxFontconfigFontEntry::~gfxFontconfigFontEntry()
> {
> }
> 
>+static bool
>+PatternHasLang(const FcPattern *aPattern, const FcChar8 *aLang)
>+{
>+    FcLangSet *langset;
>+
>+    if (FcPatternGetLangSet(aPattern, FC_LANG, 0, &langset) != FcResultMatch) {
>+        return false;
>+    }
>+
>+    if (FcLangSetHasLang(langset, aLang) != FcLangDifferentLang) {
>+        return true;
>+    }
>+    return false;
>+}
>+
> bool
> gfxFontconfigFontEntry::SupportsLangGroup(nsIAtom *aLangGroup) const
> {
>     if (!aLangGroup || aLangGroup == nsGkAtoms::Unicode) {
>         return true;
>     }
> 
>     nsAutoCString fcLang;
>     gfxFcPlatformFontList* pfl = gfxFcPlatformFontList::PlatformFontList();
>     pfl->GetSampleLangForGroup(aLangGroup, fcLang);
>     if (fcLang.IsEmpty()) {
>         return true;
>     }
> 
>     // is lang included in the underlying pattern?
>-    FcLangSet *langset;
>-    if (FcPatternGetLangSet(mFontPattern, FC_LANG, 0, &langset) != FcResultMatch) {
>-        return false;
>-    }
>-
>-    if (FcLangSetHasLang(langset, (FcChar8 *)fcLang.get()) != FcLangDifferentLang) {
>-        return true;
>-    }
>-
>-    return false;
>+    return PatternHasLang(mFontPattern, ToFcChar8Ptr(fcLang.get()));
> }
> 
> nsresult
> gfxFontconfigFontEntry::ReadCMAP(FontInfoData *aFontInfoData)
> {
>     // attempt this once, if errors occur leave a blank cmap
>     if (mCharacterMap) {
>         return NS_OK;
>@@ -1645,16 +1651,17 @@ gfxFcPlatformFontList::FindGenericFamili
> 
>     if (!faces) {
>       return nullptr;
>     }
> 
>     // -- select the fonts to be used for the generic
>     prefFonts = new PrefFontList; // can be empty but in practice won't happen
>     uint32_t limit = gfxPlatformGtk::GetPlatform()->MaxGenericSubstitions();
>+    bool foundFontWithLang = false;
>     for (int i = 0; i < faces->nfont; i++) {
>         FcPattern* font = faces->fonts[i];
>         FcChar8* mappedGeneric = nullptr;
> 
>         // not scalable? skip...
>         FcBool scalable;
>         if (FcPatternGetBool(font, FC_SCALABLE, 0, &scalable) != FcResultMatch ||
>             !scalable) {
>@@ -1662,24 +1669,35 @@ gfxFcPlatformFontList::FindGenericFamili
>         }
> 
>         FcPatternGetString(font, FC_FAMILY, 0, &mappedGeneric);
>         if (mappedGeneric) {
>             NS_ConvertUTF8toUTF16 mappedGenericName(ToCharPtr(mappedGeneric));
>             gfxFontFamily* genericFamily =
>                 gfxPlatformFontList::FindFamily(mappedGenericName);
>             if (genericFamily && !prefFonts->Contains(genericFamily)) {
>-                //printf("generic %s ==> %s\n", genericLang.get(), (const char*)mappedGeneric);
>                 prefFonts->AppendElement(genericFamily);
>-                if (prefFonts->Length() >= limit) {
>+                bool foundLang = !fcLang.IsEmpty() &&
>+                                 PatternHasLang(font, ToFcChar8Ptr(fcLang.get()));
>+                foundFontWithLang = foundFontWithLang || foundLang;
>+                // stop at the first family for which the lang matches (or
>+                // when there is no lang)
>+                if (fcLang.IsEmpty() ||
>+                    prefFonts->Length() >= limit || foundLang) {
>                     break;
>                 }
>             }
>         }
>     }
>+
>+    // if no font in the list matches the lang, trim all but the first one
>+    if (!prefFonts->IsEmpty() && !foundFontWithLang) {
>+        prefFonts->TruncateLength(1);
>+    }
>+
>     mGenericMappings.Put(genericLang, prefFonts);
>     return prefFonts;
> }
> 
> bool
> gfxFcPlatformFontList::PrefFontListsUseOnlyGenerics()
> {
>     bool prefFontsUseOnlyGenerics = true;

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17ad6b5b7eb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b058350c9753

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17ad6b5b7eb8
https://hg.mozilla.org/mozilla-central/rev/b058350c9753

Comment 9

[John Daggett (:jtd)]

Comment on attachment 8688274 [details] [diff] [review]
patch p1, add pref for max generic substitutions

Approval Request Comment
[Feature/regressing bug #]: This is one of the bugs that need to be fixed to enable the new fontconfig back end for beta/release builds. Enabling the new fontconfig back end will allow us to release unicode-range support across all platforms (bugs 1180560, 1119062).
[User impact if declined]: unicode-range support can't be enabled until FF45
[Describe test coverage new/current, TreeHerder]: landed on trunk last week, no issues reported
[Risks and why]: low risk, only affects users with special configurations
[String/UUID change made/needed]: none

Comment 10

[John Daggett (:jtd)]

Comment on attachment 8688278 [details] [diff] [review]
patch p2, modify logic of generic font lookups

Approval Request Comment

-- same as above --

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688274 [details] [diff] [review]
patch p1, add pref for max generic substitutions

Having a pref to limit number of matches we find is a good idea, Aurora44+

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688278 [details] [diff] [review]
patch p2, modify logic of generic font lookups

Aurora44+

Comment 13

[John Daggett (:jtd)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4892b39b731

Comment 14

[John Daggett (:jtd)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ac27957690f

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f4892b39b731
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5ac27957690f