493280 - unify font family/style management and matching across platforms

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: initial implementation of gfxPlatformFontList for Mac OS X

We currently have a lot of font-management functionality implemented separately on the various platforms, with resulting differences in capabilities. In some cases large families with many weights/widths/optical sizes are not handled well, because the platform APIs we use assume a more limited model of font families.

The duplication of code and the differences in behavior might not be too serious right now, but they make it difficult to add features in this area, as they would have to be reimplemented for each platform (e.g. see bug 469656).

As we plan to support additional font technologies (SVG fonts, Harfbuzz shaping) across multiple platforms, the problems will be multiplied. To ease implementation and maintenance, we should unify font management as much as possible across platforms, and allow the rest of the text system to interface with font management in a platform-neutral way.

The best model we have for font and style management is currently gfxQuartzFontCache, which manages families with large numbers of faces and multiple names. (Even optical size support was easy to add within the OS X font back-end.)

The plan here, therefore, is to refactor gfxQuartzFontCache into a platform-independent gfxPlatformFontList class, and then provide concrete implementations for each host platform; the rest of the text code will only interface with the platform-independent API. The font-matching functionality of gfxAtsuiFontGroup (and now gfxCoreTextFontGroup) and related classes will be pulled up into gfxFontGroup, as this functionality depends only on the collection of available fonts (exposed through the gfxPlatformFontList) and not on the concrete implementation of access to those fonts. Eventually, platform-specific subclasses of gfxFontGroup should disappear. (Font technology-specific font groups are a dead end anyway, because we need to allow for a mixture of font technologies within a single group.)

The initial patch here begins the process by creating gfxPlatformFontList, moving the "generic" functionality of gfxQuartzFontCache into this class, and moving related code from the platform-specific gfxFont{,Entry,Family,Group} subclasses up into the generic base classes where possible. The Windows and Linux font systems are not yet modified; this just restructures the existing Mac code so as to provide a basis for those further patches.

Marking this as blocking bug 489354 because this patch also aims to eliminate the remaining non-64bit-clean code in gfxQuartzFontCache. Josh, I'd appreciate knowing whether it succeeds in that, if you are able to try a build.

Not suggesting we try to land this on trunk just yet; awaiting further tryserver results.

Comment 1

[Josh Aas]

Attachment: further 64-bit font failures

Thanks! I applied the patch here and built, still more font-related errors in gfx in a 64-bit Mac OS X build. Not sure if they are something you want to fix in your patch, let me know if not and I'll finish my review without these fixes.

Comment 2

[Jonathan Kew [:jfkthame]]

Hm, issues in nsSystemFontsMac..... I think we should deal with that in a separate patch on bug 489354, it doesn't belong in this bug.

I'm on vacation for the next couple of weeks, but if you'd like me to look into that as well I'd be happy to give it a try later. Alternatively, if you're in a hurry and want to work on that yourself, that's fine too.

Comment 3

[Jonathan Kew [:jfkthame]]

OK, so I couldn't resist: I took a look at nsSystemFontsMac, and have posted a patch at bug 489354 that should make this 64-bit compatible. It's had minimal testing, though, so handle with care.

Comment 4

[Josh Aas]

Comment on attachment 377742 [details] [diff] [review]
initial implementation of gfxPlatformFontList for Mac OS X

>+/*
> PRBool
> gfxMixedFontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], 
>                                         const gfxFontStyle& aFontStyle)

Did you mean to leave this commented out?

Otherwise this looks good to me, I really only looked at Mac OS X API usage. This is 64-bit safe.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: updated patch to remove mTraits from gfxFont classes

This removes the commented-out method (comment 4)... this functionality has been pushed up to the gfxFontFamily class.

Also rewritten gfxFontFamily::FindWeightsForStyle to use the stretch value and eliminate the use of traits fields, requested by jdaggett in an IRC conversation. This saves space in the fontEntry and font classes, and is cleaner and more flexible for the future.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 382153 [details] [diff] [review]
updated patch to remove mTraits from gfxFont classes

John, following our IRC conversation I have reworked this to remove the use of traits in our classes; gfxQuartzFontCache just uses the Cocoa font traits to initialize our italic, stretch, and fixed-pitch flags. This involved re-writing gfxFontFamily::FindWeightsForStyle to use font-stretch instead of traits; please have a look to see if this seems ok. It appears working for me so far, including for "untidy" families like Futura.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    const PRUint32 ch;
+    gfxFont *fontToMatch;
+    PRInt32 matchRank;
+    nsRefPtr<gfxFontEntry> bestMatch;

Please use the m* convention here.

+    nsTArray<nsRefPtr<gfxFontEntry> >  mAvailableFonts;

Could we call this mAvailableFaces? I assume that "face" means a specific way of rendering characters, and "font family" means a set of faces grouped under a single name. "Font" seems ambiguous --- or does it have a specific name I'm not aware of?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to comment #7)
> +    const PRUint32 ch;
> +    gfxFont *fontToMatch;
> +    PRInt32 matchRank;
> +    nsRefPtr<gfxFontEntry> bestMatch;
> 
> Please use the m* convention here.

OK, updated this. (Also corrected the gfxTextRunCache expiration time, which I'd accidentally left extremely short - leftover from testing!)

> +    nsTArray<nsRefPtr<gfxFontEntry> >  mAvailableFonts;
> 
> Could we call this mAvailableFaces? I assume that "face" means a specific way
> of rendering characters, and "font family" means a set of faces grouped under a
> single name. "Font" seems ambiguous --- or does it have a specific name I'm not
> aware of?

We could, though the terminology "font family" and "font" is pretty widely used, both in our code (look at gfxFontFamily and gfxFont) and elsewhere (e.g., Apple and Microsoft APIs and documentation). And the mAvailableFonts member is already established in gfxQuartzFontCache and gfxUserFontSet, at least. This patch doesn't invent it, only moves it! (On the Windows side, we currently have mVariations instead, which I think is an unfortunate name - but it's going to disappear in a subsequent patch.)

FWIW, Thomas Phinney recently did a survey regarding type terminology, and "font" came out as the preferred term here, winning over "face". <http://www.thomasphinney.com/tag/terminology/>

Personally, I don't have a strong opinion one way or the other, except that I'm a bit reluctant to change given that our existing usage is well entrenched. But if you really want "face" here, say so and I'll do it.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: updated field names in FontSearch struct, corrected cache expiration - see comment #8

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, then we should probably not use the term "face" anywhere... but I guess cairo and Freetype do. Oh well.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    virtual nsresult GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8>& aBuffer) {
+        return NS_ERROR_FAILURE; // all platform subclasses should reimplement this!
+    }

So you can't make this and other functions pure virtual just because the other platforms haven't been converted yet, right?

Can you put the enums and AutoSwap types that you've moved into gfxFontUtils into the mozilla namespace?

+            MacOSFontEntry *fe = (MacOSFontEntry*) family->FindFont(&mStyle, needsBold);

Prefer static_cast to C-style casts

I don't know where the pattern came from in gfxFont.cpp of having two blank lines between functions, but we don't use that anywhere else --- let's not do it.

+    // short-circuit the single face per family case
+    if (mAvailableFonts.Length() == 1) {
+        gfxFontEntry *fe = mAvailableFonts[0];
+        PRUint32 weight = fe->Weight() / 100;
+        NS_ASSERTION(weight >= 1 && weight <= 9, "bogus font weight value!");
+        aFontsForWeights[weight] = fe;
+        return PR_TRUE;
+    }

Is this really worth having?

I think gfxFontFamily::FindWeightsForStyle could be simpler and more efficient if it used an explicit scoring function and a single pass over mAvailableFonts.

Otherwise looks good, although it's hard to review because it's hard to distinguish moved code from changed code, so I can't say I did a very thorough review.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to comment #11)
> +    virtual nsresult GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8>&
> aBuffer) {
> +        return NS_ERROR_FAILURE; // all platform subclasses should reimplement
> this!
> +    }
> 
> So you can't make this and other functions pure virtual just because the other
> platforms haven't been converted yet, right?

Right. That will be the appropriate thing to do eventually. For now, the other platforms won't actually call it, of course.

> Can you put the enums and AutoSwap types that you've moved into gfxFontUtils
> into the mozilla namespace?

I've put the AutoSwap types into mozilla namespace; for the name-table enums, it seemed most sensible to put them within the gfxFontUtils class, which is really being used as a namespace, in effect. (Are we intending to put all our classes themselves into the mozilla namespace eventually?)

> +    // short-circuit the single face per family case
> +    if (mAvailableFonts.Length() == 1) {
> +        gfxFontEntry *fe = mAvailableFonts[0];
> +        PRUint32 weight = fe->Weight() / 100;
> +        NS_ASSERTION(weight >= 1 && weight <= 9, "bogus font weight value!");
> +        aFontsForWeights[weight] = fe;
> +        return PR_TRUE;
> +    }
> 
> Is this really worth having?

Who knows? :) It's existing code from the Mac back-end, just pushed up to the generic class, see <http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxQuartzFontCache.mm#466>. I'm guessing it was implemented because it seemed worthwhile at the time, but we could try performance measurements with and without it to see what happens.

> I think gfxFontFamily::FindWeightsForStyle could be simpler and more efficient
> if it used an explicit scoring function and a single pass over mAvailableFonts.

The current approach is based on what we already had in the Mac back-end (and it's similar on Windows). I don't see how we can make FindWeightsForStyle single-pass by itself; I think we'd need to redesign FindFontForStyle to work directly from the mAvailableFonts list, instead of setting up an array of available weights for the given style (i.e. italicness and width options) and then walking this list.

I'll give this a shot.... seems like it should be a win, provided it can find the right fonts even in "irregular" families where the weights available are different depending on other style attributes.

If successful, this will make the previous question obsolete at a stroke. :)

Comment 13

[John Daggett (:jtd)]

I think we need to include gfxFT2Fonts in this too, so that the WinMobile implementation isn't out of sync with the base gfxFontEntry/gfxFont classes.  I know it's added work but the code should be fairly simple to update once Ashie-san finishes his patch for bug 490267.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: revised/simplified FindWeightsForStyle as suggested by roc

This approach seems to work at least as well as the original version of FindFontsForStyle from the Mac back-end, and should provide a well-defined place to add in future enhancements (such as integrating optical-size support into the font-selection code, or more sophisticated handling of font-stretch or other attributes).

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: WIP: implement gfxPlatformFontList for Windows

Work-in-progress posted for reference, not yet finished - currently missing LookupLocalFont (should be trivial to add, just need to move the code), and certain font-fallback cases don't work correctly. Also likely to have performance impact as it stands; may need lazy initialization to be added.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we test FindWeightsForStyle using @font-face?

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: update Mac OS X patch to current trunk

Refreshed the patch to apply on current trunk, re-checked that all reftests pass locally.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: possible test-case for weight selection

There is a little bit of FindWeightsForStyle coverage in the @font-face tests already, but this sample exercises it further. Not including in the Mac patch yet as (a) I have not tested whether this passes on other platforms, and (b) unfortunately, although the font selection is working fine, this seems to show a separate metrics-related bug that will make it fail if used as a reftest. The presence of the "font-weight: 900" entry in the table, even though it is actually resolving to the same face as earlier entries, seems to be disrupting vertical positioning, with the text baseline being slightly shifted.

This problem happens with either CoreText or ATSUI rendering, and with or without the font-management refactoring patch; it is apparently an unrelated issue. I'll be doing some further investigation and probably file a separate bug.

Comment 19

[John Daggett (:jtd)]

(In reply to comment #17)
> Created an attachment (id=385342) [details]
> update Mac OS X patch to current trunk
> 
> Refreshed the patch to apply on current trunk, re-checked that all reftests
> pass locally.

First off, I think we should separate out the 64-bit changes into a patch that doesn't involve more general structural changes, specifically the .  We probably need these changes anyways to fix bug 463861.  Not sure where changes to nsSystemFontsMac.cpp are attached.

Overall, looks good.  Some small nits below but the one thing I'm uncomfortable with is the FindWeightsForStyle function, I'm not sure a generic function is warranted here and the way it's implemented seems like we're aiming to support functionality like font-stretch that aren't completely implemented yet.  

> +namespace mozilla {
> +
> +struct AutoSwap_PRUint16 {

I see roc asked for this but it's not clear to me how this helps, seems sort of odd to have this one set of structs in their own namespace.

> +// determine which charset to use, given font name table data
> +
> +/* List of canonical decoder names we currently support:
> +Big5
> +Big5-HKSCS
> +EUC-JP
> +EUC-KR
> +GB2312
> +GEOSTD8
> +HZ-GB-2312
> +IBM850
> 
> <snip> long list of encodings

This long list isn't needed or helpful and would probably get out-of-sync with the code anyways over time.  This is based on encodings listed in nsUConvModule.cpp?

> +    case PLATFORM_ID_MAC:
> +        switch (aScript) {
> +        case ENCODING_ID_MAC_ROMAN:
> +            switch (aLanguage) {
> +            default:
> +                return "x-mac-roman";
> +            case LANG_ID_MAC_ICELANDIC:
> +                return "x-mac-icelandic";
> +            case LANG_ID_MAC_TURKISH:
> +                return "x-mac-turkish";

I really wish we didn't have to create a huge table of strings like this.  These id's really belong in uconv code as some form of constant, not here.  Could we map <platform, encoding, lang> to NS_xxxTOUNICODE_CID values and use those to instantiate the decoders?  Maybe Simon has an idea here?

> +    nsresult rv;
> +    nsCOMPtr<nsICharsetConverterManager> ccm =
> +        do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
> +    NS_ASSERTION(NS_SUCCEEDED(rv), "failed to get charset converter manager");

Add return PR_FALSE on NS_FAILED(rv).

> +void
> +gfxPlatformFontList::InitSingleFaceList()

This was required for dealing with things like Osaka-Mono.  Why does this need to be cross-platform?  Not sure I see the need.

> +gfxPlatformFontList*
> +gfxPlatformMac::CreatePlatformFontList()
> +{
> +    return new gfxQuartzFontCache();
> +}
> +

Given the restructuring here, we should rename this to something more appropriate, like "gfxMacPlatformFontList" instead of "gfxQuartzFontCache", since it's not really a font cache.  So I guess that means a new file and remove the old one.


> +    // look up the Quickdraw name
> +    ATSFontFamilyRef atsFamily = ::ATSFontFamilyFindFromQuickDrawName(qdname);
> +    if (atsFamily == (ATSFontFamilyRef)kInvalidFontFamily) {
>          return PR_FALSE;
> +    }

This looks like it should work, have you verified this?  With Futura Condensed for example?

> +static PRUint32
> +StyleDistance(const gfxFontEntry* aFontEntry, const gfxFontStyle& aFontStyle)
> +{
> +    // compute a measure of the "distance" between the requested fontStyle and the given fontEntry,
> +    // considering italicness and font-stretch but not weight
> +
> +    // italic takes precedence
> +    PRBool italic = (aFontStyle.style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) != 0;
> +    PRUint32 distance = (aFontEntry->mItalic != italic) ? 1000 : 0;
> +
> +    // then consider any discrepancy in font-stretch (values range from -3...+3)
> +    // in the usual case, both are zero, so we don't bother to do any calculation here
> +    if (aFontEntry->mStretch != aFontStyle.stretch) {
> +        // if font-stretch values have opposite signs, that's bad
> +        if (aFontEntry->mStretch * aFontStyle.stretch < 0) {
> +            distance += 10;
> +        }
> +        // then add the absolute value of the difference
> +        distance += PR_ABS(aFontEntry->mStretch - aFontStyle.stretch);
> +    }
> +
> +    return distance;
> +}
> +
> +PRBool
> +gfxFontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle)
> +{
> +    PRUint32 foundWeights = 0;
> +    PRUint32 bestMatchDistance = 0xffffffff;
> +
> +    for (PRUint32 i = 0; i < mAvailableFonts.Length(); i++) {
> +        gfxFontEntry *fe = mAvailableFonts[i];
> +        PRUint32 distance = StyleDistance(fe, aFontStyle);
> +        if (distance <= bestMatchDistance) {
> +            PRInt8 wt = fe->mWeight / 100;
> +            NS_ASSERTION(wt >= 0 && wt < 10, "invalid weight in fontEntry");
> +            if (!aFontsForWeights[wt]) {
> +                // record this as a possible candidate for weight matching
> +                aFontsForWeights[wt] = fe;
> +                ++foundWeights;
> +            } else {
> +                PRUint32 prevDistance = StyleDistance(aFontsForWeights[wt], aFontStyle);
> +                if (prevDistance > distance ||
> +                    (prevDistance == distance && mPreferLaterEntries))
> +                {
> +                    // replacing a weight we already found, so don't increment foundWeights
> +                    aFontsForWeights[wt] = fe;
> +                }
> +            }
> +            bestMatchDistance = distance;
> +        }
> +    }
> +
> +    NS_ASSERTION(foundWeights > 0, "Font family containing no faces?");
> +
> +    if (foundWeights == 1) {
> +        // no need to cull entries if we only found one weight
> +        return PR_TRUE;
> +    }
> +
> +    // we might have recorded some faces that were a partial style match, but later found
> +    // others that were closer; in this case, we need to cull the poorer matches from the
> +    // weight list we'll return
> +    for (PRUint32 i = 0; i < 10; ++i) {
> +        if (aFontsForWeights[i] &&
> +            StyleDistance(aFontsForWeights[i], aFontStyle) > bestMatchDistance)
> +        {
> +            aFontsForWeights[i] = 0;
> +        }
> +    }
> +
> +    return (foundWeights > 0);
> +}

I don't think this is a very good replacement for existing code.  I realize you're trying to adapt it to do the right thing for font-stretch and that complicates things.  For what this is doing it seems unduly complicated, mainly all the StyleDistance calculations and the multiple passes.  Especially for Windows where font families consist of some subset of (plain, bold, italic, bold italic).  Even when we extend the Windows code to support larger families, the vast majority of fonts on Windows will remain this way, whether for XP, Vista or Win7.

So I think the simple solution here is to add a flag "simple font family" or whatever that flags a font that has *just* standard faces (plain, bold, italic, bold italic) of a given width.  For those families, mAvailableFonts would be stored in a given order such specific face could be chosen via a lookup table or simple computation.

One of the reasons FindWeightsForStyle was needed was to deal with relative weights when bolder/lighter were used.  In the CSS WG we've been discussing making bolder/lighter a fixed mapping which would mean gfx code would not need to deal with relative mappings, the style system would pass in a specific weight value [100..900].

http://lists.w3.org/Archives/Public/www-style/2009Jun/0182.html

The implications of this are that it will probably be possible make things simpler still.

> -    // short-circuit the single face per family case
> -    if (mAvailableFonts.Length() == 1) {
> -        MacOSFontEntry *fe = mAvailableFonts[0];
> -        PRUint32 weight = fe->Weight() / 100;
> -        NS_ASSERTION(weight >= 1 && weight <= 9, "bogus font weight value!");
> -        aFontsForWeights[weight] = fe;
> -        return PR_TRUE;
> -    }

This was specifically to handle non-Latin fonts that have only one face.  For CJK pages this did make a difference in performance, that's why I added it.  If these fonts were included in "simple" font families this probably wouldn't be needed.

> +            PRInt8 wt = fe->mWeight / 100;
> +            NS_ASSERTION(wt >= 0 && wt < 10, "invalid weight in fontEntry");

This should be >= 1, since a font should never have a weight of 0.  I realize the array has 10 entries, that's just convenience so it's not necessary to sprinkle the code with (wt - 1).

Comment 20

[Simon Montagu :smontagu]

(In reply to comment #19)
> I really wish we didn't have to create a huge table of strings like this. 
> These id's really belong in uconv code as some form of constant, not here. 
> Could we map <platform, encoding, lang> to NS_xxxTOUNICODE_CID values and use
> those to instantiate the decoders?  Maybe Simon has an idea here?

I would think we could use a properties file in uconv, similar to http://mxr.mozilla.org/mozilla-central/find?string=charset.properties

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to comment #19)

> Overall, looks good.  Some small nits below but the one thing I'm uncomfortable
> with is the FindWeightsForStyle function, I'm not sure a generic function is
> warranted here and the way it's implemented seems like we're aiming to support
> functionality like font-stretch that aren't completely implemented yet.  

I know font-stretch isn't completely implemented, but I wanted to have a reasonable infrastructure here for when it arrives....

> > +namespace mozilla {
> > +
> > +struct AutoSwap_PRUint16 {
> 
> I see roc asked for this but it's not clear to me how this helps, seems sort of
> odd to have this one set of structs in their own namespace.

Presumably the "mozilla" namespace will collect various previously-global things that don't have any more specific natural home.

> > <snip> long list of encodings
> 
> This long list isn't needed or helpful and would probably get out-of-sync with
> the code anyways over time.  This is based on encodings listed in
> nsUConvModule.cpp?

IIRC, I used nsICharsetConverterManager::getDecoderList() to find out what charset names we had. But yes, the list doesn't need to be here, that was just for my convenience to know what I could use when filling in the mappings from OpenType IDs.

> 
> > +    case PLATFORM_ID_MAC:
> > +        switch (aScript) {
> > +        case ENCODING_ID_MAC_ROMAN:
> > +            switch (aLanguage) {
> > +            default:
> > +                return "x-mac-roman";
> > +            case LANG_ID_MAC_ICELANDIC:
> > +                return "x-mac-icelandic";
> > +            case LANG_ID_MAC_TURKISH:
> > +                return "x-mac-turkish";
> 
> I really wish we didn't have to create a huge table of strings like this. 
> These id's really belong in uconv code as some form of constant, not here. 
> Could we map <platform, encoding, lang> to NS_xxxTOUNICODE_CID values and use
> those to instantiate the decoders?

We can certainly move them, though I'm not sure what it gains us; the mapping from OpenType <platform, encoding, lang> IDs to charsets is specific to fonts, it's (unfortunately) not a duplicate of some other standard that we'd be using elsewhere.

It should work to use a properties file in uconv, as Simon suggested ("opentypecharset.properties", I guess), and then have a function that can look up <platform, encoding, lang> in there. Any idea whether this would introduce significant overhead, compared to the simple switch statements?

> > +void
> > +gfxPlatformFontList::InitSingleFaceList()
> 
> This was required for dealing with things like Osaka-Mono.  Why does this need
> to be cross-platform?  Not sure I see the need.

True, it doesn't need to be cross-platform; AFAIK we only have such a list on the Mac at the moment. But there doesn't need to be anything platform-specific about the InitSingleFaceList() function itself, so as a general rule I've tried to push whatever functionality is not actually platform-dependent into the base class, so the platform-specific subclasses will be kept as small as possible.

I really don't care where this implementation lives, though; if you feel it clutters gfxPlatformFontList, I'm fine with moving it back into the Mac subclass.

> > +gfxPlatformFontList*
> > +gfxPlatformMac::CreatePlatformFontList()
> > +{
> > +    return new gfxQuartzFontCache();
> > +}
> > +
> 
> Given the restructuring here, we should rename this to something more
> appropriate, like "gfxMacPlatformFontList" instead of "gfxQuartzFontCache",
> since it's not really a font cache.  So I guess that means a new file and
> remove the old one.

Yes, I agree; my hesitation was that renaming an existing file and class adds more churn, but for long-term consistency it's the better thing to do.

> > +    // look up the Quickdraw name
> > +    ATSFontFamilyRef atsFamily = ::ATSFontFamilyFindFromQuickDrawName(qdname);
> > +    if (atsFamily == (ATSFontFamilyRef)kInvalidFontFamily) {
> >          return PR_FALSE;
> > +    }
> 
> This looks like it should work, have you verified this?  With Futura Condensed
> for example?

Futura works as expected for me on 10.5. Trying to use "Futura Condensed" as a family name doesn't work, either with or without this patch (or in FF 3.5).

AFAICT, this code is only used on names that were stored in the profile, not names from CSS, but in either case "Futura Condensed" isn't recognized. Was that a 10.4-ism? ISTR some of these font names changed between Tiger and Leopard, but I don't have a 10.4 system available for testing.

> > +static PRUint32
> > +StyleDistance(const gfxFontEntry* aFontEntry, const gfxFontStyle& aFontStyle)
[snip]
> > +PRBool
> > +gfxFontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle)
[snip]

> I don't think this is a very good replacement for existing code.  I realize
> you're trying to adapt it to do the right thing for font-stretch and that
> complicates things.  For what this is doing it seems unduly complicated, mainly
> all the StyleDistance calculations and the multiple passes.

Is it really more complicated than the old FindWeightsForStyle() code in gfxQuartzFontCache, with its multiple calls to FindFontsWithTraits()? Yes, I was trying to allow for font-stretch as well, though we're not all the way there yet, but I don't want to have to restructure this again when we get around to looking at that more thoroughly.

> So I think the simple solution here is to add a flag "simple font family" or
> whatever that flags a font that has *just* standard faces (plain, bold, italic,
> bold italic) of a given width.  For those families, mAvailableFonts would be
> stored in a given order such specific face could be chosen via a lookup table
> or simple computation.

We could do this - I also considered the possible benefits of keeping mAvailableFonts sorted in all cases - but I didn't want to add work that happens during startup, when we build the list of families and fonts. Especially for users with thousands of fonts, checking the members of each family to decide if it's "simple", and sorting the faces, could add a significant number of milliseconds at startup.

OTOH, the benefit is an optimization during rendering, so that would be nice to have. How about having a flag in the family that records whether it has been checked for "simpleness", and doing this the first time we actually need a face from the family? That way we avoid doing the extra work for those (potentially many) families that don't ever get used, and get the efficient runtime behavior for the common cases.

Actually, just keeping mAvailableFonts in a "given order" may not be the nicest option, as if there are fewer than the standard 4 faces, we'd want to know which ones were missing (e.g. are the two fonts in family X meant to be regular and bold, or regular and italic?). Perhaps "simple" families should replace mAvailableFonts entirely and have a fixed set of 4 references to their faces (some of which might be null).

> One of the reasons FindWeightsForStyle was needed was to deal with relative
> weights when bolder/lighter were used.  In the CSS WG we've been discussing
> making bolder/lighter a fixed mapping which would mean gfx code would not need
> to deal with relative mappings, the style system would pass in a specific
> weight value [100..900].
> 
> http://lists.w3.org/Archives/Public/www-style/2009Jun/0182.html
> 
> The implications of this are that it will probably be possible make things
> simpler still.

If we do modify the style system to just pass in a specific weight, then yes, we can simplify things here. But until that happens, I guess we need to keep the relative-weight handling that we have.

(While we're here, I note that not everyone in the type world is happy about the CSS treatment of weight values as being a discrete 9-point scale. Could we end up with weight being redefined as simply a numeric value from 0-999, with the keywords like "light", "medium", etc simply mapping to known values? Personally, I think that would make more sense.)

> > -    // short-circuit the single face per family case
> > -    if (mAvailableFonts.Length() == 1) {
> > -        MacOSFontEntry *fe = mAvailableFonts[0];
> > -        PRUint32 weight = fe->Weight() / 100;
> > -        NS_ASSERTION(weight >= 1 && weight <= 9, "bogus font weight value!");
> > -        aFontsForWeights[weight] = fe;
> > -        return PR_TRUE;
> > -    }
> 
> This was specifically to handle non-Latin fonts that have only one face.  For
> CJK pages this did make a difference in performance, that's why I added it.  If
> these fonts were included in "simple" font families this probably wouldn't be
> needed.

I'll take a stab at the "simple-family" concept, in which case these families will use a direct lookup anyway. That should be a (slight) win in general, as the majority of families will fit that category.

Comment 22

[John Daggett (:jtd)]

> > I really wish we didn't have to create a huge table of strings like this. 
> > These id's really belong in uconv code as some form of constant, not here. 
> > Could we map <platform, encoding, lang> to NS_xxxTOUNICODE_CID values and use
> > those to instantiate the decoders?
> 
> We can certainly move them, though I'm not sure what it gains us; the mapping
> from OpenType <platform, encoding, lang> IDs to charsets is specific to fonts,
> it's (unfortunately) not a duplicate of some other standard that we'd be using
> elsewhere.

My lament here was having to add a whole bunch of string constants that are only
used to pass in when creating a decoder.  If there's a way to do this more efficiently
that would be nice, but it's not something that we need to spend a lot of time on.

> > > +void
> > > +gfxPlatformFontList::InitSingleFaceList()
> > 
> > This was required for dealing with things like Osaka-Mono.  Why does this need
> > to be cross-platform?  Not sure I see the need.
> 
> True, it doesn't need to be cross-platform; AFAIK we only have such a list on
> the Mac at the moment. But there doesn't need to be anything platform-specific
> about the InitSingleFaceList() function itself, so as a general rule I've tried
> to push whatever functionality is not actually platform-dependent into the base
> class, so the platform-specific subclasses will be kept as small as possible.
> 
> I really don't care where this implementation lives, though; if you feel it
> clutters gfxPlatformFontList, I'm fine with moving it back into the Mac
> subclass.

Yes, I'd rather keep this in the Mac subclass.

> > > +    // look up the Quickdraw name
> > > +    ATSFontFamilyRef atsFamily = ::ATSFontFamilyFindFromQuickDrawName(qdname);
> > > +    if (atsFamily == (ATSFontFamilyRef)kInvalidFontFamily) {
> > >          return PR_FALSE;
> > > +    }
> > 
> > This looks like it should work, have you verified this?  With Futura Condensed
> > for example?
> 
> Futura works as expected for me on 10.5. Trying to use "Futura Condensed" as a
> family name doesn't work, either with or without this patch (or in FF 3.5).
> 
> AFAICT, this code is only used on names that were stored in the profile, not
> names from CSS, but in either case "Futura Condensed" isn't recognized. Was
> that a 10.4-ism? ISTR some of these font names changed between Tiger and
> Leopard, but I don't have a 10.4 system available for testing.

This was a Firefox 2-ism.  Profiles created in FF2 which used QD-style
names could contain names like Futura Condensed.  So the test would be
to create a profile in FF2, select Futura Condensed as the default font,
then see whether your patch maps this to Futura or not.  10.4/10.5
doesn't matter.


> > > +static PRUint32
> > > +StyleDistance(const gfxFontEntry* aFontEntry, const gfxFontStyle& aFontStyle)
> [snip]
> > > +PRBool
> > > +gfxFontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle)
> [snip]
> 
> > I don't think this is a very good replacement for existing code.  I realize
> > you're trying to adapt it to do the right thing for font-stretch and that
> > complicates things.  For what this is doing it seems unduly complicated, mainly
> > all the StyleDistance calculations and the multiple passes.
> 
> Is it really more complicated than the old FindWeightsForStyle() code in
> gfxQuartzFontCache, with its multiple calls to FindFontsWithTraits()? Yes, I
> was trying to allow for font-stretch as well, though we're not all the way
> there yet, but I don't want to have to restructure this again when we get
> around to looking at that more thoroughly.

Yes, it's harder to understand at first pass.  But FindFontsWithTraits
is there more because font-stretch wasn't implemented so it's not really
the ideal either.  My real problem with this is that it's overkill
for fonts on Windows, none of this code is needed on Windows.

> > So I think the simple solution here is to add a flag "simple font family" or
> > whatever that flags a font that has *just* standard faces (plain, bold, italic,
> > bold italic) of a given width.  For those families, mAvailableFonts would be
> > stored in a given order such specific face could be chosen via a lookup table
> > or simple computation.
> 
> We could do this - I also considered the possible benefits of keeping
> mAvailableFonts sorted in all cases - but I didn't want to add work that
> happens during startup, when we build the list of families and fonts.
> Especially for users with thousands of fonts, checking the members of each
> family to decide if it's "simple", and sorting the faces, could add a
> significant number of milliseconds at startup.
> 
> OTOH, the benefit is an optimization during rendering, so that would be nice to
> have. How about having a flag in the family that records whether it has been
> checked for "simpleness", and doing this the first time we actually need a face
> from the family? That way we avoid doing the extra work for those (potentially
> many) families that don't ever get used, and get the efficient runtime behavior
> for the common cases.

We don't need to do the sorting at startup, we can do it as part of the
cmap-reading phase which takes place after startup.  The "simple" flag
would be off by default, so pages rendered at startup would take the
general path but that's not such a big deal.  Once the cmap-reading 
process completes, the simple path will be used for most fonts.

> Actually, just keeping mAvailableFonts in a "given order" may not be
> the nicest option, as if there are fewer than the standard 4 faces,
> we'd want to know which ones were missing (e.g. are the two fonts in
> family X meant to be regular and bold, or regular and italic?).
> Perhaps "simple" families should replace mAvailableFonts entirely and
> have a fixed set of 4 references to their faces (some of which might
> be null).

Sure, having "simple" ==> mAvailableFonts.Length() == 4 with a specific
ordering, nulls allowed, is fine.

> If we do modify the style system to just pass in a specific weight, then yes,
> we can simplify things here. But until that happens, I guess we need to keep
> the relative-weight handling that we have.

Yes, sure.

> (While we're here, I note that not everyone in the type world is happy
> about the CSS treatment of weight values as being a discrete 9-point
> scale. Could we end up with weight being redefined as simply a numeric
> value from 0-999, with the keywords like "light", "medium", etc simply
> mapping to known values? Personally, I think that would make more
> sense.)

Actually, it's just Thomas who seems to have a bee in his bonnet about
this one. I'm not a huge fan of 0-999 weight values, since these values
are even less consistent across fonts and the OS/2 weight value seems to
be non-authoritative on this due to hacks to work around GDI issues
(e.g. a Thin face being given weight 250).  Best to post to www-style on
this one, but keep in mind that if you propose using 0-999 you need to
define bolder/lighter behavior clearly.

> +    // then consider any discrepancy in font-stretch (values range from -3...+3)
> +    // in the usual case, both are zero, so we don't bother to do any calculation here
> +    if (aFontEntry->mStretch != aFontStyle.stretch) {
> +        // if font-stretch values have opposite signs, that's bad
> +        if (aFontEntry->mStretch * aFontStyle.stretch < 0) {
> +            distance += 10;
> +        }
> +        // then add the absolute value of the difference
> +        distance += PR_ABS(aFontEntry->mStretch - aFontStyle.stretch);
> +    }

Looking at this a little closer, this is not right.  You've given higher
precedence to italic-ness over stretch and the current spec says the
opposite.  We could change the spec, it hasn't really been flushed out
for font-stretch.  I based this precedence on WPF, where the thought was
that font-stretch would affect layout more and so should have a higher
precedence. You're creating a subrule for how to match stretch,
basically whether it's the same direction or not.  Sounds fine but those
rules need to be in the spec.

For now let's make distance this:

  distance = (italic not same ? 1 : 0) + (stretch not same ? 10 : 0)
  
Also,

> +    PRBool italic = (aFontStyle.style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) != 0;

Pass in this along with the style stretch value rather than
recalculating this each time.

This should boil down to the same as the existing code and we can deal
with font-stretch as part of bug 3512, including whatever spec changes
and testing needs to get set up.

> I'll take a stab at the "simple-family" concept, in which case these families
> will use a direct lookup anyway. That should be a (slight) win in general, as
> the majority of families will fit that category.

Or just leave the existing Windows version of FindWeightsForStyle as a
platform-specific override and we can tweak this later as part of the
bolder/lighter change.

Comment 23

[Peter Weilbacher]

Comment on attachment 385342 [details] [diff] [review]
update Mac OS X patch to current trunk

>+    // name table has a header, followed by name records, followed by string data
>+    struct NameHeader {
>+        mozilla::AutoSwap_PRUint16    format;                 // Format selector (=0).
>+        mozilla::AutoSwap_PRUint16    count;                  // Number of name records.
>+        mozilla::AutoSwap_PRUint16    stringOffset;           // Offset to start of string storage (from start of table)
>+    };
>+
>+    struct NameRecord {
>+        mozilla::AutoSwap_PRUint16    platformID;             // Platform ID
>+        mozilla::AutoSwap_PRUint16    encodingID;             // Platform-specific encoding ID
>+        mozilla::AutoSwap_PRUint16    languageID;             // Language ID
>+        mozilla::AutoSwap_PRUint16    nameID;                 // Name ID.
>+        mozilla::AutoSwap_PRUint16    length;                 // String length (in bytes).
>+        mozilla::AutoSwap_PRUint16    offset;                 // String offset from start of storage area (in bytes).
>+    };

I'm not an official reviewer but seeing these lines I wonder if
you are trying to win a prize for the "widest line in the Mozilla
codebase" or something. ;-) Seriously, for anyone not working in a
maximized editor window this makes code hard to read and follow.
Not sure what the rule here is, but I found code that wraps at
least at 90 chars to be much easier to handle. Much of Thebes is
written that way.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: updated Mac font list patch to address review comments

Comment 25

[Jonathan Kew [:jfkthame]]

Updated the patch; this is considerably better I think. In particular:

* The font name charset mappings are still here, but handled with tidier tables of constants. I looked at Simon's suggestion of a .properties file in uconv but as these mappings are private to the font-name decoding code, not for wider use, I don't see any benefit to moving them over there, and creating extra interfaces. The lookup is also complicated by the fact that we need to use varying numbers of the fields, and fall back to more general matches if we don't find an exact match (with the Mac encodings and languages). So I think the current code is about as tidy as it'll get.

* FindWeightsForStyle is updated to use the requested version of style-distance (though I think this is something we'll want to enhance in the future). However, FindFontForStyle now handles the "simple family" concept, so it will not call FindWeightsForStyle at all in most cases.

(We may still want to simplify or replace FindWeightsForStyle in the future, depending on the final outcome of the discussions about font-weight, etc.)

Comment 26

[John Daggett (:jtd)]

> +    // no match? add to set of non-matching codepoints
> +    if (!data.mBestMatch) {
> +        mCodepointsWithNoFonts.set(aCh);
> +    }
> +
> +    if (data.mBestMatch && aCh == 0xFFFD) {
> +        mReplacementCharFallbackFamily = data.mBestMatch->FamilyName();
> +    }

Use "else if (aCh == 0xFFFD)" to skip testing for mBestMatch twice.

> +    // name not found and other family names not yet fully initialized so
> +    // initialize the rest of the list and try again.  this is done lazily
> +    // since reading name table entries is expensive
> +    if (!mOtherFamilyNamesInitialized) {
> +        InitOtherFamilyNames();
> +        if ((familyEntry = mOtherFamilyNames.GetWeak(key, &found))) {
> +            return familyEntry;
> +        }
> +    }

Just a note that if this is done across all platforms, the
font.preload-names-list pref will need to be set up on Windows/Linux in
all.js.

> +        : gfxFontFamily(aName) {
> +        mPreferLaterEntries = PR_TRUE; // later entries should override earlier ones for same style
> +    }

Not sure this needs to be parameterized, since changing it will change
page rendering.  This needs to mimic current behavior, at least on the
Mac.

> +PRBool
> +gfxFontFamily::CheckForSimpleFamily()
> +{
> +    if (mSimpleFamilyInitialized) {
> +        return mIsSimpleFamily;
> +    }
> +    mSimpleFamilyInitialized = PR_TRUE;
> 
> +    if (CheckForSimpleFamily()) {

Eliminate mSimpleFamilyInitialized and use just mIsSimpleFamily.  Call
CheckForSimpleFamily() while iterating over cmaps to set
mIsSimpleFamily.  Before cmaps load, general case will get hit but
that's better than calling than having a wasted function call 99% of the
time.

> +        PRUint8 faceIndex = (wantItalic ? kItalicMask : 0) |
> +                            (wantBold ? kBoldMask : 0);
> +
> +        // order to check for faces in a simple family, depending on requested style
> +        static const PRUint8 simpleSearchOrders[][4] = {
> +            { kRegularFaceIndex, kBoldFaceIndex, kItalicFaceIndex, kBoldItalicFaceIndex },
> +            { kBoldFaceIndex, kRegularFaceIndex, kBoldItalicFaceIndex, kItalicFaceIndex },
> +            { kItalicFaceIndex, kBoldItalicFaceIndex, kRegularFaceIndex, kBoldFaceIndex },
> +            { kBoldItalicFaceIndex, kItalicFaceIndex, kBoldFaceIndex, kRegularFaceIndex }
> +        };
> +        const PRUint8* order = simpleSearchOrders[faceIndex];
> +
> +        for (PRUint8 trial = 0; trial < 4; ++trial) {
> +            // check the 4 faces in order of preference
> +            gfxFontEntry *fe = mAvailableFonts[order[trial]];
> +            if (fe) {
> +                PR_LOG(gFontSelection, PR_LOG_DEBUG,
> +                       ("(FindFontForStyle) name: %s, sty: %02x, wt: %d, sz: %.1f -> %s (trial %d)\n", 
> +                        NS_ConvertUTF16toUTF8(mName).get(),
> +                        aFontStyle.style, aFontStyle.weight, aFontStyle.size,
> +                        NS_ConvertUTF16toUTF8(fe->Name()).get(), trial));
> +                aNeedsBold = wantBold && !fe->IsBold();
> +                return fe;
> +            }
> +        }

Switch this to entering the trial loop only when the first case fails,
the common case should exit this function as soon as possible without
passing into loops and such.

> +    gfxFontEntry *faces[4] = { 0 };
> +    for (PRUint8 i = 0; i < mAvailableFonts.Length(); ++i) {
> +        gfxFontEntry *fe = mAvailableFonts[i];
> +        PRUint8 faceIndex = (fe->IsItalic() ? kItalicMask : 0) |
> +                            (fe->Weight() >= 600 ? kBoldMask : 0);
> +        if (faces[faceIndex]) {
> +            return PR_FALSE; // two faces resolve to the same slot; family isn't "simple"
> +        }
> +        faces[faceIndex] = fe;
> +    }

Probably want to add a check to assure that all faces have the same
mStretch value, such that Arial Narrow should end up as a simple family.

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: updated to current trunk, addressed latest review comments

I think this addresses all the comments.

Regarding mPreferLaterEntries, this is there in order to maintain the current (Mac) behavior, now that there is no longer a gfxMixedFontFamily subclass with its own override of FindWeightsForStyle. The point is that for "normal" system families, we sort preferred faces to the start of the list, to avoid things like Hoefler Text Ornaments that may otherwise be mistaken for "regular". But with user font sets, later additions are meant to override earlier faces if the style match is equally good.

An alternative would have been to prepend instead of append each new entry added by @font-face, but that seems more obscure, and I guess prepending to an array is typically more expensive. So I've kept the mPreferLaterEntries flag, with an added comment.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: minor update to resolve Windows breakage

The previous version turned out to break things on Windows, the primary cause being the changed function signature of FindWeightsForStyle, which meant that its Windows override was no longer being called.

This update fixes that for the Windows and FT2 font backends, though it does not actually unify the font-selection yet, they keep all their existing separately-implemented behavior.

The patch now seems OK on tryserver (modulo intermittent tryserver brokenness) and in a local Windows build.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Have we got an automated test to catch that breakage?

Comment 30

[Jonathan Kew [:jfkthame]]

(In reply to comment #29)
> Have we got an automated test to catch that breakage?

Lots! Our existing tests did a perfectly good job of failing :) as soon as I pushed it to tryserver.

Comment 31

[John Daggett (:jtd)]

Comment on attachment 388956 [details] [diff] [review]
minor update to resolve Windows breakage

Looks good.  Since this is a big patch, probably need to coordinate when this lands.

Comment 32

[Jonathan Kew [:jfkthame]]

(In reply to comment #31)
> (From update of attachment 388956 [details] [diff] [review])
> Looks good.  Since this is a big patch, probably need to coordinate when this
> lands.

I'd suggest aiming to land this ASAP *after* 1.9.2 branches. Although I believe it would be harmless, there's no compelling reason to do it before the branch, as it doesn't give us any significant benefit by itself, it's a foundation for additional post-1.9.2 work. But we should land it soon afterwards so that there's plenty of time to shake out any issues that come up.

Comment 33

[Josh Aas]

I'm using this in a mercurial queue, any chance I could get an updated patch? There are a bunch of conflicts with current trunk. I have a feeling a merge might not be simple and I don't know much about this code.

Comment 34

[Jonathan Kew [:jfkthame]]

Attachment: updated for current trunk

Josh: here you are, I updated my local copy fairly recently. This should apply cleanly on trunk as of this morning.
(Hoping to get this landed on trunk shortly after the 1.9.2 branch.)

Comment 35

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/9d372f843f72

Comment 36

[Jonathan Kew [:jfkthame]]

This caused build bustage on win-mobile, pushed fix:
http://hg.mozilla.org/mozilla-central/rev/3d7ab97c7a3c

Comment 37

[Jonathan Kew [:jfkthame]]

Additional winmo bustage fix:
http://hg.mozilla.org/mozilla-central/rev/dd0c3f430926

Comment 38

[Masatoshi Kimura [:emk]]

Attachment: fix wchar_t issue

Your patch breaks the build with bug 508905 patch.

Comment 39

[Masatoshi Kimura [:emk]]

Comment on attachment 394873 [details] [diff] [review]
fix wchar_t issue

Thank you. Please push the patch later. I have no privilege.

Comment 40

[Takuro Ashie [:ashie]]

Attachment: Bustage fix on Qt and GTK with gfxFT2Fonts

It also breaks build on Qt and GTK with gfxFT2Fonts.

Comment 41

[Jonathan Kew [:jfkthame]]

pushed fix from comment 38:
http://hg.mozilla.org/mozilla-central/rev/cf5a75603f13

pushed fix from comment 40:
http://hg.mozilla.org/mozilla-central/rev/eb017398f8f1

Comment 42

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

(In reply to comment #41)
> pushed fix from comment 38:
> http://hg.mozilla.org/mozilla-central/rev/cf5a75603f13
> 
> pushed fix from comment 40:
> http://hg.mozilla.org/mozilla-central/rev/eb017398f8f1

In the future, when including contributors' names, it's good to include the email address as well.

Comment 43

[Jonathan Kew [:jfkthame]]

Yup - sorry about that, my mistake. I realized it when I saw how they showed up in pushlog, but by then the deed was done. :(

Comment 44

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch with followups (for 1.9.2)

This is the original patch + the few followups that landed right after it + the commits mentioned in comment 41.  I'm using this to do some work with bug 490267, which depends on this patch; this patch and that one apply cleanly to 1.9.2.  Attaching it here to save someone the work if this makes it to 1.9.2.

Comment 45

[Jonathan Kew [:jfkthame]]

Attachment: corrected version of patch with followups (for 1.9.2)

This combines the original patch with the followup fixes, and applies cleanly on 1.9.2 (previous version accidentally included an extra changeset).

Requesting 1.9.2 approval for this because a chain of additional patches that are wanted on 1.9.2 depend on it (in particular, bug 490267 and bug 514968, both wanted for mobile).

I believe this is a low-risk patch, even though it is quite large; much of the size comes from moving code up the class hierarchy in preparation for future changes, rather than major rewriting of functionality in this patch.

This has baked on trunk for several weeks without incident. IMO, it seems safer to take this patch, so that 490267 and 514968 can also go to 1.9.2, than to try and reimplement those patches to work without it.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I recommend approving this, but I'm not sure who's doing Gfx approvals...

Comment 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 399592 [details] [diff] [review]
corrected version of patch with followups (for 1.9.2)

I can do so, just wanted to make sure that jfkthame didn't have anything that he thought should stop this from going in.

Comment 48

[Takuro Ashie [:ashie]]

(In reply to comment #47)
> (From update of attachment 399592 [details] [diff] [review])
> I can do so, just wanted to make sure that jfkthame didn't have anything that
> he thought should stop this from going in.

It should also includes fix for bug 513192.
Please see the bug for details.

Comment 49

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, the latest patch there isn't correct; it doesn't fix up gfxFT2Fonts.cpp or gfxFT2Fonts.h -- specifically, the definition of FontFamily::FindWeightsForStyle doesn't match what's declared in the header.

Comment 50

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2aff74c50468

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: implement gfxPlatformFontList classes for Windows

This patch restructures code from gfxWindowsPlatform and gfxWindowsFonts to harmonize with the Mac code. There are separate font-list implementations for Win32 (gfxGDIFontList) and Mobile (gfxFT2FontList) as these platforms manage fonts in completely different ways.

This is a basis for the further font-class restructuring that allows us to implement alternative shapers on top of the low-level platform font access (i.e., the Harfbuzz work).

With this patch, the Win32 build passes all reftests on try and locally. I do not have a real testing configuration for mobile builds, but have verified that a tryserver build will launch on WinMo in the device emulator, and displays text on the initial screen, so it cannot be completely broken. :)

Note that this patch is written on top of the lazy font list patch (v5) from bug 519445, it will not apply cleanly without that.

Comment 52

[John Daggett (:jtd)]

The patch here needs a refresh (Ted's fix for 530093) but it also looks like there's a big chunk of leftover code in gfxWindowsFonts.cpp.

From line 113-265 in the patched code:

// from t2embapi.h, included in Platform SDK 6.1 but not 6.0

#ifndef __t2embapi__

  .
  .
  .

    static unsigned long ReadEOTStream(void *aReadStream, void *outBuffer, 
                                       const unsigned long aBytesToRead) 
    {
        EOTFontStreamReader *eotReader = 
                               static_cast<EOTFontStreamReader*> (aReadStream);
        return eotReader->Read(outBuffer, aBytesToRead);
    }        
        
};

Comment 53

[Jonathan Kew [:jfkthame]]

Attachment: refreshed patch for windows font list

This should apply on current trunk. Also removed the redundant code inadvertently left in gfxWindowsFonts.cpp.

Comment 54

[John Daggett (:jtd)]

Comment on attachment 420296 [details] [diff] [review]
refreshed patch for windows font list

Looks good.  Because of all the code moving here, this is tough to review but hopefully I've verified all the important code paths without letting potential regressions through.  This definitely requires thorough baking before pushing to other branches.

Plus'ing this contingent on changes noted below and confirmation of no Ts/Tp regression.

+++ b/gfx/thebes/src/gfxGDIFontList.cpp
+ *
+ * Contributor(s):
+ *   Jonathan Kew <[e-mail]>

This file contains moved code from gfxWindowsFonts.cpp so you should include the list of contributors from that file.  Same for gfxGDIFontList.h, gfxFT2FontList.cpp, and gfxFT2FontList.h.

+int CALLBACK
+GDIFontFamily::FamilyAddStylesProc(const ENUMLOGFONTEXW *lpelfe,
+                                        const NEWTEXTMETRICEXW *nmetrics,
+                                        DWORD fontType, LPARAM data)
+{
+    const NEWTEXTMETRICW& metrics = nmetrics->ntmTm;
+    LOGFONTW logFont = lpelfe->elfLogFont;
+
+    FamilyAddStyleProcData *faspd = reinterpret_cast<FamilyAddStyleProcData*>(data);
+    GDIFontFamily *ff = faspd->ff;
+    HDC hdc = faspd->dc;

The HDC isn't referenced anymore so this can be trimmed out of the FamilyAddStyleProcData struct.

+++ b/gfx/thebes/src/gfxGDIFontList.h
+class AutoDC // get the global device context, and auto-release it on destruction
+{
+public:
+
+class AutoSelectFont // select a font into the given DC, and auto-restore
+{
+public:

You've included these two classes in the public header.  Do they really need to be there, they are only referenced within gfxGDIFontList.cpp?

+            if (rv == USP_E_SCRIPT_NOT_IN_FONT) {
+                sa.eScript = SCRIPT_UNDEFINED;
+                continue;
+            }

Could you add a comment describing what this is designed to catch?  A reftest that tests the handling added here would be nice too.

-    gfxFontCache *fc = gfxFontCache::GetCache();
-    if (fc)
-        fc->AgeAllGenerations();

-    // initialize ranges of characters for which system-wide font search should be skipped
-    mCodepointsWithNoFonts.SetRange(0,0x1f);     // C0 controls
-    mCodepointsWithNoFonts.SetRange(0x7f,0x9f);  // C1 controls

Looking over the code changes made to gfxWindowsPlatform::UpdateFontList() in gfxGDIFontList::InitFontList, it looks like you missed moving the two blocks above over.

+    static gfxGDIFontList* PlatformFontList() {
+        return (gfxGDIFontList*)sPlatformFontList;
+    }

Use static_cast<>.  Same for gfxMacPlatformFontList version.

What is the performance impact of this patch?  How does the total running time of InitFontList() compare to the running time of UpdateFontList()?  Seems like there should be a slight performance improvement because ReadCMAP is called more lazily.  Does your testing confirm this?

Comment 55

[John Daggett (:jtd)]

+    PR_LOG(gFontInfoLog, PR_LOG_DEBUG, ("(fontinit-cmap) psname: %s, size: %d\n", 
+                                        NS_ConvertUTF16toUTF8(mName).get(), mCharacterMap.GetSize()));

In addition, it would be nice to have font list logging enabled in both GDIFontList and MacPlatformFontList, along the lines of the userfont logging in gfxUserFontSet.cpp.  The data for each face would be especially useful:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxMacPlatformFontList.mm#397

Comment 56

[Jonathan Kew [:jfkthame]]

Attachment: updated Windows font-list patch based on review comments

I believe this update addresses everything from comments 54 and 55, with the following notes.

> Plus'ing this contingent on changes noted below and confirmation of no Ts/Tp
> regression.

I have not seen any performance problems in local testing, but don't really have a good Windows test setup yet. Ts/Tp look fine on tryserver; obviously we'll want to check those when it actually lands, too.

> The HDC isn't referenced anymore so this can be trimmed out of the
> FamilyAddStyleProcData struct.

This leaves the struct containing nothing but one pointer, so I've simplified the code to pass that directly in the LPARAM instead.

> +++ b/gfx/thebes/src/gfxGDIFontList.h
> +class AutoDC // get the global device context, and auto-release it on
> destruction
...
> +class AutoSelectFont // select a font into the given DC, and auto-restore
> You've included these two classes in the public header.  Do they really need to
> be there, they are only referenced within gfxGDIFontList.cpp?

They will also be used by other code later (see bug 502906), so might as well be public now to save moving them in the next patch.

> +            if (rv == USP_E_SCRIPT_NOT_IN_FONT) {
> +                sa.eScript = SCRIPT_UNDEFINED;
> +                continue;
> +            }
> 
> Could you add a comment describing what this is designed to catch?  A reftest
> that tests the handling added here would be nice too.

I've added a comment. I have not run into this in actual use, but it is documented as a possible return value from ScriptShape so I felt we should handle it. It might be possible to create a custom font for testing that causes this code path to be hit, but I haven't tried that yet.

> -    gfxFontCache *fc = gfxFontCache::GetCache();
> -    if (fc)
> -        fc->AgeAllGenerations();

I've restored this code, but on looking, I see that this is only being done on the Windows platform, not in the equivalent places on others. Is it actually needed? And if so, should all platforms be doing it?

> What is the performance impact of this patch?  How does the total running time
> of InitFontList() compare to the running time of UpdateFontList()?  Seems like
> there should be a slight performance improvement because ReadCMAP is called
> more lazily.  Does your testing confirm this?

The local testing I've done hasn't shown any significant performance impact (in either direction). We'll see whether Talos shows anything when it lands.

Comment 57

[Jonathan Kew [:jfkthame]]

Pushed the Windows patch:
http://hg.mozilla.org/mozilla-central/rev/99bb0c6877f0

Comment 58

[pal-moz]

font-weight:bold seems to be broken.
http://forums.mozillazine.org/viewtopic.php?p=8407935#p8407935
http://forums.mozillazine.org/viewtopic.php?p=8408045#p8408045

this or bug 530614 cause the issue ?

Comment 59

[John Daggett (:jtd)]

(In reply to comment #58)
> font-weight:bold seems to be broken.
> http://forums.mozillazine.org/viewtopic.php?p=8407935#p8407935
> http://forums.mozillazine.org/viewtopic.php?p=8408045#p8408045
> 
> this or bug 530614 cause the issue ?

Bolding works for fonts that have bold faces but not for those that don't (the typical case with Japanese fonts).  Filed bug 538730.

Comment 60

[Jonathan Kew [:jfkthame]]

Closing this, as it's basically done, with all platforms using appropriate subclasses of gfxPlatformFontList, with the exception of desktop Linux where we rely heavily on fontconfig instead of doing our own font-matching. I'm not sure it's useful or worthwhile to try and rework that to be more like the other platforms.