340590 - Fix Windows Fonts

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Stuart Parmenter]

I've been working on fixing lots of the windows font bugs for a while.  The patch is really bigger than any one bug, so I'm going to just put my patches in here.

Comment 1

[Stuart Parmenter]

Attachment: v0.84

here's the first pass.

bugs i know it fixes are:
* bitmap fonts
* font pref window font listing bugs
* better font selection for most languages

things it doesn't fix yet:
* symbol fonts. sigh. they suck.
* items with characters in multiple charsets/languages not in a single font
* gothic text
* some spacing bugs due to character vs cluster spacing

I'll be fixing as many of these issues soon.

Comment 2

[Stuart Parmenter]

Attachment: v0.90197

Now with support for gothic and symbol fonts.

Comment 3

[Peter Kasting]

Some of your functions, like SupportsLangGroup() and SupportsCharacter(), use huge if-else-else-else-etc. sequences.  Why not a little array of the various data you need?  It would cut codesize quite a bit, and make things more readable...

Comment 4

[Stuart Parmenter]

it isn't done yet.

Comment 5

[Stuart Parmenter]

Attachment: v0.944

This version does a much better job of selecting the right font on a finer scale than the previous patch did.  It has some performance issues though so I need to go through and fix those.  It also needs a bit more cleanup.

Comment 6

[Stuart Parmenter]

Attachment: v0.98

This one is pretty close to what I want to land.  It fixes most of the performance problems with the previous patch along with some nasty bugs.  This patch produces much better font selection in most cases.

There are a couple more bug fixes that need to happen with windows fonts dealing with measuring and reshaping/rendering a single uniscribe item with multiple fonts.

Asking for initial review (since it is a kinda large patch).  Another patch or two may follow.

Comment 7

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

Comment on attachment 224980 [details] [diff] [review]
v0.98

>
>Index: cairo/cairo/src/cairo-win32-private.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/cairo/cairo/src/cairo-win32-private.h,v
>retrieving revision 1.7
>diff -u -9 -p -r1.7 cairo-win32-private.h
>--- cairo/cairo/src/cairo-win32-private.h	9 Mar 2006 02:08:06 -0000	1.7
>+++ cairo/cairo/src/cairo-win32-private.h	9 Jun 2006 08:10:40 -0000
>@@ -33,19 +33,19 @@
>  *	Owen Taylor <[e-mail]>
>  */
> 
> #ifndef CAIRO_WIN32_PRIVATE_H
> #define CAIRO_WIN32_PRIVATE_H
> 
> #include <cairo-win32.h>
> #include <cairoint.h>
> 
>-#define WIN32_FONT_LOGICAL_SCALE 32
>+#define WIN32_FONT_LOGICAL_SCALE 1

Do we want to make this a runtime configurable thing?

>Index: cairo/cairo/src/cairo-win32-surface.c
>===================================================================

>+    long width = w;
>+    long height = h;

grumble.  At least put a comment above this saying why we're doing this.


>Index: thebes/public/gfxFont.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/thebes/public/gfxFont.h,v
>retrieving revision 1.17
>diff -u -9 -p -r1.17 gfxFont.h
>--- thebes/public/gfxFont.h	27 Apr 2006 14:41:10 -0000	1.17
>+++ thebes/public/gfxFont.h	9 Jun 2006 08:10:40 -0000

> /* a SPECIFIC single font family */
> class THEBES_API gfxFont {
>     THEBES_DECL_REFCOUNTING_ABSTRACT
> 
> public:
>-    gfxFont(const nsAString &aName, const gfxFontGroup *aFontGroup);
>+    gfxFont(const nsAString &aName, const gfxFontStyle *aFontGroup);
>     virtual ~gfxFont() {}
> 
>+    const nsString& GetName() const { return mName; }

GetFamilyName() ?  or even just GetFamily()?

> class THEBES_API gfxFontGroup {
> public:

>-    typedef PRBool (*FontCreationCallback) (const nsAString& aName, const nsAString& aGenericName, void *closure);
>+    typedef PRBool (*FontCreationCallback) (const nsAString& aName, const nsACString& aGenericName, void *closure);
>+    static PRBool ForEachFont(const nsAString& aFamilies,
>+                              const nsACString& aLangGroup,
>+                              FontCreationCallback fc,
>+                              void *closure);
>     PRBool ForEachFont(FontCreationCallback fc, void *closure);

Do we still need the non-static version?  I'd rather just get rid of it and have the static one.


>Index: thebes/public/gfxWindowsFonts.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/thebes/public/gfxWindowsFonts.h,v
>retrieving revision 1.25
>diff -u -9 -p -r1.25 gfxWindowsFonts.h
>--- thebes/public/gfxWindowsFonts.h	27 Apr 2006 14:41:10 -0000	1.25
>+++ thebes/public/gfxWindowsFonts.h	9 Jun 2006 08:10:40 -0000

>+#include <bitset>

How badly do you want this? :)  Should find a few other people to OK it; I won't complain about it, but others will.

>+/* Unicode subrange table
>+ *   from: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/unicode_63ub.asp
>+*/
>+struct UnicodeRangeTableEntry {
>+    PRUint8 bit;
>+    PRUint32 start;
>+    PRUint32 end;
>+    const char *info;
>+};
>+
>+static const struct UnicodeRangeTableEntry gUnicodeRanges[] = {

>+    { 8, 0x0, 0x0, "Reserved" },
>+    { 12, 0x0, 0x0, "Reserved" },
>+    { 14, 0x0, 0x0, "Reserved" },
>+    { 27, 0x0, 0x0, "Reserved" },
>+    { 53, 0x0, 0x0, "Reserved" },
>+    { 58, 0x0, 0x0, "Reserved" },

Just comment out all the reserved entries in the table; no need to examine them while iterating over the table.

>+class FontEntry

A bit of docs -- what's a FontEntry?

>+{
>+public:
>+    THEBES_DECL_REFCOUNTING
>+
>+    FontEntry(const nsAString& aName, PRUint16 aFontType) : 
>+        mName(aName), mFontType(aFontType), mCharset(0), mWeights(0), mUnicodeRanges(0)
>+    {
>+    }
>+
>+    PRBool IsCrappyFont() const {
>+        /* return if it is a bitmap or old school font */
>+        return (mFontType == 0 || mFontType == 1);
>+    }
>+
>+    PRBool MatchesGenericFamily(const nsACString& aGeneric) const {
>+        if (aGeneric.IsEmpty())
>+            return PR_TRUE;
>+
>+        // Japanese 'Mincho' fonts do not belong to FF_MODERN even if
>+        // they are fixed pitch because they have variable stroke width.
>+        if (mFamily == FF_ROMAN && mPitch & FIXED_PITCH) {
>+            return aGeneric.EqualsLiteral("monospace");
>+        }
>+
>+        // Japanese 'Gothic' fonts do not belong to FF_SWISS even if
>+        // they are variable pitch because they have constant stroke width.
>+        if (mFamily == FF_MODERN && mPitch & VARIABLE_PITCH) {
>+            return aGeneric.EqualsLiteral("sans-serif");
>+        }

We really should be using atoms for generics, even for all font names.  However, I don't understand the relationship between the comments above these two blocks and the code themselves; you're talking about Mincho and Gothic, and then testing all fonts that are FF_MODERN and are variable pitch or FF_ROMAN and have fixed pitch?  Or are Mincho and Gothic not specific font names, but font classes? (e.g. their equivalent of Serif and Sans-Serif?)


>+#if 0
>+    class gfxWindowsFontWeight {
>+    public:
>+        PRBool HasWeight(PRUint8 aWeight) {
>+            if (IsValidNumber(aWeight)) return mWeights[aValue];
>+            return PR_FALSE;
>+        }
>+        void SetWeight(PRUint8 aValue) {
>+            if (IsValidNumber(aValue)) return mWeights[aValue];
>+            return PR_FALSE;
>+        }
>+
>+    private:
>+        static IsValidNumber(PRUint8 aValue) { return (aValue > 0 && aValue <= 9); }
>+        std::bitset<20> mWeights;
>+    };
>+    gfxWindowsFontWeight mWeights;
>+#else
>+    std::bitset<20> mWeights;
>+#endif

gfxWindowsFontWeight would be useful for other platforms as well, maybe as a generic gfxFontWeights.  Also, the bitset<20> is pretty gratuitous there -- just use a PRUint32 and do the shifting in HasWeight/SetWeight.


>+
>+    nsDataHashtable<nsStringHashKey, nsRefPtr<gfxWindowsFont> > mFontCache;

Is this cache actually useful, per-fontgroup?

>Index: thebes/src/gfxFont.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v
>retrieving revision 1.10
>diff -u -9 -p -r1.10 gfxFont.cpp
>--- thebes/src/gfxFont.cpp	14 Mar 2006 23:17:54 -0000	1.10
>+++ thebes/src/gfxFont.cpp	9 Jun 2006 08:10:40 -0000

> gfxFontGroup::gfxFontGroup(const nsAString& aFamilies, const gfxFontStyle *aStyle)
>     : mFamilies(aFamilies), mStyle(*aStyle)
> {
>-    nsresult rv;
> 
>-    /* Fix up mStyle */
>-    if (mStyle.langGroup.IsEmpty())
>-        mStyle.langGroup.AssignLiteral("x-western");
>-
>-    if (!mStyle.systemFont) {
>-        nsCOMPtr<nsIPref> prefs;
>-        prefs = do_GetService(NS_PREF_CONTRACTID);
>-        if (prefs) {
>-            nsCAutoString prefName;
>-            nsXPIDLCString value;
>-
>-            /* XXX fix up min/max size */
>-
>-            // add the default font to the end of the list
>-            prefName.AssignLiteral("font.default.");
>-            prefName.Append(mStyle.langGroup);
>-            rv = prefs->GetCharPref(prefName.get(), getter_Copies(value));
>-            if (NS_SUCCEEDED(rv) && value.get()) {
>-                mFamilies.AppendLiteral(",");
>-                AppendUTF8toUTF16(value, mFamilies);
>-            }
>-        }
>-    }
> }

This isn't windows-specific code; it shouldn't move into the windows gfxWindowsFontGroup function.  Abstract it away into a member function that any FontGroup implementation can call to get the default fonts added (have it use a FontCreationCallback/closure just like ForEachFont does).

> PRBool
> gfxFontGroup::ForEachFont(FontCreationCallback fc,
>                           void *closure)
> {
>+    return ForEachFont(mFamilies, mStyle.langGroup, fc, closure);
>+}

Yep, get rid of this, just change all the callers to use the static version.

>+PRBool
>+gfxFontGroup::ForEachFont(const nsAString& aFamilies,
>+                          const nsACString& aLangGroup,
>+                          FontCreationCallback fc,
>+                          void *closure)
>+{
>     const PRUnichar kSingleQuote  = PRUnichar('\'');
>     const PRUnichar kDoubleQuote  = PRUnichar('\"');
>     const PRUnichar kComma        = PRUnichar(',');
> 
>     nsCOMPtr<nsIPref> prefs;
>     prefs = do_GetService(NS_PREF_CONTRACTID);
> 
>+    nsPromiseFlatString families(aFamilies);

Pretty sure this isn't necessary, but maybe.

>Index: thebes/src/gfxWindowsFonts.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp,v
>retrieving revision 1.37
>diff -u -9 -p -r1.37 gfxWindowsFonts.cpp
>--- thebes/src/gfxWindowsFonts.cpp	4 May 2006 17:34:49 -0000	1.37
>+++ thebes/src/gfxWindowsFonts.cpp	9 Jun 2006 08:10:40 -0000


> /**********************************************************************
>  *
>  * class gfxWindowsFontGroup
>  *
>  **********************************************************************/
> 

I don't see anything obviously wrong here or in gfxWindowsPlatform, butI don't understand the uniscribe stuff too well.

Comment 8

[Stuart Parmenter]

(In reply to comment #7)
> >-#define WIN32_FONT_LOGICAL_SCALE 32
> >+#define WIN32_FONT_LOGICAL_SCALE 1
> 
> Do we want to make this a runtime configurable thing?
> 

No, I'm planning to remove its use everywhere in a followup patch.  > 1 breaks bitmap fonts and I haven't found a single use where it gains us anything other than a big pita.

> >Index: cairo/cairo/src/cairo-win32-surface.c
> >===================================================================
> 
> >+    long width = w;
> >+    long height = h;
> 
> grumble.  At least put a comment above this saying why we're doing this.
> 
This piece is really part of a seperate bug (339708).  Ignore it from this one.  I'll check it in seperately.

> >-    gfxFont(const nsAString &aName, const gfxFontGroup *aFontGroup);
> >+    gfxFont(const nsAString &aName, const gfxFontStyle *aFontGroup);
> >     virtual ~gfxFont() {}
> > 
> >+    const nsString& GetName() const { return mName; }
> 
> GetFamilyName() ?  or even just GetFamily()?

I don't have any strong feelings here but I'm pretty happy with GetName().


> 
> > class THEBES_API gfxFontGroup {
> > public:
> 
> >-    typedef PRBool (*FontCreationCallback) (const nsAString& aName, const nsAString& aGenericName, void *closure);
> >+    typedef PRBool (*FontCreationCallback) (const nsAString& aName, const nsACString& aGenericName, void *closure);
> >+    static PRBool ForEachFont(const nsAString& aFamilies,
> >+                              const nsACString& aLangGroup,
> >+                              FontCreationCallback fc,
> >+                              void *closure);
> >     PRBool ForEachFont(FontCreationCallback fc, void *closure);
> 
> Do we still need the non-static version?  I'd rather just get rid of it and
> have the static one.
> 

If other platforms don't need the non-static version, thats fine, but I'd prefer to do changes to other platforms as seperate patches (Except where I may have broken things with this patch).

> 
> >Index: thebes/public/gfxWindowsFonts.h
> >===================================================================
> >RCS file: /cvsroot/mozilla/gfx/thebes/public/gfxWindowsFonts.h,v
> >retrieving revision 1.25
> >diff -u -9 -p -r1.25 gfxWindowsFonts.h
> >--- thebes/public/gfxWindowsFonts.h	27 Apr 2006 14:41:10 -0000	1.25
> >+++ thebes/public/gfxWindowsFonts.h	9 Jun 2006 08:10:40 -0000
> 
> >+#include <bitset>
> 
> How badly do you want this? :)  Should find a few other people to OK it; I
> won't complain about it, but others will.
> 

I'm not stuck on bitset, but as we don't have anything to replace it with, I think I'll stick with it for now.  I can write something up later to replace it with.  It won't be the first time that the Windows font code has used std:: stuff.

> >+/* Unicode subrange table> 
> >+    { 8, 0x0, 0x0, "Reserved" },
> >+    { 12, 0x0, 0x0, "Reserved" },
> >+    { 14, 0x0, 0x0, "Reserved" },
> >+    { 27, 0x0, 0x0, "Reserved" },
> >+    { 53, 0x0, 0x0, "Reserved" },
> >+    { 58, 0x0, 0x0, "Reserved" },
> 
> Just comment out all the reserved entries in the table; no need to examine them
> while iterating over the table.
> 

Done.

> >+class FontEntry
> 
> A bit of docs -- what's a FontEntry?
> 
I'll add something.


> >+{
> >+public:
> >+    THEBES_DECL_REFCOUNTING
> >+
> >+    FontEntry(const nsAString& aName, PRUint16 aFontType) : 
> >+        mName(aName), mFontType(aFontType), mCharset(0), mWeights(0), mUnicodeRanges(0)
> >+    {
> >+    }
> >+    PRBool MatchesGenericFamily(const nsACString& aGeneric) const {
> >+        if (aGeneric.IsEmpty())
> >+            return PR_TRUE;
> >+
> >+        // Japanese 'Mincho' fonts do not belong to FF_MODERN even if
> >+        // they are fixed pitch because they have variable stroke width.
> >+        if (mFamily == FF_ROMAN && mPitch & FIXED_PITCH) {
> >+            return aGeneric.EqualsLiteral("monospace");
> >+        }
> >+
> >+        // Japanese 'Gothic' fonts do not belong to FF_SWISS even if
> >+        // they are variable pitch because they have constant stroke width.
> >+        if (mFamily == FF_MODERN && mPitch & VARIABLE_PITCH) {
> >+            return aGeneric.EqualsLiteral("sans-serif");
> >+        }
> 
> However, I don't understand the relationship between the comments above these
> two blocks and the code themselves; you're talking about Mincho and Gothic, and
> then testing all fonts that are FF_MODERN and are variable pitch or FF_ROMAN
> and have fixed pitch?  Or are Mincho and Gothic not specific font names, but
> font classes? (e.g. their equivalent of Serif and Sans-Serif?)
> 

I think those are kind of weird as well -- they're from the old font code.  I'm going to remove them and do some testing and see what happens.


> 
> >+#if 0
> >+    class gfxWindowsFontWeight {
> >+    public:
> >+        PRBool HasWeight(PRUint8 aWeight) {
> >+            if (IsValidNumber(aWeight)) return mWeights[aValue];
> >+            return PR_FALSE;
> >+        }
> >+        void SetWeight(PRUint8 aValue) {
> >+            if (IsValidNumber(aValue)) return mWeights[aValue];
> >+            return PR_FALSE;
> >+        }
> >+
> >+    private:
> >+        static IsValidNumber(PRUint8 aValue) { return (aValue > 0 && aValue <= 9); }
> >+        std::bitset<20> mWeights;
> >+    };
> >+    gfxWindowsFontWeight mWeights;
> >+#else
> >+    std::bitset<20> mWeights;
> >+#endif
> 
> gfxWindowsFontWeight would be useful for other platforms as well, maybe as a
> generic gfxFontWeights.  Also, the bitset<20> is pretty gratuitous there --
> just use a PRUint32 and do the shifting in HasWeight/SetWeight.
> 
> 

I'm going to take this stuff off the FontEntry.  It is unused at the moment.  There is just no way to look up a FontEntry for all the font names that are passed in that may work (fonts can have multiple names, the windows font mappers can take partial font names, etc).  I'm going to create a seperate hashtable for weights that will be keyed off whatever the font's name is.  This is another reason to atomize font names -- something I'm planning on doing seperately as well.



> >+
> >+    nsDataHashtable<nsStringHashKey, nsRefPtr<gfxWindowsFont> > mFontCache;
> 
> Is this cache actually useful, per-fontgroup?
> 
Yeah, it is super useful due to the way we handle fallback fonts.  The group has its list of fonts that were passed in, so it holds a reference to those, but nothing holds a reference to other fonts that were found to support whatever characters we need to draw.  Since in many cases, one UniscribeItem after another will use the same fonts, having them already created and in that list is a huge speedup verses having to recreate the HFONT and cairo font stuff each time.

> >Index: thebes/src/gfxFont.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v
> >retrieving revision 1.10
> >diff -u -9 -p -r1.10 gfxFont.cpp
> >--- thebes/src/gfxFont.cpp	14 Mar 2006 23:17:54 -0000	1.10
> >+++ thebes/src/gfxFont.cpp	9 Jun 2006 08:10:40 -0000
> 
> > gfxFontGroup::gfxFontGroup(const nsAString& aFamilies, const gfxFontStyle *aStyle)
> >     : mFamilies(aFamilies), mStyle(*aStyle)
> > {
> >-    nsresult rv;
> > 
> >-    /* Fix up mStyle */
> >-    if (mStyle.langGroup.IsEmpty())
> >-        mStyle.langGroup.AssignLiteral("x-western");
> >-
> >-    if (!mStyle.systemFont) {
> >-        nsCOMPtr<nsIPref> prefs;
> >-        prefs = do_GetService(NS_PREF_CONTRACTID);
> >-        if (prefs) {
> >-            nsCAutoString prefName;
> >-            nsXPIDLCString value;
> >-
> >-            /* XXX fix up min/max size */
> >-
> >-            // add the default font to the end of the list
> >-            prefName.AssignLiteral("font.default.");
> >-            prefName.Append(mStyle.langGroup);
> >-            rv = prefs->GetCharPref(prefName.get(), getter_Copies(value));
> >-            if (NS_SUCCEEDED(rv) && value.get()) {
> >-                mFamilies.AppendLiteral(",");
> >-                AppendUTF8toUTF16(value, mFamilies);
> >-            }
> >-        }
> >-    }
> > }
> 
> This isn't windows-specific code; it shouldn't move into the windows
> gfxWindowsFontGroup function.  Abstract it away into a member function that any
> FontGroup implementation can call to get the default fonts added (have it use a
> FontCreationCallback/closure just like ForEachFont does).

Having this use a callback kind of sucks.  While I agree this code is probably not 100% windows specific, I don't think abstracting it away is very helpful.

> 
> >+PRBool
> >+gfxFontGroup::ForEachFont(const nsAString& aFamilies,
> >+                          const nsACString& aLangGroup,
> >+                          FontCreationCallback fc,
> >+                          void *closure)
> >+{
> >     const PRUnichar kSingleQuote  = PRUnichar('\'');
> >     const PRUnichar kDoubleQuote  = PRUnichar('\"');
> >     const PRUnichar kComma        = PRUnichar(',');
> > 
> >     nsCOMPtr<nsIPref> prefs;
> >     prefs = do_GetService(NS_PREF_CONTRACTID);
> > 
> >+    nsPromiseFlatString families(aFamilies);
> 
> Pretty sure this isn't necessary, but maybe.

It is.

Comment 9

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

(In reply to comment #8)
> > >Index: thebes/src/gfxFont.cpp
> > >===================================================================
> > >RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v
> > >retrieving revision 1.10
> > >diff -u -9 -p -r1.10 gfxFont.cpp
> > >--- thebes/src/gfxFont.cpp	14 Mar 2006 23:17:54 -0000	1.10
> > >+++ thebes/src/gfxFont.cpp	9 Jun 2006 08:10:40 -0000
> > 
> > > gfxFontGroup::gfxFontGroup(const nsAString& aFamilies, const gfxFontStyle *aStyle)
> > >     : mFamilies(aFamilies), mStyle(*aStyle)
> > > {
> > >-    nsresult rv;
> > > 
> > >-    /* Fix up mStyle */
> > >-    if (mStyle.langGroup.IsEmpty())
> > >-        mStyle.langGroup.AssignLiteral("x-western");
> > >-
> > >-    if (!mStyle.systemFont) {
> > >-        nsCOMPtr<nsIPref> prefs;
> > >-        prefs = do_GetService(NS_PREF_CONTRACTID);
> > >-        if (prefs) {
> > >-            nsCAutoString prefName;
> > >-            nsXPIDLCString value;
> > >-
> > >-            /* XXX fix up min/max size */
> > >-
> > >-            // add the default font to the end of the list
> > >-            prefName.AssignLiteral("font.default.");
> > >-            prefName.Append(mStyle.langGroup);
> > >-            rv = prefs->GetCharPref(prefName.get(), getter_Copies(value));
> > >-            if (NS_SUCCEEDED(rv) && value.get()) {
> > >-                mFamilies.AppendLiteral(",");
> > >-                AppendUTF8toUTF16(value, mFamilies);
> > >-            }
> > >-        }
> > >-    }
> > > }
> > 
> > This isn't windows-specific code; it shouldn't move into the windows
> > gfxWindowsFontGroup function.  Abstract it away into a member function that any
> > FontGroup implementation can call to get the default fonts added (have it use a
> > FontCreationCallback/closure just like ForEachFont does).
> 
> Having this use a callback kind of sucks.  While I agree this code is probably
> not 100% windows specific, I don't think abstracting it away is very helpful.

Why?  You do the exact same code in gfxWindowsFontGroup::gfxWindowsFontGroup (actually better code, since the old one didn't handle font.name.Foo.lang), except that instead of appending it to the families list, you call MakeFont() directly on the result and append it to the mFonts list.  This is what should happen anyway; appending the defaults to the mFamilies string was a hack.  It would also protect us from malformed family strings (e.g. the "'serif" crasher), because the known-valid default fonts would always be present (on all platforms).

Comment 10

[Justin Kerk]

(In reply to comment #7)
> Or are Mincho and Gothic not specific font names, but
> font classes? (e.g. their equivalent of Serif and Sans-Serif?)

That's exactly what they are (mincho is serif and gothic is sans-serif), although on Windows you can assume the built-in Windows fonts MS Mincho / MS PMincho and MS Gothic / MS PGothic (the P versions being proportional-width).

I assume from that code that MS Mincho and MS PGothic report their family in a way that's different from what Japanese users expect (e.g. MS PGothic should be the default sans-serif font).

Comment 11

[Stuart Parmenter]

Attachment: v0.99

Fixes everything but the fontgroup generic stuff vlad requested.  Another patch coming shortly.

Comment 12

[Stuart Parmenter]

Attachment: 0.9999

I think we're ready to go in with this.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to comment #10)
> (In reply to comment #7)
> > Or are Mincho and Gothic not specific font names, but
> > font classes? (e.g. their equivalent of Serif and Sans-Serif?)
> 
> That's exactly what they are (mincho is serif and gothic is sans-serif),
> although on Windows you can assume the built-in Windows fonts MS Mincho / MS
> PMincho and MS Gothic / MS PGothic (the P versions being proportional-width).
> 
> I assume from that code that MS Mincho and MS PGothic report their family in a
> way that's different from what Japanese users expect (e.g. MS PGothic should be
> the default sans-serif font).

Yes, this is Bug 321132. It's very important for Japanese users.

Comment 14

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

Comment on attachment 225094 [details] [diff] [review]
0.9999

r+ based on previous review and irc conversation; do make sure it doesn't blow up linux though :)

Comment 15

[Stuart Parmenter]

checked in -- please file new bugs for any followup issues.

Comment 16

[Jean-Marc Desperrier]

Sorry, but I have one comment about the patch.

Shouldn't the info member of the gUnicodeRanges structure either :
- be removed
- be localisable

My opinon is it should go away. Such a table is useful, but not hidden deep inside the font code.

Comment 17

[cls]

This checkin caused mingw bustage (bug 341128).