190031 - use xft and fontconfig for font lookups/measuring/drawing with new postscript/freetype code

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Christopher Blizzard (:blizzard)]

Need to figure out how to integrate Xft properly with the new
PostScript/FreeType code that's in the tree.  Shouldn't be too hard.

Comment 1

[Mike FABIAN]

Any news here? Did anybody try to get printing with fontembedding work in Xft2 builds of Mozilla?

Comment 2

[Jungshik Shin]

Not here, but now Xft build with freetype enabled can do what you want. (bug
219060). Because two different font lookup code paths are taken for the screen
rendering (fontconfig) and the printing(that of GFX:Gtk w/ FT2), the result is
not strictly WYSWYG, but nonetheless a lot better than just plain PS printing
module gives us (especially for CJK). See also bug 211763 (well, nothing has
been done there, either)
 

Comment 3

[Jungshik Shin]

This is a follow-up to bug 215219 comment #59 (and 62, 63, 64)

The default PS module is barely usable for non-Western-European text (only one -
i.e. no distinction between bold,italic, monospace, serif, and sans-serif - CID
or OCF can be specified, which has nothing to do with fonts used in the screen
rendering). Even for Western-European text, it's not so good because it uses a 
set of hard-coded type1 fonts (as opposed to what's used for the screen
rendering) as pointed out by bug 215219 comment #64. 

There's a FT2 (truetype) printing module that works a lot better than the
default PS printing module. Although its font selection algorithm is different
from Xft+fontconfig, it's close enough to give a rather good WYSWYG print-out.
Bug 190031 is about changing the font selection algorithm of FT2 printing module
to fontconfig and hooking up Xft with FT2 printing module.

 Besides, with bug 208213 fixed (I made a patch a long time ago, but bstell and
Sun people haven't responded to my request for review), it can be a little
better. See also bug 229378 and bug 228911.

If we're really desperate,   we might consider duplicating a whole bunch of code
in nsFontMetricsXft in nsFontMetricsPS.cpp (bug 229378 comment #5). The desire
to avoid that has been holding  me back. As an alternative, I came up with
hooking up Xft and Xprint (bug 211763),but hasn't manage to do any work.
 
blizzard, if we can come up with a way to factor out a bulk of code we have in
nsFontMetricsXft so that they can be used in both nsFontMetricsXft and
nsFontMetricsPS (without too complex class hierarchy), that's the way to go

Comment 4

[Jungshik Shin]

Attachment: patch uploaded to bug 249542 by Ervin Yan

i'm gonna make bug 249542 a dupe of this

Comment 5

[Jungshik Shin]

*** Bug 249542 has been marked as a duplicate of this bug. ***

Comment 6

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

This patch seems to depend on --enable-freetype2 being specified.  Shouldn't
this be able to work even without that?  IMO, the eventual goal would be to ship
builds that can use Fontconfig for both screen display and print display and
that would not even contain the code for using mozilla preferences to find fonts
that --enable-freetype2 currently enables.

Does this patch actually depend on freetype2 to use fontconfig for printing, or
was it just #ifdef-ed that way?

Comment 7

[Jungshik Shin]

(In reply to comment #6)

> Does this patch actually depend on freetype2 to use fontconfig for printing,

Yes, it does. Take a look at the second half of the patch. 

Comment 8

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

How much of the other code that's enabled by --enable-freetype2 does it require?

Comment 9

[Jungshik Shin]

(In reply to comment #8)
> How much of the other code that's enabled by --enable-freetype2 does it require?

I have yet to take a detailed look. Anyway, let me try. The code in
nsFontMetricsGTK.cpp/h enabled by --enable--freetype2 is not required at all for
the obvious reason. It doesn't require |nsITrueTypeFontCatalogEntry| either
because it uses fontconfig that works a lot better. On the other hand, it relies
on |nsIFreeType|. However, we can easily get rid of that dependency because in
Xft-Gtk2 build, we already assume that freetype library is avaialble at run-time
on the system so that there's no reason to call freetype APIs _indirectly_ via
nsIFreeType. We can just invoke freetype2 APIs directly. 

Btw, the code in nsFontMetricsXft is duplicated in  nsFontMetricsXft, which I
mentioned that I would like to avoid in comment #3. Because not all the font
selection logic and drawing/measuring logic in nsFontMetricsXft are duplicated
here, MathML and custom fonts are not supported in printing with this patch. We
might live with the code duplication (which is moderate) for the now and later
factor them out so that both nsFontMetricsXft.cpp and nsFontMetricsPS.cpp share
the code.

Comment 10

[Jungshik Shin]

gfx/src/ps/nsType[18].cpp also depend on nsIFreeType, but the dependency can be
removed easily (although it'd not be pretty with #ifdef's sprinkled here and
there if we want to get Gtk1/X11/Xlib built the way they're built now)

Comment 11

[Jungshik Shin]

Attachment: patch that doesn't depend on nsIFreetype

With this patch, Mozilla compiled with enable-xft but not with enable-freetype2
can print invoking freetype2 APIs directly along with Xft/fontconfig APIs.

I haven't yet tested a build with this patch (I updated freetype library to
2.1.8 and bug 234035 has to be fixed).

Comment 12

[Jungshik Shin]

I've just tested the patch (attachment 152298 [details] [diff] [review]) and it worked in that it got
compiled and produced a 'printable-PS' without 'enable-freetype'. However, I
realized that my naive assumption that its font selection code comes from that
of nsFontMetricsXft.cpp was totally wrong. Currently, it doesn't take into
account  user-pref. values and author-specified fonts (font-family in CSS or
font-face) at all, which makes the print-out very different from the screen
rendering. With this realization, we have to figure out how to avoid the
duplication between nsFontMetricsXft.cpp and nsFontMetricsPS.cpp. Would it be
acceptable to copy a bulk of the code from the fomer to the latter for the now? 

Comment 13

[Christopher Blizzard (:blizzard)]

Use the same code paths?  Add some methods to nsFontMetricsXFT.cpp that just
return the glyph stream instead of duplicating?

Comment 14

[Ervin Yan]

Attachment: patch to add user pref and CSS font support

patch to add user pref and CSS font support.

you can build mozilla with --enable-xft and --disable-freetype2.

I copy the following functions from gtk/nsFontMetricsXft.cpp:

static int	CalculateSlant	 (PRUint8  aStyle);
static int	CalculateWeight  (PRUint16 aWeight);
static void	AddLangGroup	 (FcPattern *aPattern, nsIAtom *aLangGroup);
static void	AddFFRE 	 (FcPattern *aPattern, nsCString *aFamily,
				  PRBool aWeak);
static void	FFREToFamily	 (nsACString &aFFREName, nsACString &oFamily);
static int	FFRECountHyphens (nsACString &aFFREName);
static PRBool	IsASCIIFontName  (const nsString& aName);
const MozXftLangGroup* FindFCLangGroup (nsACString &aLangGroup)

Comment 15

[Jungshik Shin]

Attachment: attachment 153378 plus a couple of extra patches

thanks for updating the patch. 

You seem to have forgotten to remove 'gXftFontLoad' when copying code from
nsFontMetricsXft.cpp. Replacing it with 'gFontMetricsPSM' got it compiled. 

This patch includes a couple of extra changes but doesn't include the change in
all.js because I made nsDeviceContextPS not refer to font.FreeType2.enable any
more. 

With 'disable-freetype2' and 'enable-xft', printing works well respecting CSS
and user-pref fonts.

Comment 16

[Ervin Yan]

jshin:  I can not pass the compilation for your new patch. seems we need
consider more to completely disable freetype2 building.

Comment 17

[Jungshik Shin]

Ervin, thanks for testing. I suspected something could go wrong and tried to
compile the whole source. I left it over the weekend and found today that indeed
the build failed as you found. It turned out that there are some other code
blocks in gfx/src to enclose with '#ifdef MOZ_ENABLE_FREETYPE2'. I've just done
that, but I still can't build (files in x11shared directory are giving me a hard
time. they're exported to gfx/src/gtk at the compilation time and I excluded
freetype related files from export, but somehow they're still exported). I'm now
building from the scratch (after distclean).

Comment 18

[Jungshik Shin]

Attachment: another attempt

As far as rendering with Xft and printing with fontconfig + freetype (type9
fonts), this patch works well. Getting rid of the dependence on FT2 is a lot
more involved than I originally thought.

Anyway, there are some more tests to conduct. With 'GDK_XFT=0', my build with
this patch (enable-xft, disable-freetype2) dumps core, which I expected because
Ithere was one spot in nsFontMetricsGTK.cpp which I suspected is problematic.
We also have to test a few different compile-time configurations including
mozilla.org's default (enable-freetype, but disable-xft, toolkit=gtk1).

Comment 19

[Ervin Yan]

jshin: Can we first create workable patch to enable-xft, and then log another
bug to disable-freetype2? 

Comment 20

[Jungshik Shin]

Attachment: another patch

This is identical to attachment 153667 [details] [diff] [review] except that I moved one misplaced
'#endif' to its proper location (that was the cause of the crash when I
observed when running my build with GDK_USE_XFT=0). I also removed 'pollution'
from other unrelated patches. 

I have yet to test it with disable-xft, enable-freetype2 -(the default
mozilla.org build configuration), but I don't think there's gonna be any
problem with that.

Comment 21

[Ervin Yan]

jshin:  just one comment for attachment 153794 [details] [diff] [review]:

in gfx/src/gtk/Makefile.in file, the line:

+LocaL_INCLUDES        += -I$(srcdir)/../freetype

seems should be:

+LOCAL_INCLUDES        += -I$(srcdir)/../freetype

Comment 22

[Ervin Yan]

I can not build the patch(attachment 153794 [details] [diff] [review]) with the following options:
"--disable-xft --enable-freetype2" and 
"--disable-xft --disable-freetype2". 

need more investigation on it.

Comment 23

[Ervin Yan]

Attachment: patch to fix "disable-xft" build problem

update the following files to fix "--disable-xft --disable-freetype2" and
"--disable-xft --enable-freetype2" building problems:
ps/nsFontMetricsPS.cpp
ps/nsFontMetricsPS.h
ps/nsType1.cpp
ps/nsType1.h
ps/nsType8.h

Comment 24

[Jungshik Shin]

Attachment: another patch

I was also working on this. It seems like attachment 153875 [details] [diff] [review] didn't fix my typos
(one of them was  'MOZ_ENABLE_FREETYPE' in place of 'MOZ_ENABLE_FREETYPE2').
With this patch, I built using the mozilla.org default (enable-freetype2,
toolkit=gtk, disable-xft) configuration as well as (disable-freetype2,
toolkit=gtk, disable-xft) and (toolkit=gtk2, enable-xft, disable-freetype2). It
turned out that there are a couple of more changes necessary for 
(disable-freetype2, toolkit=gtk, disable-xft)

I also changed configure.in to block building with enable-xft and
enable-freetype2. 

Btw, my build for disable-freetype2, disable-xft didn't go through to the end
(although gfx part went through fine) because I ran out of the disk space.

Comment 25

[Jungshik Shin]

Comment on attachment 153894 [details] [diff] [review]
another patch 

This patch has been tested for the following configurations:

1. enable-xft, disable-freetype2. 
  at the run-time, with GDK_USE_XFT=0 and GDK_USE_XFT not defined (or '1')

2. disable-xft, enable-freetype2 (the default compile-time option for the
mozilla.org build)

3. disable-xft, disable-freetype2

As noted earlier, 'enable-xft and enable-freetype2' is not supported and
explicitly blocked in configure.in

Comment 26

[Ervin Yan]

jshin: seems this patch include some patches for other bugs.
for example, in gfx/src/nsFontMetricsXft.cpp file, some new lines are not
related to this bug.

Also, when I test this patch, Mozilla always crash when I try to click font
preference via "Edit"-->"Preference"-->"Appearance"-->"Font".

Comment 27

[Jungshik Shin]

I should have deleted the patch for nsFontMetricsXft.cpp (that's for another
bug) in attachment 153894 [details] [diff] [review].

As for the crash, I couldn't reproduce it in gtk2+xft (without FT2) and  gtk1 +
x11core (without FT2). I don't have a gtk1 + X11core + FT2 build at the moment,
but I tried Edit | Pref | Appreance | Font (when I had it) and mozilla didn't crash.
My builds are a few days old, which may have made the difference.

Anyway, can you upload the stack backtrace? 
  

Comment 28

[Ervin Yan]

after recheck out all the sources and apply this path (attachment 153894 [details] [diff] [review]),
mozilla do not crash now.

Comment 29

[Jungshik Shin]

Attachment: same as above with patches for other bugs removed

A couple of patches for other bugs crept into attachment 153894 [details] [diff] [review]. I got rid of
them.

Comment 30

[Jungshik Shin]

Attachment: another patch (with the code duplication removed)

I made two new files (nsFontConfigUtils.h/cpp) in gfx/src/shared so that we
don't have to keep two copies of static functions in nsFontMetricsXft.cpp and
nsFontMetricsPS.cpp.

Comment 31

[Jungshik Shin]

Attachment: another patch ( the same except for a little shuffling and a little clean-up)

In nsFontMetricsGTK.cpp and nsFontMetricsXlib.cpp, I enclose all
nsFontCharSetInfo's for freetype with '#ifdef MOZ_ENABLE_FREETYPE2' while
putting nsFontCharsetInfo's for non-freetype in '#else'-part. This makes it
easier to read the patch. 

In nsFontMetricsPS.cpp, I changed the indentation of lines copied from
nsFontMetricsXft.cpp to follow the local convention. In addition, I replaced
'aPattern/aLangGroup' with 'pattern/langGroup' (because both are local
variables), got rid of a couple of unnecessary lines, and 'modernize' String
class usage a bit (nsString->nsAutoString, AppendWithConversion ->
LossyAppend....to...).

Comment 32

[Jungshik Shin]

Comment on attachment 154916 [details] [diff] [review]
another patch ( the same except for a little shuffling and a little clean-up)

Asking for r/sr.

I'm quite satisfied with the patch althouhg there are something to improve. 
For instance, it doesn't yet support MathML and custom fonts, which requires a
rather drastic rewrite of nsFontMetricsPS.cpp along the line of blizzard's
comment #13. I think it's too late to do any drastic change in the printing
code if we want to have a better printing in firefox 1.0. Even for mozilla 1.8,
I'm not sure if we can. Given this, I think we should go ahead with this patch
for now.

The code duplication is minimal because I factored the common part into a
separate file nsFontConfigUtils.cpp (I may have to prefix functions in the file
with 'mozFc' or something).

Comment 33

[Ervin Yan]

Attachment: patch to fix some problems on Solaris

jshin: Here is the patch to fix some problems on Solaris which based on your
last patch (attachment 154916 [details] [diff] [review]).

I just update gfx/src/ps/nsFontMetricsPS.{cpp|h} files to fix:
1. skip non truetype fonts.
2. fix potential memory leak problems when no suitable fonts for characters.

Comment 34

[Jungshik Shin]

Comment on attachment 154966 [details] [diff] [review]
patch to fix some problems on Solaris

Thanks for the update.
A few minor problems:

You keep using 'aXyzXyz' for local variables (aEntry and one more variable). In
Mozilla, 'a' prefix is used for function arguments. Local variables should be
named like 'xyzXyz'. 

In |DestroyXftEntry|, NS_ASSERTION would be better for null-checking than
NS_ENSURE_TRUE because the only call-site already checks for null. Along with
that change, it can be made to return void instead of nsresult. 

As for functions in nsFontConfigUtils.cpp, I found 'NS_' prefix was used in
similar cases (e.g gfx/src/gtk/nsFontMetricsUtils.h). Doing the same here is
probably a good idea. 

In |nsFontPSXft::CSSFontEnumCallback|, please use |LossyCopyUTF16toASCII|
instead of |AssignWithConversion|. 

BTW, you're making a copy of fontfileName, familyName, etc in |nsXftEntry
*NewXftEntryFromFcPattern(FcPattern *aFontPattern)| because ||nsXftEntry| lasts
longer than |aFontPattern|, right? So, what you're fixing with that is not a
memory leak but a dangled reference, isn't it? 

Finally, by turning nsXftEntry to a full class with the private default ctor
and member variables turned to |nsCString| from |char *|, you can streamline
some code in | NewXftEntryFromFcPattern|  and |DestroyXftEntry|. Needless to
say, they should be turned to ctor and dtor of nsXftEntry.

Comment 35

[Ervin Yan]

jshin: Thanks very much for your valuable comments. 

> You keep using 'aXyzXyz' for local variables (aEntry and one more variable). 
> In Mozilla, 'a' prefix is used for function arguments. Local variables should 
> be named like 'xyzXyz'. 

I will change all local variable name as you said.

>In |DestroyXftEntry|, NS_ASSERTION would be better for null-checking than
> NS_ENSURE_TRUE because the only call-site already checks for null. Along with
> that change, it can be made to return void instead of nsresult. 

Yes, NS_ENSURE_TRUE need be changed to NS_ASSERTION in this function.

> In |nsFontPSXft::CSSFontEnumCallback|, please use |LossyCopyUTF16toASCII|
> instead of |AssignWithConversion|. 

I do not know what is the difference between |LossyCopyUTF16toASCII| and 
|AssignWithConversion|.  anyway, I will change it.


> BTW, you're making a copy of fontfileName, familyName, etc in |nsXftEntry
> *NewXftEntryFromFcPattern(FcPattern *aFontPattern)| because ||nsXftEntry| 
> lasts longer than |aFontPattern|, right?

Yes, but I am very surprised that the previous patch do not core dump since the 
fontfileName familyName are dangled references after the FcPattern variable is 
destroyed.

> So, what you're fixing with that is not a memory leak but a dangled 
> reference, isn't it? 

I add the following lines to check whether the fontps list is already ready.

+    if (fpi.fontps->Count() > 0)
+      return nsnull;

in previous patch, When call |FindFont| every time, if one chacter is not found 
in all fonts, a list of |XftEntry| will be appended to the fontps list, if 
there are many such characters, you will find the list became longer and longer.
   
Also I add some codes to check and skip it if the font's family name is null, 
these is because, on Solaris, All Korean truetype have no family name, Mozilla 
will crash when call |FT2ToType1FontName| for Korean font file. it should be a 
problem that printing will be inconsistent with display because Xft do not care 
the family name for display. we need have a better solution for such font 
problem.


> Finally, by turning nsXftEntry to a full class with the private default ctor
> and member variables turned to |nsCString| from |char *|, you can streamline
> some code in | NewXftEntryFromFcPattern|  and |DestroyXftEntry|. Needless to
> say, they should be turned to ctor and dtor of nsXftEntry.

I think so, I will try to turn nsXftEntry to a full class.

Comment 36

[Ervin Yan]

Also I will fix the fretype 2.1.9 problems in this patch. Now I have a workable 
patch for freetype2.1.9 enabled. 

Comment 37

[Jungshik Shin]

(In reply to comment #36)
> Also I will fix the fretype 2.1.9 problems in this patch. Now I have a workable 
> patch for freetype2.1.9 enabled. 

Please, don't unless you came up with a solution that will solve a thorny
problem mentioned in bug 234035. Actually, even if you did, I don't think it's a
good idea to deal with that here. Let's just deal with one issue at a time.

Comment 38

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

Any chance of landing this before 1.8alpha3 closes on Tuesday/Wednesday?

Comment 39

[Jungshik Shin]

I can make a new patch quickly (per my comment #34) if Ervin hasn't yet

Comment 40

[Ervin Yan]

sorry, jshin, I was very busy on other things now. I have no time to work on
this patch before Wednesday. so it would be very great if you can help to
perfect the patch. Thanks very much.

Comment 41

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

On second thoughts, it may be better to wait until after alpha, since there are
likely to be bugs that will be discovered in nightly builds, and we probably
don't want too many duplicates from the alpha.  (We'll probably have another
alpha after a3.)

Comment 42

[Jungshik Shin]

Attachment: update

Ok. I addressed all the issues I raised in comment #34. 

I built three binaries, gtk2+xft, gtk1+freetype2+x11core(mozilla.org's default
build) and gtk1+x11core and tested printing  with truetype and type 1 fonts as
well as screen rendering.  They all work at least as well as without patch.   

As for bugs that might lead to too many dupes from alpha, I don't think there
are many of them (if at all) partly because mozilla.org's default build is
still gtk1+freetype2+x11core for which I changed very little if any (although a
lot of #ifdef's are introduced). 

I'm not particularly keen to check this in for 1.8a3. Let's just ask for r/sr
and see what reviewers think.
If I make it, fine. If not, that's also all right.

Comment 43

[Christopher Blizzard (:blizzard)]

The code looks more or less OK to me.  My question is one of design, though.  Is
there a reason why we're not just adding to the already-existing apis in the
nsFontMetricsXft class and just allowing it to do all the font selection and
using its measurement calls?  We can just add a function that returns the raw
freetype fonts and glyphs when it comes time to actually draw?  Right now this
patch copies a _huge_ amount of code into the PS code when it can be shared
pretty easily.

Comment 44

[Ervin Yan]

For the design for this patch, I have the following points:

1. based on Fontconfig/Freetype, not Xft/Freetype.

I have a similar patch based on Xft/Freetype, but I find I need link X/GTK
related libraries for ps printing, I think it is not acceptable. so I decide
directly base on fontconfig/freetype.

2. the font selection for ps printing is not the same with Xft for display. for
printing, we only need outline or scablable fonts for printing, while Xft can
use bdf/pcf for display.

3. Do not change any codes for nsFontMetricsXft.cpp since it works well for a
long time.

4. Only the font selection codes are duplicated with nsFontMetricsXft.cpp. It is
acceptable for me to extract the duplicated codes to a common file.

Comment 45

[Jungshik Shin]

(In reply to comment #43)

> patch copies a _huge_ amount of code into the PS code when it can be shared
> pretty easily.

  You metric may be different from mine. I wouldn't say a huge amount of code is
duplicated. I factored a significant portion of duplicated code out and put in
nsFontConfigUtils.cpp (shared by nsFontMetricsXft and nsFontMetricsPS). The
remaining duped parts are FindFont and CSSFontEnum(?) which are not that much as
Ervin wrote. 

I'd not say this is the best we can do when we have a lot more 'resources'
available than we do now. There's certainly room for improvement including a
possibly drastic design change, but for now it's pretty reasonable. For your 
approach  , perhaps it's easier to write nsFontMetricsPSXXX from the 'scratch'
than to try to impedance-match between nsFontMetricsPS and nsFontMetricsXft. 

Comment 46

[Christopher Blizzard (:blizzard)]

OK, I'm convinced.  I'm going to want to integrate this with the pango work that
I have done as well, but you guys won't have to pay the cost for that.  Owen
gave me some great implementation hints on how to make that work well, and to
avoid the problems that Ervin pointed out (hinting + antialiasing as well as
avoiding bitmap fonts.)  Should be pretty clean.

Comment 47

[Christopher Blizzard (:blizzard)]

Comment on attachment 155616 [details] [diff] [review]
update 


>+#ifdef MOZ_ENABLE_XFT
>+  if (NS_SUCCEEDED(rv)) {
>+      rv = pref->GetBoolPref("font.FreeType2.printing", &mFTPEnable);
>+      if (NS_FAILED(rv))
>+        mFTPEnable = PR_FALSE;
>+  }

Is this enabled by default in our builds?  If not, shouldn't it be?

Comment 48

[Jungshik Shin]

(In reply to comment #47)

> Is this enabled by default in our builds?  If not, shouldn't it be?

Thanks for r. It(font.FreeType2.printing)  is enabled by default in our builds. 

font.FreeType2.enable is false by default, but gtk2+xft builds don't care. This
brings up a question of whether to enclose pref. entries relevant only to
FT2-builds  with #ifdef MOZ_ENABLE_FREETYPE2/#endif). 

Comment 49

[rbs]

Comment on attachment 155616 [details] [diff] [review]
update 

+    nsCAutoString  aDefaultFont;
+    nsCAutoString aLangGroupName;

Mis-use of the prefix 'a' -- it is meant for 'a'rguments.

Also /struct fontps/ is a poor name. I see that is has been there all along, 
oh well.

I looked deeply at FindFont() and I take it that you guys are happy its
details, and the surrounding housekeeping of the patch.

Consider supporting font.name-list as well at some point.

sr=rbs

Comment 50

[Jungshik Shin]

Thanks for sr. fix checked into the trunk.  I made a patch for 1.7/aviary-1
branch, but I'm not sure what to do with them. 

(In reply to comment #49)
> (From update of attachment 155616 [details] [diff] [review])
> +    nsCAutoString  aDefaultFont;
> +    nsCAutoString aLangGroupName;
> 
> Mis-use of the prefix 'a' -- it is meant for 'a'rguments.

Oops. That didn't catch my eyes :-). Btw, where's aLangGroupName? I couldn't
find it in my patch.
I replaced aDefaultFont with defaultFont.


> Also /struct fontps/ is a poor name. I see that is has been there all along, 
> oh well.

  I agree, but ...


> I looked deeply at FindFont() and I take it that you guys are happy its
> details, and the surrounding housekeeping of the patch.

   Actually, I'm not terribly happy, but given many things coming up, it may be
all right. 

> Consider supporting font.name-list as well at some point

  I'll. I have to do it in nsFontMetricsXft.cpp as well (for which a bug was
already filed)

Comment 51

[Jungshik Shin]

Ervin, I'm sorry I forgot to acknowledge your contribution (without which this
wouldn't have been fixed ) in cvs check-in comment. 

Comment 52

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

I think this should not be put in Aviary or 1.7.

Comment 53

[rbs]

> Btw, where's aLangGroupName? I couldn't find it in my patch.

I must have inadvertently picked it from one of the other patches. [I read them
(along with the discussion in the bug) to get a better appreciation of how ideas
evolved into the the final patch.]

Comment 54

[Ulrich Drepper]

This patch seems to drag in freetype2 despite me compiling with
--disable-freetype2 and --disable-xprint.  The result is a compile error if you
use freetype 2.1.9 (e.g., the one from the current FC3).

In file included from mozilla/gfx/src/ps/nsDeviceContextPS.h:50,
                 from
mozilla/gfx/src/ps/nsDeviceContextPS.cpp:55:/home/drepper/mozilla/gfx/src/ps/nsFontMetricsPS.h:344:
error: `FTC_Image_Desc' does not name a type


The reason for this is that the freetype people decided the some definitions in
freetype/ftcache.h which apparently have been kept around for compatibility, can
go away in 2.1.9.  The new version of this file has no FTC_Image_Desc type
definition anymore.

Comment 55

[Jungshik Shin]

(In reply to comment #54)
> This patch seems to drag in freetype2 despite me compiling with

  Yes, it does. 

> --disable-freetype2 and --disable-xprint.  The result is a compile error if 

'--disable-freetyp2' does not mean we don't use freetype at all but means we
don't use nsIFreeType interface. For compiling with freetype 2.1.8 or later, see
bug 234035. Actually, your help would be appreciated there. I think I have an
idea, but I'm wondering if there's a better way to deal with rather nasty API
changes between freetype 2.1.7 and 2.1.8. 

Comment 56

[Ervin Yan]

Attachment: temp patch to support freetype 2.1.8 with enable-xft and disable-freetype2

just temp patch for building with freetype 2.1.8.

this patch also support freetype 2.1.7 or before. I modify "configure" file
directly to check the freetype version. I did not modify
"build/autoconf/freetype2.m4" because my autoconf (2.53) can not work for
Mozilla.

Comment 57

[Andrew Benton]

Thanks for the patch.

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

Some announcement would have been nice that you need to do explicit
--disable-freetype2 if you want your gtk2 moz to compile now.


Also, in configure.in, you doubled these three lines:
 if test "$MOZ_ENABLE_XFT" && test -z "$MOZ_ENABLE_GTK2"; then
     AC_MSG_ERROR([Cannot enable XFT support for non-GTK2 toolkits.])
 fi
 
+if test "$MOZ_ENABLE_XFT" && test -z "$MOZ_ENABLE_GTK2"; then
+    AC_MSG_ERROR([Cannot enable XFT support for non-GTK2 toolkits.])
+fi

Comment 59

[Joe McCabe]

As near as I can tell this has broken PS printing pretty badly.

Printing that used to work now produces an information page that the postecript
in my printer is too old and that I should pipe the job through GhostScript.
Doing so produced really craptacular results (many, many fonts missing).

Some quick poking shows that this isn't GhostScript doing bad things, it's Moz.
"Print Preview" also shows the fonts missing (I'll attach a screenshot showing
the badness).

Is this thing really done?

Comment 60

[Joe McCabe]

Attachment: Shot showing print preview gone wrong

Comment 61

[Jungshik Shin]

(In reply to comment #59)
> As near as I can tell this has broken PS printing pretty badly.
> 
> Printing that used to work now produces an information page that the postecript
> in my printer is too old and that I should pipe the job through GhostScript.

It doesn't break anything. Just set 'font.Freetype2.printing' to false in
'about:config' if your PS printer is not a PS level3 device. With that set to
false, you'll get what you got before.

Comment 62

[Joe McCabe]

(In reply to comment #61)
> It doesn't break anything. Just set 'font.Freetype2.printing' to false in
> 'about:config' if your PS printer is not a PS level3 device. With that set to
> false, you'll get what you got before.

That worked quite nicely. Thanks!

Unfortuantely the error page that is printed does not contain this
recommendation, and the work around it _does_ suggest does not work.

Any chance that the workaround page can be updated to reflect this?

Comment 63

[Jungshik Shin]

Attachment: patch for 1.7 and aviary 1.0 branches

basically identical to attachment 155616 [details] [diff] [review] but this one is for branches in case
someone wants to apply this patch to branches. 

BTW, for Freetype 2.1.8 or later, see my patches in bug 234035. Attachment
162261 [details] [diff] for that bug can be applied on top of this patch. With both patches
applied (this one and attachment 162261 [details] [diff] [review]), Mozilla/FF should work regardless of
the FT2 version.

Comment 64

[Wolfgang Rosenauer [:wolfiR]]

Jungshik, sorry for asking but could you please describe the pref combinations
overall again?

I don't know if I understand correctly:
If I have a GTK2/XFT build and NO PS Level 3 printer I have to go with
pref("font.FreeType2.enable", false);
pref("font.FreeType2.printing", false);

Or is the second used at all if the first is false?

If I have no XFT or have a PS Level 3 printer I have to 
pref("font.FreeType2.enable", true);
pref("font.FreeType2.printing", true);

?

BTW: your latest patch for 1.7 and aviary at least doesn't apply cleanly 
on a current aviary checkout. I don't know for 1_7_BRANCH.

Comment 65

[Jungshik Shin]

(In reply to comment #64)

> If I have a GTK2/XFT build and NO PS Level 3 printer 
In addition, if you don't want to use ghostscript AND your only concern is
Western European :

> I have to go with
> pref("font.FreeType2.enable", false);
> pref("font.FreeType2.printing", false);
 
> Or is the second used at all if the first is false?

The other way around. The first is NOT used at all in gfx2+xft. It's only the
second that matters. Perhaps, we have to find a way to make the first not
visible in 'about:config'.
 
> If I have no XFT or have a PS Level 3 printer I have to 
> pref("font.FreeType2.enable", true);
> pref("font.FreeType2.printing", true);

Correct except that the first doesn't matter in gfx2+xft builds.

> BTW: your latest patch for 1.7 and aviary at least doesn't apply cleanly 
> on a current aviary checkout. I don't know for 1_7_BRANCH.

YOu meant aviary-1.0 branch or aviary trunk? Attachment 162279 [details] [diff] is NOT for aviary
trunk (which already does include the patch) BUT for 1.7 branch and aviary 1.0
*branch*. As far as files my patch touches are concerned, 1.7branch and aviary
1.0 *branch* should be identical.I just updated my 1.7branch tree, made a new
diff against the branch and compared it with attachment 162279 [details] [diff] [review], the only
difference is about 40 line offset in nsFontMetricsXft.cpp due to a recent
check-in to the file. However, this shouldn't prevent attachment 162279 [details] [diff] [review] from
being applied cleanly. 

Comment 66

[Wolfgang Rosenauer [:wolfiR]]

I meant aviary-1.0-branch:

patching file gfx/src/gtk/nsFontMetricsXft.cpp
Hunk #17 succeeded at 1340 (offset 28 lines).
Hunk #18 succeeded at 1595 (offset 28 lines).
Hunk #19 succeeded at 1630 (offset 28 lines).
Hunk #20 succeeded at 1670 (offset 28 lines).
Hunk #21 succeeded at 1728 (offset 28 lines).
Hunk #22 succeeded at 1749 (offset 28 lines).
Hunk #23 succeeded at 1807 (offset 28 lines).
Hunk #24 succeeded at 1905 (offset 28 lines).
Hunk #25 succeeded at 1958 (offset 28 lines).
Hunk #26 FAILED at 2008.
...
nsFontXftUnicode::DrawStringSpec(...)
is different from your patch in my current aviary-1.0 checkout

Comment 67

[Jungshik Shin]

Attachment: patch for 1.7 and aviary 1.0 branches

Thanks for testing. It turned out that the patch for bug 240804 had been
checked into aviary-1.0 branch but not into 1.7 branch. Even with that
difference, my patch for 1.7/1.0 branches would have been applied cleanly to
both branches if I hadn't mixed up a patch from another bug (bug 215219).
Thanks to your report, I found out both problems and fixed them. This patch
should be applied cleanly to both branches.

Comment 68

[walter sheets]

Hi,
I sent this by email but I don't know if anyone ever saw it:

The code below (08-20-2004) won't compile on gcc 2.95.4 because
g++ complains about the name 'fontps' being enclosed in the struct
with the same name.  gcc 3.x does fine with the same code.  Is
there any simple fix for this compiler-specific error?

jshin%mailaps.org:  +struct fontps {
jshin%mailaps.org:  +  nsXftEntry *entry;
jshin%mailaps.org:  +  nsFontPS   *fontps;  <==========
jshin%mailaps.org:  +  FcCharSet  *charset;
jshin%mailaps.org:  +};