104075 - need X font banning

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[kill this account]

from bug 56441:
> It would be nice to ban certain fonts - some pages can be impossible to read 
> as a result of the attractive but illegible font (Tahoma springs to mind) 
> chosen.

Some X fonts are bad looking (urw fonts come to mind)

Comment 1

[kill this account]

re-assign to Roland.

Comment 2

[Roland Mainz]

Accepting bug...

----

I'd like to implement this via extended regular expressions (see egrep(1)) which
should match strings like this:
name=${FONTNAME};display=${DISPLAY};xdpi=${X_DPI};xdpi=${Y_DPI}

Example_
FONTNAME="-monotype-rockwell-medium-r-normal--14-140-72-72-p-67-iso8859-1"
DISPLAY="foobar:0.0"
X_DPI=85
Y_DPI=85

results in the following string:
name=-monotype-rockwell-medium-r-normal--14-140-72-72-p-67-iso8859-1;display=foobar:0.0;x_dpy=85;y_dpi=85

The idea behind this "match strings" is to get a flexible+extensible(=e.g.
adding more ';name=value' items on demand) font "ban" mechanism which allows to
set [per-font|per-display|per-resolution] banns (rejecting fonts only based on
font names may not be a good idea if the user moves his/her desktop between
Xservers) ...

Comments ?

Comment 3

[Roland Mainz]

Attachment: Patch for 2001-10-26-08-trunk

Comment 4

[Roland Mainz]

Filed patch for Xlib toolkit.

bstell, wanna review this one, I'll file a final patch+port to GTK+ gfx once
we're done with review... OK ?

Comment 5

[Roland Mainz]

Font banning can now be controlled like this:
1. The following will only allow fonts which contain "0-0-0-0" and reject all
fonts with the string "urw":
-- snip --
user_pref("font.x11.acceptfontpattern", "0-0-0-0");
user_pref("font.x11.rejectfontpattern", "urw");
-- snip --
2. Only allow non-scaleable fonts on display ":0.1" (but not for other
displays):
-- snip --
user_pref("font.x11.acceptfontpattern",
"scaleable=false;.*xdisplay=:0.1|xdisplay=.*:0.[023456789]");
-- snip --

etc. etc.

Comment 6

[kill this account]

Will this support an array of fonts to ban?

Is this part of font banning?
-        /*
-         * We used to disable special characters like smart quotes
-         * in CJK fonts because if they are quite a bit larger than
-         * western glyphs and we did not want glyph fill-in to use them
-         * in single byte documents.
-         *
-         * Now, single byte documents find these special chars before
-         * the CJK fonts are searched so this is no longer needed
...

Comment 7

[Roland Mainz]

> Will this support an array of fonts to ban?

Yes... via '|' ("OR" in extended regular expression grammar"):
user_pref("font.x11.rejectfontpattern", "bad_font|mad_font") // should bann both
"badfont" and "madfont".

> Is this part of font banning?
> -        /*
> -         * We used to disable special characters like smart quotes
> -         * in CJK fonts because if they are quite a bit larger than
> -         * western glyphs and we did not want glyph fill-in to use them
> -         * in single byte documents.
> -         *
> -         * Now, single byte documents find these special chars before
> -         * the CJK fonts are searched so this is no longer needed

Nope, that's Xlib toolkit specific. I got problems in Xlib land after banning
some fonts and figured out that somehow (no clue why) this fixes my problems
(code already exists in GTK+ gfx source, I only ported it to Xlib gfx) ...

Comment 8

[kill this account]

This list is likely to be between 10 and 100 fonts.

> Yes... via '|' ("OR" in extended regular expression grammar"):
> user_pref("font.x11.rejectfontpattern", "bad_font|mad_font") // should bann 
> both "badfont" and "madfont".

Is there some way to get handle a list of banned fonts?

Comment 9

[Roland Mainz]

What about concat'ting strings:
-- snip --
user_pref("font.x11.rejectfontpattern", 
"bad_font|" + 
"mad_font1|" + 
"mad_font2|" + 
"mad_font3|" + 
"mad_font4|" + 
"broken_font")
-- snip -- 
?

:-)

Comment 10

[Roland Mainz]

bstell:
The patch works as expected, even with long "bann" lists (I added 50 entries and
it still works good) ...

Comment 11

[kill this account]

Roland: would you bring the patch up-to-date (de-rot it)?

Comment 12

[Roland Mainz]

Comment on attachment 55429 [details] [diff] [review]
Patch for 2001-10-26-08-trunk

bstell wrote:
> Roland: would you bring 
> the patch up-to-date 
> (de-rot it)?

Sure... new patch in 1-2 hours ...

Comment 13

[Roland Mainz]

BTW: An answer to the question:
Q: Why don't we create a list of fonts and stick it into a seperate file or a
prefs array ?
A: Because performance would be horrible. It consumes far more time to run
n-times a regexp match with short strings than run a large regexp string
containing n expressions (the regexp "compiles" the large string into an
internal optimized format - but the "startup" overhead for regexp comparisations
is still high...)...

Comment 14

[Roland Mainz]

Attachment: New patch for 2001-11-18-08-trunk Xlib gfx

Can someone r= that patch, please ?
I'll "port" it to GTK+ gfx after the r= ...

Comment 15

[Roland Mainz]

Comment on attachment 58587 [details] [diff] [review]
New patch for 2001-11-18-08-trunk Xlib gfx

Found a minor issue in error handling ...

Comment 16

[Roland Mainz]

Attachment: Better Xlib gfx patch for 2001-11-18-08-trunk

I filed an issue in |FreeGlobals()| and added seperate prefs names for Xprint
devices.

Ready for r= ...

Comment 17

[Roland Mainz]

s/I filed an issue/I fixed an issue/

Comment 18

[kill this account]

Roland: a few questions

1) if the code fails to get the reject patterns will mozilla quit?

+  rv = gPref->GetCharPref("font.x11.rejectfontpattern", 
getter_Copies(fbpattern));
+  if (NS_SUCCEEDED(rv)) {
+    gFontRejectRegEx = new regex_t;
+    if (!gFontRejectRegEx) {
+      FreeGlobals();
+      return NS_ERROR_OUT_OF_MEMORY;

2) there is a fair amount of non-font-banning changes. Should
   this be done in a separate bug?

+  // always try to return a map even if it is empty
+  //
+  nsCompressedCharMap empty_ccmapObj;
+  aSelf->mCCMap = empty_ccmapObj.NewCCMap();
+
+  // return false if unable to alloc a map
+  if (aSelf->mCCMap == nsnull)
+    return PR_FALSE;

Comment 19

[Roland Mainz]

> Roland: a few questions
> 
> 1) if the code fails to get the reject patterns will mozilla quit?
> 
> +  rv = gPref->GetCharPref("font.x11.rejectfontpattern", 
> getter_Copies(fbpattern));
> +  if (NS_SUCCEEDED(rv)) {
> +    gFontRejectRegEx = new regex_t;
> +    if (!gFontRejectRegEx) {
> +      FreeGlobals();
> +      return NS_ERROR_OUT_OF_MEMORY;

Yup. It should quit. However, in reality it does not really quit because the
underlying code does not care about those error codes - but that is not my
fault... wanna file a bug for that issue... ? :)

> 2) there is a fair amount of non-font-banning changes. Should
>    this be done in a separate bug?

No ...

> +  // always try to return a map even if it is empty
> +  //
> +  nsCompressedCharMap empty_ccmapObj;
> +  aSelf->mCCMap = empty_ccmapObj.NewCCMap();
> +
> +  // return false if unable to alloc a map
> +  if (aSelf->mCCMap == nsnull)
> +    return PR_FALSE;

... I had font problems after I "banned" some fonts - and it looks that this is
the "fix" for the problem. The code already exists in GTK+ code - e.g. that's
only a port. See #7 (http://bugzilla.mozilla.org/show_bug.cgi?id=104075#c7) in
this bug ...
I always prefer a working Xlib toolkit Zilla before and after my checkins,
moving it to a seperate bug may cause that this one goes in first and may render
Xlib Zilla unuseable ...

The port to GTK+ won't contain those changes...

Who wants to r= the patch ? :)

Comment 20

[kill this account]

> 1) if the code fails to get the reject patterns will mozilla quit?
> 
> Yup. It should quit. ...

Even if no pattern is specified?

Comment 21

[Roland Mainz]

bstell wrote:
> Even if no pattern is specified?

If there is no prefs then the regexp won't be set up (e.g. no filtering will
occur, Mozilla will start normally).

Specifying an empty ("") patter is valid, too.
Try this:
% ls -l | egrep ""

(short: it's a NOP... :)

Comment 22

[kill this account]

r=[e-mail]

Comment 23

[Roland Mainz]

bstell wrote:
> r=[e-mail]

Thanks!
I'll file a patch for GTK+ once my builds have finished (~14-16 hours from now)
...

Comment 24

[Roland Mainz]

Attachment: All-in-one patch for GTK+ and Xlib gfx for 2001-11-24-08-trunk

I added sample config to unix.js prefs file, too ...

Comment 25

[kill this account]

is gdk_screen_width_mm the best choice?

http://developer.gnome.org/doc/API/gdk/gdk-general.html#GDK-SCREEN-WIDTH-MM

  Returns the width of the screen in millimeters. Note that on many X 
  servers this value will not be correct.

Comment 26

[Roland Mainz]

> is gdk_screen_width_mm the best choice?

Same as in Xlib code - it's just one of the silly wrappers in GDK code...

http://developer.gnome.org/doc/API/gdk/gdk-general.html#GDK-SCREEN-WIDTH-MM

>  Returns the width of the screen in millimeters. Note that on many X 
>  servers this value will not be correct

One of the usual dumb-users-rants problems in X - the people do not configure
the right DPI value in the server[1] (man Xserver, option -dpi) and then wonder
why the get weired results ("ugly" fonts for example).
Maybe someone should bite the Gnome folks to update their docs instead of
inventing stupid workarounds for wrong configured Xservers (the "workarounds"
used in Gnome code usually work horrible wrong with the "correctly" configured
Xservers... ;-(( ) and then blame the X API for their home-grown problems... ;-(

[1]=some Xserver implementations are able to peek this value somehow from the
hardware, otherwise they fall back to the default (which is AFAIK 75DPI (for
X.org/Xfree86.org) servers, 90DPI (for Solaris/Xsun) or 300DPI (Xprt, X.org
print server)).

Comment 27

[Akkana Peck]

Mozilla has a pref to override the dpi, in case the X server's idea is wrong. 
Should we be using the mozilla dpi (or nearest equivalent)?

Comment 28

[Roland Mainz]

No, we should use the DPI from the Xserver directly. Our application may
"override" the DPI for it's own use - but X fonts are managed by the
server-side, therefore we should use the server's value for that...

Comment 29

[kill this account]

Comment on attachment 59107 [details] [diff] [review]
All-in-one patch for GTK+ and Xlib gfx for 2001-11-24-08-trunk

r=[e-mail]

pls add a comment to the unix.js file demo'ing that the accept/reject lines can
be multi-line.

Comment 30

[Roland Mainz]

Just a hint:
The idea for the xdpi=;ypi= filter rules is "stolen" from the "xrdb" filter
stuff (see xrdb(1) manual page, X_RESOLUTION= and Y_RESOLUTION= values) - they
use the Xserver's values, too - without relying on any Gnome'isch hacks...

Comment 31

[Roland Mainz]

> pls add a comment to the unix.js file demo'ing that the accept/reject lines 
> can be multi-line.

Mhhh, I thought that the reference to the egrep(1) manual page was enougth...
=:-)
Anyway, I'll file a final patch after sr= ...

----

blizzard:
Requesting sr=, please ...

Comment 32

[kill this account]

> > pls add a comment to the unix.js file demo'ing that the accept/reject lines 
> > can be multi-line.
> 
> Mhhh, I thought that the reference to the egrep(1) manual page was enougth...

It is but the fact that JavaScript allows line concatenation in the preference 
file is not common knowledge. Besides, people always appreciate an example.

I don't the comment changes needs review/super-review:

From a private email from brendan to bstell:
> > PS: Do "property file only" changes fall in the "doesn't need a
> > super review" category?
> 
> I hereby give rs= for all such changes -- so long as you're sure of the
> "only" part.  Any whiff of code changes, or effects on code that knows
> something about a property, or property naming conventions, etc., and we
> need to have full review.
> 
> /be

As such I believe this extends to comments. So you can just add the comments
at checkin time.

Comment 33

[Jungshik Shin]

It may be nice to add reject lines in unix.js to ban
what attachment 53094 [details] [diff] [review] is banning (a peculiar case of
non-zero pixelSize and pointSize and zero averageWidth)
I believe what's
bannded by attachment 53094 [details] [diff] [review] affects the majority of XF86 users
so that if it's included in the default unix.js, it does not
have to be 'release-noted'.  

Comment 34

[Brendan Eich [:brendan]]

Are we sure we have regex.h everywhere we have GTK+?  We don't have it on all
platforms that use X, so I think more work is needed here.  Should this feature
depend on autoconf-style tests, and be absent on platforms that lack regex.h?

/be

Comment 35

[kill this account]

could I get a pointer to the list of supported systems?

Comment 36

[Roland Mainz]

<Dauphin> from glibc 2.1.3's regexp.h:
<Dauphin> The contents of this header file was first standardized in X/Open
<Dauphin>    System Interface and Headers Issue 2, originally coming from SysV.
<Dauphin>    In issue 4, version 2, it is marked as TO BE WITDRAWN.
<Dauphin>    This code shouldn't be used in any newly written code.  It is
<Dauphin>    included only for compatibility reasons.  Use the POSIX definition
<Dauphin>    in <regex.h> for portable applications and a reasonable interface. 

My patch uses <regex.h>, e.g. the POSIX API - this would AFAIK be available on
all platforms - even Win32 ... :)

Comment 37

[kill this account]

Attachment: patch; add configure test for regex.h

I would prefer to actually find out if regex.h is available but here is
a patch configure.in to test for it. 

Here is an example use:

      #ifdef HAVE_REGEX_H
	 // regex include and code
      #endif

Comment 38

[Roland Mainz]

1. Isn't it easier to log into the single tinderbox machines and check whether
the file is available or not ?
2. AFAIK all platforms covered by this patch should have it. Platforms which do
not support regex.h are listed in
http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/ldap/include/regex.h#1

I never saw a Unix-platform which does not have regular expression support in
libc in the last couple of years...

Comment 39

[Roland Mainz]

BTW: This won't help...
-- snip --
      #ifdef HAVE_REGEX_H
         // regex include and code
      #endif
-- snip --
... because we need functional X font banning on all X11 platforms - not only
"some"...

I wish we could simply use X resources for this - then the implemnentation would
be far far easier... ;-(

Comment 40

[Daniel (Leaf) Nunes]

BSD/OS otaku.mcom.com 4.2 BSDI BSD/OS 4.2 Kernel #0: Wed Oct 25 17:38:20 MDT
2000    
[e-mail]:/mnt/proto/4.2-i386/usr/src/sys/compile/GENERIC  i386

does not have REG_OK defined... i don't see what's wrong with at least checking
for the header, as brian suggests. For systems that have it, we should use it.
For systems that *don't* have it, we should either fail gracefully or build
anyway, if possible.

Comment 41

[Mike Shaver (:shaver emeritus)]

I don't understand why we need regexes at all for this.  It looks like we need a
simple glob expansion of "*", which is not hard at all to hand-code, or borrow
from the libjar (?) stuff.

Can someone give me an example of other apps doing font banning with regular
expressions?

Comment 42

[Roland Mainz]

leaf wrote:
> does not have REG_OK defined...

We can simply add a
-- snip --
#ifndef REG_OK
#define REG_OK (0)
#endif
-- snip --
after including <regex.h>

> i don't see what's wrong with at least checking
> for the header, as brian suggests.

Sure... but at some point we have to make it mandatory...

> For systems that have it, we should use it.
> For systems that *don't* have it, we should either fail gracefully or build
> anyway, if possible.

We may build but display broken fonts on their displays. That's why we need X
font banning stuff - on all platforms. Otherwise we will build on a platform -
but it may be unuseable... ;-(

Comment 43

[Roland Mainz]

shaver:
We need a way to ban fonts in a smart way. Eith ban them all or ban them in a
per display, per-resolution or per-device way. That's why I am using regular
expressions here.

Comment 44

[Mike Shaver (:shaver emeritus)]

You're using regular expressions, in the only entries I see in unix.js, as a
really expensive glob substitution.

Why don't globs work?  What apps need and use the full power of a regular
expression implementation, rather than a simple glob?

(I don't think "need to ban fonts based on [listed criteria]" logically requires
the inclusion of regular expressions.  Someone will have to talk me through that
reasoning, I fear.)

Comment 45

[Roland Mainz]

shaver:
A "globbing" solution will us never allow to bann certain
font/size/attribute/display/resolution combinations. I want to give the system
admins a usefull tool to control that.
For example - the regex in this code replaces the |#ifdef xyz #endif| used by
xrdb to turn on/off some fonte definitions in a per
display/resolution/other-xserver-attribute manner.

Comment 46

[Mike Shaver (:shaver emeritus)]

So, wait, you want the admin to build up one master regex of all rejections and
acceptances?  I don't think that's going to work very well, but I'm no font
admin.  I know it doesn't work well for other filtering cases, like logs and mail.

How do the accept and reject lists interact?  If a font matches both, what
happens?  If it matches neither, what happens?  (I'm sure there's a system for
this, but we should document it.)

Comment 47

[Roland Mainz]

> So, wait, you want the admin to build up one master regex 

Note that this regex uses the egrep(1) syntax (which is more powerfull than
normal grep synatax) - and that we may use JS string concatenation to get a
readable version of the prefs (BTW: See comment #13)...

> of all rejections and acceptances? 

Yup, that's this bug about. We'd like to get a smart and powerfull way to bann
some fonts/-families/-shapes - and give the admins a way to bann fonts which may
only look ugly on some Xservers but not on all (for example we have some fonts
which look great on Sun workstations with 120DPI - but they s*ck at the default
Xsun resolution (=90DPI)).
I want a tool which is at the level of "usefullness" of the xrdb stuff.

> I don't think that's going to work very well, but I'm no font admin. 

WFM. 
I have a good control over what fonts are banned and which not (reject pattern).
And as an "extra" I can set-up patterns which fonts are allowed and which not
(accept pattern) (for example: I can force Zilla to use only handdrawn,
non-scaled bitmap fonts).
And I can control all this in a fine-grained per display- or display-attribute
manner.
What do you want more ? :-))

X/CDE uses a similar way to select fonts based on symbols from xrdb... and it's
working very well.

> How do the accept and reject lists interact? 

Filtering order:
1. If it matches the reject pattern the font will be rejected.
2. If it does not match the accect pattern it will be rejected.

> If a font matches both, what happens? 

Well, if it matches the reject pattern then it will be rejected (step [1]).
If it passes that step (no match) - then comes the "positive" filter (accept
pattern) - that's step [2]...

> If it matches neither, what happens?  (I'm sure there's a system for
> this, but we should document it.)

See step [2]. No match for the accept pattern --> font rejected.

No pattern given == no rejecting will be done.

----

I have thougth a couple of days "what's required for a good and flexible
solution to bann fonts for an application".
This one gives fine-grained control, even in the worst-case scenarios I've seen
in the last couple of years...

Comment 48

[Roland Mainz]

Requesting sr= ...

Comment 49

[Roland Mainz]

Attachment: Updated patch for 2001-11-30-08-trunk

Changes:
- Added |#ifdef ENABLE_X_FONT_BANNING| around the X font banning code. We can
implement a quick fix if any platform does not have <regex.h> (should not
happen, and we still did not found any X11 platform which does not support it)
- Added code to catch if |REG_OK| is not defined in <regex.h>
- Added comment about JS string concatenation and added an example to unix.js
prefs file.

Comment 50

[kill this account]

r=[e-mail]

Comment 51

[Roland Mainz]

Comment on attachment 60044 [details] [diff] [review]
Updated patch for 2001-11-30-08-trunk

timeless found some smallish issues ...

Comment 52

[Roland Mainz]

Attachment: Updated patch per timeless's comments

Comment 53

[Roland Mainz]

Attachment: New patch for 2001-11-30-08-trunk

Comment 54

[Roland Mainz]

Comment on attachment 60616 [details] [diff] [review]
New patch for 2001-11-30-08-trunk

I did not remove the hardcoded font ban (workaround for bug 103159) from the
code.
Instead I left it "in place" and forward this issue (e.g. turning it into a
regex filter rule and removing the hardcoded ban) to another, later bug.

This would avoid that we need to backout/checkin the whole patch multiple times
if we regress anything by accident...

Comment 55

[Roland Mainz]

Attachment: New patch for 2001-11-30-08-trunk

Fixed patch to compile if Xprint module has been disabled via --disable-xprint

Comment 56

[Roland Mainz]

Requesting r=/sr= ...

Comment 57

[kill this account]

I would strongly recommend (but not require) that you add a NS_FONT_DEBUG for 
font banning so you can debug problems in the field.

minor nit: when changing the indenting I would recommend you follow the
convention to indent 2 spaces (or whatever is common in the surrounding code).

Since the comment is for users not programmers you may want to change the
comment in unix.js to look the way the user would write it; eg: change

+ * "fname=%s;scalable=%s;outline_scaled=%s;...

to 

+ * "fname=.*;scalable=.*;outline_scaled=.*;...

minor nit: change "accept pattern dot not match it" to "accept pattern does 
not match it"

r=[e-mail]

Comment 58

[Roland Mainz]

Attachment: New patch for 2001-11-30-08-trunk to match r=bstell

> I would strongly recommend (but not require) that you add a NS_FONT_DEBUG for

> font banning so you can debug problems in the field.

Fixed for GTK+ toolkit. But it may be better to turn all the NS_FONT_DEBUG
stuff into PR_LOG() calls like Xlib toolkit does ...

> Since the comment is for users not programmers you may want to change the
> comment in unix.js to look the way the user would write it; eg: change
> + * "fname=%s;scalable=%s;outline_scaled=%s;...
> to 
> + * "fname=.*;scalable=.*;outline_scaled=.*;...

Fixed.

> minor nit: change "accept pattern dot not match it" to "accept pattern does 
> not match it"

Fixed.

Comment 59

[kill this account]

> Fixed for GTK+ toolkit. But it may be better to turn all the NS_FONT_DEBUG
> stuff into PR_LOG() calls like Xlib toolkit does ...

I'm interested.

1) Is this available in non-debug builds?

2) How would the user turn on the output?

3) How would the user find/collect the output so that it could be attached
   to a bug report?

Comment 60

[kill this account]

humm... I'm not trying to be picky but DEBUG_PRINTF is too coarse. It is on
when *any* NS_FONT_DEBUG bit is set. Would it be possible to do something like
this instead:

+  #define NS_FONT_DEBUG_BANNED_FONT 0x20
...
+ #define BANNED_FONT_PRINTF(x) \
+          DEBUG_PRINTF_MACRO(x, NS_FONT_DEBUG_BANNED_FONT)
+ 
...
+ BANNED_FONT_PRINTF(("rejecting font '%s' (via hardcoded workaround for bug ...
...

Comment 61

[Mike Shaver (:shaver emeritus)]

Everything you'd ever want to know about PR_LOG, courtesy of the first hit on
the mozilla.org search engine:
http://www.mozilla.org/projects/nspr/reference/html/prlog.html 

Share and enjoy!

Comment 62

[Brendan Eich [:brendan]]

Should I expect another patch that uses NSPR logging?

/be

Comment 63

[Roland Mainz]

Brendan Eich:
> Should I expect another patch that uses NSPR logging?
Yes, but I'd like to forward this to a later bug - it looks that such a patch
will be far larger than the current one... IMO too much "risk"...

I'll file a new patch which implements bstell's suggestion in the next hours and
then I'll open a seperate bug to move all the NS_FONT_DEBUG stuff to use NSPR
logging (maybe I hack NSPR logging to support some kind of flag/keyboard-based
filtering scheme)...

Comment 64

[kill this account]

> I'll open a seperate bug to move all the NS_FONT_DEBUG stuff to use NSPR
> logging 

Please talk with me before doing so. The NS_FONT_DEBUG printfs have been
an important tool to debug font issues in the field.

Comment 65

[Roland Mainz]

Attachment: New patch for 2001-12-09-08-trunk

I've implemented bstell's suggestion and changed DEBUG_PRINTF() to
BANNED_FONT_PRINTF().

Font banning can now be observed via setting the NS_FONT_DEBUG env var like
this:
% export NS_FONT_DEBUG=0x20
% ./mozilla

Comment 66

[Roland Mainz]

Brian Stell:
> > I'll open a seperate bug to move all the NS_FONT_DEBUG stuff to use NSPR
> > logging 
> Please talk with me before doing so. The NS_FONT_DEBUG printfs have been
> an important tool to debug font issues in the field.

I know that. I'll CC: you to the bug and ask you for the r= when the patch is
"ready" ... :)

Comment 67

[kill this account]

minor bits:

> BANNED_FONT_PRINTF(("Invalid font.x11.rejectfontpattern\n"));

The '\n' is not needed as the macro addes the file name, 
line number and a new line. Could we add the pattern that failed?

 BANNED_FONT_PRINTF(("Invalid font.x11.rejectfontpattern '%s'", 
fbpattern.get()));
 
> screen_xres = (float(::gdk_screen_width())  / (float(::g ...

Should this explicitly cast the float to an int for assignment?

> +  DEBUG_PRINTF(("Loading font '%s'\n", aPattern));

Can this be changed to BANNED_FONT_PRINTF or this style:

2070 #ifdef NS_FONT_DEBUG_LOAD_FONT
2071       if (gDebug & NS_FONT_DEBUG_LOAD_FONT) {
2074         printf("invalid font \"%s\", %s %d\n", mName, __FILE__, __LINE__);
2077       }
2078 #endif

I've been meaning to change this comment:

+           * Now, single byte documents find these special chars before
+           * the CJK fonts are searched so this is no longer needed
+           * and should be removed, as requested in bug 100233

Can we change this (both Gtk and Xlib) to:

+           * Now, single byte documents find these special chars before
+           * the CJK fonts are searched so this is no longer needed
+           * but is useful when trying to determine which font(s) the
+           * special chars are found in.

r=[e-mail]

> I'll CC: you to the bug ...

Thanks, all I'm concerned about is retaining the functionality regardless
of the implemetation.

Comment 68

[kill this account]

PS: Since all the bits are minor I do not see any need for posting/reviewing
a new patch.

Comment 69

[timeless]

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

Comment 70

[kill this account]

has this gone out for sr= ?

Comment 71

[Roland Mainz]

Attachment: New patch for 2002-01-01-08-trunk to match r=bstell

Comment 72

[jag (Peter Annema)]

Comment on attachment 63321 [details] [diff] [review]
New patch for 2002-01-01-08-trunk to match r=bstell

One \n left in BANNED_FONT_PRINTF, fix that and sr=jag

Comment 73

[Roland Mainz]

Fix checked in, marking bug as FIXED.
Performance issues are discussed in bug 117877 ...