streamline pref font logic within gfxFontGroup::BuildFontList

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(7 attachments)

(Assignee)

Description

3 years ago
The current font pref lookup logic for generic fonts is a bit of a tangled mess. I think we should streamline this a bit for perf reasons.

I also think we should do a better job of handling the special Linux font pref generic handling logic. Within the font prefs UI under Linux, the default is that 'Serif' shows up in the pop-up menu for serif, 'Sans-serif' for sans-serif, etc. These values have a special, Linux-only meaning of "use fontconfig to map the generics to actual fonts". This is currently handled in gfxFcPlatformFontList::FindFamily:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFcPlatformFontList.cpp#1243

I think it would be better to handle it within the code in gfxFontGroup::ResolveGenericFontNames, probably under a Linux-only build flag. The logic is more complicated for Linux since if a special generic is used (i.e. font.name.serif.xxx ==> "Serif") then the *language* rather than the langGroup should be used when querying fontconfig. Other platforms will always map language ==> langGroup and simply look up the font families.

Goals to achieve here:

- better caching of lang/langGroup ==> pref font-family-list
- need to allow for multiple families within fontconfig handling (bug 1173260)
- use Navigator::GetAcceptLanguages rather than custom accept-lang logic
- eliminate some of the language atom/string/enum redundancy

Rough sketch of proposed logic:

  lang ==> langGroup
  langGroup ==> pref-list-of-font-families-string
  if (pref-list == generic) { // Linux only!
    lang ==> font-family-array
  } else {
    langGroup ==> font-family-array
  }

(I'm omitting the caching logic here for simplicity)
(Assignee)

Updated

3 years ago
Depends on: 1203809
(Assignee)

Updated

3 years ago
Blocks: 1205946
(Assignee)

Comment 1

3 years ago
Created attachment 8666350 [details] [diff] [review]
patch p1 - copy old generic lookup methods into gfxPangoFontGroup

Duplicate the existing fontlist lookup methods into gfxPangoFontGroup so the interfaces can be changed more easily. This duplicates code but that code will be trimmed out when gfxPangoFontGroup is obsoleted.
Attachment #8666350 - Flags: review?(cam)
(Assignee)

Comment 2

3 years ago
Created attachment 8666351 [details] [diff] [review]
patch p2 - count generic lookups
Attachment #8666351 - Flags: review?(cam)
(Assignee)

Comment 3

3 years ago
Created attachment 8666352 [details] [diff] [review]
patch p3 - move generic lookup methods into platform fontlist code

Move generic lookup methods out of gfxFontGroup into platform fontlist code. This allows it to be cached and the behavior overridden in the Linux case.
Attachment #8666352 - Flags: review?(cam)
(Assignee)

Comment 4

3 years ago
Created attachment 8666353 [details] [diff] [review]
patch p4 - move generic font utility routines

Move generic font util methods from gfxPlatform into platform fontlist code
Attachment #8666353 - Flags: review?(cam)
(Assignee)

Comment 5

3 years ago
Created attachment 8666354 [details] [diff] [review]
patch p5 - cache generic lookups per lang group

Rather than reading the font.name.xxx.lang prefs each time 'serif' or 'sans-serif' is used, cache the resulting list of font families that these map to.
Attachment #8666354 - Flags: review?(cam)
(Assignee)

Comment 6

3 years ago
Created attachment 8666355 [details] [diff] [review]
patch p6 - use the cached pref font families in font fallback

The code in gfxFontGroup::WhichPrefFontSupportsChar caches pref fonts used in fallback. Eliminate this and use the cached generic font families instead.
Attachment #8666355 - Flags: review?(cam)
Comment on attachment 8666350 [details] [diff] [review]
patch p1 - copy old generic lookup methods into gfxPangoFontGroup

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

General nit: while touching lines that aren't an exact move of a chunk of code, you may as well fix up style issues such as "*"s and "&"s not being next to their type.

::: gfx/thebes/gfxFontconfigFonts.h
@@ +97,5 @@
> +    // old helper methods from gfxFontGroup, moved here so that those methods
> +    // can be revamped without affecting the legacy code here
> +
> +    // iterate over the fontlist, lookup names and expand generics
> +    void EnumerateFontListPFG(nsIAtom *aLanguage, void *aClosure);

"PFG" is Pango Font group, I guess?  (For a moment I thought it might be something like "per font group".)

::: gfx/thebes/gfxTextRun.h
@@ +1126,3 @@
>  
>      // lookup and add a font with a given name (i.e. *not* a generic!)
> +    void FindPlatformFont(const nsAString& aName, bool aUseFontSet);

Something to fix in the future: I think the names of these methods are not sufficiently descriptive of what they do.  EnumerateFontList doesn't just enumerate them, it enumerates them and builds mFonts.  Find{Generic,Platform}Font don't just find the font, they find it and add the corresponding faces to mFonts.
Attachment #8666350 - Flags: review?(cam) → review+
Comment on attachment 8666351 [details] [diff] [review]
patch p2 - count generic lookups

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

::: gfx/thebes/gfxTextRun.cpp
@@ +1564,5 @@
>  {
>      nsAutoTArray<nsString, 5> resolvedGenerics;
>      ResolveGenericFontNames(aGenericType, aLanguage, resolvedGenerics);
>      uint32_t g = 0, numGenerics = resolvedGenerics.Length();
> +    if (mTextPerf) {

Nit: I feel like this would be better done either at the very start or very end of the function.  At least I think it shouldn't split up the g/numGenerics declarations from the loop they're used in.
Attachment #8666351 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #7)
> Something to fix in the future: I think the names of these methods are not
> sufficiently descriptive of what they do.  EnumerateFontList doesn't just
> enumerate them, it enumerates them and builds mFonts. 
> Find{Generic,Platform}Font don't just find the font, they find it and add
> the corresponding faces to mFonts.

I see this gets better in p3.
Comment on attachment 8666352 [details] [diff] [review]
patch p3 - move generic lookup methods into platform fontlist code

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

(Apologies for suggestions for improvements in code that you are mostly just moving...)

::: gfx/thebes/gfxPlatformFontList.h
@@ +201,5 @@
>          aLoaderState = (uint32_t) mState;
>      }
>  
> +    virtual void
> +    AddGenericFonts(mozilla::FontFamilyType aGenericType, gfxFontStyle* aStyle,

Does this need to be virtual?  I don't see it overridden.

@@ +294,5 @@
>      void RebuildLocalFonts();
>  
> +    void
> +    ResolveGenericFontNames(mozilla::FontFamilyType aGenericType,
> +                            nsIAtom *aLanguage,

Nit: "*" next to type.

::: gfx/thebes/gfxTextRun.cpp
@@ -1568,5 @@
> -    if (mTextPerf) {
> -        mTextPerf->current.genericLookups++;
> -    }
> -    for (g = 0; g < numGenerics; g++) {
> -        FindPlatformFont(resolvedGenerics[g], false);

After your patch queue will there be any users of FindPlatformFont that pass in false for the second argument any more?  Can it be removed, and have it always look in the user font set?

@@ +1579,5 @@
> +    gfxPlatformFontList *pfl = gfxPlatformFontList::PlatformFontList();
> +
> +    // lookup fonts in the fontlist
> +    uint32_t i, numFonts = fontlist.Length();
> +    for (i = 0; i < numFonts; i++) {

It would be good to avoid declaring these variables outside the loop (and re-using the later in the function).  In fact, since we don't need to use i at all, how about changing it to:

  for (const FontFamilyName& name : fontlist) {

or even:

  for (const FontFamilyName& name : mFamilyList.GetFontList()) {

and you can skip storing it in the fontlist variable.

@@ +1583,5 @@
> +    for (i = 0; i < numFonts; i++) {
> +        gfxFontFamily* family;
> +        const FontFamilyName& name = fontlist[i];
> +        if (name.IsNamed()) {
> +            family = FindPlatformFont(name.mName, true);

What do you think about making FindPlatformFont takes fonts as an argument and have it do the append (and then rename the method appropriately)?  I think having the pfl->AddGenericFonts(..., fonts) call parallel an AddPlatformFont(..., fonts) call would be nice, like the old code had parallel Find{PlatformFont,GenericFonts} calls.

@@ +1607,5 @@
> +
> +    // build the fontlist from the specified families
> +    numFonts = fonts.Length();
> +    for (i = 0; i < numFonts; i++) {
> +        AddFamilyToFontList(fonts[i]);

Same here, you could replace this with:

  for (gfxFontFamily* font : fonts) {
    AddFamilyToFontList(font);
  }

@@ +1647,5 @@
> +    aFamily->FindAllFontsForStyle(mStyle, fontEntryList, needsBold);
> +    // add these to the fontlist
> +    uint32_t n = fontEntryList.Length();
> +    for (uint32_t i = 0; i < n; i++) {
> +        gfxFontEntry* fe = fontEntryList[i];

for (gfxFontEntry* fe : fontEntryList) {
Attachment #8666352 - Flags: review?(cam) → review+
Attachment #8666353 - Flags: review?(cam) → review+
Comment on attachment 8666354 [details] [diff] [review]
patch p5 - cache generic lookups per lang group

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

r=me with comments addressed (particularly the use of nsAutoPtrs).

::: gfx/thebes/gfxFontFamilyList.h
@@ +28,5 @@
>    eFamily_named,
>    eFamily_named_quoted,
>  
>    // generics
> +  eFamily_serif,         // pref font code relies on this ordering!!!

Three exclamation marks seems excessive. :-)

What specifically about the ordering is required here?  I can see that we require eFamily_serif to be the first and eFamily_fantasy to be the last -- is that all?  If so, mention that.  Consider adding

  FontFamilyType_Generic_First = eFamily_serif,
  FontFamilyType_Generic_Last = eFamily_fantasy,
  FontFamilyType_Generic_Count =
    FontFamilyType_Generic_Last - FontFamilyType_Generic_Last + 1,

to the end of the enum, so that there is only a single place that relies on those being the start/end values.  Then you can use the Generic_Start/Count names rather than serif/fantasy in your assertions, and in place of kNumGenerics.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +886,5 @@
> +    prefFontListName.Append('.');
> +    prefFontListName.Append(langGroupStr);
> +    gfxFontUtils::AppendPrefsFontList(prefFontListName.get(), genericFamilies);
> +
> +    nsIAtom *langGroup = GetLangGroupForPrefLang(aPrefLang);

Nit: "*" next to type.

@@ +891,5 @@
> +    NS_ASSERTION(langGroup, "null lang group for pref lang");
> +
> +    // lookup and add platform fonts uniquely
> +    uint32_t numFamilyNames = genericFamilies.Length();
> +    for (uint32_t g = 0; g < numFamilyNames; g++) {

Just do:

  for (const nsString& genericFamily : genericFamilies) {

and get rid of numFamilyNames/g.

@@ +897,5 @@
> +            FindFamily(genericFamilies[g], langGroup, false);
> +        if (family) {
> +            bool notFound = true;
> +            uint32_t numFamilies = aGenericFamilies->Length();
> +            for (uint32_t v = 0; v < numFamilies; v++) {

for (const gfxFontFamily* f : *aGenericFamilies) {
  if (f == family) {
    notFound = false;
    break;
  }
}

@@ +919,5 @@
>      printf("\n");
>  #endif
>  }
>  
> +PR_STATIC_ASSERT((uint32_t(eFamily_fantasy) - uint32_t(eFamily_serif) + 1) == \

You can use static_assert these days.

@@ +937,5 @@
> +    uint32_t offset = uint32_t(aGenericType) - uint32_t(eFamily_serif);
> +    return uint32_t(aPrefLang) * kNumGenerics + offset;
> +}
> +
> +nsTArray<nsRefPtr<gfxFontFamily> >*

Nit: No space between ">"s.

@@ +948,5 @@
> +    }
> +
> +    NS_ASSERTION(mLangGroupPrefFonts.Length(), "pref fonts not set up correctly");
> +    uint32_t index = GenericPrefFontIndex(aGenericType, aPrefLang);
> +    nsTArray<nsRefPtr<gfxFontFamily> >* prefFonts = mLangGroupPrefFonts[index];

And here.

@@ +950,5 @@
> +    NS_ASSERTION(mLangGroupPrefFonts.Length(), "pref fonts not set up correctly");
> +    uint32_t index = GenericPrefFontIndex(aGenericType, aPrefLang);
> +    nsTArray<nsRefPtr<gfxFontFamily> >* prefFonts = mLangGroupPrefFonts[index];
> +    if (MOZ_UNLIKELY(!prefFonts)) {
> +        prefFonts = new nsTArray<nsRefPtr<gfxFontFamily> >;

And here.

@@ +964,4 @@
>                                       nsTArray<gfxFontFamily*>& aFamilyList)
>  {
> +    // map lang ==> langGroup
> +    nsIAtom *langGroup = nullptr;

Nit: "*" next to type.

@@ +1455,5 @@
> +        if (prefFonts) {
> +            delete prefFonts;
> +            mLangGroupPrefFonts[i] = nullptr;
> +        }
> +    }

This can be reduced to:

  for (nsTArray<nsRefPtr<gfxFontFamily>>*& prefFonts : mLangGroupPrefFonts) {
    prefFonts = nullptr;
  }

if we use nsAutoPtrs (as described in a comment below), or

  for (auto& prefFonts : mLangGroupPrefFonts) {
    prefFonts = nullptr;
  }

if you're happy to elide the type.

::: gfx/thebes/gfxPlatformFontList.h
@@ +336,5 @@
>  
>      void
>      ResolveGenericFontNames(mozilla::FontFamilyType aGenericType,
> +                            eFontPrefLang aPrefLang,
> +                            nsTArray<nsRefPtr<gfxFontFamily> >* aGenericFamilies);

Spaces no longer required between ">"s.

@@ +338,5 @@
>      ResolveGenericFontNames(mozilla::FontFamilyType aGenericType,
> +                            eFontPrefLang aPrefLang,
> +                            nsTArray<nsRefPtr<gfxFontFamily> >* aGenericFamilies);
> +
> +    nsTArray<nsRefPtr<gfxFontFamily> >*

And here.

@@ +340,5 @@
> +                            nsTArray<nsRefPtr<gfxFontFamily> >* aGenericFamilies);
> +
> +    nsTArray<nsRefPtr<gfxFontFamily> >*
> +    GetPrefFontsLangGroup(mozilla::FontFamilyType aGenericType,
> +                          eFontPrefLang aPrefLang);

Why do we use "PrefFont" in some places but "FontPref" in others?  Is there a difference, or is it just inconsistent naming?

@@ +391,5 @@
>      // cached pref font lists
>      // maps list of family names ==> array of family entries, one per lang group
>      nsDataHashtable<nsUint32HashKey, nsTArray<nsRefPtr<gfxFontFamily> > > mPrefFonts;
>  
> +    nsTArray<nsTArray<nsRefPtr<gfxFontFamily> >* > mLangGroupPrefFonts;

And here.

Can you add a comment above this that says what this stores compared to mPrefFonts?

You might like to do this as a follow up, but rather than use a dynamically sized array that always is given a length of kNumGenerics * ArrayLength(gPrefLangNames), and computing indexes based on the generic/preflang, how about we use fixed-sized arrays?  The lengths are known at compile time, and using mozilla::RangedArray we can index them directly with FontFamilyType and eFontPrefLang values.

  typedef nsTArray<nsRefPtr<gfxFontFamily>> PrefFontList;
  typedef mozilla::RangedArray<PrefFontList*,
                               FontFamilyType_Generic_First,
                               FontFamilyType_Generic_Count> PrefFontsForLangGroup;
  mozilla::RangedArray<PrefFontsForLangGroup,
                       eFontPrefLang_First,
                       eFontPrefLang_Count> mLangGroupPrefFonts;

(If you add those eFontPrefLang_{First,Count} values to eFontPrefLang.)

In GetPrefFontsLangGroup for example that would allow us to replace:

  uint32_t index = GenericPrefFontIndex(aGenericType, aPrefLang);
  nsTArray<nsRefPtr<gfxFontFamily> >* prefFonts = mLangGroupPrefFonts[index];

with:

  nsTArray<nsRefPtr<gfxFontFamily>>* prefFonts =
    mLangGroupPrefFonts[aPrefLang][aGenericType];

Whether or not you do this now, we should use nsAutoPtrs for the nsTArrays inside here, rather than raw pointers.  That would allow you to skip the the ClearLangGroupPrefFonts call in the destructor, and also skip the memset() call in ClearLangGroupPrefFonts.
Attachment #8666354 - Flags: review?(cam) → review+
Comment on attachment 8666355 [details] [diff] [review]
patch p6 - use the cached pref font families in font fallback

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

::: gfx/thebes/gfxFT2FontList.cpp
@@ +1135,5 @@
>  {
>      gfxFontCache *fc = gfxFontCache::GetCache();
>      if (fc)
>          fc->AgeAllGenerations();
> +    ClearLangGroupPrefFonts();

Should this addition be in the previous patch?

::: gfx/thebes/gfxPlatformFontList.cpp
@@ -1527,5 @@
> -        // follow the refPtrs stored in it, because they point to entries
> -        // already owned and accounted-for by the main font list.
> -        aSizes->mFontListSize +=
> -            iter.Data().ShallowSizeOfExcludingThis(aMallocSizeOf);
> -    }

This makes me realize you need to count mLangGroupPrefFonts's dynamically allocated arrays in here in the previous patch.

::: gfx/thebes/gfxPlatformFontList.h
@@ +203,5 @@
>      AddGenericFonts(mozilla::FontFamilyType aGenericType,
>                      nsIAtom* aLanguage,
>                      nsTArray<gfxFontFamily*>& aFamilyList);
>  
> +    nsTArray<nsRefPtr<gfxFontFamily> >*

Nit: no space between ">"s.

::: gfx/thebes/gfxTextRun.cpp
@@ +3082,2 @@
>          eFontPrefLang currentLang = prefLangs[i];
> +        mozilla::FontFamilyType defaultGeneric =

Nit: There's a |using namespace mozilla;| at the top of the file so no need for "mozilla::" here.

@@ +3082,4 @@
>          eFontPrefLang currentLang = prefLangs[i];
> +        mozilla::FontFamilyType defaultGeneric =
> +            pfl->GetDefaultGeneric(currentLang);
> +        nsTArray<nsRefPtr<gfxFontFamily> >* families =

As above.

@@ +3092,3 @@
>          for (j = 0; j < numPrefs; j++) {
>              // look up the appropriate face
> +            gfxFontFamily *family = (*families)[j];

Nit: move "*" next to type.
Attachment #8666355 - Flags: review?(cam) → review+
(Assignee)

Comment 13

3 years ago
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #7)
> Comment on attachment 8666350 [details] [diff] [review]
> patch p1 - copy old generic lookup methods into gfxPangoFontGroup
> 
> Review of attachment 8666350 [details] [diff] [review]:

Agree completely with your comments but since this code will be obsolete within a few months, I'm not going to bother fixing the nits for this patch.
(Assignee)

Comment 14

3 years ago
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #8)
> Comment on attachment 8666351 [details] [diff] [review]
> patch p2 - count generic lookups
> 
> Review of attachment 8666351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxTextRun.cpp
> @@ +1564,5 @@
> >  {
> >      nsAutoTArray<nsString, 5> resolvedGenerics;
> >      ResolveGenericFontNames(aGenericType, aLanguage, resolvedGenerics);
> >      uint32_t g = 0, numGenerics = resolvedGenerics.Length();
> > +    if (mTextPerf) {
> 
> Nit: I feel like this would be better done either at the very start or very
> end of the function.  At least I think it shouldn't split up the
> g/numGenerics declarations from the loop they're used in.

Yup, but in the final version of BuildFontList, this is done at the end.
(Assignee)

Comment 15

3 years ago
Created attachment 8667025 [details] [diff] [review]
patch p7 - fixup various things based on review comments

This patch address various things in the review comments above. I switched to using arrays of nsAutoPtr's for the pref font lists. However, the mozilla::RangedArray object doesn't support C++11 iteration constructs so had to revert to index-based iteration. Also added in memory reporting for the pref font lists.
Attachment #8667025 - Flags: review?(cam)
Comment on attachment 8667025 [details] [diff] [review]
patch p7 - fixup various things based on review comments

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

Looks great!

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +104,5 @@
>      #include "gfxFontPrefLangList.h"
>      #undef FONT_PREF_LANG
>  };
>  
> +static_assert(MOZ_ARRAY_LENGTH(gPrefLangNames) == uint32_t(eFontPrefLang_Count), \

Nit: trailing backslash shouldn't be needed.

::: gfx/thebes/gfxPlatformFontList.h
@@ +381,5 @@
> +    typedef nsTArray<nsRefPtr<gfxFontFamily>> PrefFontList;
> +    typedef mozilla::RangedArray<nsAutoPtr<PrefFontList>,
> +                                 mozilla::eFamily_generic_first,
> +                                 mozilla::eFamily_generic_count> PrefFontsForLangGroup;
> +    mozilla::RangedArray<PrefFontsForLangGroup,

Please add a comment describing what mLangGroupPrefFonts contains.
Attachment #8667025 - Flags: review?(cam) → review+
(Assignee)

Comment 17

3 years ago
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #11)

> @@ +340,5 @@
> > +                            nsTArray<nsRefPtr<gfxFontFamily> >* aGenericFamilies);
> > +
> > +    nsTArray<nsRefPtr<gfxFontFamily> >*
> > +    GetPrefFontsLangGroup(mozilla::FontFamilyType aGenericType,
> > +                          eFontPrefLang aPrefLang);
> 
> Why do we use "PrefFont" in some places but "FontPref" in others?  Is there
> a difference, or is it just inconsistent naming?

Yeah, I think you're right here, the xFontPrefyyy thingies should really be renamed to xPrefFontyyy. But that's a fairly big delta so I think it would be best to do this on a follow-up bug.
You need to log in before you can comment on or make changes to this bug.