1056479 - implement gfxPlatformFontList subclass for fontconfig

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

Now that we rely on harfbuzz for shaping on all platforms, I think we should work on a subclass for gfxPlatformFontList for use with fontconfig under Linux. We duplicate certain functions of gfxFontGroup such that changes like implementing unicode-range require one version for most platforms and another for just Linux. This slows down new feature development and hurts Linux users. It also hurts us from a testing perspective because Linux devs aren't using the font handling code used on other platforms. Since many sharp Gecko devs are Linux-based, it's unfortunate that they aren't using the same font handling code used on other platforms.

Specifically, for implementing unicode-range more efficiently, I want to eliminate gfxPangoFontGroup and merge the code in gfxFontconfigUtils into a new subclass of gfxPlatformFontList to minimize Linux-specific code as much as possible. My thinking is that the global fontlist would be populated lazily without the need for any form of global font enumeration.

Comment 1

[Zack Weinberg (:zwol)]

Might that also mean that we wouldn't require a restart to pick up newly-added system fonts?

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to Zack Weinberg (:zwol) from comment #1)
> Might that also mean that we wouldn't require a restart to pick up
> newly-added system fonts?

I don't think that's directly related to changes here.
Check <rescan> in your fontconfig configuration files, and nsFontCache may be preventing you from seeing updates.
But getting font lists for displaying preferences should override fontconfig's rescan value, and selecting a different preferred font at least used to clear nsFontCache.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, part 1 - add lang to FindFamily parameters

Add the lang to FindFamily so that it can be used by fontconfig when looking up family names.

Comment 4

[John Daggett (:jtd)]

Attachment: patch, part 2 - new fontconfig platform fontlist

Maintain a list of platform fonts using fontconfig. This differs from current code by (1) maintaining an explicit list of font families and (2) using code shared across all platforms to do font matching.

The current code is set up behind a pref that is enabled by default. Switching it off enables the current gfxPangoFontGroup code path. Once we're comfortable with the new code, the pref along with the code in  gfxPangoFontGroup and gfxFontconfigUtils can be removed.

Comment 5

[John Daggett (:jtd)]

Attachment: patch, part 3 - fixup reftests affected by new platform fontlist implementation

With new platform fontlist code, unicode-range now functions under Linux.

Comment 6

[John Daggett (:jtd)]

Attachment: patch, part 4 - fix accessibility font-weight code

Accessibility tests were failing because of workarounds in the way the gfxPangoFontGroup code dealt with font weights.

Comment 7

[John Daggett (:jtd)]

Attachment: patch, part 5 - fixup print preview tests

Print preview tests rely on differences in interface elements that are almost offscreen. Adjust the size of the canvas so that differences are captured correctly.

Comment 8

[John Daggett (:jtd)]

tryserver build of the patches attached here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-195902c1d668/try-linux/

Comment 9

[John Daggett (:jtd)]

Still need to write an additional patch for dealing with fontconfig updates, currently handled within gfxFontconfigUtils::UpdateFontListInternal. Need to periodically check for updates and update the fontlist when that occurs.

Comment 10

[John Daggett (:jtd)]

Attachment: patch, part 6 - handle font updates

Use an interval timer to check for font changes, based on fontconfig's rescan value.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8544996 [details] [diff] [review]
patch, part 1 - add lang to FindFamily parameters

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

I guess this is fine, though I'm wondering if it'd be better to avoid having multiple trailing parameters with defaults, and let callers explicitly pass null if they don't have a language.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8544997 [details] [diff] [review]
patch, part 2 - new fontconfig platform fontlist

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

In general, this seems reasonable to me, but I'd like Karl to have a look as he has a much better understanding of the existing fontconfig integration and how all that works...

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +48,5 @@
> +{
> +    uint32_t n = 0, en = 0;
> +    FcChar8* lang;
> +    while (FcPatternGetString(aFont, aLangField, n, &lang) == FcResultMatch) {
> +        if (strncmp((char*)lang, "en", 2) == 0) {

This assumes no other language code will ever start with "en", which in practice is likely to be true (as the three-letter ISO639 codes of the form "en?" are pretty obscure), but we shouldn't rely on it. So we should check that either |lang| is no longer than two chars, or its third char is a hyphen (can fontconfig return languages such as en-US, en-GB, etc. here?)

@@ +101,5 @@
> +static uint16_t
> +MapFcWeight(int aFcWeight)
> +{
> +    if (aFcWeight <= (FC_WEIGHT_THIN + FC_WEIGHT_EXTRALIGHT) / 2)
> +        return 100;

This function is lacking braces on all the ifs.

(I wish there was a nicer way to do this mapping, but the values seem so arbitrary that I don't have any good ideas...)

@@ +196,5 @@
> +{
> +    // italic
> +    int slant;
> +    if (FcPatternGetInteger(aFontPattern, FC_SLANT, 0, &slant) != FcResultMatch) {
> +      slant = FC_SLANT_ROMAN;

Indent is generally 4 spaces in thebes code.

@@ +205,5 @@
> +
> +    // weight
> +    int weight;
> +    if (FcPatternGetInteger(aFontPattern, FC_WEIGHT, 0, &weight) != FcResultMatch) {
> +      weight = FC_WEIGHT_REGULAR;

indent

@@ +212,5 @@
> +
> +    // width
> +    int width;
> +    if (FcPatternGetInteger(aFontPattern, FC_WIDTH, 0, &width) != FcResultMatch) {
> +      width = FC_WIDTH_NORMAL;

indent

@@ +425,5 @@
> +            (aStyle->style & (NS_FONT_STYLE_ITALIC | NS_FONT_STYLE_OBLIQUE)) &&
> +            aStyle->allowSyntheticStyle;
> +
> +    if (needsOblique) {
> +        const double kSkewFactor = 0.25;

Use OBLIQUE_SKEW_FACTOR (defined in gfxFont.h) rather than a local constant.

@@ +644,5 @@
> +    bool needsBold;
> +    gfxFontStyle normal;
> +    gfxFontEntry *fe = FindFontForStyle(
> +                  (aMatchData->mStyle == nullptr) ? *aMatchData->mStyle : normal,
> +                  needsBold);

Surely the null-check is backwards here?

I'd suggest rewriting this as

    gfxFontEntry *fe =
        FindFontForStyle(aMatchData->mStyle ? *aMatchData->mStyle
                                            : gfxFontStyle(),
                         needsBold);

It looks like we have the same issue in the existing gfxFontFamily::FindFontForChar, where you copied this from. I've filed this as bug 1119423.

@@ +679,5 @@
> +            return;
> +        }
> +
> +         // omitting from original windows code -- family name, lang group, pitch
> +         // not available in current FontEntry implementation

nit: please fix indent of this comment

@@ +733,5 @@
> +        // not scalable? skip...
> +        FcBool scalable;
> +        if (FcPatternGetBool(font, FC_SCALABLE, 0, &scalable) != FcResultMatch ||
> +            !scalable) {
> +            continue;

I think the current PangoFonts code supports non-scalable fonts, doesn't it (or at least it originally intended to?) Are we OK with dropping that? This could be seen as a regression by some users, I guess... Karl, WDYT?

@@ +742,5 @@
> +        FcChar8* canonical = nullptr;
> +        FcPatternGetString(font, FC_FAMILY, cIndex, &canonical);
> +        if (!canonical) {
> +            continue;
> +        }

If you check the result of FcPatternGetString instead, there should be no need to explicitly initialize or null-check |canonical|.

@@ +817,5 @@
> +}
> +
> +gfxFontEntry*
> +gfxFcPlatformFontList::LookupLocalFont(const nsAString& aFontName,
> +	                   uint16_t aWeight,

It'd be tidier to line up the parameters here, I think.

@@ +857,5 @@
> +}
> +
> +gfxFontEntry*
> +gfxFcPlatformFontList::MakePlatformFont(const nsAString& aFontName,
> +	                    uint16_t aWeight,

and here

@@ +887,5 @@
> +{
> +    return mozilla::FontFamilyName::Convert(aFamily).IsGeneric();
> +}
> +
> +#define MAX_FONTCONFIG_SUBSTITUTIONS 5

Where does this number come from, and what happens if the fontconfig setup lists more?

@@ +1039,5 @@
> +        // the default lang mapping.
> +
> +        // -- get the BCP47 string representation of the lang group
> +        if (mozLangGroup) {
> +            aLangStr.Assign(mozLangGroup->defaultLang);

Is it valid to pass a null pointer (which is what you'd get for the Unicode langGroup) to nsACString.Assign()?

::: gfx/thebes/gfxFontEntry.h
@@ +698,5 @@
>                           bool& aNeedsSyntheticBold);
>  
>      // checks for a matching font within the family
>      // used as part of the font fallback process
> +    virtual void FindFontForChar(GlobalFontMatch *aMatchData);

This is a pretty hot function, so it makes me sad if it has to become virtual; that'll make it more expensive across all platforms just for the sake of this one backend. Any way we can avoid that?

::: gfx/thebes/gfxPlatformFontList.h
@@ +212,5 @@
>                                                 void* userArg);
>  
> +    // Lookup family name in global family list without substitutions or
> +    // localized family name lookup. Used for common font fallback families.
> +    gfxFontFamily* FindFamilyInFamilyList(const nsAString& aFamily) {

Rather than creating a new method for this, how about a flag for the existing FindFamily() method - something like |aCheckAlternateNames|? If this isn't set, it would just return null if it doesn't find the canonical name.

FindFamily() currently takes an optional bool |aUseSystemFont|; I suggest we replace that with a flags word and handle both that flag and a new alternateNames one there.

Comment 13

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Comment on attachment 8544997 [details] [diff] [review]
> patch, part 2 - new fontconfig platform fontlist

> @@ +733,5 @@
> > +        // not scalable? skip...
> > +        FcBool scalable;
> > +        if (FcPatternGetBool(font, FC_SCALABLE, 0, &scalable) != FcResultMatch ||
> > +            !scalable) {
> > +            continue;
> 
> I think the current PangoFonts code supports non-scalable fonts, doesn't it
> (or at least it originally intended to?) Are we OK with dropping that? This
> could be seen as a regression by some users, I guess... Karl, WDYT?

I just don't see much point to supporting non-scalable fonts at this point. Supporting them adds complexity and it doesn't seem necessary. The Skia code used by Chrome includes similar logic. I think this is in keeping with the way DirectWrite dropped support of FON fonts. We need to deprecate infrequently used features that make code more complex.

Comment 14

[John Daggett (:jtd)]

Attachment: patch, alternate method for global font fallback

(In reply to Jonathan Kew (:jfkthame) from comment #12)

> >      // checks for a matching font within the family
> >      // used as part of the font fallback process
> > +    virtual void FindFontForChar(GlobalFontMatch *aMatchData);
> 
> This is a pretty hot function, so it makes me sad if it has to become
> virtual; that'll make it more expensive across all platforms just for the
> sake of this one backend. Any way we can avoid that?

The main reason for this is that we really *don't* want to pull in cmaps for all fonts on the platform when the fontconfig pattern for each face already has charmap data.

I think the alternative is to simply use build-time conditionals to special-case the code within gfxFontFamily::FindFontForChar for the fontconfig case. Not a thing of beauty but it keeps the special-case code localized.

Another alternative would be to derive the entire set of methods under gfxPlatformFontList::GlobalFontFallback, but that's much more grotty I think.

Let me know what you think. If this seems okay, I'll move it into the code in part 2.

Comment 15

[John Daggett (:jtd)]

Note: patch above assumes the patch on bug 1119253 has already been applied.

Comment 16

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #12)

> > +    // Lookup family name in global family list without substitutions or
> > +    // localized family name lookup. Used for common font fallback families.
> > +    gfxFontFamily* FindFamilyInFamilyList(const nsAString& aFamily) {
> 
> Rather than creating a new method for this, how about a flag for the
> existing FindFamily() method - something like |aCheckAlternateNames|? If
> this isn't set, it would just return null if it doesn't find the canonical
> name.
> 
> FindFamily() currently takes an optional bool |aUseSystemFont|; I suggest we
> replace that with a flags word and handle both that flag and a new
> alternateNames one there.

I don't think this is such a great way of doing this. That method is a very small inline helper method that doesn't represent a lot of duplicated code. But adding flags to FindFamily that may or may not be used for a given derived class of gfxPlatformFontList adds extra bits that are set up and checked needlessly during typical font family lookups.

Comment 17

[John Daggett (:jtd)]

Attachment: patch, part 2 v2 - new fontconfig platform fontlist

Updated based on review comments. Carrying forward the feedback+ from jfkthame.

Comment 18

[John Daggett (:jtd)]

Attachment: patch, alternate method for global font fallback

fixup non-linux compile error

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8549426 [details] [diff] [review]
patch, alternate method for global font fallback

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

OK... yeah, this is kinda ugly but I think it's better than making FindFontForChar a virtual method, which will cost performance everywhere. (And better than duplicating a whole raft more code, which will become a maintenance headache.)

Comment 20

[John Daggett (:jtd)]

Attachment: patch, part 2 v3 - new fontconfig platform fontlist

New version of part 2 patch, now including the alternate method for global font fallback using conditional code rather than virtualizing FindFontForChar.

Carrying forward feedback+=jfkthame

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8551535 [details] [diff] [review]
patch, part 2 v3 - new fontconfig platform fontlist

It'll take me a little time to catch up on the code involved here and understand what these changes mean, so just some superficial comments for now.

>+    static bool UseFcFontlist() { return sUseFcFontlist; }

Thanks for adding this pref.  It'll make it easy to test whether any bug
reports are related to this change.

There is some inconsistency about whether the font list is one word or two.
Usually it is FontList, so UseFcFontList and sUseFcFontList would be more
consistent with the other code I've seen here.

>+#if defined(MOZ_WIDGET_GTK)
>+#include "gfxPlatformGtk.h" // xxx - for UseFcFontlist
>+#endif

Can you put this with the other headers at the top of the file please?

I wonder whether having it in the middle of code might cause some surprises if
a well-meaning person were ever to add a namespace block around the methods.

>-    void BuildFontList();
>+    virtual void BuildFontList();

I don't see an override for this method.  Perhaps this is left over from an
earlier version?

>+# enable new platform fontlist for linux on GTK2 platform

There is a GTK3 port also, which uses the same code, so just say "GTK" here.

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> > +        // not scalable? skip...
> > +        FcBool scalable;
> > +        if (FcPatternGetBool(font, FC_SCALABLE, 0, &scalable) != FcResultMatch ||
> > +            !scalable) {
> > +            continue;
> 
> I think the current PangoFonts code supports non-scalable fonts, doesn't it
> (or at least it originally intended to?) Are we OK with dropping that? This
> could be seen as a regression by some users, I guess... Karl, WDYT?

I guess it's more likely going to be a problem for some less common languages,
but I suspect computing has moved on far enough that scalable fonts are a
necessity for many things, and so are probably going to be available.

Dropping support for fixed size fonts probably makes sense if it means we can
avoid some complexity.

Comment 22

[John Daggett (:jtd)]

Attachment: patch, part 2 v4 - new fontconfig platform fontlist

Updated based on Karl's comments. Carrying forward feedback+ from jfkthame.

Karl, please let me know if there are parts of the new code that you need more explanation about, either in the bug here or in code comments. I did iterate a lot on the system font fallback sections to make the performance match current code.

Comment 23

[John Daggett (:jtd)]

Attachment: patch, part 4 v2 - fix accessibility font-weight code

Updated based on review comments.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8565214 [details] [diff] [review]
patch, part 4 v2 - fix accessibility font-weight code

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

::: accessible/base/TextAttrs.cpp
@@ +636,5 @@
> +
> +  // Under Linux, when gfxPangoFontGroup code is used,
> +  // font->GetStyle()->weight will give the absolute weight requested of the
> +  // font face. The gfxPangoFontGroup code uses the gfxFontEntry constructor
> +  // which doesn't initialize the weight field.

Can't we just fix gfxPangoFontGroup so that it *does* initialize the weight field appropriately, and get rid of all the ugliness here?

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8545715 [details] [diff] [review]
patch, part 6 - handle font updates

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

Hmm. Shouldn't we be explicitly holding a reference to the config we're storing in mLastConfig here (see FcConfigReference/FcConfigDestroy)? Otherwise, who's to say FC mightn't destroy the old config and create a new one that happens to be at the same address, and we wouldn't notice.

Comment 26

[John Daggett (:jtd)]

Review ping?

Comment 27

[Karl Tomlinson (:karlt)]

I guess the needinfo is asking me for my impressions so far, but I'd prefer
not to spend time on any potentially pointless discussions until I've looked at
everything.  I have spent quite a bit of time on this but I'm not ready to give much of an update on comment 21 yet.  There is a lot to review here.  It took
considerable time to achieve the performance and behaviour of the previous
code, so it is reasonable to expect it to take time to replace.  I need to fit
this in with everything else and the extra effort here shouldn't delay other
reviews excessively.

One thing that might help is a list of the expected behavior changes.  I think
we should be up front about behavior we are giving away so that it can be
considered with the benefits.  The checkin comment I think could and should
give a more obvious indication that it is replacing the font selection code.

Comment 28

[Jet Villegas (inactive)]

AIUI the main benefit here is to get unicode-range across all platforms which enables quite a few use cases that are currently impractical due to the bandwidth taken up by large font families. It's still not clear to me who is actually affected by the Linux functionality we're looking to remove/replace and I've asked for telemetry on that. We've been going back and forth on this one for years now, and I'm inclined to ship unicode-range everywhere but Linux if we're really not willing to change it.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, having unicode-range support and having more common font code is a big win; as a Linux user I'm expecting *some* regression in our level of platform integration, although hopefully not too much.  I think that's a good tradeoff.

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to David Baron [:dbaron] (UTC-8) from comment #29)
> Yeah, having unicode-range support and having more common font code is a big
> win; as a Linux user I'm expecting *some* regression in our level of
> platform integration, although hopefully not too much.  I think that's a
> good tradeoff.

I agree.  I'm just trying to work out what that *some* regression is.  I think there is a solution here, and this approach feels like it is the right track, but it is taking me some time to catch up.  It's been a while and things have changed.

Comment 31

[John Daggett (:jtd)]

Just to give some background, I wanted to comment here about the general differences between existing gfxPangoFontGroup code and the new platform fontlist code here.

Font selection using platform fontlists works by enumerating the names of all font families on the system and, when a given family is referenced, enumerating the styles of faces available within that family. With faces enumerated, the font selection process is done using platform-agnostic code (i.e. gfxFontFamily::FindFontForStyle). The code in gfxPangoFontGroup effectively instead uses fontconfig by pushing the list of family names and style attributes into the pattern used for font selection.

I've tried to make the new code respect fontconfig substitution rules. The code that looks up a family name in gfxFcPlatformFontList::FindFamily runs the substitution rules first. And the code in gfxFontConfigFontEntry::CreateFontInstance used to instantiate a size-specific gfxFont object hopefully is picking up whatever font-specific modifications a user has included. I think the most likely place for regressions to occur is in the handling of complex fontconfig pattern rules in local fontconfig config files.

The fallback behavior here will also probably differ from current code since just using simple fontconfig calls to do system font fallback produced large perf regressions. The current code searches across all patterns for an appropriate font in the case of system fallback rather than explicitly asking fontconfig to suggest a font. I think we can probably get more performance out of this code by tuning it for a variety of distributions (I primarily am testing on Ubuntu while the test machines are running CentOS).

The big problem with the current code is that it's an implementation unique to the Linux code. The impact is that it's hard to modify and maintain and it's held up features like unicode-range substantially.

The current patches contain a pref that allows one to easily flip between current code and the new code. One option we have is to use this pref for delaying the release of the new code in release builds for an extra release cycle so that there's more testing in Nightly builds.

Comment 32

[John Daggett (:jtd)]

Attachment: patch, part 6 v2 - handle font updates

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> Comment on attachment 8545715 [details] [diff] [review]
> patch, part 6 - handle font updates
> 
> Review of attachment 8545715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm. Shouldn't we be explicitly holding a reference to the config we're
> storing in mLastConfig here (see FcConfigReference/FcConfigDestroy)?
> Otherwise, who's to say FC mightn't destroy the old config and create a new
> one that happens to be at the same address, and we wouldn't notice.

The code on the original patch is the same as existing code. There's already a
comment in the current code referring to the problem you're thinking about but,
point taken, probably better to do the right thing and keep a reference.

try build:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d5aeef4ef38e

Comment 33

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #24)
> Comment on attachment 8565214 [details] [diff] [review]
> patch, part 4 v2 - fix accessibility font-weight code
> 
> Review of attachment 8565214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/TextAttrs.cpp
> @@ +636,5 @@
> > +
> > +  // Under Linux, when gfxPangoFontGroup code is used,
> > +  // font->GetStyle()->weight will give the absolute weight requested of the
> > +  // font face. The gfxPangoFontGroup code uses the gfxFontEntry constructor
> > +  // which doesn't initialize the weight field.
> 
> Can't we just fix gfxPangoFontGroup so that it *does* initialize the weight
> field appropriately, and get rid of all the ugliness here?

We could but I don't think it's worth the effort. The gfxFcFontEntry in the gfxPangoFontGroup code can reference multiple patterns, I don't think it's quite that simple to work out what the true weight is. Since this code will be obsoleted by the new code, I think we should skip trying to fix it. The messy #ifdef's in TextAttrs.cpp will disappear once gfxPangoFontGroup code goes away.

Comment 34

[Jonathan Kew [:jfkthame]]

Comment on attachment 8575075 [details] [diff] [review]
patch, part 6 v2 - handle font updates

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

I'm not deeply familiar with all our various smart-pointer templates etc., but AFAICS this looks OK.

Comment 35

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #33)
> (In reply to Jonathan Kew (:jfkthame) from comment #24)
> > Comment on attachment 8565214 [details] [diff] [review]
> > patch, part 4 v2 - fix accessibility font-weight code
> > 
> > Review of attachment 8565214 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/base/TextAttrs.cpp
> > @@ +636,5 @@
> > > +
> > > +  // Under Linux, when gfxPangoFontGroup code is used,
> > > +  // font->GetStyle()->weight will give the absolute weight requested of the
> > > +  // font face. The gfxPangoFontGroup code uses the gfxFontEntry constructor
> > > +  // which doesn't initialize the weight field.
> > 
> > Can't we just fix gfxPangoFontGroup so that it *does* initialize the weight
> > field appropriately, and get rid of all the ugliness here?
> 
> We could but I don't think it's worth the effort. The gfxFcFontEntry in the
> gfxPangoFontGroup code can reference multiple patterns, I don't think it's
> quite that simple to work out what the true weight is.

I was assuming we'd simply set it to the requested weight from the gfxFontStyle, when creating the entry, so then it'd return the same thing we get from the conditionals in TextAttrs.cpp, but without the mess. Doesn't that work more simply and cleanly?

Comment 36

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #34)

> I'm not deeply familiar with all our various smart-pointer templates etc.,
> but AFAICS this looks OK.

This is the same pattern used in our existing fontconfig code:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontconfigUtils.h#20

Comment 37

[Karl Tomlinson (:karlt)]

I think I understand enough of the proposed change that I expect we can go
ahead with this approach with some modifications.  There will be some
differences in behaviour that are obvious to many of our users, but
differences that we can accept.  There are also some issues that we can and
should resolve.  There are some on the scale in-between too.

In the interests of making it possible to parallelize the work involved here,
I'll start raising the issues that I've seen in the parts that I already think I
understand, even though I haven't reviewed everything here.  I don't mind
whether you'd like to start addressing those now or wait for my complete review,
but I may continue to review and quote earlier revisions, instead of always
updating my copy to the latest. 

The performance is at least as good as current code on the test cases that
I've tried (PDF rendering, Wikipedia home page, MathML are where I've seen
perf issues before).
We should still keep an eye on the performance.
The only talos job that doesn't have tests dependent on font performance I
think is dromaeojs.
[1] is useful for this provided you can select revisions that are either all
PGO or non-PGO.  Some m-c revisions have no PGO builds and so can be easily
compared with derived try revisions.  Or the alternative is to push a control
revision to try.  There will be other changes required to the code, so you may
like to hold off doing that and posting a link, or you might like to check it
now for advanced notice of any potential issues.

[1] http://perf.snarkfest.net/compare-talos/index.html

Comment 38

[Karl Tomlinson (:karlt)]

The changes in behaviour that will stand out most I think are going to be in the UI.  This is because font selection is very dependent on the language and so things get most interesting when the language is wrong or missing.

The worst culprit for failing to get the language right is our own chrome,
which provides no language.

For example, in an eastern locale, the URL and search bars will use an eastern
font to render Latin text, including URLs and protocols, which are very often
Latin.  This produces odd rendering such as full-width spaces around
half-width characters, and the characters are typically not rendered as nicely
as with western fonts.  It will be very different from what users see used for
Latin in the UI of other applications.

  Currently, when no language is specified, the script of the characters is
  used to determine the language for font selection, as in other GTK/Pango
  apps.  This sometimes works well, but sometimes badly when there are
  characters common to many scripts and the font used for these characters
  changes as characters of different scripts are added to the text input.

  The use of the locale's language for URLs is in some ways more consistent
  with the rest of the UI, so I think this change of behaviour may be OK.

  It would require some significantly different code to generally handle
  generics differently according to the script.  If the URL almost
  always has Latin text, then perhaps it could be a special case with a Latin
  lang attribute, but that may have unintended consequences.  I'm not sure.

When it gets really ugly though is when a user in a non-Latin locale but they
have a US l10n of the product.  The Latin text looks very wrong with
double width spaces.  This situation includes our very helpful Nightly users,
which are rarer from non-Latin locales, but more needed.  I would like to do
something about this if at all possible.

  The problem is the chrome not providing the language of its text.  Could the
  fallback language of a chrome document be set according to the l10n?

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Karl Tomlinson (:karlt) from comment #38)
>   The problem is the chrome not providing the language of its text.  Could
> the
>   fallback language of a chrome document be set according to the l10n?

Getting the language set in chrome documents in some way sounds like a good idea.  Would you mind filing a separate bug on it?

Comment 40

[Karl Tomlinson (:karlt)]

(In reply to David Baron [:dbaron] (UTC-8) from comment #39)
> Getting the language set in chrome documents in some way sounds like a good
> idea.  Would you mind filing a separate bug on it?

The code being removed here was addressing bug 91190, as noted in the code comment, but this proposal addresses only part of bug 91190, so I filed bug 1142378 to track that separately.

Comment 41

[Karl Tomlinson (:karlt)]

Comment on attachment 8565213 [details] [diff] [review]
patch, part 2 v4 - new fontconfig platform fontlist

>+gfxFontFamily*
>+gfxFcPlatformFontList::FindFamily(const nsAString& aFamily,
>+                                  nsIAtom* aLanguage,
>+                                  bool aUseSystemFonts)

I think the use of FcConfigSubstitute with FcMatchPattern here probably
captures the key features of family name aliasing.  Because it is not doing
everything that is normally done with fontconfig, there is the possibility of
ending up with a different family in some situations, but still I think this
gets the main uses.  I'm not so concerned with getting the right aliases for
document-specified families, but getting the right preferred and UI fonts is
important.  People are usually going to use real family names of installed
fonts (if not a generic) for the UI font or preferred font, so aliases are not
likely to be involved here often.

Processing one family at a time instead of all families misses out on
imitating the <alias>/<default> or <edit mode="append_last> behavior of
fontconfig, but, anyway, I haven't seen that used effectively when the family
list contains a generic.  I would have guessed that doing FcConfigSubstitute
on each family would be too slow, but I don't see any problem with
performance.

This approach also doesn't consider language matches, but I guess aliasing one
family to one that covers a different set of languages would not be common.
The exception that I'm aware of is Fedora and Debian mapping language-specific
families "Droid Sans Japanese", etc. to a "Droid Sans" super family, but that
is OK because it is using a family with a superset of languages to cover a
family with a subset.

>+    // generics? use fontconfig to determine the family for the given lang
>+    if (IsGeneric(aFamily)) {
>+        return FindGenericFamily(aFamily, aLanguage);
>+    }

Can you add a comment or rename IsGeneric(), please, to clarify that this is
testing for fontconfig generics.  (CSS generics have already been resolved.)

Generics are kind of arbitrary in a configuration language.  Convention has
that sans-serif, serif, and monospace are the generics used with fontconfig,
but "mono", "sans", and "sans serif" also need to be supported.  This is
important because the default family for GTK is "Sans".

>+    // choices. To sniff out explicit substitutions, compare the substitutions
>+    // for "font, serif" to "serif", using the first font in the list of
>+    // serif substitutions as a sentinel.

>+        if (firstserif && FcStrCmp(substName, firstserif) == 0) {
>+            if (i == 0) {
>+                // fontlist name was the default serif font
>+                return gfxPlatformFontList::FindFamily(subst);

I think it is quite possible for this to miss noticing that the preferred
family for aFamily is the same as that for "serif", if that family is not at
i == 0.

What I would do I think is a single FcConfigSubstitute() with familyToFind and
a sentinel such as "-moz-sentinel" for FC_FAMILY in the pattern.  Anything up
to "-moz-sentinel" can be considered.

>+    FcPatternAddString(fontSerif, FC_FAMILY, (FcChar8*)(familyToFind.get()));

The ToFcChar8() and ToCString() safe cast utilities in gfxFontconfigUtils are
useful here and elsewhere to type check all the reinterpret casts.

Comment 42

[Karl Tomlinson (:karlt)]

Comment on attachment 8565213 [details] [diff] [review]
patch, part 2 v4 - new fontconfig platform fontlist

>+gfxFontFamily*
>+gfxFcPlatformFontList::FindGenericFamily(const nsAString& aGeneric,
>+                                         nsIAtom* aLanguage)

The FcFontSort is finding a font that supports the requested language, so it
is necessary to pass the language from PrefFontCallbackData.  If you make the
aLanguage parameter to FindFamily() non-optional, then we can see clearly any
other places that may need this parameter.

Also watch out that FindStyleVariations() etc exclude non-scalable fonts from
the family, so they should be excluded here too, or this could pick a family
where we ignore the support for the language.  I think it is sufficient to set
FC_SCALABLE on the sort pattern.  It has higher priority than language.

Returning only one family for the generic will cause the issue described in
https://code.google.com/p/chromium/issues/detail?id=403915 for some people.
To avoid some of the issues described in comment 38 here, some people have set
a family supporting Latin characters as the first preferred family for an
eastern language.  The second or subsequent preferred family contains support
for the eastern language.  If we consider only the first family, then the
family used for the eastern script characters will be somewhat arbitrary,
which causes problems with languages sharing the same code points (Chinese,
Japanese, Korean) at least.

  This kind of configuration was shipped in RedHat distributions for a while
  but is no longer, so this may only affect those that have added there own
  configurations.  I don't know how big an issue this is.  Perhaps there are
  now free fonts for eastern languages with better Latin glyphs.  CC'ing
  :masayuki in case he has any experience.

  If I were writing the code, I would probably keep this support by collecting
  families up to the first one that supports the language.  However, I don't
  know whether or not it is worth your time adding support for this.

  If our font-list preferences are documented well enough, then perhaps these
  people who are clever enough and care enough to configure fontconfig can
  find Gecko's preferences and achieve the same results there.  Integrating
  with platform prefs is better, but there is a workaround.

That raises a similar issue with the way that some fonts in a family are
ignored.  This is likely to be a problem with the Droid Sans family on Debian
and Fedora where it supports different languages with different fonts in the
same family.  If some fonts are ignored, then the family's support for the
particular language can be ignored, and this can lead to the same problem of using an arbitrary font or set of fonts for the language, or script.

  In a similar way to how all downloaded fonts in a family are considered, all
  style-compatible fonts in the system family should be considered.  The
  difference is that where downloaded fonts are sorted by order of definition,
  system fonts need to be sorted by order of language and fontversion.

  This is a generalization of the use of FindNonItalicFaceForChar().
  FindAllFontsForStyle() could return upright fonts sorted after the italic.
  Similarly, where Droid Sans has separate bold fonts only for the base
  languages, the fonts for other languages (without separate bold fonts, and
  depending on artificial embolden) should not be ignored just because some of
  the languages have bold support through separate fonts.

  This may be useful for other platforms also, and generalizes
  FindNonItalicFaceForChar() nicely, but it could be done only for this
  platform if you like.

Currently family fonts are sorted only by mStandardFace but that is not
used with gfxFontConfigFontEntry IIUC, and language is not fixed for a
family.

>+        nsAutoRef<FcFontSet> faces(FcFontSort(0, genericPattern, 0, 0, &result));

When passing constants for parameters, please pass the same type as the
parameter.  That makes it easier to determine from the call site whether it is
a pointer or boolean parameter, for example.  The only place that I've seen
this '0' pattern before is Skia.

Comment 43

[Karl Tomlinson (:karlt)]

gfxPlatform::ForEachPrefFont(), used when text of a different language appears
in an element, makes no attempt to keep the serif/sans/etc generic
kind of the font group.

When a font for a non-Latin locale is rendering most Latin characters with a
sans-serif font and fallback to a Latin font occurs for a character outside
the range of basic support, the fallback font will have serifs.

Comment 44

[Karl Tomlinson (:karlt)]

What is the reasoning for making a gfxSparseBitSet character map for each font
entry?  I see this is skipped in font fallback, but why not always skip it?
Reading the CMAP requires opening the font face and file, while the
map in the pattern should be readily available, and I wouldn't have expected
using the pattern map to take significantly longer than a pre-calculated
gfxSparseBitSet.

Perhaps this is related to creating a font entry character map to Union
for the family, so that the whole family can be considered with one test.
However, this seems to be only happen in FindNonItalicFaceForChar(), which uses FindFontForStyle() and so doesn't need to look through every face in the family anyway.

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 8565213 [details] [diff] [review]
patch, part 2 v4 - new fontconfig platform fontlist

As indicated in comment 41, I expect it should be OK to use the approach of
processing one CSS family at a time to generate a list of FamilyFaces, as
indicated in comment 31, so as to use existing code for unicode-range, and
other potential changes.

However, there are a number of other changes proposed in this patch that
involve replacing platform-specific code with different platform-specific
code, and these changes need not be bundled in with the FamilyFaces change.

Rewriting and changing behaviour more than necessary means there is more to
consider and review and get right and slows progress on the rest of the
changes.

The way to make progress and get fast review is to make single complete
changes at each step as described at
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch
and at
https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/#many-small-commits

For the bug, the major single complete change for FamilyFaces may be best done as a switch to a gfxPlatformFontList, which will still be large.  I'm fine with that, but I'm not OK with all the unnecessary changes riding along in the same patch.  It is not fair on reviewers to slip in all these other changes in a
single patch, particularly without a description of what has changed or why.

Much of the new code performs the same task as the old code, but in a
different way, producing different results.  Even if it needs to be moved or
copied with the intention of deleting the original, the first iteration can
perform the same tasks in the same way.  That will be the fastest way to get
support for unicode-range on Linux.  Additional changes in behaviour can be
considered separately.

>+class gfxFontConfigFont : public gfxFT2FontBase {

GetGlyphRenderingOptions() seems to have been lost.

>+gfxFcPlatformFontList::LangGroupToLangString(nsIAtom* aLanguage,
>+                                             nsACString& aLangStr)

This has essentially the same API as
gfxFontconfigUtils::GetSampleLangForGroup() but sometimes returns different
results.

I suggest using gfxFontconfigUtils for now where there are existing methods
and move the code to gfxFcPlatformFontList when gfxPangoFonts.cpp has been
removed.

What the plan is for Qt?  I assume we don't want to keep gfxPangoFonts around
for Qt, but if we do, then sharing code becomes more important.

>+        const double kSkewFactor = OBLIQUE_SKEW_FACTOR;

fontconfig ships with a default of 0.2.
It seems this value is usually tweaked to match the system, but we can keep
the code to use the system settings.

>+gfxFontConfigFontEntry::CreateScaledFont(FcPattern* aRenderPattern,
>+                                         const gfxFontStyle *aStyle)
>+{
>+    if (!mCairoFontFace) {
>+        mCairoFontFace = cairo_ft_font_face_create_for_pattern(aRenderPattern);
>+        if (mFontData) {

Now that the gfxFontEntry maps to a font face rather than a cairo_font_face_t,
it would be better not to reuse the same cairo_font_face_t for different
aRenderPatterns here because the cairo_font_face_t represents a set of
rendering options as well as the font face.  The rendering options may differ
according to size, for example, if there are settings to turn off antialiasing
at certain sizes as WINNT sometimes does.

cairo_ft_font_face_create_for_pattern() will reuse existing objects when the
FT_Face or file and index match, and return the same cairo_font_face_t when
everything matches.

>+    cairo_font_options_t *fontOptions = cairo_font_options_create();
>+
>+    scaledFont = cairo_scaled_font_create(mCairoFontFace,
>+                                          &sizeMatrix,
>+                                          &identityMatrix, fontOptions);

I don't know why the code setting the cairo_font_options_t is being dropped
here, but that seems like something that can be handled separately with
appropriate consideration, rather than folding into this bug.

This may be the cause of some positioning issues I'm seeing with some fonts
(Sazanami Gothic), but I'm not sure.  (It could be one of the other changes.)

>+    nsAutoRef<FcPattern> pattern(FcPatternCreate());
>+    FcPatternAddDouble(pattern, FC_PIXEL_SIZE, aFontStyle->size);
>+
>+    nsAutoRef<FcPattern> renderPattern
>+        (FcFontRenderPrepare(nullptr, pattern, mFontPattern));

Screen font rendering options should be applied to pattern here before calling 
FcFontRenderPrepare().

Not doing FcDefaultSubstitute() means that embeddedbitmap is not set, which
means that some fonts look completely different in Firefox than in other apps.
PRGNAME is also not set, which could make it hard to workaround Gecko's
quirks.

It's hard to guess the consequences from not applying the style like
gfxFontconfigUtils::NewPattern() does, but if only FcDefaultSubstitute() is
performed then one consequence would be that embeddedbitmap is not unset for
synthetic italic.

Cairo's supports using FreeType to apply FC_EMBOLDEN to outlines.  Switching
to drawing two or more times for synthetic bold would be a notable regression.

>+gfxFontConfigFontEntry::gfxFontConfigFontEntry(const nsAString& aFaceName,
>+                                               uint16_t aWeight,
>+                                               int16_t aStretch,
>+                                               bool aItalic,
>+                                               const uint8_t *aData,
>+                                               FT_Face aFace)
>+    : gfxFontEntry(aFaceName), mFontPattern(FcPatternCreate()),
>+      mCairoFontFace(nullptr), mFTFace(aFace), mFTFaceInitialized(true),
>+      mAspect(0.0), mFontData(aData)
>+{
>+    mWeight = aWeight;
>+    mItalic = aItalic;
>+    mStretch = aStretch;
>+    mIsDataUserFont = true;
>+
>+    // Make a new pattern and store the face in it so that cairo uses
>+    // that when creating a cairo font face.
>+    FcPatternAddFTFace(mFontPattern, FC_FT_FACE, mFTFace);

Even if you don't want to keep the FcFreeTypeQueryFace (and I can understand
that), it would be helpful to add the PostScript name at least) to provide
some fontconfig configuation support for downloaded fonts.

There are a number of poor fonts that sites are using and so it is useful to
be able to configure hinting options to the font.

Setting weight would support keeping the nice synthetic bold and slant helps
with synthetic slant.  (I'm assuming there are no special rules around
synthetic variants of @font-face fonts.)

>+    if (FcPatternGetString(aFont, FC_POSTSCRIPT_NAME, 0, &psname) == FcResultMatch) {
>+        aPostscriptName = NS_ConvertUTF8toUTF16((char*)psname);
>+    }

NS_ConvertUTF8toUTF16 is a class, so AppendUTF8toUTF16() to skip the extra
copies.  The method seems to already assume aPostscriptName is empty (in the
cases where GetString fails).  Either assert or Truncate().

>+// helper class taken from gfxFT2FontEntry code

I think you mean FT2FontEntry.

>+#define MAX_FONTCONFIG_SUBSTITUTIONS 5

I suspect this limit may be a bit too small.  There are 8 families aliased in
the Arial group by 30-metric-aliases.conf.  This limit should only be
necessary if something very odd in happening in the configs, and the hash
table look-up should be fast anyway, so make it something large like 20 and
then we don't need to worry about it.

>+    nsAutoRef<FcFontSet> faces(FcFontList(0, familyPat, 0));

The problem with using FcFontList() in this way is that it makes a copy of the
font pattern.  That takes time and space, much of which is due to the
character map.  Particularly when going to hold on to a reference to the
faces, as here, it is better to search through the existing lists of font
patterns.

So that all font patterns need only be searched once, and because families
have already been identified in AddFontSetFamilies(), I would store their font
patterns there, probably creating perhaps partially initialized FontEntrys
there.  That would also make unnecessary the mapping face name to family in
mLocalNames.

>+GetFaceNames(FcPattern* aFont, const nsAString& aFamilyName,
>+             nsAString& aPostscriptName, nsAString& aFullname)

>+        nsAutoString faceName, psname, fullname;
>+        GetFaceNames(face, mName, psname, fullname);

Modern Gecko APIs use the convention that out (or in/out) parameters are
passed by pointer in contrast to in parameters by value or const reference.
The means that readers of the call site can tell which parameters are in or
out, making it easier to follow what the code is doing.

>+        if (!psname.IsEmpty()) {
>+            faceName = psname;
>+        } else {
>+            faceName = fullname;
>+        }

I expect a const reference could save a copy here:

  const nsAutoString& faceName = !psname.IsEmpty() ? psname : fullname;

>+        // Cairo keeps it's own FT_Library object for creating FT_Face
>+        // instances, so use that. There's no simple API for accessing this
>+        // so use the hacky method below of making a font and extracting
>+        // the library pointer from that.

Please keep a comment here explaining the issue with shutdown order addressed
by using this FT_Library.

>+        gfxFontFamily* family =
>+            gfxPlatformFontList::PlatformFontList()->
>+                FindFamily(NS_LITERAL_STRING("serif"), nsGkAtoms::x_western);

GetDefaultFont().

>+    // used for system fonts with explicit patterns
>+    explicit gfxFontConfigFontEntry(const nsAString& aFaceName,

>+    // used for local system fonts with explicit patterns

Can you add something about @font-face or similar to clarify the difference
here, please?

fontconfig is one word so the 'c' should not be capitalized.

>+    virtual ~gfxFontConfigFontFamily() { }

Since bug 1028132 et al, the idea is to make destructors of ref-counted
classes non-public.

>+    virtual gfxFontEntry*
>+    FindFontForPattern(FcPattern *aFontPattern);

I don't see this used.

>+    // initialize font lists
>+    virtual nsresult InitFontList();

>+    virtual gfxFontFamily*
>+    GetDefaultFont(const gfxFontStyle* aStyle);

>+    virtual gfxFontEntry*
>+    LookupLocalFont(const nsAString& aFontName, uint16_t aWeight,
>+                    int16_t aStretch, bool aItalic);

>+    virtual gfxFontEntry*
>+    MakePlatformFont(const nsAString& aFontName, uint16_t aWeight,
>+                     int16_t aStretch, bool aItalic,
>+                     const uint8_t* aFontData,
>+                     uint32_t aLength);

>+    virtual gfxFontFamily* FindFamily(const nsAString& aFamily,
>+                                      nsIAtom* aLanguage = nullptr,
>+                                      bool aUseSystemFonts = false);

Please mark the virtual functions "override".

>+    // FTFace - initialized when needed
>+    FT_Face   mFTFace;

FT_Done_Face seems to only be called for downloaded fonts.

File descriptors are a limited resource and associated FreeType resources are
significant enough that cairo makes an effort to limit the number of faces it
has open concurrently.

The code that managed the font entry's instance of the face seems to have been
lost.

>+++ b/gfx/thebes/gfxFontEntry.h
>+// used as part of font matching
>+#define RANK_MATCHED_CMAP   20

I don't see any new uses of this.

>+    if (sUseFcFontList) {
>+        gfxPlatformFontList::PlatformFontList()->GetFontList(aLangGroup,
>+                                                             aGenericFamily,
>+                                                             aListOfFonts);

Looking at the preferences dialog, this is not allowing users to select the
fontconfig generic families, which are our default values, and what we should
encourage users to use.  It is apparently no longer respecting the aLangGroup
parameter to limit the list to those supporting the language.

The family names are no longer presented in the native language for the locale (when available).  This method is apparently now always preferring the English name.

>+    if (sUseFcFontList) {
>+        gfxPlatformFontList::PlatformFontList()->
>+            GetStandardFamilyName(aFontName, aFamilyName);
>+        return NS_OK;

The dialog says that the serif font for Japanese is DejaVu Serif, even though
that doesn't support the language and the pref is set to "serif".

>+// xxx - this is ubuntu centric, need to go through other distros and flesh
>+// out a more general list

A preference might be handy for a global fallback list, or perhaps use the
existing x-unicode, which seems to be already used for fallback in
GetLangPrefs()

https://hg.mozilla.org/mozilla-central/annotate/5330c6f461a4/gfx/thebes/gfxPlatform.cpp#l1563

>+    // add fonts for CJK ranges
>+    // xxx - this isn't really correct, should use the same CJK font ordering
>+    // as the pref font code
>+    if (aCh >= 0x3000 &&
>+        ((aCh < 0xe000) ||
>+         (aCh >= 0xf900 && aCh < 0xfff0) ||
>+         ((aCh >> 16) == 2))) {
>+        aFontList.AppendElement(kFontTakaoPGothic);
>+        aFontList.AppendElement(kFontDroidSansFallback);
>+        aFontList.AppendElement(kFontWenQuanYiMicroHei);
>+        aFontList.AppendElement(kFontNanumGothic);

Would putting these family names in the font.name-list.*.ja/zh etc prefs make
this code unnecessary?

Comment 46

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (back Apr 16 :karlt) from comment #38)
> The changes in behaviour that will stand out most I think are going to be in
> the UI.  This is because font selection is very dependent on the language
> and so things get most interesting when the language is wrong or missing.
> 
> The worst culprit for failing to get the language right is our own chrome,
> which provides no language.

I'm not really clear on whether this comment applies to the new patches or both these patches and the existing gfxPangoFontGroup implementation. Our chrome code makes use of system font keywords (e.g. "font: menu") so I'm wondering for which locale or combination of Firefox and system locale causes differences between the old and new code. The code in the gtk version of nsLookAndFeel.cpp will do the same thing in either case I suspect:

http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#816

Comment 47

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #46)
> (In reply to Karl Tomlinson (back Apr 16 :karlt) from comment #38)
> > The changes in behaviour that will stand out most I think are going to be in
> > the UI.  This is because font selection is very dependent on the language
> > and so things get most interesting when the language is wrong or missing.
> > 
> > The worst culprit for failing to get the language right is our own chrome,
> > which provides no language.
> 
> I'm not really clear on whether this comment applies to the new patches or
> both these patches and the existing gfxPangoFontGroup implementation.

The second paragraph, providing no language for chrome, applies irrespective
of font selection algorithm.

The difference is in how the font backends deal with the situation of no
language.  gfxPangoFontGroup followed the Pango approach of selecting the font
using a language for the script, while the new patches use the locale.

> Our chrome code makes use of system font keywords (e.g. "font: menu") so I'm
> wondering for which locale or combination of Firefox and system locale
> causes differences between the old and new code. The code in the gtk version
> of nsLookAndFeel.cpp will do the same thing in either case I suspect:
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#816

Yes, that code will do the same thing in either case.

For a GTK default font description of "Sans 10", the font used depends on the
language used in the resolution of "Sans".  To workaround the bug with "Sans'
described in comment 41, set the GTK font to "sans-serif 10".  (gtk-font-name
= "sans-serif 10" in ~/.gtkrc-2.0 might be enough, or the desktop environment
might have a configuration mechanism that overrides this.)

The combination that I was most concerned about was a US/Latin Firefox l10n in
a non-Latin system locale.

Comment 48

[Karl Tomlinson (:karlt)]

Attachment: Nightly UI font

This is how a US l10n in LC_ALL=ja_JP.UTF-8 currently looks.

Comment 49

[Karl Tomlinson (:karlt)]

Attachment: fontlist UI font

This is what a US l10n in LC_ALL=ja_JP.UTF-8 looks like on my system with [e-mail]-d5aeef4ef38e. i.e. using the Mac-port font selection code with gfxFcPlatformFontList.

However, on removing the Sazanami fonts, I instead get TakaoPGothic, which
looks much better.

So whether this is a major issue depends on what fonts are being used.
Perhaps it's something to investigate a bit, see whether I was just unlucky,
whether there is a quick solution, and if not move on.

Comment 50

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (back Apr 16 :karlt) from comment #49)
> Created attachment 8587136 [details]
> fontlist UI font
> 
> This is what a US l10n in LC_ALL=ja_JP.UTF-8 looks like on my system with
> [e-mail]-d5aeef4ef38e. i.e. using the Mac-port font selection
> code with gfxFcPlatformFontList.
> 
> However, on removing the Sazanami fonts, I instead get TakaoPGothic, which
> looks much better.
> 
> So whether this is a major issue depends on what fonts are being used.
> Perhaps it's something to investigate a bit, see whether I was just unlucky,
> whether there is a quick solution, and if not move on.

Karl, what's the distro you're using for these? The menu fonts I see using Ubuntu is specified within the UI code, not within gfx code.

Comment 51

[John Daggett (:jtd)]

Attachment: patch, part 2 v8 - new fontconfig platform fontlist

Updated based on review comments.

Comment 52

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (:karlt) from comment #45)

> Generics are kind of arbitrary in a configuration language. 
> Convention has that sans-serif, serif, and monospace are the generics
> used with fontconfig, but "mono", "sans", and "sans serif" also need
> to be supported.  This is important because the default family for GTK
> is "Sans".

Hmmm, so each one of these need to be treated the same as the magic "sans-serif", "serif" and "monospace" families?

> >+gfxFontFamily*
> >+gfxFcPlatformFontList::FindGenericFamily(const nsAString& aGeneric,
> >+                                         nsIAtom* aLanguage)
> 
> The FcFontSort is finding a font that supports the requested language,
> so it is necessary to pass the language from PrefFontCallbackData.  If
> you make the aLanguage parameter to FindFamily() non-optional, then we
> can see clearly any other places that may need this parameter.
>
> Returning only one family for the generic will cause the issue
> described in
> https://code.google.com/p/chromium/issues/detail?id=403915 for some
> people. To avoid some of the issues described in comment 38 here, some
> people have set a family supporting Latin characters as the first
> preferred family for an eastern language.  The second or subsequent
> preferred family contains support for the eastern language.  If we
> consider only the first family, then the family used for the eastern
> script characters will be somewhat arbitrary, which causes problems
> with languages sharing the same code points (Chinese, Japanese,
> Korean) at least.

Ok, so this needs to return a set of fonts and not just a single one.

> That raises a similar issue with the way that some fonts in a family
> are ignored.  This is likely to be a problem with the Droid Sans
> family on Debian and Fedora where it supports different languages with
> different fonts in the same family.  If some fonts are ignored, then
> the family's support for the particular language can be ignored, and
> this can lead to the same problem of using an arbitrary font or set of
> fonts for the language, or script.

Argh, what madness. Ok, I'll have to specialize the style lookup to deal with the language. So-called "super families" are a really poor way to configure font families, especially when multiple fonts support the same language. Using fc-match, it appears that Debian is configured to choose Droid Sans Japanese over Droid Sans Fallback for lang=ja but this is going to be brittle I think.

> What is the reasoning for making a gfxSparseBitSet character map for
> each font entry?  I see this is skipped in font fallback, but why not
> always skip it? Reading the CMAP requires opening the font face and
> file, while the map in the pattern should be readily available, and I
> wouldn't have expected using the pattern map to take significantly
> longer than a pre-calculated gfxSparseBitSet.

To assure that the cmap used in shaping is the one used for matching. It will only be read when the font is used and not part of general fallback.

> Much of the new code performs the same task as the old code, but in a
> different way, producing different results.  Even if it needs to be
> moved or copied with the intention of deleting the original, the first
> iteration can perform the same tasks in the same way.  That will be
> the fastest way to get support for unicode-range on Linux.  Additional
> changes in behaviour can be considered separately.

I've tried to minimize the changes but the structure of the code is fundamentally different so it was often difficult to figure out how to move the old code into something similar in the new code.

> This has essentially the same API as
> gfxFontconfigUtils::GetSampleLangForGroup() but sometimes returns
> different results.
> 
> I suggest using gfxFontconfigUtils for now where there are existing
> methods and move the code to gfxFcPlatformFontList when
> gfxPangoFonts.cpp has been removed.

This is a static method but it initializes a static that's maintained by gfxFontconfigUtils code. To avoid entangling the new code, I simplified it.

> What the plan is for Qt?  I assume we don't want to keep gfxPangoFonts
> around for Qt, but if we do, then sharing code becomes more important.

I have no plan. ;) It would be simple to move the existing Qt code to the new code but I think until we're ready to obsolete gfxPangoFontGroup I don't think we need to tackle it. If there's someone actively maintaining this code, I can work with them to update the code.

> I don't know why the code setting the cairo_font_options_t is being
> dropped here, but that seems like something that can be handled
> separately with appropriate consideration, rather than folding into
> this bug.
> 
> This may be the cause of some positioning issues I'm seeing with some
> fonts (Sazanami Gothic), but I'm not sure.  (It could be one of the
> other changes.)

What exactly are your referring to here?

> Screen font rendering options should be applied to pattern here before calling 
> FcFontRenderPrepare().
> 
> Not doing FcDefaultSubstitute() means that embeddedbitmap is not set, which
> means that some fonts look completely different in Firefox than in other apps.
> PRGNAME is also not set, which could make it hard to workaround Gecko's
> quirks.

Ok, where/what do I need to add to the pattern? Where do I need to call FcDefaultSubstitute()? I'm sort of mystified what that function does, given the docs...

> It's hard to guess the consequences from not applying the style like
> gfxFontconfigUtils::NewPattern() does, but if only
> FcDefaultSubstitute() is performed then one consequence would be that
> embeddedbitmap is not unset for synthetic italic.
> 
> Cairo's supports using FreeType to apply FC_EMBOLDEN to outlines. 
> Switching to drawing two or more times for synthetic bold would be a
> notable regression.

The new patch deals with disabling embeddedbitmap and enabling embolden appropriately.

> Even if you don't want to keep the FcFreeTypeQueryFace (and I can understand
> that), it would be helpful to add the PostScript name at least) to provide
> some fontconfig configuation support for downloaded fonts.

This really doesn't sound like a feature that's going to work very well. Font services can do all sorts of font optimizations for a font that shares the same postscript name in the font data.

> >+    nsAutoRef<FcFontSet> faces(FcFontList(0, familyPat, 0));
> 
> The problem with using FcFontList() in this way is that it makes a
> copy of the font pattern.  That takes time and space, much of which is
> due to the character map.  Particularly when going to hold on to a
> reference to the faces, as here, it is better to search through the
> existing lists of font patterns.

Hmmm, so you're saying explicitly grabbing the system font sets doesn't make a copy but using FcFontList will? So it would be better to keep around ref's to the set of patterns associated with a family until FindStyleVariations is called?

> >+GetFaceNames(FcPattern* aFont, const nsAString& aFamilyName,
> >+             nsAString& aPostscriptName, nsAString& aFullname)
> 
> >+        nsAutoString faceName, psname, fullname;
> >+        GetFaceNames(face, mName, psname, fullname);
> 
> Modern Gecko APIs use the convention that out (or in/out) parameters are
> passed by pointer in contrast to in parameters by value or const reference.
> The means that readers of the call site can tell which parameters are in or
> out, making it easier to follow what the code is doing.

I know that some Gecko coders prefer this but this isn't actually a Mozilla code style rule. For C++ it's generally better to use references to avoid the need to null-check things at runtime.

> >+        // Cairo keeps it's own FT_Library object for creating FT_Face
> >+        // instances, so use that. There's no simple API for accessing this
> >+        // so use the hacky method below of making a font and extracting
> >+        // the library pointer from that.
> 
> Please keep a comment here explaining the issue with shutdown order addressed
> by using this FT_Library.

Hmmm, not sure what you mean here.

> >+    // FTFace - initialized when needed
> >+    FT_Face   mFTFace;
> 
> FT_Done_Face seems to only be called for downloaded fonts.
> 
> File descriptors are a limited resource and associated FreeType resources are
> significant enough that cairo makes an effort to limit the number of faces it
> has open concurrently.
> 
> The code that managed the font entry's instance of the face seems to have been
> lost.

The face is managed by the user data object associated with the cairo font face. Let me know if you think that's not going to work. The Android code uses something like this.

> >+// xxx - this is ubuntu centric, need to go through other distros and flesh
> >+// out a more general list
> 
> A preference might be handy for a global fallback list, or perhaps use the
> existing x-unicode, which seems to be already used for fallback in
> GetLangPrefs()

This is really an adhoc list based on analyzing what available fonts generally support. The code doesn't rely on these, they are there to simply short-circuit the need to do general system font fallback. Turning this into a pref would be complicated.

The new patch I think addresses all the minor issues. I think there are three additional patches (or follow-up bugs):

1. deal with style selection for "super families" like Droid Sans on Debian
2. eliminate the use of FcFontList in FindStyleVariations
3. enable the mapping of generics to a set of families rather than just one

Comment 53

[John Daggett (:jtd)]

Attachment: patch, part 2 v8a - new fontconfig platform fontlist

Updating to latest trunk code.

Comment 54

[Karl Tomlinson (:karlt)]

Attachment: systemfontlist textrun log

Log from loading
http://people.mozilla.org/~jdaggett/tests/systemfontlist.html
with a build including
https://hg.mozilla.org/users/jdaggett_mozilla.com/workinprogress/file/a4371b548924/new-fontconfig-fontlist-v8b.patch
with NSPR_LOG_MODULES=textrunui:5,textrun:5 in the environment.

Comment 55

[John Daggett (:jtd)]

Attachment: patch, part 2 v9 - new fontconfig platform fontlist

Eliminates the use of FcFontList within in FindStyleVariations. Instead, keep a reference within each family to all patterns associated with that family when enumerating the system font set. When the font is matched or otherwise needed, enumerate these patterns within FindStyleVariations. This eliminates the extra copy associated with FcFontList.

Comment 56

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #52)
> (In reply to Karl Tomlinson (:karlt) from comment #45)
> 
> > Generics are kind of arbitrary in a configuration language. 
> > Convention has that sans-serif, serif, and monospace are the generics
> > used with fontconfig, but "mono", "sans", and "sans serif" also need
> > to be supported.  This is important because the default family for GTK
> > is "Sans".
> 
> Hmmm, so each one of these need to be treated the same as the magic
> "sans-serif", "serif" and "monospace" families?

Yes.  See
http://cgit.freedesktop.org/fontconfig/tree/fonts.conf.in?id=d6a5cc665a1d7e91332944353e92c83ad114368c#n32

> > What the plan is for Qt?  I assume we don't want to keep gfxPangoFonts
> > around for Qt, but if we do, then sharing code becomes more important.
> 
> I have no plan. ;) It would be simple to move the existing Qt code to the
> new code but I think until we're ready to obsolete gfxPangoFontGroup I don't
> think we need to tackle it.

Makes sense.

> > What is the reasoning for making a gfxSparseBitSet character map for
> > each font entry?  I see this is skipped in font fallback, but why not
> > always skip it? Reading the CMAP requires opening the font face and
> > file, while the map in the pattern should be readily available, and I
> > wouldn't have expected using the pattern map to take significantly
> > longer than a pre-calculated gfxSparseBitSet.
> 
> To assure that the cmap used in shaping is the one used for matching. It
> will only be read when the font is used and not part of general fallback.

gfxFT2FontBase ProvidesGlyphWidths() so GetGlyph() is used in preference to
fetching the CMAP in shaping.

The existing code doesn't fill a gfxSparseBitSet.

> > I don't know why the code setting the cairo_font_options_t is being
> > dropped here, but that seems like something that can be handled
> > separately with appropriate consideration, rather than folding into
> > this bug.
> > 
> > This may be the cause of some positioning issues I'm seeing with some
> > fonts (Sazanami Gothic), but I'm not sure.  (It could be one of the
> > other changes.)
> 
> What exactly are your referring to here?

The code mapping from pattern values to cairo_font_options_t in the existing
CreateScaledFont() method.  It begins at
https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/gfx/thebes/gfxPangoFonts.cpp#l1915

> > Screen font rendering options should be applied to pattern here before calling 
> > FcFontRenderPrepare().
> > 
> > Not doing FcDefaultSubstitute() means that embeddedbitmap is not set, which
> > means that some fonts look completely different in Firefox than in other apps.
> > PRGNAME is also not set, which could make it hard to workaround Gecko's
> > quirks.
> 
> Ok, where/what do I need to add to the pattern? Where do I need to call
> FcDefaultSubstitute()? I'm sort of mystified what that function does, given
> the docs...

The documentation of FcFontRenderPrepare() is accurate but not necessarily
helpful in understanding how to use it.  libXft was the benchmark in how to
use fontconfig and other clients have attempted to emulate that.

fc-match is a utility that comes with fontconfig and is also useful as a much
simpler example program than libXft.  However it doesn't read screen config
options, instead taking options from the pattern on the command line.

From the application's perspective, there are two phases of user/system
configuration of the pattern representing the application's desired font
properties.

  phase 1: <match target="pattern"> elements and <match> elements with no
           target specified apply before font selection, and generally are
           there to affect the font selection.

  Screen options and defaults are then applied to the desired pattern.  It is
  then used for font selection and passed together with a selected font
  pattern to FcFontRenderPrepare().  There is merging of some values from the
  desired pattern into the font pattern to produce a new font pattern
  containing any additional config values from screen options or phase 1.

  phase 2: <match target="font"> elements apply to the new font pattern, but
           desired pattern is also provided to support synthetic oblique for
           example.  These elements generally affect rendering options, such
           as hinting, subpixel aa, embeddedbitmap.

In the existing code, phase 1, screen options, and defaults are handled in
PrepareSortPattern().
https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/gfx/thebes/gfxPangoFonts.cpp#l1204
That function name may be confusing because screen options and some defaults
are more relevant to rendering than sorting.

Things probably need to be a little different with the approach used in this
bug as passing the pattern used for font selection through to
CreateFontInstance() may not be practical.

Probably the best overview documentation is at 
http://www.freedesktop.org/software/fontconfig/fontconfig-user.html
and that indicates that the intention was to allow applications to use the
configuration parts of fontconfig without the font selection.

The code in PrepareSortPattern() providing screen options and defaults should
be present in CreateFontInstance().  I think it is reasonable to skip phase 1
(i.e. FcConfigSubstitute(nullptr, aPattern, FcMatchPattern)).  There may be
some teething issues if people have configured rendering options in phase 1,
but this is fixable in the config.  Rendering options in phase 1 generally
don't work well anyway because screen options will override.

> > Even if you don't want to keep the FcFreeTypeQueryFace (and I can understand
> > that), it would be helpful to add the PostScript name at least) to provide
> > some fontconfig configuation support for downloaded fonts.
> 
> This really doesn't sound like a feature that's going to work very well.
> Font services can do all sorts of font optimizations for a font that shares
> the same postscript name in the font data.

I've used fullname successfully to date.
I suggested it postscriptname because I expected it to be easier to implement,
and I don't expect to be more problematic than fullname.

I think this is a minimum effort solution to keep some of the existing
functionality.

> Hmmm, so you're saying explicitly grabbing the system font sets doesn't make
> a copy but using FcFontList will? So it would be better to keep around ref's
> to the set of patterns associated with a family until FindStyleVariations is
> called?

Yes.

> > Modern Gecko APIs use the convention that out (or in/out) parameters are
> > passed by pointer in contrast to in parameters by value or const reference.
> > The means that readers of the call site can tell which parameters are in or
> > out, making it easier to follow what the code is doing.
> 
> I know that some Gecko coders prefer this but this isn't actually a Mozilla
> code style rule. For C++ it's generally better to use references to avoid
> the need to null-check things at runtime.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
says "Use pointers instead of references for function out parameters, even for
primitive types."

FWIW the Google C++ style guide is similar in this respect.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments

> > >+        // Cairo keeps it's own FT_Library object for creating FT_Face
> > >+        // instances, so use that. There's no simple API for accessing this
> > >+        // so use the hacky method below of making a font and extracting
> > >+        // the library pointer from that.
> > 
> > Please keep a comment here explaining the issue with shutdown order addressed
> > by using this FT_Library.
> 
> Hmmm, not sure what you mean here.

https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/gfx/thebes/gfxPangoFonts.cpp#l1665

> > >+    // FTFace - initialized when needed
> > >+    FT_Face   mFTFace;
> > 
> > FT_Done_Face seems to only be called for downloaded fonts.
> > 
> > File descriptors are a limited resource and associated FreeType resources are
> > significant enough that cairo makes an effort to limit the number of faces it
> > has open concurrently.
> > 
> > The code that managed the font entry's instance of the face seems to have been
> > lost.
> 
> The face is managed by the user data object associated with the cairo font
> face. Let me know if you think that's not going to work. The Android code
> uses something like this.

This is a suitable approach for downloaded fonts, and I assume similar to
existing code.

However I'm concerned about the fonts opened from files.
Please use the original code where practical to minimize the changes.

> I think there are
> three additional patches (or follow-up bugs):
> 
> 1. deal with style selection for "super families" like Droid Sans on Debian
> 2. eliminate the use of FcFontList in FindStyleVariations
> 3. enable the mapping of generics to a set of families rather than just one

I think Droid Sans (1) is more important than 3.  3 can be worked around with
preferences.  I don't know of a workaround for 1 and I expect it to show up more often that 3.

Comment 57

[John Daggett (:jtd)]

Attachment: patch, part 2 v10 - new fontconfig platform fontlist

Updated based on review comments.

Comment 58

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #56)

I think I've taken care of most of the issues pointed out above. Below are some comments related to specific issues. I'd like to push to get this landed before the cutoff date for the next uplift, May 11. I think we can handle minor regressions in follow-on bugs, I don't think we need to gate the unicode-range implementation any longer.

> > Hmmm, so each one of these need to be treated the same as the magic
> > "sans-serif", "serif" and "monospace" families?
> 
> Yes.  See

As per our IRC chat, the patch switches the deprecated names into the standard names.

> > > What is the reasoning for making a gfxSparseBitSet character map for
> > > each font entry?  I see this is skipped in font fallback, but why not
> > > always skip it? Reading the CMAP requires opening the font face and
> > > file, while the map in the pattern should be readily available, and I
> > > wouldn't have expected using the pattern map to take significantly
> > > longer than a pre-calculated gfxSparseBitSet.
> > 
> > To assure that the cmap used in shaping is the one used for matching. It
> > will only be read when the font is used and not part of general fallback.
> 
> gfxFT2FontBase ProvidesGlyphWidths() so GetGlyph() is used in preference to
> fetching the CMAP in shaping.
> 
> The existing code doesn't fill a gfxSparseBitSet.

You're right. I switched the implementation of TestCharacterMap to use the pattern charmap for system fonts. Downloadable fonts will still read the cmap from the font data directly.

> > > This may be the cause of some positioning issues I'm seeing with some
> > > fonts (Sazanami Gothic), but I'm not sure.  (It could be one of the
> > > other changes.)
> > 
> > What exactly are your referring to here?
> 
> The code mapping from pattern values to cairo_font_options_t in the existing
> CreateScaledFont() method.  It begins at
> https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/gfx/thebes/
> gfxPangoFonts.cpp#l1915

See PrepareFontOptions. Do we really need the #ifdef FC_HINT_STYLE at this point? Guessing we can delete the else branch there.

> The code in PrepareSortPattern() providing screen options and defaults should
> be present in CreateFontInstance().  I think it is reasonable to skip phase 1
> (i.e. FcConfigSubstitute(nullptr, aPattern, FcMatchPattern)).  There may be
> some teething issues if people have configured rendering options in phase 1,
> but this is fixable in the config.  Rendering options in phase 1 generally
> don't work well anyway because screen options will override.

See PreparePattern. This follows along the lines of PrepareSortPattern but omits fiddling with the adjusted size, as per IRC discussion.

> > > Even if you don't want to keep the FcFreeTypeQueryFace (and I can understand
> > > that), it would be helpful to add the PostScript name at least) to provide
> > > some fontconfig configuation support for downloaded fonts.
> > 
> > This really doesn't sound like a feature that's going to work very well.
> > Font services can do all sorts of font optimizations for a font that shares
> > the same postscript name in the font data.
> 
> I've used fullname successfully to date.
> I suggested it postscriptname because I expected it to be easier to
> implement,
> and I don't expect to be more problematic than fullname.
> 
> I think this is a minimum effort solution to keep some of the existing
> functionality.

This is a *really* specialized use case, specifying hinting overrides for downloadable fonts. As such, I'm highly dubious that this is something even advanced Linux users need/want/actually use. I think we should track this in a separate bug that doesn't gate the unicode-range implementation.

Logged bug 1160510 to deal with this issue.

> > Hmmm, so you're saying explicitly grabbing the system font sets doesn't make
> > a copy but using FcFontList will? So it would be better to keep around ref's
> > to the set of patterns associated with a family until FindStyleVariations is
> > called?
> 
> Yes.

The code in AddFontSetFamilies now stores a reference to the patterns. When font matching occurs, FindStyleVariations iterates over the stored patterns and clears out the patterns after font entry objects have been created.

> > > Modern Gecko APIs use the convention that out (or in/out) parameters are
> > > passed by pointer in contrast to in parameters by value or const reference.
> > > The means that readers of the call site can tell which parameters are in or
> > > out, making it easier to follow what the code is doing.
> > 
> > I know that some Gecko coders prefer this but this isn't actually a Mozilla
> > code style rule. For C++ it's generally better to use references to avoid
> > the need to null-check things at runtime.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
> says "Use pointers instead of references for function out parameters, even
> for
> primitive types."
> 
> FWIW the Google C++ style guide is similar in this respect.
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.
> html#Reference_Arguments

That's listed in the XPCOM, string section which sort of implies it's intended for more general API guidelines rather than internal interfaces. It was also added in 2013 which makes me think it's more something that someone prefers rather than a guideline we generally adhere to. I know the argument for this way of coding but I also know the counter-argument, which is that pointers means you run the risk of dereferencing a null pointer at runtime. There are no null pointer dereferences possible using reference parameters.

Stack overflow thread on this:
http://stackoverflow.com/questions/4028413/out-parameters-and-pass-by-reference

Quote from Herb Sutter and Andrei Alexandrescu's "C++ Coding Standards: 101 Rules, Guidelines and Best Practices":

  Prefer passing by reference if the argument is required and the
  function won't store a pointer to it or otherwise affect its
  ownership. This states that the argument is required and makes the
  caller responsible for providing a valid object.

But for this bug I don't think it's a priority to worry about this kind of thing. I think you're commenting on several methods that are virtual, so there are already other places in gfx code using the same pattern.

If there are specific non-virtual methods you think I should change, point them out and I will change them so we don't need to argue about coding standards.

> > I think there are
> > three additional patches (or follow-up bugs):
> > 
> > 1. deal with style selection for "super families" like Droid Sans on Debian
> > 2. eliminate the use of FcFontList in FindStyleVariations
> > 3. enable the mapping of generics to a set of families rather than just one
> 
> I think Droid Sans (1) is more important than 3.  3 can be worked around with
> preferences.  I don't know of a workaround for 1 and I expect it to show up
> more often that 3.

To enable us to land the patches here and enable unicode-range for the next uplift, I think we should deal with the Debian super family issue in a follow up bug.

Logged bug 1160506 to deal with the Debian super family issue.

Comment 59

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #58)
> See PrepareFontOptions. Do we really need the #ifdef FC_HINT_STYLE at this
> point? Guessing we can delete the else branch there.

Feel free to delete the else branch if you like.
FC_HINT_STYLE is at least 10 years old now.

Comment 60

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #52)
> (In reply to Karl Tomlinson (:karlt) from comment #45)
> 
> > >+// xxx - this is ubuntu centric, need to go through other distros and flesh
> > >+// out a more general list
> > 
> > A preference might be handy for a global fallback list, or perhaps use the
> > existing x-unicode, which seems to be already used for fallback in
> > GetLangPrefs()
> 
> This is really an adhoc list based on analyzing what available fonts
> generally support. The code doesn't rely on these, they are there to simply
> short-circuit the need to do general system font fallback. Turning this into
> a pref would be complicated.

What is the difference in results between putting families in the x-unicode prefs and putting them in GetCommonFallbackFonts()?

(In reply to Karl Tomlinson (ni?:karlt) from comment #45)
> >+    // add fonts for CJK ranges
> >+    // xxx - this isn't really correct, should use the same CJK font ordering
> >+    // as the pref font code
> >+    if (aCh >= 0x3000 &&
> >+        ((aCh < 0xe000) ||
> >+         (aCh >= 0xf900 && aCh < 0xfff0) ||
> >+         ((aCh >> 16) == 2))) {
> >+        aFontList.AppendElement(kFontTakaoPGothic);
> >+        aFontList.AppendElement(kFontDroidSansFallback);
> >+        aFontList.AppendElement(kFontWenQuanYiMicroHei);
> >+        aFontList.AppendElement(kFontNanumGothic);
> 
> Would putting these family names in the font.name-list.*.ja/zh etc prefs make
> this code unnecessary?

Similarly.

Comment 61

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #45)
> >+++ b/gfx/thebes/gfxFontEntry.h
> >+// used as part of font matching
> >+#define RANK_MATCHED_CMAP   20
> 
> I don't see any new uses of this.

Is moving to the header necessary?

>+    // metric for how close a given font matches a style
>+    static int32_t
>+    CalcStyleMatch(gfxFontEntry *aFontEntry, const gfxFontStyle *aStyle);

Similarly.

> >+    if (sUseFcFontList) {
> >+        gfxPlatformFontList::PlatformFontList()->GetFontList(aLangGroup,
> >+                                                             aGenericFamily,
> >+                                                             aListOfFonts);

> The family names are no longer presented in the native language for the
> locale (when available).  This method is apparently now always preferring
> the English name.

Was this addressed?

It may be faster to keep using gfxFontconfigUtils here until this is addressed.

Comment 62

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #42)
> The FcFontSort is finding a font that supports the requested language, so it
> is necessary to pass the language from PrefFontCallbackData.  If you make the
> aLanguage parameter to FindFamily() non-optional, then we can see clearly any
> other places that may need this parameter.

We need this language parameter for WhichPrefFontSupportsChar() to be effective.
(This is independent from the issue of multiple fonts for a generic - I'm not clear whether comment 52 was expecting that it was related.)

Comment 63

[Behdad Esfahbod]

Note that on the Linux desktop we ignore the fontconfig rescan interval and use file monitoring and xsettings to propagate a change notification.  Applications using Gtk+ can subscribe to be notified on the notify::gtk-fontconfig-timestamp signal of GtkSettings object.  At this point, I don't know any system that actually uses the fontconfig rescan interval.

Comment 64

[Behdad Esfahbod]

See here for a discussion of how that work:
http://mces.blogspot.ca/2015/05/how-to-use-custom-application-fonts.html
(towards the end).

Comment 65

[John Daggett (:jtd)]

(In reply to Behdad Esfahbod from comment #63)
> Note that on the Linux desktop we ignore the fontconfig rescan interval and
> use file monitoring and xsettings to propagate a change notification. 
> Applications using Gtk+ can subscribe to be notified on the
> notify::gtk-fontconfig-timestamp signal of GtkSettings object.  At this
> point, I don't know any system that actually uses the fontconfig rescan
> interval.

Thanks Behdad. This pertains to the code in patch 6. Do you have a pointer to sample code that handles the subscription logic? Does this code call out the specific font files added/removed/updated? Or the app just gets notified that "something changed" and then the app checks the FcConfig manually?

Comment 66

[Behdad Esfahbod]

(In reply to John Daggett (:jtd) from comment #65)
> (In reply to Behdad Esfahbod from comment #63)
> > Note that on the Linux desktop we ignore the fontconfig rescan interval and
> > use file monitoring and xsettings to propagate a change notification. 
> > Applications using Gtk+ can subscribe to be notified on the
> > notify::gtk-fontconfig-timestamp signal of GtkSettings object.  At this
> > point, I don't know any system that actually uses the fontconfig rescan
> > interval.
> 
> Thanks Behdad. This pertains to the code in patch 6. Do you have a pointer
> to sample code that handles the subscription logic?

For GTK+-using apps, this is a good start:

  https://git.gnome.org/browse/gtk+/commit/?id=07f4c9c31505f25d1bc5e436a0dd260138171646

> Does this code call out
> the specific font files added/removed/updated? Or the app just gets notified
> that "something changed" and then the app checks the FcConfig manually?

By the time apps get the Gtk+ notification, the "current" FcConfig has already been updated.  Apps also get a style_changed signal, so if they redraw using Gtk+, then everything updates automatically.  But if app has cached results of FcFontList, etc, then you need to redo that upon the signal.  So yes, it's a "something changed" signal because using the fontconfig API directly, there's no way to update for one file added/removed/updated anyway and everything needs to be rebuilt.

Comment 67

[Behdad Esfahbod]

Checking your patch 6, it should be trivial to update it to use the Gtk+ notification.

Comment 68

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #60)
> (In reply to John Daggett (:jtd) from comment #52)
> > (In reply to Karl Tomlinson (:karlt) from comment #45)
> > 
> > > >+// xxx - this is ubuntu centric, need to go through other distros and flesh
> > > >+// out a more general list
> > > 
> > > A preference might be handy for a global fallback list, or perhaps use the
> > > existing x-unicode, which seems to be already used for fallback in
> > > GetLangPrefs()
> > 
> > This is really an adhoc list based on analyzing what available fonts
> > generally support. The code doesn't rely on these, they are there to simply
> > short-circuit the need to do general system font fallback. Turning this into
> > a pref would be complicated.
> 
> What is the difference in results between putting families in the x-unicode
> prefs and putting them in GetCommonFallbackFonts()?

The difference here is that the prefs are dealing with the possibly user-selected default font choice for a particular language. While the idea behind the common fallback fonts are fonts that we already know have some coverage within a given codepoint range. The common fallbacks are simply an attempt to short-circuit a more time-consuming system font fallback process.

So the key distinction is that one is a user preference which hopefully covers fallback cases while the other is a hard-wired attempt to speed up the system font fallback process. I don't think exposing these hard-wired fallback fonts (e.g. within about:config) is a good idea because it's based on an analysis of cmap coverage that few users would understand.

Comment 69

[John Daggett (:jtd)]

(In reply to Behdad Esfahbod from comment #67)
> Checking your patch 6, it should be trivial to update it to use the Gtk+
> notification.

Good. Spun off as bug 1162369 since we should do this for both the current gfxPangoFontGroup code and the newer code here.

Comment 70

[John Daggett (:jtd)]

Attachment: patch, part 2 v11 - new fontconfig platform fontlist

Updated based on review comments.

> > >+    if (sUseFcFontList) {
> > >+        gfxPlatformFontList::PlatformFontList()->GetFontList(aLangGroup,
> > >+                                                             aGenericFamily,
> > >+                                                             aListOfFonts);
> 
> > The family names are no longer presented in the native language for the
> > locale (when available).  This method is apparently now always preferring
> > the English name.
> 
> Was this addressed?
> 
> It may be faster to keep using gfxFontconfigUtils here until this is
> addressed.

For this I took the code from the gfxFontconfigUtils version and tweaked it to work with the code on this bug. The patch here excludes non-scalable faces while current code does not.

The font patterns returned by FcFontList differ from those contained in the FcConfig font sets in at least one subtle way. Font patterns returned by FcFontList are sorted so that family names matching default lang prefs are moved to the first in the list (rather than the pattern simply reflecting the ordering of family names in the font).

Comment 71

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #62)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #42)
> > The FcFontSort is finding a font that supports the requested language, so it
> > is necessary to pass the language from PrefFontCallbackData.  If you make the
> > aLanguage parameter to FindFamily() non-optional, then we can see clearly any
> > other places that may need this parameter.
> 
> We need this language parameter for WhichPrefFontSupportsChar() to be
> effective.
> (This is independent from the issue of multiple fonts for a generic - I'm
> not clear whether comment 52 was expecting that it was related.)

I'll post a separate patch for this and then fold it in after since I don't think this will change the current patch dramatically.

Comment 72

[John Daggett (:jtd)]

Attachment: patch, part 7 - fix assertion in non-italic fallback code

Avoid italic to regular fallback for downloadable fonts. Current logic didn't quite cover all codepaths and this caused reftest assertions.

Comment 73

[John Daggett (:jtd)]

Comment on attachment 8565214 [details] [diff] [review]
patch, part 4 v2 - fix accessibility font-weight code

(In reply to Jonathan Kew (:jfkthame) from comment #35)

> > > ::: accessible/base/TextAttrs.cpp
> > > @@ +636,5 @@
> > > > +
> > > > +  // Under Linux, when gfxPangoFontGroup code is used,
> > > > +  // font->GetStyle()->weight will give the absolute weight requested of the
> > > > +  // font face. The gfxPangoFontGroup code uses the gfxFontEntry constructor
> > > > +  // which doesn't initialize the weight field.
> > > 
> > > Can't we just fix gfxPangoFontGroup so that it *does* initialize the weight
> > > field appropriately, and get rid of all the ugliness here?
> > 
> > We could but I don't think it's worth the effort. The gfxFcFontEntry in the
> > gfxPangoFontGroup code can reference multiple patterns, I don't think it's
> > quite that simple to work out what the true weight is.
> 
> I was assuming we'd simply set it to the requested weight from the
> gfxFontStyle, when creating the entry, so then it'd return the same thing we
> get from the conditionals in TextAttrs.cpp, but without the mess. Doesn't
> that work more simply and cleanly?

That's not really a straight forward process in the case of gfxSystemFcFontEntry objects, which is what the test is using primarily. These are created via patterns that are initialized within gfxFontconfigUtils::NewPattern. The font entry only has the underlying pattern associated with it, so you'd need to backtrack the style/weight/width data out of the pattern. That's possible I guess but at this point I have to say I don't understand why this is needed. The patch is just shifting around the conditionals basically. Once the gfxPangoFontGroup code is obsoleted, the messiness will all go away.

Using font->GetStyle()->weight is what is failing without the changes in this patch. The reason that the current code works is that it's adjusting the style used in the font cache lookup before storing it:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPangoFonts.cpp#1818

The code on other platforms looks up using the gfxFontStyle directly passed in based on computed style values.

So I guess I'd like to ask you to review it again with that in mind.

Comment 74

[Jonathan Kew [:jfkthame]]

Comment on attachment 8565214 [details] [diff] [review]
patch, part 4 v2 - fix accessibility font-weight code

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

OK, I guess we can live with this for the time being.

Comment 75

[John Daggett (:jtd)]

Attachment: patch, part 1a - pass pref lang into FindFamily

Map the pref lang to a langGroup and pass it into FindFamily so that pref lang lookups with fontconfig are language-sensitive.

Comment 76

[John Daggett (:jtd)]

Attachment: patch, part 0 - rename GetFontList under Android

The Android code has an unrelated 'GetFontList' method with a different signature than other platforms. To prevent confusion and name collisions, rename this to GetSystemFontList.

Comment 77

[John Daggett (:jtd)]

tryserver run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcce4676229

The tp numbers for both 32 and 64-bit builds are consistent with existing results.

Comment 78

[Karl Tomlinson (:karlt)]

Comment on attachment 8602499 [details] [diff] [review]
patch, part 2 v11 - new fontconfig platform fontlist

Given some push to get this to release ASAP, I think, with language info in
the recent patch, assuming talos results are good (comment 37), we can enable
this on Nightly to start getting feedback on any as yet unknown issues.

However, a number of followups are required and some regressions depend on
changes to XP code so the new pref should be false when RELEASE_BUILD is
defined.  Flipping the pref for RELEASE_BUILD can be uplifted if/when enough
of the known regression fixes are uplifted.

I haven't had time to actually test the code, but I've identified the issues
I know of and I think this is very close to ready enough for Nightly, but there are still a large enough number of little things remaining that I'm marking r- for another look.

>+// canonical name ==> first en name or first name if no en name
>+static uint32_t
>+FindCanonicalNameIndex(FcPattern* aFont, const char* aLangField)
>+{
>+    uint32_t n = 0, en = 0;
>+    FcChar8* lang;
>+    while (FcPatternGetString(aFont, aLangField, n, &lang) == FcResultMatch) {
>+        // look for 'en' or variants, en-US, en-ja etc.

Can you add another comment explaining the motivation for the en/first
strategy here, please?

Do you mean en-GB or en-JP when you say en-ja?

>+bool
>+gfxFontConfigFontEntry::SupportsLangGroup(nsIAtom *aLangGroup) const {
>+    if (!aLangGroup || aLangGroup == nsGkAtoms::Unicode) {

function '{' on a new line, as elsewhere in this file.

>+gfxFontConfigFontEntry::TestCharacterMap(uint32_t aCh)
>+{
>+    // for system fonts, use the charmap in the pattern
>+    if (!mIsDataUserFont) {
>+        return HasChar(mFontPattern, aCh);
>+    }
>+
>+    // otherwise, read in the cmap
>+    if (!mCharacterMap) {
>+        ReadCMAP();
>+        NS_ASSERTION(mCharacterMap, "failed to initialize character map");
>+    }
>+    if (!mHasCmapTable) {
>+        // no cmap ==> use the charset in the pattern
>+        if (HasChar(mFontPattern, aCh)) {
>+            mCharacterMap->set(aCh);
>+            return true;
>+        }
>+        return false;
>+    }
>+    return mCharacterMap->test(aCh);

The second HasChar always returns false here, so chain up to the base class
when mIsDataUserFont.

>+        // disable embedded bitmaps (mimics behavior in 90-synthetic.conf)
>+        FcPatternAddBool(aRenderPattern, FC_EMBEDDED_BITMAP, FcFalse);

Use FcPatternDel() before adding, which appends to existing values.
There is an existing value from FcDefaultSubstitute().

>+        // for data fonts, add the face/data pointer to the cairo font face
>+        // so that it gets deleted whenever cairo decides
>+        NS_ASSERTION(mFTFace, "FT_Face is null when setting user data");
>+        NS_ASSERTION(mUserFontData, "user font data is null when setting user data");
>+        cairo_font_face_set_user_data(face,
>+                                      &sFcFontlistUserFontDataKey,
>+                                      new FTUserFontDataRef(mUserFontData),
>+                                      FTUserFontDataRef::Destroy);

Perhaps "when cairo is no longer using the FT_Face." instead of
"whenever cairo decides"

This works but could be much simpler.

There is only one (at the most) FTUserFontData per gfxFontconfigFontEntry, so
the gfxFontconfigFontEntry can be referenced and released instead of the
FTUserFontData.

Also the FTUserFontDataRef merely holds a pointer to the ref-counted object.
It's not necessary to allocate a new object just to hold a pointer; AddRef()
can be called before cairo_font_face_set_user_data() and the destroy callback
can Release().

Can be a follow-up if you like.

>+    if (mHasStyles) {

>+    SetHasStyles(true);

No need for both of these.

>+            // Add pointers to other localized family names. Most fonts only have
>+            // a single name, so the first call to GetString will usually not match

Wrap within 80 columns here.

>+        NS_ASSERTION(fontFamily, "font must belong to a font family");
>+        if (fontFamily) {

No need for the if (fontFamily) check.

>+        // map the psname, fullname ==> font family for local font lookups
>+        nsAutoString psname, fullname;
>+        GetFaceNames(font, familyName, psname, fullname);
>+        if (!psname.IsEmpty()) {
>+            ToLowerCase(psname);
>+            mLocalNames.Put(psname, fontFamily);
>+        }
>+        if (!fullname.IsEmpty()) {
>+            ToLowerCase(fullname);
>+            mLocalNames.Put(fullname, fontFamily);
>+        }

AFAIK all fonts have a single postscriptname.
However, this is only considering one of the fullnames.
A follow-up for the other fullnames is fine.

>+gfxFcPlatformFontList::GetStandardFamilyName(const nsAString& aFontName,
>+                                             nsAString& aFamilyName)
>+{
>+    // The fontconfig list of fonts includes generic family names in the
>+    // font list. For these, just use the generic name.
>+    if (aFontName.EqualsLiteral("serif") ||
>+        aFontName.EqualsLiteral("sans-serif") ||
>+        aFontName.EqualsLiteral("monospace")) {
>+        aFamilyName.Assign(aFontName);
>+        return true;
>+    }
>+
>+    gfxFontFamily *family = FindFamily(aFontName);
>+    if (family) {
>+        family->LocalizedName(aFamilyName);
>+        return true;
>+    }
>+
>+    return false;

LocalizedName() is not localized AFAICS.

A followup is OK.

>+#define MAX_FONTCONFIG_SUBSTITUTIONS 20

unused now.

>+    // fontconfig allows conditional substitutions in such a way that it's
>+    // difficult to distinguish an explicit substitution from other suggested
>+    // choices. To sniff out explicit substitutions, compare the substitutions
>+    // for "font, serif" to "serif", using the first font in the list of
>+    // serif substitutions as a sentinel.
>+    //
>+    // Example:
>+    //
>+    //   serif ==> DejaVu Serif, ...
>+    //   Helvetica, serif ==> Helvetica, TeX Gyre Heros, Nimbus Sans L, DejaVu Serif
>+    //
>+    // In this case fontconfig is including Tex Gyre Heros and
>+    // Nimbus Sans L as alternatives for Helvetica.

This could do with some updating for the recent changes.

>+        if (sentinelFirstFamily && FcStrCmp(substName, sentinelFirstFamily) == 0) {
>+            break;
>+        }

I think this will work better thanks.

What I meant was FcStrCmp(substName, kSentinelName) so that the first
FcConfigSubstitute() is unnecessary, but that can be a later optimization.

>+    if (FT_Err_Ok != FT_Select_Charmap(face, FT_ENCODING_UNICODE)) {
>+        FT_Done_Face(face);
>+        NS_Free((void*)aFontData);
>+        return nullptr;
>+    }

This isn't in the existing code.  The change looks good, but please document
in the commit message, or preferably do this in a separate patch.

>+    bool SupportsLangGroup(nsIAtom *aLangGroup) const override;

I guess you are dropping the "virtual" here and elsewhere, in anticipation of
Ehsan's style change proposal.  It seems that doesn't have universal support
at this point.  Currently the style guide says virtual and override.  FWIW, I
missed that this is virtual because I looked at the beginning of the line.

>+    double GetAspect();

Please mark this non-public.

Please move the declarations of classes that are not intended to be public out
of the header file, into the cpp file.
i.e. gfxFontconfigFontEntry, gfxFontconfigFontFamily, gfxFontconfigFont,

>+    // Lookup family name in global family list without substitutions or
>+    // localized family name lookup. Used for common font fallback families.
>+    gfxFontFamily* FindFamilyInFamilyList(const nsAString& aFamily) {

I suggest s/FindFamilyInFamilyList/FindFamilyByCanonicalName/ if that is
accurate, but i think jfkthame kinda reviewed that; your choice.

(In reply to John Daggett (:jtd) from comment #52)
> > This has essentially the same API as
> > gfxFontconfigUtils::GetSampleLangForGroup() but sometimes returns
> > different results.
> > 
> > I suggest using gfxFontconfigUtils for now where there are existing
> > methods and move the code to gfxFcPlatformFontList when
> > gfxPangoFonts.cpp has been removed.
> 
> This is a static method but it initializes a static that's maintained by
> gfxFontconfigUtils code. To avoid entangling the new code, I simplified it.

The simplification has removed the key code and kept just the fallback code.
AFAIK it is still better to use the user's language so please keep that code.

If the static you refer to is gLangService, then that is OK as long as the
static Shutdown() method is called.

If using the existing method is not practical then this may be best as a
follow-up.

The "LangGroupToLangString" name implies the conversion is well defined, but it is not, so please keep a name such as "GetSampleLangForGroup" to indicate the
imperfection here.

(In reply to Karl Tomlinson (ni?:karlt) from comment #27)
> The checkin comment I think could and should
> give a more obvious indication that it is replacing the font selection code.

Though, if you move switching the pref to another patch, then that change can
be noted in that patch.

(In reply to Karl Tomlinson (ni?:karlt) from comment #41)
> >+    // generics? use fontconfig to determine the family for the given lang
> >+    if (IsGeneric(aFamily)) {
> >+        return FindGenericFamily(aFamily, aLanguage);
> >+    }
> 
> Can you add a comment or rename IsGeneric(), please, to clarify that this is
> testing for fontconfig generics.  (CSS generics have already been resolved.)

Please make this "// fontconfig generics? [...]"
to avoid confusion because FontFamilyName::IsGeneric() is usually CSS generics.

> The ToFcChar8() and ToCString() safe cast utilities in gfxFontconfigUtils are
> useful here and elsewhere to type check all the reinterpret casts.

Much of the code here is transplanted with these replaced with C style
reinterpret_casts.  Please add some equivalent static methods if you want to
avoid gfxFontconfigUtils.

(In reply to Karl Tomlinson (ni?:karlt) from comment #45)

> >+    if (FcPatternGetString(aFont, FC_POSTSCRIPT_NAME, 0, &psname) == FcResultMatch) {
> >+        aPostscriptName = NS_ConvertUTF8toUTF16((char*)psname);
> >+    }
> 
> NS_ConvertUTF8toUTF16 is a class, so AppendUTF8toUTF16() to skip the extra
> copies.  The method seems to already assume aPostscriptName is empty (in the
> cases where GetString fails).  Either assert or Truncate().

Some other places too.

> So that all font patterns need only be searched once, and because families
> have already been identified in AddFontSetFamilies(), I would store their
> font
> patterns there, probably creating perhaps partially initialized FontEntrys
> there.  That would also make unnecessary the mapping face name to family in
> mLocalNames.

The approach you ended up with here was a little different but good, thanks.

However, this still leaves the lookup by family in LookupLocalFont().  It
works but is a bit convoluted.

Here face names are linked to family names in mLocalNames, and that is used to
look up the gfxFontFamily, to make all gfxFontEntrys in the family, to look up the gfxFontEntry by face name from another hash table, to get the pattern from the font entry to make a new gfxFontEntry.

Instead, mLocalNames can link the face names to patterns, which can be used
directly to make the gfxFontEntry, like the existing code.  The single hash
table is sufficient.

A followup issue.

>+    // if not found, ask fontconfig to pick the appropriate font
>+    if (!genericFamily) {

An early return here when genericFamily is found would save a level of indent
for most of this function.

>+        FcChar8* mappedGeneric = nullptr;
>+        for (int i = 0; i < faces->nfont; i++) {
>+            FcPattern* font = faces->fonts[i];
>+
>+            // not scalable? skip...
>+            FcBool scalable;
>+            if (FcPatternGetBool(font, FC_SCALABLE, 0, &scalable) != FcResultMatch ||
>+                !scalable) {
>+                continue;
>+            }
>+
>+            FcPatternGetString(font, FC_FAMILY, 0, &mappedGeneric);

Can you move the declaration and initialization of mappedGeneric inside the
loop, please?  That clarifies that there is no intent to use a value from a
previous iteration.

> >+    // used for system fonts with explicit patterns
> >+    explicit gfxFontConfigFontEntry(const nsAString& aFaceName,

> fontconfig is one word so the 'c' should not be capitalized.

(In reply to Karl Tomlinson (ni?:karlt) from comment #42)
> Currently family fonts are sorted only by mStandardFace but that is not
> used with gfxFontConfigFontEntry IIUC

Please remove the call to SortAvailableFonts() as that doesn't do anything
AFAICS.

Comment 79

[Karl Tomlinson (:karlt)]

Comment on attachment 8603010 [details] [diff] [review]
patch, part 1a - pass pref lang into FindFamily

>+// this needs to match the list of pref font.default.xx entries listed in all.js!
>+// the order *must* match the order in eFontPrefLang

This is precarious.
Better to use a case statement, and the compiler can optimize.

If you prefer to do that in a followup, then please add a similar comment at
the eFontPrefLang definition for now.

Please don't fold in this patch as it makes sense on its own.

Comment 80

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> >+    gfxFontFamily* FindFamilyInFamilyList(const nsAString& aFamily) {
> 
> I suggest s/FindFamilyInFamilyList/FindFamilyByCanonicalName/ if that is
> accurate, but i think jfkthame kinda reviewed that; your choice.

I think I like Karl's suggestion, fwiw.

Comment 81

[Karl Tomlinson (:karlt)]

Comment on attachment 8603010 [details] [diff] [review]
patch, part 1a - pass pref lang into FindFamily

>+        nsIAtom* lang = nullptr;
>+        lang = gfxPlatform::GetLangGroupForPrefLang(aLang);

Please remove the nullptr and initialize at declaration.

Comment 82

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> >+    if (FT_Err_Ok != FT_Select_Charmap(face, FT_ENCODING_UNICODE)) {
> >+        FT_Done_Face(face);
> >+        NS_Free((void*)aFontData);
> >+        return nullptr;
> >+    }
> 
> This isn't in the existing code.  The change looks good, but please document
> in the commit message, or preferably do this in a separate patch.

Actually "When a new face is created (either through FT_New_Face or FT_Open_Face), the library looks for a Unicode charmap within the list and automatically activates it." so this should not be necessary.  Please leave out this new code.

http://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_CharMap

Comment 83

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #82)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> > >+    if (FT_Err_Ok != FT_Select_Charmap(face, FT_ENCODING_UNICODE)) {
> > >+        FT_Done_Face(face);
> > >+        NS_Free((void*)aFontData);
> > >+        return nullptr;
> > >+    }
> > 
> > This isn't in the existing code.  The change looks good, but please document
> > in the commit message, or preferably do this in a separate patch.
> 
> Actually "When a new face is created (either through FT_New_Face or
> FT_Open_Face), the library looks for a Unicode charmap within the list and
> automatically activates it." so this should not be necessary.  Please leave
> out this new code.

Isn't this guarding against the possibility that the font has *no* Unicode charmap available, in which case we probably want to consider it unusable?

Comment 84

[Karl Tomlinson (:karlt)]

(In reply to Jonathan Kew (:jfkthame) from comment #83)
> Isn't this guarding against the possibility that the font has *no* Unicode
> charmap available, in which case we probably want to consider it unusable?

Ah, thanks.  Sounds good.

Comment 85

[John Daggett (:jtd)]

Attachment: patch, part 2 v12 - new fontconfig platform fontlist

Updated based on review comments.

(In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> Comment on attachment 8602499 [details] [diff] [review]
> patch, part 2 v11 - new fontconfig platform fontlist

> Also the FTUserFontDataRef merely holds a pointer to the ref-counted object.
> It's not necessary to allocate a new object just to hold a pointer; AddRef()
> can be called before cairo_font_face_set_user_data() and the destroy callback
> can Release().
> 
> Can be a follow-up if you like.

Will file follow-up bug.

> >+        // map the psname, fullname ==> font family for local font lookups
> >+        nsAutoString psname, fullname;
> >+        GetFaceNames(font, familyName, psname, fullname);
> >+        if (!psname.IsEmpty()) {
> >+            ToLowerCase(psname);
> >+            mLocalNames.Put(psname, fontFamily);
> >+        }
> >+        if (!fullname.IsEmpty()) {
> >+            ToLowerCase(fullname);
> >+            mLocalNames.Put(fullname, fontFamily);
> >+        }
> 
> AFAIK all fonts have a single postscriptname.
> However, this is only considering one of the fullnames.
> A follow-up for the other fullnames is fine.

No, fullname lookup only occurs on the english name (or first name if no english name available). Names from other locales are *not* used for fullname lookups.

> LocalizedName() is not localized AFAICS.
> 
> A followup is OK.

This isn't needed in this case because the use of FcFontList assures that the localized name is the family name. The logic here simply follows the logic used on other platforms.

> >+    if (FT_Err_Ok != FT_Select_Charmap(face, FT_ENCODING_UNICODE)) {
> >+        FT_Done_Face(face);
> >+        NS_Free((void*)aFontData);
> >+        return nullptr;
> >+    }
> 
> This isn't in the existing code.  The change looks good, but please document
> in the commit message, or preferably do this in a separate patch.

This is from the gfxFT2FontList code:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2FontList.cpp#270

This is rejecting fonts without Unicode cmaps.

> Please move the declarations of classes that are not intended to be public
> out
> of the header file, into the cpp file.
> i.e. gfxFontconfigFontEntry, gfxFontconfigFontFamily, gfxFontconfigFont,

This is inconsistent with other code in other platform fontlist code. So I'd prefer not to make this change. Or do it across all classes, not just the code in this patch.

> > > I suggest using gfxFontconfigUtils for now where there are existing
> > > methods and move the code to gfxFcPlatformFontList when
> > > gfxPangoFonts.cpp has been removed.
> > 
> > This is a static method but it initializes a static that's maintained by
> > gfxFontconfigUtils code. To avoid entangling the new code, I simplified it.
> 
> The simplification has removed the key code and kept just the fallback code.
> AFAIK it is still better to use the user's language so please keep that code.
> 
> If the static you refer to is gLangService, then that is OK as long as the
> static Shutdown() method is called.
> 
> If using the existing method is not practical then this may be best as a
> follow-up.

Will file follow-up for this.

> Here face names are linked to family names in mLocalNames, and that is used
> to
> look up the gfxFontFamily, to make all gfxFontEntrys in the family, to look
> up the gfxFontEntry by face name from another hash table, to get the pattern
> from the font entry to make a new gfxFontEntry.
> 
> Instead, mLocalNames can link the face names to patterns, which can be used
> directly to make the gfxFontEntry, like the existing code.  The single hash
> table is sufficient.
> 
> A followup issue.

Will file follow-up for this.

> > >+    // used for system fonts with explicit patterns
> > >+    explicit gfxFontConfigFontEntry(const nsAString& aFaceName,
> 
> > fontconfig is one word so the 'c' should not be capitalized.

Changed in a separate patch to avoid disrupting patch queue.

> >+    bool SupportsLangGroup(nsIAtom *aLangGroup) const override;
> 
> I guess you are dropping the "virtual" here and elsewhere, in anticipation of
> Ehsan's style change proposal.  It seems that doesn't have universal support
> at this point.  Currently the style guide says virtual and override.  FWIW, I
> missed that this is virtual because I looked at the beginning of the line.

Um, really? This doesn't seem like a necessary change and I don't think at this point it's really a good one either.

Comment 86

[John Daggett (:jtd)]

Attachment: patch, part 8 - change class name capitalization

Changed gfxFontConfig ==> gfxFontconfig

Comment 87

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #81)
> Comment on attachment 8603010 [details] [diff] [review]
> patch, part 1a - pass pref lang into FindFamily
> 
> >+        nsIAtom* lang = nullptr;
> >+        lang = gfxPlatform::GetLangGroupForPrefLang(aLang);
> 
> Please remove the nullptr and initialize at declaration.

Will do.

Comment 88

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #79)
> Comment on attachment 8603010 [details] [diff] [review]
> patch, part 1a - pass pref lang into FindFamily
> 
> >+// this needs to match the list of pref font.default.xx entries listed in all.js!
> >+// the order *must* match the order in eFontPrefLang
> 
> This is precarious.
> Better to use a case statement, and the compiler can optimize.
> 
> If you prefer to do that in a followup, then please add a similar comment at
> the eFontPrefLang definition for now.
> 
> Please don't fold in this patch as it makes sense on its own.

Will file follow-up for this.

Comment 89

[Karl Tomlinson (:karlt)]

Comment on attachment 8603930 [details] [diff] [review]
patch, part 2 v12 - new fontconfig platform fontlist

If you are trying to get this in tonight, then perhaps jfkthame may be able to
tick off the remaining issues from the diff.

 +    FcChar8* lastFamilyName = (FcChar8*)"";
 +    gfxFontFamily* fontFamily = nullptr;
-+    nsAutoString familyName;
 +    for (int f = 0; f < aFontSet->nfont; f++) {
 +        FcPattern* font = aFontSet->fonts[f];

 +        // same as the last one? no need to add a new family, skip
++        nsAutoString familyName;
 +        if (FcStrCmp(canonical, lastFamilyName) != 0) {
 +            lastFamilyName = canonical;
 +
 +            // add new family if one doesn't already exist
-+            familyName = NS_ConvertUTF8toUTF16((char*)canonical);
++            AppendUTF8toUTF16(ToCharPtr(canonical), familyName);

Need to leave the familyName declaration where it was because familyName is
used below even when canonical is the same as the last one.

A familyName.Truncate() before AppendUTF8toUTF16() will make that work.

(In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> Given some push to get this to release ASAP, I think, with language info in
> the recent patch, assuming talos results are good (comment 37), we can enable
> this on Nightly to start getting feedback on any as yet unknown issues.

Are you able to link to the results for the other talos tests, please?
(i.e. not just Tp.)

> However, a number of followups are required and some regressions depend on
> changes to XP code so the new pref should be false when RELEASE_BUILD is
> defined.  Flipping the pref for RELEASE_BUILD can be uplifted if/when enough
> of the known regression fixes are uplifted.

+#ifdef MOZ_WIDGET_GTK
+pref("gfx.font_rendering.fontconfig.fontlist.enabled", true);
+#endif

Make this conditional on !defined(RELEASE_BUILD) please.

We're landing this earlier on Nightly than we would under normal procedures,
but I'm not yet ready to make that concession for release.

dbaron can override me if he disagrees, so feel free to put this in a separate
patch and ask dbaron for review if you like.

> >+// canonical name ==> first en name or first name if no en name
> >+static uint32_t
> >+FindCanonicalNameIndex(FcPattern* aFont, const char* aLangField)
> >+{
> >+    uint32_t n = 0, en = 0;
> >+    FcChar8* lang;
> >+    while (FcPatternGetString(aFont, aLangField, n, &lang) == FcResultMatch) {
> >+        // look for 'en' or variants, en-US, en-ja etc.
> 
> Can you add another comment explaining the motivation for the en/first
> strategy here, please?
> 
> Do you mean en-GB or en-JP when you say en-ja?
> 
> >+bool
> >+gfxFontConfigFontEntry::SupportsLangGroup(nsIAtom *aLangGroup) const {
> >+    if (!aLangGroup || aLangGroup == nsGkAtoms::Unicode) {
> 
> function '{' on a new line, as elsewhere in this file.

> >+        // disable embedded bitmaps (mimics behavior in 90-synthetic.conf)
> >+        FcPatternAddBool(aRenderPattern, FC_EMBEDDED_BITMAP, FcFalse);
> 
> Use FcPatternDel() before adding, which appends to existing values.
> There is an existing value from FcDefaultSubstitute().

> (In reply to Karl Tomlinson (ni?:karlt) from comment #27)
> > The checkin comment I think could and should
> > give a more obvious indication that it is replacing the font selection code.

A few things missed here.

The commit message can't be changed in a follow-up so please address this.

> Though, if you move switching the pref to another patch, then that change can
> be noted in that patch.

(In reply to John Daggett (:jtd) from comment #85)
> > AFAIK all fonts have a single postscriptname.
> > However, this is only considering one of the fullnames.
> > A follow-up for the other fullnames is fine.
> 
> No, fullname lookup only occurs on the english name (or first name if no
> english name available). Names from other locales are *not* used for
> fullname lookups.

OK.  This is a change of behavior on this platform so please document in the
commit message.

> > This isn't in the existing code.  The change looks good, but please document
> > in the commit message, or preferably do this in a separate patch.
> 
> This is from the gfxFT2FontList code:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2FontList.
> cpp#270
> 
> This is rejecting fonts without Unicode cmaps.

That's good thanks.  Please update the commit message because this is a change
in behavior for this platform.

Comment 90

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #85)
> > LocalizedName() is not localized AFAICS.
> > 
> > A followup is OK.
> 
> This isn't needed in this case because the use of FcFontList assures that
> the localized name is the family name. The logic here simply follows the
> logic used on other platforms.

FcFontList is used only in GetSystemFontList().

LocalizedName() uses the canonical mName, which does not come from
GetSystemFontList().

gfxMacFontFamily uses localizedNameForFamily, and gfxDWriteFontFamily seems to
have some special code.

> > Please move the declarations of classes that are not intended to be public
> > out
> > of the header file, into the cpp file.
> > i.e. gfxFontconfigFontEntry, gfxFontconfigFontFamily, gfxFontconfigFont,
> 
> This is inconsistent with other code in other platform fontlist code. So I'd
> prefer not to make this change. Or do it across all classes, not just the
> code in this patch.

gfxMacFontFamily is not exposed in gfxMacPlatformFontList.h

Whether to put them in the header files depends on whether any platform-specific
methods will be required elsewhere, but moving them is not urgent.

Comment 91

[John Daggett (:jtd)]

Filed follow-up bugs.

Bug 1163482 - eliminate FTUserFontDataRef in gfxFcPlatformFontList.cpp
Bug 1163487 - determine whether LANGUAGE value should affect font selection within fontconfig font lookup code
Bug 1163488 - replace pref lang lookup arrays with switch statements
Bug 1163491 - simplify local fontname lookup within gfxFcPlatformFontList code

Comment 92

[John Daggett (:jtd)]

Attachment: patch, part 2 v13 - new fontconfig platform fontlist

(In reply to Karl Tomlinson (ni?:karlt) from comment #89)

Updated based on review comments. I added a commit comment that notes the various changes from existing code.

> Comment on attachment 8603930 [details] [diff] [review]
> patch, part 2 v12 - new fontconfig platform fontlist

> > However, a number of followups are required and some regressions depend on
> > changes to XP code so the new pref should be false when RELEASE_BUILD is
> > defined.  Flipping the pref for RELEASE_BUILD can be uplifted if/when enough
> > of the known regression fixes are uplifted.
> 
> +#ifdef MOZ_WIDGET_GTK
> +pref("gfx.font_rendering.fontconfig.fontlist.enabled", true);
> +#endif
> 
> Make this conditional on !defined(RELEASE_BUILD) please.

Ok, changed. I'm not really happy about delaying this further but we can discuss this after the patch lands.

Comment 93

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #90)
> (In reply to John Daggett (:jtd) from comment #85)
> > > LocalizedName() is not localized AFAICS.
> > > 
> > > A followup is OK.
> > 
> > This isn't needed in this case because the use of FcFontList assures that
> > the localized name is the family name. The logic here simply follows the
> > logic used on other platforms.
> 
> FcFontList is used only in GetSystemFontList().
> 
> LocalizedName() uses the canonical mName, which does not come from
> GetSystemFontList().

Ok, will file a follow-up bug.

Comment 94

[John Daggett (:jtd)]

tryserver run, including full talos set
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8579764be1c3

Comment 95

[Karl Tomlinson (:karlt)]

Comment on attachment 8603967 [details] [diff] [review]
patch, part 2 v13 - new fontconfig platform fontlist

Thanks.  I wonder whether your mail client is hiding some of the review
comments.  Please address these here or in followups:

(In reply to Karl Tomlinson (ni?:karlt) from comment #78)
> >+// canonical name ==> first en name or first name if no en name
> >+static uint32_t
> >+FindCanonicalNameIndex(FcPattern* aFont, const char* aLangField)
> >+{
> >+    uint32_t n = 0, en = 0;
> >+    FcChar8* lang;
> >+    while (FcPatternGetString(aFont, aLangField, n, &lang) == FcResultMatch) {
> >+        // look for 'en' or variants, en-US, en-ja etc.
> 
> Can you add another comment explaining the motivation for the en/first
> strategy here, please?
> 
> Do you mean en-GB or en-JP when you say en-ja?
> 
> >+bool
> >+gfxFontConfigFontEntry::SupportsLangGroup(nsIAtom *aLangGroup) const {
> >+    if (!aLangGroup || aLangGroup == nsGkAtoms::Unicode) {
> 
> function '{' on a new line, as elsewhere in this file.

> >+        // disable embedded bitmaps (mimics behavior in 90-synthetic.conf)
> >+        FcPatternAddBool(aRenderPattern, FC_EMBEDDED_BITMAP, FcFalse);
> 
> Use FcPatternDel() before adding, which appends to existing values.
> There is an existing value from FcDefaultSubstitute().

Comment 96

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8579764be1c3&newProject=mozilla-central&newRevision=c50a3514dd86

Comment 97

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=c50a3514dd86&newProject=try&newRevision=8579764be1c3
(comment 96 had old and new swapped)

perf.snarkfest.net has some memory figures not in treeherder, and they look good.
This comparison doesn't know which is better (higher or lower) for some tests.
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=5e8adf0e7f2c&newRev=8579764be1c3&submit=true

Comment 98

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #97)
> This comparison doesn't know which is better (higher or lower) for some
> tests.

This "old" revision avoids the confusion of pgo results:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=863caf540d9a&newRev=8579764be1c3&submit=true

Comment 99

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #95)
> Comment on attachment 8603967 [details] [diff] [review]
> patch, part 2 v13 - new fontconfig platform fontlist
> 
> Thanks.  I wonder whether your mail client is hiding some of the review
> comments.  Please address these here or in followups:

Sorry, I did those edits but somehow they I must have backed out the changes accidently. I'll go through an confirm the full set of edits from comment 78 again before landing.

Comment 100

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d74ae8fcaac9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a651c240979d
https://hg.mozilla.org/integration/mozilla-inbound/rev/408e078cc18d
https://hg.mozilla.org/integration/mozilla-inbound/rev/130bea3f8623
https://hg.mozilla.org/integration/mozilla-inbound/rev/5260b93eb0b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bd67204fac
https://hg.mozilla.org/integration/mozilla-inbound/rev/0933391809c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b35e8a463d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc65b714dd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b1f2b78915

Comment 101

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9768393&repo=mozilla-inbound

Comment 102

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6127b1d28cb5

Comment 103

[John Daggett (:jtd)]

Attachment: patch, part 9 - fix build bustage

Fix up the build bustage on B2G Linux and Linux static

Comment 104

[Brian Birtles (:birtles)]

Comment on attachment 8604522 [details] [diff] [review]
patch, part 9 - fix build bustage

r=me with the second explicit removed as discussed on IRC

Comment 105

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cff5c0c09e72
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd3335d77d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72465a4c424
https://hg.mozilla.org/integration/mozilla-inbound/rev/1223b73d62dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/194a4c39c2a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a517a5188e1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/51287191cc06
https://hg.mozilla.org/integration/mozilla-inbound/rev/f630169085c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18546e7f271
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8fca346b1ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858d2530520

Comment 106

[Carsten Book [:Tomcat]]

sorry had to back this out again seems this broke the mulet and  B2G Desktop Linux x64 opt  test suites with errors like https://treeherder.mozilla.org/logviewer.html#?job_id=9775894&repo=mozilla-inbound

Comment 107

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/194bf0d7b9aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e007625001
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b53114ec9d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b243afafbb75
https://hg.mozilla.org/integration/mozilla-inbound/rev/1911efabecd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f0d9ea3c59
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee8af61e5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc04168ea51e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba5b3c2a1e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b376f7182133
https://hg.mozilla.org/integration/mozilla-inbound/rev/70244d662f4f

Comment 108

[John Daggett (:jtd)]

Attachment: patch, part 10 - activate bundled fonts

The B2G Desktop platform builds with bundled fonts enabled. The new fontconfig platform fontlist code wasn't properly activating bundled fonts which produced gaia integration test failures.

Comment 109

[John Daggett (:jtd)]

Attachment: patch, part 11 - annotate mulet reftests that fail with fontconfig fontlist

There are several tests that fail on Mulet Linux that were already marked as failing for B2G. So simply include Mulet in the list of annotated platforms.

Comment 110

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87c080e0543b
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c404060d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6ce93ef752
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cc66ab1814
https://hg.mozilla.org/integration/mozilla-inbound/rev/c524ab1d666e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25a0c92e943
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea4df703d98a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc9ce0ef5bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a747351d2e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ae0abd22dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2269fe4c20
https://hg.mozilla.org/integration/mozilla-inbound/rev/9067a3d5eb20
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d25651b082

Comment 111

[Daniel Holbert [:dholbert]]

Comment on attachment 8603967 [details] [diff] [review]
patch, part 2 v13 - new fontconfig platform fontlist

>+++ b/gfx/thebes/gfxPlatformGtk.h
[...]
>     virtual nsresult UpdateFontList() override;
> 
>+    virtual void
>+    GetCommonFallbackFonts(uint32_t aCh, uint32_t aNextCh,
>+                           int32_t aRunScript,
>+                           nsTArray<const char*>& aFontList) override;
>+
>+    virtual gfxPlatformFontList* CreatePlatformFontList();

This ^^ was missing an 'override' annotation, which causes clang 3.6 &
newer to spam a "-Winconsistent-missing-override" build warning, which
busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/7da7fb5c7d38

Comment 112

[John Daggett (:jtd)]

(In reply to Daniel Holbert [:dholbert] from comment #111)

> This ^^ was missing an 'override' annotation, which causes clang 3.6 &
> newer to spam a "-Winconsistent-missing-override" build warning, which
> busts --enable-warnings-as-errors builds (with clang 3.6+).
> 
> I pushed a followup to add that keyword, with blanket r+ that ehsan
> granted me for fixes of this sort over in bug 1126447 comment 2:
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/7da7fb5c7d38

Thanks Daniel!

Comment 113

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87c080e0543b
https://hg.mozilla.org/mozilla-central/rev/79c404060d67
https://hg.mozilla.org/mozilla-central/rev/0e6ce93ef752
https://hg.mozilla.org/mozilla-central/rev/c0cc66ab1814
https://hg.mozilla.org/mozilla-central/rev/c524ab1d666e
https://hg.mozilla.org/mozilla-central/rev/f25a0c92e943
https://hg.mozilla.org/mozilla-central/rev/ea4df703d98a
https://hg.mozilla.org/mozilla-central/rev/1bc9ce0ef5bb
https://hg.mozilla.org/mozilla-central/rev/5a747351d2e1
https://hg.mozilla.org/mozilla-central/rev/93ae0abd22dd
https://hg.mozilla.org/mozilla-central/rev/ab2269fe4c20
https://hg.mozilla.org/mozilla-central/rev/9067a3d5eb20
https://hg.mozilla.org/mozilla-central/rev/d6d25651b082
https://hg.mozilla.org/mozilla-central/rev/7da7fb5c7d38

Comment 114

[Nicholas Nethercote [inactive]]

I think one of these patches added a new static constructor. I'm currently trying to reduce the number of static constructors because they hurt Fennec start-up time. See http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1431390993000,1431563793000&displayrange=7&datatype=geo. The good news is that static constructors are often easy to fix, e.g. by using a singleton.

Comment 115

[John Daggett (:jtd)]

(In reply to Nicholas Nethercote [:njn] from comment #114)
> I think one of these patches added a new static constructor. I'm currently
> trying to reduce the number of static constructors because they hurt Fennec
> start-up time. See
> http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1431390993000,
> 1431563793000&displayrange=7&datatype=geo. The good news is that static
> constructors are often easy to fix, e.g. by using a singleton.

I don't think these patches added any static constructors. Could you point me at the code you suspect adds a new static constructor?

Comment 116

[Nicholas Nethercote [inactive]]

> I don't think these patches added any static constructors. Could you point me
> at the code you suspect adds a new static constructor?

It's the gPrefLangToLangGroups table in part 1a.

I determined this by looking through all the patches looking for |static|
variables that were added. Part 1a and part 2 were the only patches that
matched. I started by looking at part 1a. I used |objdump -d
$OBJDIR/dist/bin/libxul.so| to get the assembly code, and searched for
gPrefLangToLangGroups and found it in this function, which you can tell is a
static constructor by its name, "_GLOBAL__I_a":

0000000000929f00 <_GLOBAL__I_a>:
  929f00:       48 8b 05 d9 bc 38 04    mov    0x438bcd9(%rip),%rax        # 4cb5be0 <_ZN9nsGkAtoms9x_westernE>
  929f07:       48 89 05 52 52 38 04    mov    %rax,0x4385252(%rip)        # 4caf160 <_ZL21gPrefLangToLangGroups>
  929f0e:       48 8b 05 13 d5 38 04    mov    0x438d513(%rip),%rax        # 4cb7428 <_ZN9nsGkAtoms8JapaneseE>
  929f15:       48 89 05 4c 52 38 04    mov    %rax,0x438524c(%rip)        # 4caf168 <_ZL21gPrefLangToLangGroups+0x8>
  929f1c:       48 8b 05 15 d5 38 04    mov    0x438d515(%rip),%rax        # 4cb7438 <_ZN9nsGkAtoms9TaiwaneseE>
  929f23:       48 89 05 46 52 38 04    mov    %rax,0x4385246(%rip)        # 4caf170 <_ZL21gPrefLangToLangGroups+0x10>
  929f2a:       48 8b 05 ff d4 38 04    mov    0x438d4ff(%rip),%rax        # 4cb7430 <_ZN9nsGkAtoms7ChineseE>
  929f31:       48 89 05 40 52 38 04    mov    %rax,0x4385240(%rip)        # 4caf178 <_ZL21gPrefLangToLangGroups+0x18>
  929f38:       48 8b 05 01 d5 38 04    mov    0x438d501(%rip),%rax        # 4cb7440 <_ZN9nsGkAtoms15HongKongChineseE>
  929f3f:       48 89 05 3a 52 38 04    mov    %rax,0x438523a(%rip)        # 4caf180 <_ZL21gPrefLangToLangGroups+0x20>
  929f46:       48 8b 05 03 d5 38 04    mov    0x438d503(%rip),%rax        # 4cb7450 <_ZN9nsGkAtoms2koE>
  929f4d:       48 89 05 34 52 38 04    mov    %rax,0x4385234(%rip)        # 4caf188 <_ZL21gPrefLangToLangGroups+0x28>
  929f54:       48 8b 05 15 d5 38 04    mov    0x438d515(%rip),%rax        # 4cb7470 <_ZN9nsGkAtoms10x_cyrillicE>
  929f5b:       48 89 05 2e 52 38 04    mov    %rax,0x438522e(%rip)        # 4caf190 <_ZL21gPrefLangToLangGroups+0x30>
  929f62:       48 8b 05 c7 d5 38 04    mov    0x438d5c7(%rip),%rax        # 4cb7530 <_ZN9nsGkAtoms2elE>
  929f69:       48 89 05 28 52 38 04    mov    %rax,0x4385228(%rip)        # 4caf198 <_ZL21gPrefLangToLangGroups+0x38>
  929f70:       48 8b 05 a1 96 38 04    mov    0x43896a1(%rip),%rax        # 4cb3618 <_ZN9nsGkAtoms2thE>
  929f77:       48 89 05 22 52 38 04    mov    %rax,0x4385222(%rip)        # 4caf1a0 <_ZL21gPrefLangToLangGroups+0x40>
  929f7e:       48 8b 05 f3 d4 38 04    mov    0x438d4f3(%rip),%rax        # 4cb7478 <_ZN9nsGkAtoms2heE>
  929f85:       48 89 05 1c 52 38 04    mov    %rax,0x438521c(%rip)        # 4caf1a8 <_ZL21gPrefLangToLangGroups+0x48>
  929f8c:       48 8b 05 ed d4 38 04    mov    0x438d4ed(%rip),%rax        # 4cb7480 <_ZN9nsGkAtoms2arE>
  929f93:       48 89 05 16 52 38 04    mov    %rax,0x4385216(%rip)        # 4caf1b0 <_ZL21gPrefLangToLangGroups+0x50>
  929f9a:       48 8b 05 e7 d4 38 04    mov    0x438d4e7(%rip),%rax        # 4cb7488 <_ZN9nsGkAtoms12x_devanagariE>
  929fa1:       48 89 05 10 52 38 04    mov    %rax,0x4385210(%rip)        # 4caf1b8 <_ZL21gPrefLangToLangGroups+0x58>
  929fa8:       48 8b 05 e1 d4 38 04    mov    0x438d4e1(%rip),%rax        # 4cb7490 <_ZN9nsGkAtoms7x_tamilE>
  929faf:       48 89 05 0a 52 38 04    mov    %rax,0x438520a(%rip)        # 4caf1c0 <_ZL21gPrefLangToLangGroups+0x60>
  929fb6:       48 8b 05 db d4 38 04    mov    0x438d4db(%rip),%rax        # 4cb7498 <_ZN9nsGkAtoms6x_armnE>
  929fbd:       48 89 05 04 52 38 04    mov    %rax,0x4385204(%rip)        # 4caf1c8 <_ZL21gPrefLangToLangGroups+0x68>
  929fc4:       48 8b 05 d5 d4 38 04    mov    0x438d4d5(%rip),%rax        # 4cb74a0 <_ZN9nsGkAtoms6x_bengE>
  929fcb:       48 89 05 fe 51 38 04    mov    %rax,0x43851fe(%rip)        # 4caf1d0 <_ZL21gPrefLangToLangGroups+0x70>
  929fd2:       48 8b 05 cf d4 38 04    mov    0x438d4cf(%rip),%rax        # 4cb74a8 <_ZN9nsGkAtoms6x_cansE>
  929fd9:       48 89 05 f8 51 38 04    mov    %rax,0x43851f8(%rip)        # 4caf1d8 <_ZL21gPrefLangToLangGroups+0x78>
  929fe0:       48 8b 05 c9 d4 38 04    mov    0x438d4c9(%rip),%rax        # 4cb74b0 <_ZN9nsGkAtoms6x_ethiE>
  929fe7:       48 89 05 f2 51 38 04    mov    %rax,0x43851f2(%rip)        # 4caf1e0 <_ZL21gPrefLangToLangGroups+0x80>
  929fee:       48 8b 05 c3 d4 38 04    mov    0x438d4c3(%rip),%rax        # 4cb74b8 <_ZN9nsGkAtoms6x_georE>
  929ff5:       48 89 05 ec 51 38 04    mov    %rax,0x43851ec(%rip)        # 4caf1e8 <_ZL21gPrefLangToLangGroups+0x88>
  929ffc:       48 8b 05 bd d4 38 04    mov    0x438d4bd(%rip),%rax        # 4cb74c0 <_ZN9nsGkAtoms6x_gujrE>
  92a003:       48 89 05 e6 51 38 04    mov    %rax,0x43851e6(%rip)        # 4caf1f0 <_ZL21gPrefLangToLangGroups+0x90>
  92a00a:       48 8b 05 b7 d4 38 04    mov    0x438d4b7(%rip),%rax        # 4cb74c8 <_ZN9nsGkAtoms6x_guruE>
  92a011:       48 89 05 e0 51 38 04    mov    %rax,0x43851e0(%rip)        # 4caf1f8 <_ZL21gPrefLangToLangGroups+0x98>
  92a018:       48 8b 05 b1 d4 38 04    mov    0x438d4b1(%rip),%rax        # 4cb74d0 <_ZN9nsGkAtoms6x_khmrE>
  92a01f:       48 89 05 da 51 38 04    mov    %rax,0x43851da(%rip)        # 4caf200 <_ZL21gPrefLangToLangGroups+0xa0>
  92a026:       48 8b 05 b3 d4 38 04    mov    0x438d4b3(%rip),%rax        # 4cb74e0 <_ZN9nsGkAtoms6x_mlymE>
  92a02d:       48 89 05 d4 51 38 04    mov    %rax,0x43851d4(%rip)        # 4caf208 <_ZL21gPrefLangToLangGroups+0xa8>
  92a034:       48 8b 05 ad d4 38 04    mov    0x438d4ad(%rip),%rax        # 4cb74e8 <_ZN9nsGkAtoms6x_oryaE>
  92a03b:       48 89 05 ce 51 38 04    mov    %rax,0x43851ce(%rip)        # 4caf210 <_ZL21gPrefLangToLangGroups+0xb0>
  92a042:       48 8b 05 af d4 38 04    mov    0x438d4af(%rip),%rax        # 4cb74f8 <_ZN9nsGkAtoms6x_teluE>
  92a049:       48 89 05 c8 51 38 04    mov    %rax,0x43851c8(%rip)        # 4caf218 <_ZL21gPrefLangToLangGroups+0xb8>
  92a050:       48 8b 05 81 d4 38 04    mov    0x438d481(%rip),%rax        # 4cb74d8 <_ZN9nsGkAtoms6x_kndaE>
  92a057:       48 89 05 c2 51 38 04    mov    %rax,0x43851c2(%rip)        # 4caf220 <_ZL21gPrefLangToLangGroups+0xc0>
  92a05e:       48 8b 05 8b d4 38 04    mov    0x438d48b(%rip),%rax        # 4cb74f0 <_ZN9nsGkAtoms6x_sinhE>
  92a065:       48 89 05 bc 51 38 04    mov    %rax,0x43851bc(%rip)        # 4caf228 <_ZL21gPrefLangToLangGroups+0xc8>
  92a06c:       48 8b 05 8d d4 38 04    mov    0x438d48d(%rip),%rax        # 4cb7500 <_ZN9nsGkAtoms6x_tibtE>
  92a073:       48 89 05 b6 51 38 04    mov    %rax,0x43851b6(%rip)        # 4caf230 <_ZL21gPrefLangToLangGroups+0xd0>
  92a07a:       48 8b 05 c7 d3 38 04    mov    0x438d3c7(%rip),%rax        # 4cb7448 <_ZN9nsGkAtoms7UnicodeE>
  92a081:       48 89 05 b0 51 38 04    mov    %rax,0x43851b0(%rip)        # 4caf238 <_ZL21gPrefLangToLangGroups+0xd8>
  92a088:       c3                      retq
  92a089:       00 00                   add    %al,(%rax)
  92a08b:       00 00                   add    %al,(%rax)
  92a08d:       00 00                   add    %al,(%rax)

I filed a fix for this in bug 1164735.

Comment 117

[Mathieu Pellerin]

I believe this has resulted in a significant regression, described in bug 1165788.