542162 - [@font-face] src local lookup fails for some locales

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

When the default system locale on Windows is set to something other than English, src local fullname lookups within @font-face rules sometimes fail.

Steps to reproduce:

1. Switch the default system locale to Dutch (Holland)
2. Run Minefield
3. Click on the URL

Result: the second and third lines do not display in Arial Bold and Arial Italic, respectively.

Expected results: the three lines display using normal, bold and italic faces of Arial.

The GDI lookup of 'Arial Bold' and 'Arial Italic' is failing in this situation at this point in the code:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxGDIFontList.cpp#652

The docs for CreateFontIndirect indicate that it should find the English name:

"The font mapper for CreateFont,CreateFontIndirect, and CreateFontIndirectEx  recognizes both the English and the localized typeface name, regardless of locale."

http://msdn.microsoft.com/en-us/library/dd183499%28VS.85%29.aspx

Reproduced both on XP and Win7.

Comment 1

[John Daggett (:jtd)]

With the system locale set to Dutch, GDI allows lookups based on 'Arial Vet' but not 'Arial Bold'.  Argh...  

Experimented with various API's, using GetThreadLocale/SetThreadLocale and using CreateFontIndirectEx instead of CreateFontIndirect, none of these appear to make a difference.

We may need to set up a hashtable for these after all on Windows (both GDI and DirectWrite).  The logic should probably be incorporated into the ReadOtherFamilyNames procedure since that's already reading in the name table.

Comment 2

[John Daggett (:jtd)]

Attachment: patch, v.0.1, set up fullname and psname hash tables

Sets up two hash tables for fullname and Postscript name lookups.  I reworked the ReadOtherFamilyName routines to integrate in these lookups with the existing name table reads.  Eliminated the AddOtherFamilyNameFunctor class, passing in a platform font list pointer instead.  Platforms that don't require these lookup tables can skip this by setting the mNeedFullnamePostscriptNames flag to false.

I'll rework the GDI local lookup routine tomorrow.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, v.0.2, GDI local lookups using lookup hash tables

Adds code to gfxGDIFontList::LookupLocalFont to lookup the font entry by the Postscript name or fullname, then uses the face name from that font entry in creating the new font entry.

There's some subtle GDI usage here, when reading over the faces in a given family (FamilyAddStylesProc) the elfFullName field of the ENUMLOGFONTEX structure passed in for each face contains the localized face name while the lfFaceName in the LOGFONT structure contains the family name.  The localized GDI face name can be used to lookup a specific face.  With the system locale set to Dutch, 'Arial Vet' can be used to lookup the bold face of Arial but 'Arial Bold' cannot.  This is despite the MSDN docs saying either the localized or English name should work.

Also corrected an error in one of the reftests, the wrong Postscript name for Arial Italic was being used.

Comment 4

[John Daggett (:jtd)]

Tryserver build of v0.2 patch:
https://build.mozilla.org/tryserver-builds/[e-mail]-fullnamelookup/

Passed win32 reftests.

Tested on XP and Win7 with system locale set to:
Dutch(Holland), Czech, French(France), German(Switzerland), Japanese (OS default)

Testcase succeeds across all these locales.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 423941 [details] [diff] [review]
patch, v.0.2, GDI local lookups using lookup hash tables

Seems fine overall - I didn't actually run tests but your local and tryserver tests should be ok. Just a few comments below re possible adjustments that I think would improve things.

> void
> gfxFT2FontList::InitFontList()
> {
>     mFontFamilies.Clear();
>     mOtherFamilyNames.Clear();
>     mOtherFamilyNamesInitialized = PR_FALSE;
>+    mFullnames.Clear();
>+    mPostscriptNames.Clear();
>+    mFaceNamesInitialized = PR_FALSE;
>     mPrefFonts.Clear();
>     CancelLoader();
> 
>     // initialize ranges of characters for which system-wide font search should be skipped
>     mCodepointsWithNoFonts.reset();
>     mCodepointsWithNoFonts.SetRange(0,0x1f);     // C0 controls
>     mCodepointsWithNoFonts.SetRange(0x7f,0x9f);  // C1 controls

I'm getting increasingly unhappy with the amount of this code that's duplicated across the various platform InitFontList() implementations; while you're here, maybe it'd be worth pushing all the "common" stuff into a gfxPlatformFontList method, and let each of the specific implementations call that and then do their own custom stuff in addition?

Actually, we already have the UpdateFontList() method in gfxPlatformFontList that simply forwards to InitFontList(); perhaps we could just move the resetting of the various generic hashes into UpdateFontList before it calls the platform-specific InitFontList methods.


> gfxPlatformFontList::gfxPlatformFontList()
>     : mStartIndex(0), mIncrement(kNumFontsPerSlice), mNumFamilies(0)
> {
>     mFontFamilies.Init(100);
>     mOtherFamilyNames.Init(30);
>     mOtherFamilyNamesInitialized = PR_FALSE;
>+
>+    mFullnames.Init(100);
>+    mPostscriptNames.Init(100);
>+    mFaceNamesInitialized = PR_FALSE;
>+    mNeedFullnamePostscriptNames = PR_TRUE;

It seems a pity to Init() the fullname and psname hashes in the case where they're not actually going to be used (true, that only applies on Mac OS X at the moment, but still....).

How about simply giving the gfxPlatformFontList constructor a boolean parameter specifying whether these hashes are needed, and using that to initialize mNeedFullnamePostscriptNames? The parameter could even have a default of TRUE to save having to explicitly add it in all the subclasses. Then this constructor would know whether it needs to Init the hashes.


>+    // name lookup table methods
>+
>+    virtual void AddOtherFamilyName(gfxFontFamily *aFamilyEntry, nsAString& aOtherFamilyName);
>+
>+    virtual void AddFullname(gfxFontEntry *aFontEntry, nsAString& aFullname);
>+
>+    virtual void AddPostscriptName(gfxFontEntry *aFontEntry, nsAString& aPostscriptName);

Is there any reason to make these virtual? I don't think so.


>+    static PLDHashOperator InitFaceNameListsProc(nsStringHashKey::KeyType aKey,
>+                                                    nsRefPtr<gfxFontFamily>& aFamilyEntry,
>+                                                    void* userArg);

Adjust indentation.

Comment 6

[John Daggett (:jtd)]

Attachment: patch, v.0.2a, revisions based on review comments

Conditionally initialize fullname and Postscript name lookup tables.  Move common init code into base class method and call from derived classes of gfxPlatformFontList.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 423985 [details] [diff] [review]
patch, v.0.2a, revisions based on review comments

Looks good to me. One nit caught my eye - a couple of the comments here seem to have acquired a bit of extra indent, so it'd be good to fix that when pushing:

>-    // canonical family name ==> family entry (unique, one name per family entry)
>-    nsRefPtrHashtable<nsStringHashKey, gfxFontFamily> mFontFamilies;    
>+      // canonical family name ==> family entry (unique, one name per family entry)
>+    nsRefPtrHashtable<nsStringHashKey, gfxFontFamily> mFontFamilies;

>-    // family entry, only names *other* than the canonical names are stored here)
>-    nsRefPtrHashtable<nsStringHashKey, gfxFontFamily> mOtherFamilyNames;    
>+    // other family name ==> family entry (not unique, can have multiple names per
>+      // family entry, only names *other* than the canonical names are stored here)
>+    nsRefPtrHashtable<nsStringHashKey, gfxFontFamily> mOtherFamilyNames;

Comment 8

[John Daggett (:jtd)]

Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/f48bbee9fd0c

Comment 9

[John Daggett (:jtd)]

(In reply to comment #7)
> (From update of attachment 423985 [details] [diff] [review])
> Looks good to me. One nit caught my eye - a couple of the comments here seem to
> have acquired a bit of extra indent, so it'd be good to fix that when pushing:

Argh, somehow missed this at checkin, will fix this up next checkin.