Closed Bug 126919 (xft) Opened 23 years ago Closed 22 years ago

Xft support on X Mozilla

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: keithp, Assigned: blizzard)

References

(
URL
)

Details

(Keywords: intl, Whiteboard: [driver:blizzard])

Attachments

(4 files, 33 obsolete files)

strace mozilla 2> moztrace.log when loading ZDNN crash
30.69 KB, text/plain
Details
screenshot showing rendering of MARC under Mozilla-Xft
79.75 KB, image/png
Details
stack trace from cnet hang
4.26 KB, text/plain
Details
patch
191.70 KB, patch
rbs
: review+
rbs
: superreview+
shaver
: approval+
Details | Diff | Splinter Review 
This bug is a forum to discuss how to integrate Xft support into X-based Mozilla
builds, both changes needed within the Mozilla code base and changes needed in
Xft and the underlying Fontconfig libraries.

I'm using Mozilla as a target for Xft development and will be keeping a
patch current.  That work can be seen at http://keithp.com/mozilla

The current design uses the font matching inside the Fontconfig library, which
is nearly CSS2 compliant (except for weight fill-in, and a few other minor
nits).  My plan is to build CSS2/3 compliant matching by fixing the Fontconfig
matching and adding a small amount of helper code within Mozilla.
Keywords: intl
QA Contact: ruixu → ylong
Stealing!
Assignee: keithp → blizzard
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch config patch (obsolete) — — Splinter Review 
This is a patch that makes the required changes to configure.in and all of the
toplevel changes that need to happen to get Xft building in the tree.
a=brendan@mozilla.org for 0.9.9.

/be
Blocks: 122050
Keywords: mozilla0.9.9+
The config changes have been checked in.  Now for real code...
Attached patch code changes (obsolete) — — Splinter Review 
Code changes.
Whiteboard: [driver:blizzard]
Sorry to spam this bug, but does the merged code need CVS Xfree86 to work, just
like the patches did on keithp.com?
No, it doesn't.  It uses private copies of Xrender and Xft which are already in
our tree.  It should also work with any X server.
+void
+nsFontGTK::GetFontProperties(float &aScale,
+                             nscoord &aAscent,
+                             nscoord &aDescent,
+                             nscoord &aLineSpacing,
+                             nscoord &aWidthMaxBounds,
+                             float &aSpaceWidth,
+                             float &aXHeight,
+                             float &aUnderlinePosition,
+                             nscoord &aUnderlineThickness,
+                             nscoord &aSuperscriptOffset,
+                             nscoord &aSubscriptOffset)

In the long run the items we get will evolve and rather
than have one mondo function that grows and grows I'd 
recommend small distinct functions that rarely need
to be changed.


+xftFindMozLangGroup (nsACString &mozLangGroup)
+{
+  for (unsigned int i = 0; i < NUM_XFT_LANG_GROUPS; i++)
+  {
+    if (mozLangGroup.Equals (xftLangGroups[i].aMozLangGroup,
+                            nsCaseInsensitiveCStringComparator()))
+    {
+      return &xftLangGroups[i];
+    }
+  }
+  return 0;
+}

Could you make this a hash for better performance?

+PRBool
+UseXft (void)
+{
+  static PRBool          been_here_done_that = PR_FALSE;
+  static PRBool          use_xft;
+  if (!been_here_done_that)
+  {
+    been_here_done_that = PR_TRUE;
+    use_xft = getenv ("MOZILLA_XFT") != 0;
+  }
+  return use_xft;
+}

Can we make this a pref?

Does this patch integrate with the existing X core font
support?

FYI, that wasn't meant to be a final patch. :)  I'll respond to your comments
tomorrow.
The freetype library in other-licenses/freetype does not work for objdir
builds.  I modified the build to use the system version of libfreetype if the
system version >= 2.0.8, which is the version checked into the tree (verified
with diff).  Since there is an actual released version of the required library,
can we punt the in-tree version?

Also, discovered a small bug.  When we add additional arguments for
sub-configures at the end of configure.in, we were never resetting
ac_configure_args for the next configure call. This patch fixes that as well.
Comment on attachment 71641 [details] [diff] [review]
Check for system version of freetype

r=bryner
Attachment #71641 - Flags: review+
Comment on attachment 71641 [details] [diff] [review]
Check for system version of freetype

a=shaver for 0.9.9
Attachment #71641 - Flags: approval+
Index: gfx/src/gtk/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/gtk/Makefile.in,v
retrieving revision 1.75
diff -u -r1.75 Makefile.in
--- gfx/src/gtk/Makefile.in	2002/02/19 08:53:50	1.75
+++ gfx/src/gtk/Makefile.in	2002/02/27 10:00:37
@@ -46,6 +46,7 @@
 
	  content \
 
	  layout \
 
	  necko \
+
	  Xft \
 
	  $(NULL)


That seems like it shouldn't be in the patch.  Plus, if you need the freetype on
the system, it doesn't set the requirement.
Depends on: 128153
The Xft in REQUIRES comes from the previous code changes patch, attachment 71528 [details] [diff] [review].

> Plus, if you need the freetype on the system, it doesn't set the requirement.

I thought that you said that the freetype headers were installed in
dist/include/freetype2 ?  My build works fine w/o Xft in REQUIRES so I guess it
can be removed.

What's the status here? Has the 0.9.9 critical work been checked in? Can we move
on this today so we can start looking for a release build tomorrow? 
This shouldn't be on the 0.9.9 radar.
No longer blocks: 122050
Attached patch patch (obsolete) — — Splinter Review 
Nothing new here, just checkpointing.
Reminder: make sure the list in prefs is sorted.
I've been using the experimental build (2002-02-28-14) for about a week now, and
it's been nothing short of excellent.  I know that there are a few
anti-antialiasing fanatics out there, though.

I definately think that this code should be enabled in 1.0, with the simple
addition of a run-time preference for the anti-aa crowd.  Performance, in terms
of speed, rendering accuracy, and the total lack of crashes, etc., of the build
has been great.

What do we need to do to get this into 1.0.0?

-SteveK
I'm not sure if this is a bug, or just not implemented yet, but I've found that
when using this build, the font preferences panel is pretty broken:

1) The list of available fonts seems to be only a subset of the fonts I actually
have available (and configured in my .fonts.conf file.  I also made sure that
other font sources were using similar paths, i.e. my xfs config, and
/etc/fonts.conf).

2) Changing the font preferences seems to have no effect; I currently have moz
set to use serif for proportional, and serif is set to a copperplate font, and
I'm still seeing the same, "times roman-type" serif font everywhere..

3) There's still some performance issues in there, but they're all manageable;
it takes a while to create a new .fonts.cache file (startup), and opening the
fonts preference dialog takes a few seconds (maybe 5?), during which mozilla
seems to block.  Again, these are noticable but not a big deal; as long as page
rendering is quick, which it is, these are almost trivial problems.


I'm assuming this patch is separate from the freetype support that was in 0.9.9
(although apparently not enabled in some of the mozilla.org builds of 0.9.9).

A definite benefit of this patch over the existing freetype stuff, besides
better integration with system font handling & etc, is the 'cleartype'-like
antialiasing for LCDs which is in the Xft package but *not* in the current
mozilla antialiasing functionality.  Using Xft would (hopefully) mean that
Mozilla inherits cleartype as well, which is a big help for us laptop users!

Am I understanding the situation correctly, or should I be filing 'no cleartype
currently on machine w/ LCDs' as a bug somewhere else?
> 1) The list of available fonts seems to be only a subset of the fonts I
> actually have available (and configured in my .fonts.conf file.  I also made
> sure that other font sources were using similar paths, i.e. my xfs config, and
> /etc/fonts.conf).

The patch lists only fonts appropriate for each language; it extracts this
information from the OS/2 table in the font.  Fonts without an OS/2 table aren't
(currently) listed.  There is an alternate selection mechanism based on
the presense of 'key' characters for each language, but that won't separate the
various Han factions.  Suggestions on how to make it work for fonts that
don't advertise any OS/2 tables are welcome.

> 2) Changing the font preferences seems to have no effect; I currently have moz
> set to use serif for proportional, and serif is set to a copperplate font, and
> I'm still seeing the same, "times roman-type" serif font everywhere..

Hmm. It may be that your fonts.conf files is overriding these selections by
'preferring' some other font, or it may be that the font you've requested
doesn't advertise support for the desired language group.

> 3) There's still some performance issues in there, but they're all manageable;
> it takes a while to create a new .fonts.cache file (startup), and opening the
> fonts preference dialog takes a few seconds (maybe 5?), during which mozilla
> seems to block.  Again, these are noticable but not a big deal; as long as
> page rendering is quick, which it is, these are almost trivial problems.

Once the core XFree86 system is using fontconfig, the font caches should
be built automatically whenever new fonts are installed.  The font preference
dialog does take a while to pop up, and I'm not quite sure of the reason --
my recollection is that the last time I profiled it, it was spending a lot of
time building the UI elements that showed the list of fonts.
*** Bug 31296 has been marked as a duplicate of this bug. ***
To add to comment #21.

I tried the patch using fonts that I copied from windows. There was a total of
137 fonts in the directory, but I only could chose between 6 of them. 

If Windows is able to make us of all of them, shouldn't we be able to use them too?
Chris,

Is there a newer version of the xft patch available? I tried the latest one from
here and mozilla freezes on startup with MOZ_XFT=1. Your experimental binary
builds run fine with AA.
I'm using build 20020301 with xft that was on blizzard's rh page. It works
really well, except that the font metrics seem to be sometimes incorrect. When I
go to certain pages, the font size changes abruptly in the middle of the text
without any good reason.
Example: http://news.bbc.co.uk/hi/english/uk/newsid_622000/622457.stm

Another problem that I have and that seems related to the first one is when I go
certain pages, the font size also changes abruptly, but the text using the
larger font seems to be layed out as if it was using a smaller font.. Links are
written over the rest of the text and the text seems to "leak" out of its box
and over the right side of the page.
Example: http://www.wired.com/news/politics/0,1283,51425,00.html

I noticed that those builds are gone... Is it because there is a newer patch
somewhere that fixes that? Has this bug been pushed to post 1.0?
The news.bbc.co.uk site is (unfortunately) a mess. Have a look through the
source or try validating it if you're feeling brave. It's pretty hard to pin
down the font size to any particular error, but the unclosed FONT tag on line
741 is a good contender.

I have a style sheet I can send you that makes it a whole lot more readable, but
I don't think mozilla supports user style sheets yet (I'm using galeon).

The wired.com one does look like a metric problem, but also occurs on a vanilla
0.9.9 install, so probably isn't Xft related. Perhaps Bug #116273 ?
Bob, potential evangelism spin-off bug opportunity in comment 28.

/be
tristan has it covered in bug 56800
Is the approved patch in this bug ready to land or is that something old?
This patch isn't up to date, no.
Well, I've got some comments from my testing of the 1.0rc2 experimental Xft build...

Generally rather impressive, sometimes the fonts get rather skinny and fuzzy
when the point sizes get very low (mostly CSS pages I'm guessing), but otherwise
a huge improvement from standard X fonts.  It's such a relief to see some better
fonts, I'm used to this level of quality and better, because I've had the
fortune to work on a system that I'd argue still has the best font renderer out
there (can anyone guess? :))

_However_, I've found at least one problem, with Traditional Chinese pages. I'm
using the Microsoft MingLiu font, and found that on many/all Traditional Chinese
web pages Mozilla simply locks up and never returns, the only way out being to
kill the process. Pages I've seen go wrong include:
http://tw.yahoo.com/
http://chinese.yahoo.com/
sub-pages on http://www.ibm.com/tw/

My system is RedHat 7.1, XFree86 4.1.0, freetype 2.0.1
John: Have you tried the TrueType support that is already in Linux/Unix Mozilla?

http://www.mozilla.org/projects/fonts/unix/enabling_truetype.html
In response to comment 34 :

I've not tried the direct Freetype support, as there are no (Redhat) Linux
binary builds with this support switched on. It also seems to be an
inappropriate way to get antialiased fonts in X11, which is why the Xft
experimental build came along AFAICT. The browser shouldn't need to know all
about different font renderers, and where the fonts for these renderers are
stored...that should be provided by the system. 

In the case of the experimental build, this system is Xft, which admittedly
seems to be tied to Freetype at the moment (someone correct me if I'm wrong).

The direct freetype support currently in the tree looks really horrible on my
laptop. I really require Xft support. I still use a 0.9.8 rpm build with Xft
that was on blizzard's people.redhat.com page. It would be really great if Xft
worked for 1.0... And if someone made Xft-enabled RPMs...
I have had problems using type1 fonts with xft enabled mozilla. When initially
setting up my fonts.conf file without any MS core web fonts, I can list out and
select type1 fonts, eg. Bitstream Carter, but as soon as I install the MS fonts
(in a separate directory), the type1 fonts no longer show up. Can this be an
encoding issue?

I also looked for a debug environment variable similar to export
NS_FONT_DEBUG=400, which works with the freetype enabled mozilla. Do such a
debug variable exist for the Xft version?
Try FC_DEBUG=1.
> Try FC_DEBUG=1.

That doesn't work with the redhat rc2 rpm's at least, running as 'export
FC_DEBUG=1 ; mozilla'.
With the release of Mozilla 1.0

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/xft/

has disappeared (along with all of http://ftp.mozilla.org/pub/mozilla/nightly/)

Is there any chance of getting a Mozilla 1.0 RPMs with Xft support. I'd like to
get the last few fixes, but couldn't live without the Xft support. :)


Those directories are still there.  You just have to use ftp.  I'll build one
soonish.
Any idea if xft support goes into the trunk anytime soon?
In response to comment 42 :

As far as I can tell the FTP site has been completely cleaned out, and the only
things on it are Mozilla 1.0 archives.

Can't see anything but /pub/mozilla/releases/mozilla1.0/ via ftp or http

See bug 149542
its on 207.200.85.37, but not .38 or .39...
Is the 1.0 release on ftp://207.200.85.37 in the xft 1.0 area really a proper
xft release? Setting FC_DEBUG=1 doesn't work. The smooth subpixel rendering I
had with rc3 doesn't work, it doesn't react to changes in /etc/fonts/fonts.conf,
nor changes in /etc/X11/Xft. And it only gives me dreadfull type1 fonts, none of
my ttf fonts are found...

Any ideas? The build id says 2002060615.

And shouldn't there be a way of identifying non-trunk builds?
I had problems with Mozilla with xft on scrolling. It would cause X to start
eatting all the cpu, hence a keyboard/mouse/display lockup. If I logged in over
the network and killed X the system would totally lockup. This is running as a
normal user. It seemed to be somewhat random. At first it would take a number of
hours to reproduce and then it went for 3-4 days without happening. I suspect
this is related to comments I have read that the latest xft patch for Mozilla is
designed for XFree86 CVS and not XFree86 4.2.0 which I am using.
It now seems that mozilla uses it's private fonts.conf in
/usr/lib/mozilla-1.0.0/res/Xft.
*** Bug 155167 has been marked as a duplicate of this bug. ***
What is the status of Xft these days?

I recently saw a message where Keith seemed to say that support for non render 
systems was still not released:

http://XFree86.Org/pipermail/fonts/2002-July/001873.html

> Posted by Keith on Fri, 12 Jul 2002 22:37:23 -0700 to fonts@XFree86.Org
> 
> > The new Xft (not yet released, but in CVS) doesn't even need Render and
> > will work on any X server.

Is this correct?

Will anyone provide xft mozilla rpm's for 1.1beta?
Don't even bother looking at this, it's not done yet.  It's making me nervous
staying on my hard drive, though, so I have to put it somewhere.
In such a large change (12K+ line patch) I would hope to see us continue to 
share more code between the gfs/src/gtk and the gfx/src/xlib.

Is there any reason not to share class definitions like 
gfx/src/gtk/nsFontHandleGtk.h?

Even code like gfx/src/gtk/nsFontInstanceCore.cpp seems like it ought to be
common to both.

I'm not entirely opposed to sharing code.  I'm just trying to break out the core
code at the moment so that nsFontMetricsGTK.cpp can be built with or without it.
 I'm pretty close.  However, I have to admit that sharing that code is
relatively low on my priority list.
It would be unfortunate if the 12,000+ line patch had to be duplicated by
Roland to keep the gfx/src/xlib version in sync (which we have been supporting
for several years now).
Yes, it sure would be a shame.
Attached patch another checkpoint (obsolete) — — Splinter Review 
New patch, core fonts work OK with this patch.	Xft is not yet integrated, nor
is freetype re-integrated.
Attachment #71280 - Attachment is obsolete: true
Attachment #71528 - Attachment is obsolete: true
Attachment #71641 - Attachment is obsolete: true
Attachment #73429 - Attachment is obsolete: true
Attachment #92521 - Attachment is obsolete: true
Attached patch checkpointing changes (obsolete) — — Splinter Review 
Rendering and text measurement for xft is limping along now.  Western text
seems solid.  Unicode isn't done yet, but is starting to get there.  Good place
to take a snapshot before re-merging with the tip and fixing the font selection
algorithms.
Attachment #93735 - Attachment is obsolete: true
If this patch is complied in, will moz run if Xft or FreeType2 is not available?

If not, what should be done so Xft support can, by default, be build into moz 
and moz can continue to run on systems without Xft?

> If this patch is complied in, will moz run if Xft or FreeType2 is not available?
> 
> If not, what should be done so Xft support can, by default, be build into moz 
> and moz can continue to run on systems without Xft?

As the patch stands today, you either compile it in and have a hard reference to
fontconfig and Xft, or you don't compile it.  The number of Xft + Fontconfig
calls that are made via this patch is actually quite small so it wouldn't hard
to do a dlopen and dispatch system, much like how the current freetype code works.

However, that's not one of the goals of the patch right now.  I'm more
interested in getting good system intergration with systems that have a proper
xft and fontconfig installation.  Maybe down the road a bit I'll worry about
dispatching functions.  It shouldn't be too hard to add.  I just want to get the
font rendering working properly first.
Another option is to make the Xft/FontConfig code part of a loadable XPCom 
module.

It's fine to defer working on the linkage issue for now. Of course, this will 
have to be resolved before any patch can be considered for serious review. 
It sounds like a good idea to make these experimental patches to get some 
feedback.
Most of patch looks like it is not directly needed for Xft. 

This obscures the Xft changes.

Separating out the Xft changes from the reorg would make it much easier for
people to see what you are actually doing that is Xft related.

As it stands now this patch makes significant non-Xft related changes to the 
font subsystem.

Perhaps you could separate this into a separate bug so it can be discussed
separately from the Xft issues.

Before any one codes up a massive change to the font subsystem I would hope 
that we would give other developers time to add their input. I am fairly 
positive that other font developers may have thoughts and it seems reasonable 
to include, communicate, and coordinate with them. I believe we may find that
they have

  1) other goals and directions
  2) other work in progress
  3) other planned work
  4) other requested work/features

It would also be good to discuss the particulars of the changes implied in
this patch:

  1) the extent and goals of this particular change
  2) how this particular significant fits the font subsystem architecture
  3) how to manage any possible negative user impact

The most compatible way to add Xft support would be to make another font 
subclass just like the direct FreeType2 code does:

 class nsFreeTypeFont : public nsFontGTK {

only for Xft it would be something like this:

 class nsXftFont : public nsFontGTK {

This would fit in smoothly with moz's font subsystem and allow it to select 
from Xft as well as any of the another font technologies that it currently 
uses.

This would also seperate the font selection issues (problematic) from the 
rendering issues (not so problematic).

This code should be enabled (default off) by a pref just as the direct freetype 
code is.

This should detect if Xft is available and disable itself if not available.
There's been a while since there was a public test build (1.0), how about
providing one for 1.1beta?
The goal that I have is pretty simple.  Use Xft and fontconfig for font matching
and rendering.

Given that fontconfig handles all the matching, it's far easier to seperate the
code instead of intermingling it with the old matching code.  That's why I'm not
interested in just making an nsFontGTK subclass.  (There's an older patch that
essentially does this.)

I'll also throw this out there, since it's going to be a serious bone of
contention - I'm also not interested in supporting the old X11 interface for
font matching or listing those fonts as even available if you're using Xft and
fontconfig for fonts.

At the same time, it's important that the current code build without Xft and it
would be nice if the code built without X11 support for those of us that don't
need it anymore.  Plus, it would be nice to have a run time switch to be able to
choose between the two of them at run time, assuming you've built with both.

So, given those broad goals the easiest way to get from point A to point B is to
seperate the old X11 code from the Xft code so that you can build with one or
the other or both.  That's pretty easy:

o Turn nsFontMetricsGTK into a class that doesn't know anything about freetype,
X11 or fontconfig.

o Build an interface that is the basic definition for font matching and
rendering at a pretty high level.  (nsFontInstance)

o Move the existing X11 code into its own class that knows how to do that
matching and rendering. (nsFontInstanceGTK)

o Create a new Xft class that knows how to do all of its own matching and
rendering. (nsFontInstanceXft)

o Put a big fat switch, frankenstein style, in nsFontMetricsGTK that knows how
to dispatch to the right instance, depending on build options and runtime options.

...and that's about it.

The way that these goals play out into an implementation, the Xft code is almost
all new code, and doesn't have to integrate with the old code.\

What do other developers need, given that I'm not making any changes to the old
code, other than moving it?  This is as pretty close to zero impact on the X11
code as it gets, other than not doing it at all, and that's not an option.
> I'm also not interested in supporting the old X11 interface for font
> matching or listing those fonts as even available if you're using Xft and
> fontconfig for fonts.

This would eliminate the bulk of the fonts on most Unix system and result
in very serious display regressions.

Until most Unix systems (and variants like Redhat) have a good supply of
TrueType and/or Type1 fonts this strategy is completely unrealistic for X 
Windows Mozilla and Netscape.

If you want to make an experimental build that is fine but I will strongly
resist allowing this in standard Mozilla and Netscape builds (including rpms).

Xft2 can display PCF fonts, so that shouldn't really be a problem.  Also, most
linux distributions come with a fair number of free Type1/TTF fonts (the URW and
Luxi fonts come to mind).
> This would eliminate the bulk of the fonts on most Unix system and result
> in very serious display regressions.

That's why the "big switch" exists -- the default installation would continue to
provide both the core and Xft paths and continue to use the core path by
default.  Vendors redistributing Mozilla could choose to provide a suitable set
of fonts accessible by Xft and eliminate the core support in their private builds.

> Xft2 can display PCF fonts, so that shouldn't really be a problem.

Xft depends on FreeType for font access and FreeType has relatively primitive
PCF support at this point.  It doesn't deal with compressed (.gz or .Z) files
and it has no support for encodings other than Latin-1 or 10646.  Vendors would
have to make a small effort to ensure that the necessary fonts could be accessed
by Xft applications.

My plan at this point is to migrate XFree86 from pcf to sfnt for bitmap fonts.
I've seen some preliminary results from the people involved which are very
encouraging; less disk space with better performance.  I'm hoping that work will
be finished in the next couple of months.  This seems a more profitable route
than adding code to FreeType to support decompression and non-Unicode encodings,
and so I've terminated efforts along that line.
> 
> This would eliminate the bulk of the fonts on most Unix system and result
> in very serious display regressions.
> 
> Until most Unix systems (and variants like Redhat) have a good supply of
> TrueType and/or Type1 fonts this strategy is completely unrealistic for X 
> Windows Mozilla and Netscape.

That's why I'm suggesting that we make it a build option, and that they can be
seperated.  It's going to be a while before we have the same kind of coverage
that we do with pcf fonts.  However, I suspect it's going to be a lot shorter
than you think.

> 
> If you want to make an experimental build that is fine but I will strongly
> resist allowing this in standard Mozilla and Netscape builds (including rpms).

We're not talking about the default build here at all.  Besides, the Xft and
fontconfig dependency are hard to satisfy at this point.  Although the next Red
Hat release will ship with this as the only font selection mechanism available
to Gtk and Qt apps, it's going to be a while before it gets into the field.

But, then again, there will be quite a few early adoptors including Red Hat.
Why don't you give this effort its own name?

This way it will be easier for users to tell which one they are using, the press
can easily tell which one they are reviewing, and we can setup a separate 
component in bugzilla for you.

You could call it xftzilla or blizilla or whatever.
I just can't resist putting in my 2c here.

The current X font system sucks.  It really really sucks.  Almost anything will
be better, and I think that Keith's Xft stuff will be a lot better.

For quite some time, I've been using the old Xft patches, because it's the only
thing that makes mozilla not look like crap on Unix.  Heck, I'm still using 1.0
because I just can't look at the "mainstream" builds.  It still doesn't look as
good as mozilla does on Mac OS X, but it holds it's own.

I think Chris is on the right path in developing a new font subsystem based on
Xft, and keeping the old one relatively intact as a "legacy" system for users on
older X11 systems.  Making spaghetti-code which mixes and matches renderers
(Xft, X11) seems like it will be _more_ difficult to maintain, and I don't see a
good reason to mix renderers on-screen anyway.

Call it what you want while he's writing it, but by time the convergence of this
code, the Xft system in X11, and vendor support for the two comes together,
you'll probably see all the new systems using this.  I, for one, am very happy
to see this work progressing, because with the standard X11 code, X11 users are
getting a really sub-standard experience, which makes Unix systems look bad next
to their proprietary counterparts.

Agreeably, the need for legacy X11 font support will be important for quite some
time, but anything that helps move the drive for a modern, unified X11 font
handling system is in the entire community's best interest.

Once the bugs are ironed out (and there seems like there's plenty of work there)
I'd hope that press would be forewarned when using the legacy code, instead of
this new, modern code, so they don't accidentally judge Mozilla on Unix based on
it's current font handling system.


I would really encourage people to dream a bigger dream: 

  Dump the whole X server!

The goal to allow a user to do graphics over a network is now done via the HTTP 
and the web.

Why keep a system where graphic operations require a process context switch? 
This is very expensive (which is why no other UI does this).

Very few users use the remote capabilities of X. Its silly to make the typical 
case (client and framebuffer on the same machine) suffer for the odd unused case 
(remote frame buffer). If people want remote access then make helper processes
that communicate with remote processes and access the local framebuffer.


bstell, I can only assume that the previous comment was posted sarcastically, in
which case it is not very helpful. 

Given that Chris' patch can be compiled away, what specific objections do you
have to it?

(If you were being serious about X being redundant because of HTTP and the web,
then you have bigger problems)
It is a completely serious statement. If we are going to rip up the user's 
environment (one more time) then lets move from X's very inefficient design.
If we are going to hang on to a 1980's design then I'm reluctant to disturb 
users.

> Given that Chris' patch can be compiled away, what specific objections do you
> have to it?

1) It needs to be out of the normal font path so it does not disturb normal
   font development. This would help reduce bitrot. This would also simpilify 
   the review process. It could be something like gfx/src/qt.

2) It needs to be easily identifiable by the user so they know what to expect / 
   not expect. The best way would be to have a separate executable name.

3) It needs its own bugzilla component to help make the bug reporting / 
   assignment manageable.


> It is a completely serious statement. If we are going to rip up the user's 
> environment (one more time) then lets move from X's very inefficient design.
> If we are going to hang on to a 1980's design then I'm reluctant to disturb 
> users.

I'm not sure how to react to something this ludicrous.  It's kind of like having
a conversation where you say "Hey, how are you doing today?" and the response
being "YOU SMELL LIKE CABBAGE!"  A total non sequitur.

> 
>> Given that Chris' patch can be compiled away, what specific objections do you
>> have to it?
> 
> 1) It needs to be out of the normal font path so it does not disturb normal
>    font development. This would help reduce bitrot. This would also simpilify 
>    the review process. It could be something like gfx/src/qt.

I'm not going to fork all of gfx/src/gtk to just change fonts.  That's pretty
silly.  Also, since this patch moves the code without making any changes and
seperates the code paths entirely, I'm doing exactly what you state here - it
does not disturb normal font development.  It just means that you do your
hacking in another file.

> 
> 2) It needs to be easily identifiable by the user so they know what to expect / 
>    not expect. The best way would be to have a separate executable name.

That's completely silly.

> 
> 3) It needs its own bugzilla component to help make the bug reporting / 
>    assignment manageable.
> 

That I don't mind at all.  Totally reasonable.
I want Xft in Mozilla because 1) our fonts look awful without it, especially on
an otherwise-Xft desktop like GNOME 2, and 2) it's already configured by my
distribution/desktop.  It's a disruption to my environment to _not_ have Xft
present, which is why I'm not upgrading to 1.0.1/1.1 until there are Xft-enabled
builds.  (And I am _far_ from the only person in this situation.  And as Sun and
HP and Red Hat start to ship GNOME2-based desktops in the very near future,
along with 3rd-party desktop shops like Ximian, the number of people who will
expect Mozilla to use their font config will only increase.)

Demanding another binary name is just silly.  We use the name "mozilla"
everywhere (including binary naming on Windows and Mac), it's a protected
trademark, etc.  If you build with xlib, you get "mozilla", not "xlibzilla". 
Let's just forget that suggestion was even made.

> it does not disturb normal font development.

Have you forgotten so quickly that you just told people to stay out of your way
because you were going to remove the direct FreeType2 code? This was a clear 
attempt to disturb normal font developement.

http://bugzilla.mozilla.org/show_bug.cgi?id=144664#c11
> -- Addl. Comment #11 From Christopher Blizzard 2002-07-22 09:01 --
> 
> Careful.  I'm about to whack this code hard, removing most of the 
> direct free type interfaces in favor of Xft.


> [different executable name] Let's just forget that suggestion was even made.

No lets not. Remember what Chris is proposing:

> I'm also not interested in supporting the old X11 interface ... or listing
> those fonts as even available if you're using Xft and fontconfig for fonts.

This will cause major regressions for many users. Lots of users are going
to open bugs that say "why can't moz see my fonts anymore?. 

Do I hear a volunteer to be the default assignee that looks at X bugs? That 
person will be burdened with the repeated task of asking the question "which 
version are you using?" (Mike/Chris?)

It is unfair to burden the current font developers with this. Unless someone
agrees to do this then we need to make it as easy as possible to tell which
font system is in place. The easiest way to do this is by having a separate
executable name.
> Have you forgotten so quickly that you just told people to stay out of your way
> because you were going to remove the direct FreeType2 code? This was a clear 
> attempt to disturb normal font developement.

What??  I never said that.  Once again, for the umpteenth time you have
misrepresented what I've said.  I only said that I was going to be working in a
specific area and they should be aware of it.  I never said stop.  I never said
don't do this.  I was just letting people know.  You know, communicating and stuff?

> This will cause major regressions for many users. Lots of users are going
> to open bugs that say "why can't moz see my fonts anymore?. 

Only if you choose to build with Xft enabled.  You don't have to do this. 
Mozilla doesn't have to do this.  Netscape doesn't have to do this.  People that
want to use Xft can deal with those bugs.

> 
> Do I hear a volunteer to be the default assignee that looks at X bugs? That 
> person will be burdened with the repeated task of asking the question "which 
> version are you using?" (Mike/Chris?)
> 
> It is unfair to burden the current font developers with this. Unless someone
> agrees to do this then we need to make it as easy as possible to tell which
> font system is in place. The easiest way to do this is by having a separate
> executable name.
> 

As opposed to burdening the gtk developers with waiting for roland to copy code
over and over into another directory every time they fix something?  Why don't
we have x11zilla?  That's an example of another huge forked code base.

I've already said that I'm perfectly happy with an xft specific component. 
That's fine with me.  You shouldn't have to do a lot of triage, since this
probably won't be coming from mozilla.org in the near future.
> What??  I never said that.  Once again, for the umpteenth time you have
> misrepresented what I've said.  I only said that I was going to be working 
> in a specific area and they should be aware of it.  I never said stop.  I 
> never said don't do this.  I was just letting people know.  You know, 
> communicating and stuff?

In the future, if you want to suggest to other developers that you are
trying to _cooperate_ with them, then would you kindly _not_ say: 

  "Careful.  I'm about to whack this code hard" 

and then clarify that statement by saying 

  "think about it since I'm working on something that might conflict". 

Both these statements indicate that you are doing something with will break
their work and they had better watch out. Since neither of these comments
hint at cooperation, it makes it very hard to gather that you want to work
with others. They both sound like you will be causing the other developers
problems. In the future, perhaps you could say something like this:

  I'm planning big changes in related code and I would like to coordinate
  with you to minimize any impact on your work.

>> This will cause major regressions for many users. Lots of users are going
>> to open bugs that say "why can't moz see my fonts anymore?. 
>
> Only if you choose to build with Xft enabled.  You don't have to do this.
> Mozilla doesn't have to do this.  Netscape doesn't have to do this.  People
> that want to use Xft can deal with those bugs.

Xft is only available on a very few systems and lacks a variety of features
that X mozilla has. If someone wants to build an executable that lacks moz's
current features and will not run on systems without Xrender, Xft, and
FontConfig I have no issue as long as it has a different name. It is fine to
add features and keep the common name but if we subtract features then we
should make it _very_ clear that the build is missing these features. Simply
having some release notes is not enough. It would need to be very plain to
even the most novice user. In a case like this a different name would be an
appropriate way to let users know.

Having said all this I would _much_ prefer that Xft support be built into
the regular builds by default, and automatically enabled when moz finds the 
needed libraries and X extension are available on the user's system.

FontConfig is a bit more difficult but I would prefer that moz use the parts
that are ready now and then integrate the other parts as they become ready.

> As opposed to burdening the gtk developers with waiting for roland to copy 
> code over and over into another directory every time they fix something? 
> Why don't we have x11zilla? 

We already started working on this a long time ago. Its called
gfx/src/x11shared and I'm sure Roland would love to have you participate.

> I've already said that I'm perfectly happy with an xft specific component. 

Would you please clarify what you mean here?

  Do you want to fit Xft in to the current moz font code?

  Or are you just talking about bugzilla?

> You shouldn't have to do a lot of triage, since this probably won't be 
> coming from mozilla.org in the near future.

Keith, do you know that I _already_ spend time helping people figure out if
they have your Xft rpm version? As long as it is called mozilla users will
file bugzilla bug reports against mozilla and mozilla font developers will
have to spend time helping users determine which version they have.
Let me list the issues I see to dropping the current moz font code and just 
using Xft/FontConfig.

1) Only percentage of Unix/Linux systems have Xft installed. Is there a
   place where users can download the needed libraries to upgrade the
   various flavors of Unix/Linux (eg: Solaris, HPUX, ...)? I don't
   think it is acceptable to simply assume a typical user will download
   the source, build, and install it.

2) Has Xft released a version with support for non Xrender systems?
   I know Keith wrote the code but I never saw an announcement saying
   it was available. Again: how do users get it on their systems?

3) FontConfig has not (yet) been released. The posting I've seen indicate
   that it is just now at RC3.  Again: how do users get it on their systems?

4) Does Xft/FontConfig support MathML?

5) When doing fallback font lookup does Xft/FontConfig distinguish between
   special characters like CJK (wide) smart quotes and western (narrow) smart
   quotes and choose the appropriate one for the document's language?

6) Does Xft/FontConfig support anti-aliased scaled bitmaps (AASB)? Most
   Unix/Linux systems have limited sizes for CJK fonts. Using AASB mozilla
   can generate good looking intermediate sizes which leads to much better
   page layout for CJK pages. This is very important for systems that lack
   outline scalable font like Truetype.

7) Does Xft/FontConfig support GB18030? The People's Republic of China
   requires any software distributed there to support this.

8) Does Xft/FontConfig support Unicode surrogates?

9) Does Xft/FontConfig support Korean precomposed Hangul using hanterm fonts?

10) Does Xft/FontConfig have a way to ban ugly X fonts? Unfortunately, there
    are a fair number of bad looking X fonts.

11) Does Xft/FontConfig support user defined font encodings? We still have
    users that use this to display pages where the font or encoding is
    not a standard.

12) Does Xft/FontConfig support transliteration? Ie: what does Xft/FontConfig
    do for characters like copyright when no font has a copyright symbol?

13) Does Xft/FontConfig support a way for the font developer(s) to debug
    what fonts are being selected by a users system? This is important
    because the fonts installed on users' systems varies widely and it is not
    always possible to reproduce a user's problem on a developer's system.

14) Does Xft/FontConfig support Indic shaping? We have work in progress here.

15) Does Xft/FontConfig support embedding Truetype fonts in Postscript printing
    output? We have work in progress here.


{argh, mid-air collision... well here's what i said _before_ the last comment
post...}

ok, this is getting a bit ridiculous.  i'm cc-ed on this bug because i'm very
interested in the development of an xft-enabled mozilla, not because i want to
hear about what bstell think's about blizzard's communication ability.  if you
have a problem with how blizzard post comments to other bugs, and feel the need
to flame him over how he spends his coding time, please use some other private
means such as email.  the 'conversation' is pretty much pointless at this time
anyway, since compromise or agreement is rarely reached while the participants
are **** off at each other, as the case appears to be here.  so please, calm down.

with that said, my perspective:  i think moz looks _awful_ without the xft code.
 i have been using 1.1a since it came out, simply because i can't stand 1.1b or
the trunk without the xft code.  yes, i happen to understand the technical and
support problems associated with making a big change in something as important
as font rendering. but:

1) as blizzard has repeatedly stated, xft/fontconfig will not be in the default
build until it can replace the current system (or, i would hope, coexist with it).
2) i believe blizzard mentioned somewhere that his current vision is that the
core x font code and xft font code will eventually be able to sit side by side,
with the option of using either.
3) this is all just initial work to get the code _operational_, nothing about it
being usable for the masses.  it is likely to undergo many changes before it's
used mainstream.
4) having a different application name is ridiculous (though i _do_ like how
'blizzilla' sounds).  how does a new font renderer make it a different application?
5) if people aren't reporting bugs on the xft-enabled RPM correctly, and you
(bstell) are required to sort them all out, then this is 1) a deficiency in
bugzilla's bug-reporting facilities, and you should file a bug for that, and 2)
a problem with how the xft-enabled RPMs are being built and distributed - e.g.
the name should be something like mozilla-1.0.0xft.i386.rpm.  then that way, a
quick query of what package version the user has installed will clear up any
problems about who should be dealing with the bug.

i really want to see the xft/fontconfig code working, and i think that _after_
the code is working, effort should be put on trying to integrate it as
seamlessly as possible with the current system (even if that means changes to
the current system), with the ability to fallback to core x font support when
xft/fontconfig isn't available.  this way it can be enabled in the default
build, but not give users problems when they don't have the necessary pieces to
make use of it.

i'm sorry if i'm a bit out of line, and i'm sorry if i sound condescending, but
mozilla is something i care about very much (as i'm sure is true of the
developers involved as well), and i just want to see useful work getting done
without wasting time on arguing over things that haven't even happened yet.
Please note the following correction to comment #81:

s/Keith, do you know/Chris, do you know/
> 2) i believe blizzard mentioned somewhere that his current vision is that
> the core x font code and xft font code will eventually be able to sit side
> by side, with the option of using either.

It should not be an either or choice. We should use the best parts of both.

> 4) how does a new font renderer make it a different application?

When we drop features it makes it a different application.

> 5) if people aren't reporting bugs on the xft-enabled RPM correctly, and 
> you (bstell) are required to sort them all out, then this is 1) a 
> deficiency in bugzilla's bug-reporting facilities, and you should file a 
> bug for that, and

If we use the name mozilla how will novice users know?

> 2) a problem with how the xft-enabled RPMs are being built and distributed 
> - e.g. the name should be something like mozilla-1.0.0xft.i386.rpm.  then
> that way, a quick query of what package version the user has installed will
> clear up any problems about who should be dealing with the bug.

What happens when the installer is a different person from the user?

What happens when the installer forgets or does not realize the significance?

> i'm sorry if i'm a bit out of line, and i'm sorry if i sound condescending,
> but mozilla is something i care about very much (as i'm sure is true of the
> developers involved as well), 

You are not out of line. We all care about moz's development.

> and i just want to see useful work getting done without wasting time on 
> arguing over things that haven't even happened yet.

It is important to plan ahead. We need to be getting the best of both not 
dropping the existing features. It is far better to spend this time up front
before a developer has invested a large amount of work. I certianly do not plan
to see Chris' work suffer the fate the fate of getting to the review stage only
to be force back to the design stage because the design stage was incomplete.

> 1) Only percentage of Unix/Linux systems have Xft installed. Is there a
 >   place where users can download the needed libraries to upgrade the
>    various flavors of Unix/Linux (eg: Solaris, HPUX, ...)? I don't
>    think it is acceptable to simply assume a typical user will download
>    the source, build, and install it.

If Mozilla binaries are made available for these systems, fontconfig/Xft
binaries can be provide at the same time. If binaries aren't available for 
Mozilla, I don't see how they are needed for Xft/fontconfig. Mozilla
could certainly include private versions of the libraries and have
a:

 --with-included-xft 

command line option, though I wouldn't recommend it.

> 2) Has Xft released a version with support for non Xrender systems?
>    I know Keith wrote the code but I never saw an announcement saying
>    it was available. Again: how do users get it on their systems?

Should be released this week.

> 3) FontConfig has not (yet) been released. The posting I've seen indicate
>    that it is just now at RC3.  Again: how do users get it on their systems?

Should be released this week

> 4) Does Xft/FontConfig support MathML?

If fonts are encoded using some sort of standard,it should just be
a layout level issue. If it involves recognizing specific fonts, and
knowing how they are encoded, it will be a little bit difficult to
fit that sort of hackery on top o fthe fontconifg framework, though Mozilla 
could certainly just ask for those fonts specifically.

> 5) When doing fallback font lookup does Xft/FontConfig distinguish between
>    special characters like CJK (wide) smart quotes and western (narrow) smart
>    quotes and choose the appropriate one for the document's language?

If it's a matter of font selection, fontconfig will reorder it's choice of 
fonts based on language tag. If it's a matter of code point selection,
then it's a layout level issue, not a fontconfig/Xft issue.

> 6) Does Xft/FontConfig support anti-aliased scaled bitmaps (AASB)? Most
>    Unix/Linux systems have limited sizes for CJK fonts. Using AASB mozilla
>    can generate good looking intermediate sizes which leads to much better
>    page layout for CJK pages. This is very important for systems that lack
>   outline scalable font like Truetype.

If this is something to care about, it should be added to freetype.
Putting it in Mozilla is putting it in the wrong layer, since the idea
is not Web-browser specific in any way.

> 7) Does Xft/FontConfig support GB18030? The People's Republic of China
>    requires any software distributed there to support this.

Character encodings have nothing to do with the font system, in general. Both
KDE and GNOME run fine in GB18030 on top of Xft/fontconfig.

> 8) Does Xft/FontConfig support Unicode surrogates?

Encoding issue, again. XftDrawStringUtf16 doesn't exist so, you'd have
to convert to UTF-8 or 32-bit characters first, but Xft definitely 
clean for non-BMP.

> 9) Does Xft/FontConfig support Korean precomposed Hangul using hanterm fonts?

Do you mean "composing Jamos on the fly" as opposed to using precposed
syllables? This is basically a layout level issue, though depending on
what fonts you are talking about, there might be some connection to
the custom encoding question. I've never seen a TrueType or postscript
font for use with combine-on-the-fly Jamos, so it's hard to say if 
Xft could handle such a font or not. I suspect it could.

> 10) Does Xft/FontConfig have a way to ban ugly X fonts? Unfortunately, there
>     are a fair number of bad looking X fonts.

fontconfig is a separate font database from the core X system, so generally
the solution is simply not to have these fonts in the database. I'm not
sure I understand this question completely, though.

> 11) Does Xft/FontConfig support user defined font encodings? We still have
>     users that use this to display pages where the font or encoding is
>     not a standard.

The only place I've ever seen this is in Indic web pages, where typical
the usage isn't custom encoded fonts but *mis-encoded* fonts where ASCII
and/or latin-1 code points have been reused. Fontconfig can't detect this,
so sure, it will ``work''. You can't simply create a clean custom encoding
for a TrueType fonts, so people don't do it, but reuse other encodings.

> 12) Does Xft/FontConfig support transliteration? Ie: what does Xft/FontConfig
>     do for characters like copyright when no font has a copyright symbol?

Layout issue, not an Xft issue. The layout layer can find if the font has
a copyright symbol or not with no problem.

> 13) Does Xft/FontConfig support a way for the font developer(s) to debug
>     what fonts are being selected by a users system? This is important
>     because the fonts installed on users' systems varies widely and it is not
>     always possible to reproduce a user's problem on a developer's system.

Yes.

> 14) Does Xft/FontConfig support Indic shaping? We have work in progress here.

Layout issue, not an Xft issue; Pango, for example, has quite complete Indic
OpenType support on top of Xft.

> 15) Does Xft/FontConfig support embedding Truetype fonts in Postscript > printing
>     output? We have work in progress here.

You have access to the font files and FT_Face objects, I don't see how this
is otherwise relevant to Xft/fontconfig. There has been discusison of
finding a way to share the code for this between all the different users
of fontconfig, but using fontconfig certainly doesn't hinder Mozilla 
from going its' own way here.

The issues above seem to indicate some confusion about what fontconfig
and Xft are; in basic terms:

 - fontconfig is an API for maintaining font catalogs, and doing lookups
   in them. 

 - Xft is an API for rendering glyphs from fonts located by fontconfig.

Most of the things mentioned above either are non-issues or are at
different levels from Xft/fontconfig. The only ones I see of being 
any concern at all for the Xft/fontconfig layer are the ones involving
encoding - MathML, Hangul combining Jamos, and custom encodings. I don't 
really understand any of these problems fully. If the needs could be
articulated cleary, then I'm sure if Xft doesn't meet the needs in
this area now, then it could be extended.

The reason we need Xft/fontconfig support in Moizlla is simply that it will 
shortly be  *the* font system on the majority of Linux and Unix systems. 
(We could argue how big that majority is; I'd say "vast majority"
in fact.) On such systems, not using Xft/fontconfig for Mozilla makes
no more sense than not using the Win32 font API on Windows.

I'd argue that on other Linux/Unix systems, Mozilla might as well use 
Xft/fontconfig as well, since all the problems need to be solved
for Xft/fontconfig.

I don't think moving Xft/fontconfig-2.0.0 will meet the overwhelming
majority of Mozilla's needs. If Mozilla has other different needs beyond
that, then the reason they aren't addressed by Xft/fontconfig is because
the only people who understand them haven't yet worked hands-on with
Xft/fontconfig so no concrete proposal for a solution hsa been arrived
at.

What I don't want to see happening (and seems to be happening already)
is:

 Month1: We can't switch to Xft/fontconfig because the Xft/fontconfig
  codebase doesn't support X .
 
 [ Work continues on the custom Mozilla code base, and support for Y is
   added ] 

 Month2: We can't switch to Xft/fontconfig because the Xft/fontconfig
  codebase doesn't support X and Y.

 [ Work continues on both code bases, Xft/fontconfig gets X, Z is
    added to the custom font code base ]
  
 Month3: We can't switchIf  to Xft/fontconfig because the Xft/fontconfig
  codebase doesn't support Y and Z.

And AFAIK, the big issue right now isn't switching over, but just
getting Xft/fontconfig support in the tree?
> I don't think moving Xft/fontconfig-2.0.0 will meet the overwhelming
> majority of Mozilla's needs. If Mozilla has other different needs beyond
> that, then the reason they aren't addressed by Xft/fontconfig is because
 the only people who understand them haven't yet worked hands-on with
> Xft/fontconfig so no concrete proposal for a solution hsa been arrived
> at.

That is clearly incoherent; What I meant to say was:

 I think Xft/fontconfig-2.0.0 will meet the overwhelming majority of Mozilla's
 needs...
> If Mozilla binaries are made available for these systems, fontconfig/Xft
> binaries can be provide at the same time. If binaries aren't available for
> Mozilla, I don't see how they are needed for Xft/fontconfig.

Mozilla/Netscape _is_ being distributed in binary form to new and older
Unix/Linux systems. Xft/FontConfig needs to get a similar distribution
system in place.

> Mozilla could certainly include private versions of the libraries and
> have a: --with-included-xft
> command line option, though I wouldn't recommend it.

Does Mozilla/Netscape want to distribute Xft/FontConfig? If you or someone
else wants to pursue this that's fine with me.

> > 2) Has Xft released a version with support for non Xrender systems?
...
> Should be released this week.
>
> > 3) FontConfig has not (yet) been released. The posting I've seen indicate
...
> Should be released this week

It will be good to see them finally get some field time.

> > 4) Does Xft/FontConfig support MathML?
>
> If fonts are encoded using some sort of standard,it should just be
> a layout level issue. If it involves recognizing specific fonts, and
> knowing how they are encoded, it will be a little bit difficult to
> fit that sort of hackery on top o fthe fontconifg framework, though
> Mozilla could certainly just ask for those fonts specifically.

I do not particularly care how it is done but I do not want to lose this
feature. Perhaps someone can convince Roger Sidje to add MathML support
to Xft/FontConfig.

> > 5) When doing fallback font lookup does Xft/FontConfig distinguish
> >    between special characters like CJK (wide) smart quotes and western
> >    (narrow) smart quotes and choose the appropriate one for the
> >    document's language?
>
> If it's a matter of font selection, fontconfig will reorder it's choice
> of fonts based on language tag. If it's a matter of code point selection,
> then it's a layout level issue, not a fontconfig/Xft issue.

Agreed, this is a font selection issue. It is still a requirement that
needs to be satisfied and I am curious not if FontConfig can do this but
if FontConfig does do this. This is an easy one, just get it in FontConfig
and lets be done with the discussion.

> > 6) Does Xft/FontConfig support anti-aliased scaled bitmaps (AASB)? Most
> >    Unix/Linux systems have limited sizes for CJK fonts. Using AASB
> >    mozilla can generate good looking intermediate sizes which leads to
> >    much better page layout for CJK pages. This is very important for
> >    systems that lack outline scalable font like Truetype.
>
> If this is something to care about, it should be added to freetype.

This is actually an interesting point. Some kind of AASB should be in
FreeType since AASB is part of the Truetype spec. It is not clear to
me if FreeType should do this for non-Truetype fonts although it might
be reasonable to ask them to do it. Regardless of how it is done I do
not want to lose this feature.

> Putting it in Mozilla is putting it in the wrong layer, since the idea
> is not Web-browser specific in any way.

I would love to see it in other places like Xft or FreeType. It is very
important for CJK users with only bitmap fonts. When it is available in
a 3rd party library we can remove the code in mozilla. Until then moz
still needs it.

> > 7) Does Xft/FontConfig support GB18030? The People's Republic of China
> >    requires any software distributed there to support this.
>
> Character encodings have nothing to do with the font system, in general.
> Both KDE and GNOME run fine in GB18030 on top of Xft/fontconfig.
> ... XftDrawStringUtf16 doesn't exist,

Mozilla is a UTF-16 application so regardless of how it is implemented
(Xft/FontConfig or Chris' patch) non BMP support needs to continue to
work for both Truetype fonts and X fonts.

> > 9) Does Xft/FontConfig support Korean precomposed Hangul using hanterm
> >    fonts?
>
> Do you mean "composing Jamos on the fly" as opposed to using precposed
> syllables? This is basically a layout level issue, though depending on
> what fonts you are talking about, there might be some connection to
> the custom encoding question. I've never seen a TrueType or postscript
> font for use with combine-on-the-fly Jamos, so it's hard to say if
> Xft could handle such a font or not. I suspect it could.

There are X users that do not have fonts with precomposed Jamos. If they
do have non-composed Jamos moz will use multiple non-composed glyphs to
represent the precomposed characters. This feature need to be retained.

> > 10) Does Xft/FontConfig have a way to ban ugly X fonts? Unfortunately, there
> >     are a fair number of bad looking X fonts.
>
> fontconfig is a separate font database from the core X system, so generally
> the solution is simply not to have these fonts in the database. I'm not
> sure I understand this question completely, though.

There are some bad looking fonts (the URW fonts come to mind). How will it
be managed so the novice user does not see these? This is an easy one, I
just want to know that it is actually done.

> > 11) Does Xft/FontConfig support user defined font encodings? We still have
> >     users that use this to display pages where the font or encoding is
> >     not a standard.
>
> The only place I've ever seen this is in Indic web pages, where typical
> the usage isn't custom encoded fonts but *mis-encoded* fonts where ASCII
> and/or latin-1 code points have been reused. Fontconfig can't detect this,
> so sure, it will ``work''. You can't simply create a clean custom encoding
> for a TrueType fonts, so people don't do it, but reuse other encodings.

However it gets done is fine as long as we retain this feature.

> > 12) Does Xft/FontConfig support transliteration? Ie: what does 
Xft/FontConfig
> >     do for characters like copyright when no font has a copyright symbol?
>
> Layout issue, not an Xft issue. The layout layer can find if the font has
> a copyright symbol or not with no problem.

In mozilla the layout code does layout not character transliteration.
Regardless of where it is done this feature needs to be retained.

> > 13) Does Xft/FontConfig support a way for the font developer(s) to debug
> >     what fonts are being selected by a users system? This is important
> >     because the fonts installed on users' systems varies widely and it is 
not
> >     always possible to reproduce a user's problem on a developer's system.
>
> Yes.

Could you provide details? How does a font maintainer find out what fonts
are being used on the user's system?

> > 14) Does Xft/FontConfig support Indic shaping? We have work in progress
> >     here.
>
> Layout issue, not an Xft issue; Pango, for example, has quite complete Indic
> OpenType support on top of Xft.

This is not a layout issue. Layout should deal with the Unicode character
values not the glyph numbers. The handling of ligatures in moz belongs at
the font subsystem layer.

> > 15) Does Xft/FontConfig support embedding Truetype fonts in Postscript
> >     printing output? We have work in progress here.
>
> You have access to the font files and FT_Face objects, I don't see how this
> is otherwise relevant to Xft/fontconfig. There has been discusison of
> finding a way to share the code for this between all the different users
> of fontconfig, but using fontconfig certainly doesn't hinder Mozilla
> from going its' own way here.

It does in the sense that the proposal here currently plans to rip out the
code that the printing work currently uses. This is an important feature and
we should not lose it.

> The issues above seem to indicate some confusion about what fontconfig
> and Xft are; in basic terms:
>
>  - fontconfig is an API for maintaining font catalogs, and doing lookups
>    in them.
>
>  - Xft is an API for rendering glyphs from fonts located by fontconfig.
>
> Most of the things mentioned above either are non-issues or are at
> different levels from Xft/fontconfig. The only ones I see of being
> any concern at all for the Xft/fontconfig layer are the ones involving
> encoding - MathML, Hangul combining Jamos, and custom encodings. I don't
> really understand any of these problems fully. If the needs could be
> articulated cleary, then I'm sure if Xft doesn't meet the needs in
> this area now, then it could be extended.

Xft/FontConfig may or may not be the exact place where the features are
provided. Regardless of where the code is, the features need to continue to
work in mozilla.

> The reason we need Xft/fontconfig support in Mozilla is simply that it will
> shortly be  *the* font system on the majority of Linux and Unix systems.
> (We could argue how big that majority is; I'd say "vast majority"
> in fact.) On such systems, not using Xft/fontconfig for Mozilla makes
> no more sense than not using the Win32 font API on Windows.

Can we agree that "vast majority" is 95%? Or is this too high?
The question here is "what percentage of users is it okay to cut off?".

Whatever percentage we agree represents "vast majority" we need to look at
what percentage of systems in the field today have Xft?

When will the number of systems with Xft reach 50%, 75%, 90%, 95%?

What about FontConfig?

I know it sounds easy to add a new library to moz but I don't suppose you
were around when Darin tried to start using uconv.
http://bugzilla.mozilla.org/show_bug.cgi?id=129279
Uconv has been out for years and they still had problems getting it to
work on all the various versions of Unix.

Using a new library seems like an easy thing but getting the all the details
are done is difficult.

The easiest to get Xft in moz without losing features is to integrate Xft
into the current code and use the best features of both. Doing it this way
we get minimal developer impact and we do not lose features.

Repeating my question; I'm a regular mozilla 1.0 xft experimental build user.
Will someone release a 1.1 build with xft support on 

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/xft/Red_Hat_7x_RPMS/

?
I'll try to make some 1.1 rpms this week, assuming I find the time to finish the
major parts of my patch.
> > If Mozilla binaries are made available for these systems, fontconfig/Xft
> > binaries can be provide at the same time. If binaries aren't available for
> > Mozilla, I don't see how they are needed for Xft/fontconfig.
> 
> Mozilla/Netscape _is_ being distributed in binary form to new and older
> Unix/Linux systems. Xft/FontConfig needs to get a similar distribution
> system in place.

I suspect that Keith would be happy to put binary packages on his site
if people provided them; however, distributing them in the same place
as the Mozilla binary packages makes quite a bit of sense to me.

[...]

> > > 2) Has Xft released a version with support for non Xrender systems?
> ...
> > Should be released this week.
>>
> > > 3) FontConfig has not (yet) been released. The posting I've seen indicate
> ...
> > Should be released this week
>
> It will be good to see them finally get some field time.

Xft2 and fontconfig are a central part of the current Red Hat betas, which
have thousands of of users. We are using them as the only
rendering engine for both GTK+ and Qt. This is not exactly "unused code".

> > > 4) Does Xft/FontConfig support MathML?
>>
> > If fonts are encoded using some sort of standard,it should just be
> > a layout level issue. If it involves recognizing specific fonts, and
> > knowing how they are encoded, it will be a little bit difficult to
> > fit that sort of hackery on top o fthe fontconifg framework, though
> > Mozilla could certainly just ask for those fonts specifically.
>
> I do not particularly care how it is done but I do not want to lose this
> feature. Perhaps someone can convince Roger Sidje to add MathML support
> to Xft/FontConfig.

It seemed to me that the big stumbling block earlier was just
that people weren't sure how MathML fonts should be encoded. 
If we can get the Xft code into the tree, people will be able
to start playing with the the combination instead of just
theorizing about it.

> > > 5) When doing fallback font lookup does Xft/FontConfig distinguish
< > >    between special characters like CJK (wide) smart quotes and western
> > >    (narrow) smart quotes and choose the appropriate one for the
> > >    document's language?
> >
> > If it's a matter of font selection, fontconfig will reorder it's choice
> > of fonts based on language tag. If it's a matter of code point selection,
> > then it's a layout level issue, not a fontconfig/Xft issue.
> 
> Agreed, this is a font selection issue. It is still a requirement that
> needs to be satisfied and I am curious not if FontConfig can do this but
> if FontConfig does do this. This is an easy one, just get it in FontConfig
> and lets be done with the discussion.

If your language tag is "ja-jp", then fontconfig favours fonts that support
Japanese, if your language tag is "en-us", then fontconfig does not
do such reordering of the fallback fonts. This should be sufficient
handling for ambiguious width characters.

> > > 10) Does Xft/FontConfig have a way to ban ugly X fonts? Unfortunately, there
> > >     are a fair number of bad looking X fonts.
> >
> > fontconfig is a separate font database from the core X system, so generally
> > the solution is simply not to have these fonts in the database. I'm not
> > sure I understand this question completely, though.
> 
> There are some bad looking fonts (the URW fonts come to mind). How will it
> be managed so the novice user does not see these? This is an easy one, I
> just want to know that it is actually done.

The main URW fonts render quite well here, thank you. (I've fixed
a bunch of Freetype Bugs that were causing glitches with their 
rendering.) If the novice shouldn't see a font, then I don't 
know why it is in the fontconfig catalog. It doesn't seem to me
that a blacklist mechanism is necessary.

> > > 12) Does Xft/FontConfig support transliteration? Ie: what does 
> Xft/FontConfig
> > >     do for characters like copyright when no font has a copyright symbol?
> >
> > Layout issue, not an Xft issue. The layout layer can find if the font has
> > a copyright symbol or not with no problem.
> 
> In mozilla the layout code does layout not character transliteration.
> Regardless of where it is done this feature needs to be retained.

I guess there is confusion here. By "layout" I mean the level above
Xft. The thing in Pango's place. You clearly are thinking of the
Mozilla layout engine.

> > > 13) Does Xft/FontConfig support a way for the font developer(s) to debug
> > >     what fonts are being selected by a users system? This is important
> > >     because the fonts installed on users' systems varies widely and it is 
> not
> > >     always possible to reproduce a user's problem on a developer's system.
> >
> > Yes.
> 
> Could you provide details? How does a font maintainer find out what fonts
> are being used on the user's system?

 fc-list lists the full set of fonts
 FC_DEBUG=[number] will provide as much detail as you want on matching

> > > 14) Does Xft/FontConfig support Indic shaping? We have work in progress
> > >     here.
> >
> > Layout issue, not an Xft issue; Pango, for example, has quite complete Indic
> > OpenType support on top of Xft.
>
> This is not a layout issue. Layout should deal with the Unicode character
> values not the glyph numbers. The handling of ligatures in moz belongs at
> the font subsystem layer.

Again, confusion over the term "layout".

> > > 15) Does Xft/FontConfig support embedding Truetype fonts in Postscript
> > >     printing output? We have work in progress here.
> >
> > You have access to the font files and FT_Face objects, I don't see how this
> > is otherwise relevant to Xft/fontconfig. There has been discusison of
> > finding a way to share the code for this between all the different users
> > of fontconfig, but using fontconfig certainly doesn't hinder Mozilla
> > from going its' own way here.
> 
> It does in the sense that the proposal here currently plans to rip out the
> code that the printing work currently uses. This is an important feature and
> we should not lose it.

I'd think with just a bit of cooperation, code that can be shared
could be moved to a common place. 

[...]

> > The reason we need Xft/fontconfig support in Mozilla is simply that it will
> > shortly be  *the* font system on the majority of Linux and Unix systems.
> > (We could argue how big that majority is; I'd say "vast majority"
> > in fact.) On such systems, not using Xft/fontconfig for Mozilla makes
> > no more sense than not using the Win32 font API on Windows.
> 
> Can we agree that "vast majority" is 95%? Or is this too high?
> The question here is "what percentage of users is it okay to cut off?".

Why do you think that adding Xft/fontconfig support is cutting
people off? Because they won't be able to compile Xft/fontconfig
on their system? Because their system will be too slow to
use anything but core X fonts?

> Whatever percentage we agree represents "vast majority" we need to look at
> what percentage of systems in the field today have Xft?
> 
> When will the number of systems with Xft reach 50%, 75%, 90%, 95%?
> 
> What about FontConfig?

Xft2 and fontconfig go together. Right now, the percentage of systems
with Xft2 is miniscule. But every system that runs with XFree86
will get Xft2 when upgraded to XFree86-4.3. Every Red Hat system
will have Xft2 as of the next release. By sometime next year,
more than 95% of new Unix and Linux installs will be Xft2 and
fontconfig based. Pushing Xft2 and fontconfig to older systems
will depend on whether software people want to use requires it.

> I know it sounds easy to add a new library to moz but I don't suppose you
> were around when Darin tried to start using uconv.
> http://bugzilla.mozilla.org/show_bug.cgi?id=129279
> Uconv has been out for years and they still had problems getting it to
> work on all the various versions of Unix.
> 
> Using a new library seems like an easy thing but getting the all the details
> are done is difficult.

> The easiest to get Xft in moz without losing features is to integrate Xft
> into the current code and use the best features of both. Doing it this way
> we get minimal developer impact and we do not lose features.

I'm really not catching why this is the case. As I understand
it, Chris's proposal is to add conditional compilation between:;

 - Current X + freetype code
 - Xft2

With a clear separation between the two code bases, while your
proposal is to add conditional #ifdef support for Xft support
mixed in with the current code?

Since I don't see any reason anyone would want to:

 - Mix core X fonts with Xft
 - Runtime switch betweeen freetype and Xft

The only advantage I see of your approach is that a user could
possibly choose to start up Mozilla using core X fonts,
if their Xft configuration wasn't working, or performance
turned out to be a problem with Xft.

But even if startup-time switching was desired, it seems
to me that separated code bases would be easiest. The advantages
of separated code bases are:

 - If Xft support turns out to be a disaster, it could be dropped
   with minimal disruption.
 - If Xft support works out well, the old font code could
   be removed with minimal distruption.
 - The Xft codebase doesn't need to conform exactly to the 
   structure of the current code or vice versa.
 - Allowing mixed runtime operation simply makes everything
   more complex -- you are switching over the type of font
   all over the place.

Of course, regressions in functionality are a bad thing, and
code that is still useful in Xft codebase should be reused,
but even here, separating things out seems like a good
thing:

 - the Xft codebase won't be blocked until feature parity
   is reached, or won't require broken stubs all over
   the place.
 - It will make it easy to check for parity, instead of wondering
   what backend is rendering some AA font.
 - People who need the more obscure features that might be
   temporarily lost can simply use the old code; vendors,
   whether Netscape, or Red Hat, will be able to switch
   when they feel the Xft code is ready (if that ever happens)
   without worrying about leakage of brokeness.
>  - Runtime switch betweeen freetype and Xft

Actually, my patch allows for this to happen quite easily.  I had planned to
make it possible to choose at runtime via a pref whether or not to use Xft,
assuming that you had built with the code with both enabled (which is also
supported.)
> > There are some bad looking fonts (the URW fonts come to mind). How will it
> > be managed so the novice user does not see these? This is an easy one, I
> > just want to know that it is actually done.
>
> The main URW fonts render quite well here, thank you. (I've fixed
> a bunch of Freetype Bugs that were causing glitches with their
> rendering.) If the novice shouldn't see a font, then I don't
> know why it is in the fontconfig catalog. It doesn't seem to me
> that a blacklist mechanism is necessary.

Its the URW X bitmap fonts that look bad. If you've ever wondered why
mozilla's X font pref includes the foundry its because that is the way
Erik choose to let the user select Adobe X fonts over URW X fonts of the
same name.

> > > > 12) Does Xft/FontConfig support transliteration? Ie: what does
> > Xft/FontConfig
> > > >     do for characters like copyright when no font has a copyright 
symbol?
> > >
> > > Layout issue, not an Xft issue. The layout layer can find if the font has
> > > a copyright symbol or not with no problem.
> >
> > In mozilla the layout code does layout not character transliteration.
> > Regardless of where it is done this feature needs to be retained.
>
> I guess there is confusion here. By "layout" I mean the level above
> Xft. The thing in Pango's place. You clearly are thinking of the
> Mozilla layout engine.

Yes, in the context of adding Xft to mozilla I hope we can agree that the
term "layout" refers to mozilla's layout.

> > > > 15) Does Xft/FontConfig support embedding Truetype fonts in Postscript
> > > >     printing output? We have work in progress here.
> > >
> > > You have access to the font files and FT_Face objects, I don't see how 
this
> > > is otherwise relevant to Xft/fontconfig. There has been discusison of
> > > finding a way to share the code for this between all the different users
> > > of fontconfig, but using fontconfig certainly doesn't hinder Mozilla
> > > from going its' own way here.
> >
> > It does in the sense that the proposal here currently plans to rip out the
> > code that the printing work currently uses. This is an important feature and
> > we should not lose it.
>
> I'd think with just a bit of cooperation, code that can be shared
> could be moved to a common place.

The plan is to put the part that is specific to the direct FreeType2 code
and Xft behind an IDL interface. This way the code does not need to know
where the information came from.

> [...]
>
> > > The reason we need Xft/fontconfig support in Mozilla is simply that it 
will
> > > shortly be  *the* font system on the majority of Linux and Unix systems.
> > > (We could argue how big that majority is; I'd say "vast majority"
> > > in fact.) On such systems, not using Xft/fontconfig for Mozilla makes
> > > no more sense than not using the Win32 font API on Windows.
> >
> > Can we agree that "vast majority" is 95%? Or is this too high?
> > The question here is "what percentage of users is it okay to cut off?".
>
> Why do you think that adding Xft/fontconfig support is cutting
> people off? Because they won't be able to compile Xft/fontconfig
> on their system? Because their system will be too slow to
> use anything but core X fonts?

Its not a developer issue.

The "cut off" occurs if we _drop_ support for core X fonts and the user's
system does not yet have Xft/FontConfig.

> > Whatever percentage we agree represents "vast majority" we need to look at
> > what percentage of systems in the field today have Xft?
> >
> > When will the number of systems with Xft reach 50%, 75%, 90%, 95%?
> >
> > What about FontConfig?
>
> Xft2 and fontconfig go together. Right now, the percentage of systems
> with Xft2 is miniscule. But every system that runs with XFree86
> will get Xft2 when upgraded to XFree86-4.3. Every Red Hat system
> will have Xft2 as of the next release. By sometime next year,
> more than 95% of new Unix and Linux installs will be Xft2 and
> fontconfig based. Pushing Xft2 and fontconfig to older systems
> will depend on whether software people want to use requires it.

Mozilla should continue to run on the older systems and if/when a user
add Xft/FontConfig mozilla should automatically start using it.

This means 2 things:

 1) We need to retain our support for the core X fonts until some high
    percentage of systems have Xft/FontConfig. Once the percentage of
    users with Xft/FontConfig is high the older code will atrophy naturally
    from lack of interest.

 2) all versions of mozilla should have Xft/FontConfig support built in
    and moz should be able to use both at once.

> > The easiest to get Xft in moz without losing features is to integrate Xft
> > into the current code and use the best features of both. Doing it this way
> > we get minimal developer impact and we do not lose features.
>
> I'm really not catching why this is the case. As I understand
> it, Chris's proposal is to add conditional compilation between:;
>
>  - Current X + freetype code
>  - Xft2
>
> With a clear separation between the two code bases, while your
> proposal is to add conditional #ifdef support for Xft support
> mixed in with the current code?

I propose a runtime switch that would work just like the direct FreeType2
code:

  if Xft is there then use it

> Since I don't see any reason anyone would want to:
>
>  - Mix core X fonts with Xft

This is an easy one: not every user has all fonts in all languages
Moz should be able to use all the fonts that are available.

>  - Runtime switch betweeen freetype and Xft

I'm not interested in a runtime switch between the direct FreeType2
code and Xft; if Xft is available there is no reason to use the direct
FreeType2 code. I'm interested in being able to use both the current
core X font code and Xft simultaneously.

> The only advantage I see of your approach is that a user could
> possibly choose to start up Mozilla using core X fonts,
> if their Xft configuration wasn't working,

Yes, moz needs to startup even if Xft is not available/working. Lets not
have multiple executables with the same name but lacking such basic
features.

> or performance turned out to be a problem with Xft.

I think we can all agree that Xft's performance is likely to be
better than the current direct FreeType access code.

> But even if startup-time switching was desired, it seems
> to me that separated code bases would be easiest. The advantages
> of separated code bases are:
>
>  - If Xft support turns out to be a disaster, it could be dropped
>    with minimal disruption.

Its not likely to be a disaster. However, its availability on every system
will be an issue for a long time to come. Lets make a plan where this
does not interfer with getting Xft into mozilla.

>  - If Xft support works out well, the old font code could
>    be removed with minimal distruption.

By allowing the current core X font code to work simultaneously we can put
Xft support in all moz X products now and the issue of how long it takes
for Xft's deployment into the field is not an issue for mozilla. In this
way mozilla continues to "just work". Once Xft is on the vast majority of
users' systems then we can pull out the old code.

>  - The Xft codebase doesn't need to conform exactly to the
>    structure of the current code or vice versa.

Why is this important?

Are there reasons why this would be considered more important than
not disrupting the user?

>  - Allowing mixed runtime operation simply makes everything
>    more complex -- you are switching over the type of font
>    all over the place.

Yes, exactly, moz should be able to use all the font resources on a system
to provide the best text display. An alternate would be if someone setup
a font distribution system where all the commonly needed fonts could
automatically be downloaded. This is one of the ways Microsoft solved their
deployment issue and it would be a great alternative but I do not see this
on the horizon for the X market.

> Of course, regressions in functionality are a bad thing, and
> code that is still useful in Xft codebase should be reused,
> but even here, separating things out seems like a good
> thing:
>
>  - the Xft codebase won't be blocked until feature parity
>    is reached, or won't require broken stubs all over
>    the place.

How/why would Xft need to be blocked if it can work simultaneously with the
core X font code?

>  - It will make it easy to check for parity, instead of wondering
>    what backend is rendering some AA font.

I've always suggested that Xft preferred over the direct FreeType2 code.
However, regardless of what form of FreeType2 based font code moz uses
moz should still be able to access the core X fonts.

>  - People who need the more obscure features that might be
>    temporarily lost can simply use the old code; vendors,
>    whether Netscape, or Red Hat, will be able to switch
>    when they feel the Xft code is ready (if that ever happens)
>    without worrying about leakage of brokeness.

I do not want a "switch" point driven by the _supplier_. As the needed
libraries are available then the feature should begin working in moz.
The current Xft mozilla already suffers from this design; users can
either:

 have Xft in a really old version of mozilla

or

 have a current mozilla and no Xft

We need to get Xft working with the current code and building in all
versions. It needs to run when it can; ie: as Xft/FontConfig are deployed.

OK, I'm not going to quote that whole long message because it's getting out of
hand.  There seem to be a few major stumbling points here:

o Brian wants the ability to load Xft and fontconfig at runtime instead of
linking against it directly.

I don't personally think that this is a great idea, but I'm not passionate about
it.  The simple fact is that the code that would be required to build a big
dispatch table is relatively trivial and shouldn't be an item to prevent this
code from going into the tree.  It's something that can be fixed later.  It's a
couple hundred lines of code, at the very most.

o Simultaneous use of core fonts and fontconfig.

This is the big stumbling block.  I'll come out and say it - I have zero
interest in trying to mix fontconfig's matching results and the current core
matching code.  There will be user impact here, yes, but if they already have
fontconfig on their systems it can be assumed that they already have their fonts
set up properly and they don't need the core infastructure anymore.  Fontconfig
is a replacement for the core fonts, not an additional mechanism for selecting
fonts to mix with core font selection.

I realize that there will be some user impact here, but it shouldn't be too bad.
 It's perfectly acceptable to me and our beta Red Hat users who have made the
switch to entirely fontconfig based systems and they aren't screaming their
heads off yet, so we have to be doing something right.

It's important that Mozilla match exactly the same as all the other applications
on the desktop when it comes to choosing which fonts go with which families and
aliases.  If Mozilla is using an entirely different mechanism for matching,
which is what you are suggesting, this won't be the case and Mozilla will
continue to integrate poorly into the Unix desktop.

o Interfaces for printing

The interfaces that I've seen are all based around using the current matching
algorithms.  The interfaces describe the fonts - sizes, families, etc.  They
assume that you will doing your matching by hand.  At least, that was the last
time I looked.

o Confusion over what mozilla is - supplier?

I see some confusion about not wanting to have to wait for a "supplier" in order
to get new functionality.  I think this shows a pretty fundamental
misunderstanding of what Mozilla is and what our roles are.

Mozilla is not an end user application.  Period.  We do not cater to end users,
we do not make builds with end users in mind.  The only reason that we build
binaries and put them on an ftp server at all is so that QA is easier.  Some of
our QA team is made up of people who might be considered end users, but don't
let that fact confuse you into believing that we are an end user product.  We're
not.

So, your exasperated comment about having to wait for suppliers in order to get
code into the field makes almost no sense whatsoever.  Of course, we want have
to wait for suppliers to get code into the field!  They are the best way to do
so.  What this means is that each of the suppliers (really, we call them mozilla
distributors) gets to make their own decision about when they want to ship what
features.

So, for example, Red Hat will probably be one of the first shipping Xft and Gtk2
because our next release will be all set up to handle it.  We'll be able to have
a completely configured system ready to drop mozilla into.  We have that luxury
and we should be able to do that with the Mozilla code.

Sun, on the other hand might want to ship Gtk2 for accessibility reasons but
they don't have Xrender in their servers and don't ship fontconfig or Xft at
this point (but will have to soon if they want a version of Gtk2 that works in
the future.)  They probably won't ship Xft any time soon.  The prefer core fonts
+ freetype because it's a good match for their system.

Netscape, on the other hand, might want to hold off since they need to work on a
wider variety of hardware because they aren't system integrators - they
distribute end user applications - so compatibility is more important than what
features are included.

So, when you think about who your target audience is it's important that you
understand that there are more people out there than just Netscape.  There are
also people who are ready to make the final switch and have the infastructure in
place to do it.  Eventually, the rest of the world will catch up and we will be
able to remove the core code entirely, but I know that's not going to happen any
time soon which is why I'm creating this careful seperation of the two code bases.

o Do we want to build Xft by default into Mozilla?

The answer to the last question leads to this one.  And at this point, I don't
have an easy answer.  I guess that my immediate answer would be 'no' since as
the patch stands right now you have to link directly.  But eventually, once some
of the build boxes have xft installed and I set up a patch that does dynamic
loading, we can.  I don't mind that at all.  It widens our test coverage and
makes my job easier in the end since I'm going to be supporting this code.

In conclusion, I'll just say that I think that fontconfig and Xft are up to the
task to be both the selection and rendering component for Mozilla.  Red Hat's
experience with our next release has shown this to be true.  And the relatively
minor user impact isn't enough to prevent the code from going into the tree
using the current design IMHO.
Note that in the Red Hat Linux release we're winding up now, and in all future
releases, it's a bug if an app uses both fontconfig fonts and core X fonts; 
we have moved the old PCF fonts we want to use into fonts.conf so fontconfig 
will find them, and if duplicate or extra fonts not found in fonts.conf show up
in the UI then it's a bug. Neither GTK2 nor Qt3 will use core fonts under any
circumstances in our release. If we wanted the old fonts to appear, we would put
them in fonts.conf.

So I would appreciate an easy way to make Mozilla work in this environment, as
it's broken for users if the list of available fonts is different in Mozilla vs. 
other applications. Mozilla needs to display exactly the same set of fonts 
displayed by GTK2 and Qt3 font selectors or it's confusing. This is the purpose
of fontconfig, to get every toolkit on the same font framework.

(On a more editorial and less factual note, I don't think merging the two 
font backends ever makes sense; having both available yes, using both at once, 
no. But even allowing both at once is fine from my point of view as long as
there's a way to set it up so that core X fonts are entirely ignored - not
loaded, no extra overhead from them, not displayed to the user.)
Similarly, the next release of Ximian Desktop will feature GTK 2.0 and Pango 1.1
with the Xft2/fontconfig backend across all of our supported platforms (perhaps
with the exception of Solaris), and it will be the *only* method that all of our
GTK and GNOME-based apps use.  When an Xft-enabled Mozilla lands, we will start
shipping it, and we likely won't even enable the core X font backend in the
build at all (and definitely not at runtime).

From a desktop integration standpoint, it's absolutely paramount that a single
font mechanism exist across the whole desktop, in all applications.  This is the
entire reason why fontconfig exists today.  We're taking a similar approach to
Red Hat in that all of the PCF fonts will be added to the fontconfig catalog, so
I agree with Havoc in that it would make our lives a lot easier if we didn't
have to patch that out in our Mozilla builds.
Joe: If you want apps to stop using core X fonts you could just remove all the 
core X fonts from the X server (excluding "fixed" which is required).
Removing core X fonts from the server isn't right, because you want old 
apps to keep working, while making progress with new apps. Plus Ximian doesn't 
usually replace the X server, only the desktop.
We can always dream.
> o Simultaneous use of core fonts and fontconfig.
> 
> This is the big stumbling block.  I'll come out and say it - I have zero
> interest in trying to mix fontconfig's matching results and the current core
> matching code.

Why would mozilla intentionally want to produce a version that is incompatable
with all previous versions of Redhat?

I can certianly see how it might fit with Redhat's goals but for mozilla it adds
an unwanted burden to mozilla's font development, font maintenace, build system, 
distribution system, and bug tracking system. 

Having 2 versions of mozilla for Redhat that work on some systems but not
others adds a level of user confusion.

The proposed Xft only version will lose features like MathML and AASB (which is 
important for CJK users).

My request is just that we have an easy way for the mozilla that's _included_
with _newer_ versions to use fontconfig/Xft2 only. (And of course the
fontconfig/Xft2 version should plan to support MathML and AASB.)
> Why would mozilla intentionally want to produce a version that is incompatable
> with all previous versions of Redhat?
> 
> I can certianly see how it might fit with Redhat's goals but for mozilla it adds
> an unwanted burden to mozilla's font development, font maintenace, build system, 
> distribution system, and bug tracking system. 

This is exactly the misunderstanding that I was talking about.  Mozilla does not
ship to end users.  Netscape ships to end users.  Red Hat ships to end users. 
So, you need to re-ask the question differently:

Q: "Why would Netscape want to produce a version that is incompatible with all
previous versions of Red Hat?"

A: I don't know, I don't speak for Netscape.  But I can guess.  I would guess
that Netscape probably wouldn't want to ship something that was incompatible
with previous versions.  And I'm not asking them to do anything like that.

ok, the other side of the story:

Q: "Why would Red Hat want to produce a version that is incompatible with all
previous versions of Red Hat?"

A: Easy, we don't have to worry about it because we can take an entire system
view.  We want to ship this as part of our entire platform and we can set the
capibilities for the platform to include Xft and therefore we don't have to
worry about previous versions Red Hat.

So, as you can see, the scenarios that we are arguing for are very different.

There's also the opposite of this question: "Why would Mozilla want to produce a
version that isn't compatible with any new versions of Red Hat?"  It would be an
important question, if Mozilla were shipping to end users.

There's the issue of support and integration, of course.  This isn't without
cost to you, and I realize that.  But I've done as much as I can to seperate the
two code bases so you can work on what you need to and I can work on what I need
to as part of my job.  I'm minimized the impact to you ask much as possible.

What else can I do?  This code isn't going to go away, that's for sure and
you're seriously risking a pretty big fork if you're going to reject it out of hand.

> Having 2 versions of mozilla for Redhat that work on some systems but not
> others adds a level of user confusion.

No, it doesn't because Red Hat ships exactly one version.  In fact, for Red Hat
it's pretty bad because Mozilla has a completely different set of fonts for
everything than all the rest of the desktop.  Not having this code in is causing
more problems than are supposedly caused by including them.

> 
> The proposed Xft only version will lose features like MathML and AASB (which is 
> important for CJK users).
> 

MathML I can fix, even if it's a solution that's explicitly for MathML.  I'm not
concerned about that at all.

AASB I'm not too worried about, either, since we do AA and from a system
standpoint we have a somewhat decent set of fonts.  Not perfect, but certainly
good enough given what it's replacing.  Once again, these things are important
from Netscape's standpoint, for sure, but I'm not Netscape.  Plus, all of us on
the Xft side of the fence would like to see a generalized solution to this in
freetype/Xft instead of a one-off solution for a single application.  I'm 100%
sure that Keith and friends would love to see this integrated into Freetype and
Xft.  You're more than welcome to help.
Chief Lizard Wrangler Mitchell Baker had this to say on the point now being argued:

"Mozilla releases are intended for the development community rather than the
general consumer market. Mozilla.org is not set up for marketing, distribution
or support for the consumer market; we rely on companies building Mozilla
products to do that. So we're not expecting millions of consumers to rush out
and test Mozilla. The end-user applications in Mozilla (browsing, mail, news and
chat) are end-user quality, but we don't have the organization to reach this
market. So we're not expecting millions of consumers to download and review Mozilla.

"That being said, Mozilla does have an end-user community. It includes both
developers who use Mozilla technology for their projects, and people who simply
want the browsing, mail news and chat applications we offer. It's important that
Mozilla 1.0 be useful both to people who want to use the applications and to
people who use Mozilla technology to build products. Good reviews are one
measure of this."

See <http://newsforge.com/newsforge/02/05/10/1356208.shtml>. I'll leave the
interpretation to you, though it seems obvious enough to me.
Mitchell and I are in violent agreement.
Xft and fontconfig 2.0 is now available from: http://nexp.cs.pdx.edu/fontconfig/ .
Fontconfig 2.0 is supposed to be available from http://fontconfig.org, but the
DNS hasn't caught up yet.  The existing hostname (nexp.cs.pdx.edu) will probably
change in the next couple of days, so I was waiting for a more stable name
before making an official announcement.
What does "in violent agreement" mean?

Chris, you say: "Mozilla is not an end user application."

Michelle says: "Mozilla does have an end-user community. It includes ... people 
who simply want the browsing, mail news and chat applications we offer"

Aren't these in conflict?
> Chris, you say: "Mozilla is not an end user application."
> 
> Michelle says: "Mozilla does have an end-user community. It includes ... people 
> who simply want the browsing, mail news and chat applications we offer"
> 
> Aren't these in conflict?

(Mitchell, as a lawyer, would love me for this.)

No.  We don't produce Mozilla to be an end user product.  As a result of our QA
process we just happen to produce something that's usable by end users so we end
up with a community of people who look like end users.
Market share of IE:      96.0% (or something like that)
Market share of Mozilla:  0.4% (or something like that)

Does Mozilla has an end-user community?  Yes.
Is Mozilla an end-user application?  No.

That's one way to explain the agreement...
I donot know IE has such a large community.
Can you tell me where you got this statistic? Or you mean only on Windows  System?
I found most of the user of Unix/Linux are using Netscape/Mozilla.

The links:
    http://news.com.com/2100-1023-955734.html                                       
    http://zdnet.com.com/2100-1104-938865.html                                     
    http://maccentral.macworld.com/news/0112/19.linux.php

However, the actual numbers are not important.  I've been using
Mozilla since M18, and everyone here must also be a long-time Mozilla
user (or even developer).  However, the vast majority of "end users"
still uses IE.  The reason is _not_ that IE is 240 times better than
Mozilla.  It's because IE is bundled with the OS (updating through
Windows Update is also considered "bundled with ths OS", IMO).

And that's where distributions like Red Hat come in.  If/when Red Hat
grabs N% desktop users from MS, "the browser bundled with Red Hat"
simply gets N% market share (and becomes an "end-user application").
>> 9) Does Xft/FontConfig support Korean precomposed Hangul using hanterm fonts?

> Do you mean "composing Jamos on the fly" as opposed to using precposed
> syllables? This is basically a layout level issue, though depending on
> what fonts you are talking about, there might be some connection to
> the custom encoding question.

  Owen, you have this implemented in Pango (perhaps thanks to Changwoo Ryu) :-)
What Brian wrote about is on-the-fly generation of glyphs for Hangul syllables 
(both a full set of modern precomposed syllables in U+AC00 block and a subset of
pre-1933 orthography syllables) using a BDF font with glyphs for multiple sets
of glyphs for Hangul Jamos.  The relevant file in Mozilla source is
http://lxr.mozilla.org/seamonkey/source/intl/uconv/ucvko/nsUnicodeToX11Johab.cpp
As you wrote, there's a connection to the custom encoding (in this case,
'johab-0', 'johabs-0' and 'johabsh-0' : the last two fields of XLFD). 
Although it's nice to have this, the importance of this feature got diminished
with the availablity of iso10646-1 encoded (both as X11 core fonts and TTF)
fonts. (see http://jshin.net/i18n/korean/hunmin.html. the content of
the page is outdated in that it does not yet mention the changes brought about
by James Kass' new CODE2000 font that enabled Mozilla-Windows and
Opera-Windows to render the page more or less legibly.)



> I've never seen a TrueType or postscript
> font for use with combine-on-the-fly Jamos, so it's hard to say if 
> Xft could handle such a font or not. I suspect it could.

  In case of TTF/PS fonts with _custom_ encodings(there are some 
in Korean LaTeX package. MS Office 2000 also has a couple of 
such fonts for pre-1933 orthography syllables. Ogulim.ttf, Obatang.ttf,
Odotum.ttf), I'm not sure if Xft has to deal with it or a higher layer has to 
handle it (e.g. Pango).  

 BTW, I found that Mozilla-Xft (with Chris' patch) has a problem
not present in Mozilla-FT2(directly accessing truetype fonts) when
it comes to rendering pre-1933 Korean text with the newest
version of James Kass' CODE2000 font with glyphs
for Hangul Jamos(U+1100) made in such a way that
simple-minded overstriking them does the magic of forming
many (not all) syllables out of glyphs for Jamos (eventually,
gsub/morph and other OT tables have to be used)

http://linux.mizi.com/~ganadist/gedit.png            (gedit with gtk2 ,
fontconfig rc3)
http://linux.mizi.com/~ganadist/moz.png              (Mozilla-Xft,    fontconfig
rc3)
http://jshin.net/i18n/korean/hunmin_moz_ft2.jpg   (Mozilla-FT2)

U+115F(Hangul Choseong filler) and U+1160(Hangul Jungseong filler) 
in the first and third screenshots  are rendered as non-spacing(and empty) 
as they should be. In the second screenshot,they're apparently
rendered with '.notdef' glyph (hollow box). Because gedit works correctly
with the same version of fontconfig package as used for Mozilla-Xft,
I guess it's XFt patch that needs some fixing in this regards. Perhaps, it should
be a simple fix, but I couldn't figure out how to fix. Anyway, 
it'd be nice to see this taken care of because it's  important for Korean users 
who wish to view pre-1933 Korean text. 


G'd day, if this isn't the right place to ask, please, redirect me to the proper
URL.
Thank you very much in advance.

My goal is to compile Mozilla with XFT support like the RPM package provided by
Blizzard some time ago for Red Hat boxes (I'm using RH 7.2). I have not found any
RPM for the new Mozilla versions, so I have to built it myself. I've installed
Keith 
Packard's fcpackage-2.0 and then used the  following patch 
       http://fontconfig.org/mozilla/patches/mozilla.2002-08-02.diff
with Mozilla 1.1b (Mozilla refuses to apply the patch to a newer version).

So far so good, but when I type: 

             gmake -f client.mk build 

it starts building the Lizard until the following error appears:

../../../dist/include/gfx/nsRenderingContextImpl.h:137: warning:   by 
`nsRenderingContextImpl::DrawImage (imgIContainer *, const nsRect *, const
nsPoint *)'
In file included from /usr/include/gtk-1.2/gdk/gdktypes.h:33,
                 from /usr/include/gtk-1.2/gdk/gdk.h:31,
                 from /usr/include/gtk-1.2/gtk/gtk.h:31,
                 from nsDrawingSurfaceGTK.h:46,
                 from nsRenderingContextGTK.h:56,
                 from nsDeviceContextGTK.h:48,
                 from nsFontMetricsGTK.h:42,
                 from nsFreeType.cpp:45:
/usr/include/glib-1.2/glib.h:1308:23: warning: ISO C does not permit named
variadic macros
/usr/include/glib-1.2/glib.h:1311:25: warning: ISO C does not permit named
variadic macros
/usr/include/glib-1.2/glib.h:1314:26: warning: ISO C does not permit named
variadic macros
/usr/include/glib-1.2/glib.h:1317:25: warning: ISO C does not permit named
variadic macros
In file included from nsDrawingSurfaceGTK.h:56,
                 from nsRenderingContextGTK.h:56,
                 from nsDeviceContextGTK.h:48,
                 from nsFontMetricsGTK.h:42,
                 from nsFreeType.cpp:45:
/usr/X11R6/include/X11/Xft/Xft.h:236: type specifier omitted for 
parameter
/usr/X11R6/include/X11/Xft/Xft.h:236: parse error before `,'
gmake[5]: *** [nsFreeType.o] Error 1
gmake[5]: Saliendo directorio
`/home/chessy/appz/internet/navegadores/mozilla/mozilla-source-1.1a/mozilla/gfx/src/gtk'

Any help would be very much appreciated. 

   Juanan Pereira    (Chessy)
Attached file checkpointing changes (obsolete) — 
Checkpointing changes again.  Catch up with the tip from a long absence.  Need
to re-merge the freetype code and fix some non-latin text issues.

(this is a gzipped patch)
Attachment #94891 - Attachment is obsolete: true
Any hope for a binary build or rpms now?
1) You claim copyright for code that was moved from existing files. This code 
was written by others; eg:

RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp,v

- * The Initial Developer of the Original Code is 
- * Netscape Communications Corporation.
- * Portions created by the Initial Developer are Copyright (C) 1998
- * the Initial Developer. All Rights Reserved.
+ * The Initial Developer of the Original Code Christopher Blizzard
+ * <blizzard@mozilla.org>.  Portions created by the Initial Developer
+ * are Copyright (C) 2001 the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
- *   Pierre Phaneuf <pp@ludusdesign.com> 
- *   Roland Mainz <roland.mainz@informatik.med.uni-giessen.de>
- *   Brian Stell <bstell@ix.netcom.com>
- *   Morten Nilsen <morten@nilsen.com>
- *
  *

These changes are factually incorrect, insulting to the developers who wrote 
this code, and attempt to steal copyright rights that belongs to others.

2) There is a large amount of moving of code from one file to another. By moving 
the code you lose the cvs history. There needs to be a very strong reason to 
justify losing the history.

3) You have renamed files which will make it much harder for others who have 
experience in the code to help fix bugs. Are you planning to take responsibility 
for fixing *all* font bugs? If not, there will need to be a strong reason
to rename files.

4) If you are really going to use FontConfig you probably do not want to use 
FFRE. That is a bit of history that is very related to historical X fonts. You 
probably to put the switch somewhere in the nsRenderingContextGTK.cpp code 
long before it calls into the nsFontMetricsGTK.cpp code.

> 1) You claim copyright for code that was moved from existing files. This code 
> was written by others; eg:
> [...]
> These changes are factually incorrect, insulting to the developers who wrote 
> this code, and attempt to steal copyright rights that belongs to others.

...and are almost certainly a simple error.  But you never know -- that blizzard
has been known to attempt to "steal copyright rights" from so many people in the
past!

You do yourself a disservice with lunacy like this, Brian.
> These changes are factually incorrect, insulting to the developers who wrote 
> this code, and attempt to steal copyright rights that belongs to others.

I meant to re-add the contributors, I just forgot.  Simple oversight there.  As
for the copyright assignment, when I started this patch I started with a
completely fresh nsFontMetricsGTK.cpp.  No code.  That's why the license header
is new - it's my template.  I pulled in bits and pieces from the old code here
and there but the code is almost entirely new.

> 
> 2) There is a large amount of moving of code from one file to another. By moving 
> the code you lose the cvs history. There needs to be a very strong reason to 
> justify losing the history.

It's totally worth it.

> 
> 3) You have renamed files which will make it much harder for others who have 
> experience in the code to help fix bugs. Are you planning to take responsibility 
> for fixing *all* font bugs? If not, there will need to be a strong reason
> to rename files.

Oh, come on.  One thing does not lead to the other.  The code that was in
nsFontMetricsGTK.cpp and nsRenderingContext.cpp are both in
nsFontInstanceCore.cpp.  If anything the code paths are cleaner than they were
before.  I've said it before and I'll say it again.  The code is largely the
same.  Anyone who was able to support the old code should be able to support
this as well, without problems.

> 
> 4) If you are really going to use FontConfig you probably do not want to use 
> FFRE. That is a bit of history that is very related to historical X fonts. You 
> probably to put the switch somewhere in the nsRenderingContextGTK.cpp code 
> long before it calls into the nsFontMetricsGTK.cpp code.

I think that's an artifact of wanting to support old preferences with the new
code.  I'll revisit it.
> I meant to re-add the contributors, I just forgot.  Simple oversight there.

Good, then there will be no issue correcting this.

> > 2) There is a large amount of moving of code from one file to another. By 
> > moving the code you lose the cvs history. There needs to be a very strong 
> > reason to justify losing the history.
> 
> It's totally worth it.

Why? there does not see to be much difference here; you even say:

> The code that was in nsFontMetricsGTK.cpp and nsRenderingContext.cpp 
> are both in nsFontInstanceCore.cpp. ... The code is largely the
> same.

What specifically justifies losing the cvs history?

> If anything the code paths are cleaner than they were before. 

Could you be specific on what "cleaner" means?


If losing the CVS history is really a showstopper, you can easily do some CVS
surgery to preserve it in the new files.
Doh.  my misunderstanding.  I thought that one file was being broken up into
two, not two being merged into one.  So at best you'd lose the history of one
file.  apologies for the spam.
>> > 2) There is a large amount of moving of code from one file to another. By 
>> > moving the code you lose the cvs history. There needs to be a very strong 
>> > reason to justify losing the history.
>> 
>> It's totally worth it.
> 
> Why? there does not see to be much difference here; you even say:
> 
>> The code that was in nsFontMetricsGTK.cpp and nsRenderingContext.cpp 
>> are both in nsFontInstanceCore.cpp. ... The code is largely the
>> same.
> 
> What specifically justifies losing the cvs history?

Adding Xft in a clean way.  Making sure that the rendering context doesn't have
to know any details about the specific types of fonts that are being used to do
the rendering or measurement.  Etc.

> 
>> If anything the code paths are cleaner than they were before. 
> 
> Could you be specific on what "cleaner" means?
> 

For example, sometimes when you draw instead of the code paths looking something
like:

rendering context -> font metrics -> xfont -> rendering context

now with some of this cleanup it looks like:

rendering context -> font metrics -> xfont

There are a couple of other things that I personally think are cleaner where the
code is less incestuous.


Why can't we maintain the Xft fontmetrics code in a seperate source&&header for
now (like nsFontMetricsGTKXft.cpp/nsFontMetricsGTKXft.h) and add Makefile.in
glue to make it switchable at compile time for now (a runtime switch would be
better to maintain "compatibility" to non-Xfree86 Xservers) ?
> Adding Xft in a clean way.  Making sure that the rendering context doesn't 
> have to know any details about the specific types of fonts that are being 
> used to do the rendering or measurement.  Etc.

This sounds like a change in the virtual layer; eg: the API. How exactly does 
this require a change in which file the code lives in?

> For example, sometimes when you draw instead of the code paths looking 
> something like:
> 
> rendering context -> font metrics -> xfont -> rendering context

Would you please point out this/these specific code path(s)?

> There are a couple of other things that I personally think are cleaner where 
> the code is less incestuous.

Would you please point these out?


> Why can't we maintain the Xft fontmetrics code in a seperate source&&header for
> now (like nsFontMetricsGTKXft.cpp/nsFontMetricsGTKXft.h) and add Makefile.in
> glue to make it switchable at compile time for now (a runtime switch would be
> better to maintain "compatibility" to non-Xfree86 Xservers) ?

That's exactly what I'm doing now, more or less.  That's why we have
nsFontInstanceCore and nsFontInstanceXft.  The nsFontMetricsGTK code is just a
big switch.

Plus, all this code should work with non-xfree86 servers.
> This sounds like a change in the virtual layer; eg: the API. How exactly does 
> this require a change in which file the code lives in?

No, there are no api changes here.

>> For example, sometimes when you draw instead of the code paths looking 
>> something like:
>> 
>> rendering context -> font metrics -> xfont -> rendering context
> 
> Would you please point out this/these specific code path(s)?

I'm not going to spend a huge amount of time on this but you can see it if you
follow the usage of nsRenderingContextGTK::my_gdk_draw_text().

> 
>> There are a couple of other things that I personally think are cleaner where 
>> the code is less incestuous.
> 
> Would you please point these out?
> 
> 

Yeah, sure - just architecture.  The rendering context doesn't know anything
about the specific font type or what fonts are being used to render, nor does it
have to walk the list of loaded fonts and do measurement on its own.  Xft
doesn't have to know anything about the core font code, core font code doesn't
have to know anything about xft.  The header files have been seperated out a bit
so that nsFontMetricsGTK.h and the .cpp file don't have include the huge mess
that is nsFontGTK and friends.
Christopher Blizzard wrote:
> Plus, all this code should work with non-xfree86 servers.

Does it use the plain X11 font API then instead or how is the "compatibility
mode" implemented ?
It works more or less like the freetype code does.  Sucks down an XImage,
scribbles, and puts it back.
Christopher Blizzard wrote:
> It works more or less like the freetype code does.  Sucks down an XImage,
> scribbles, and puts it back.

Please correct me if I am wrong:
- This will make font rendering awfull slow - and over remote X connections we
will make Mozilla a pain (I am sure Xterminal/SunRay/LTSP/etc. people will veto
here)
- We loose support for non-TrueType/non-PCF fonts (like F3 or other
vendor-specific fonts)
- There is no "compatibility", this is just a (very slow) emulation mode until
all vendors (except Xfree86) have "upgraded" their Xserver code (which will need
at least 2-5 years from now)
> - This will make font rendering awfull slow - and over remote X connections we
> will make Mozilla a pain (I am sure Xterminal/SunRay/LTSP/etc. people will veto
> here)

When you disable anti-aliasing, Xft draws text without GetImage/PutImage so that
performance returns to a reasonable level.

I've been providing this kind of client-side font support over the wire for
about six years in an X-terminal environment.

> - We loose support for non-TrueType/non-PCF fonts (like F3 or other
> vendor-specific fonts)

Bitstream is building a FreeType plugin to handle many more formats than the
current X servers are able to.  

Commercial fonts are moving towards OpenType and FreeType will be there to
support that while existing X systems again lag far behind.  Client-side font
support means that moving to a new format requires only changes in the FreeType
library; we can roll out a new library without requiring simultaneous rollout of
new X server functionality to match.

Xft provides significantly better forward compatibility than the existing
mechanisms; additional backward compatibilty may require additional code in the
client side font library, but will not require any changes to Xft, Mozilla or
the X server system.

> - There is no "compatibility", this is just a (very slow) emulation mode
> until all vendors (except Xfree86) have "upgraded" their Xserver code 
> (which will need at least 2-5 years from now)

Xft uses techniques for providing client side text that have been prevalent in X
applications for about 15 years.  If you've ever used FrameMaker under X, you've
experienced client-side font support just as Xft provides.  Open source
applications like Ghostscript and Xdvi also use client-side fonts.  Xft provides
a general API for client-side fonts and takes advantage of the Render extension
where possible to accelerate that.

HP is moving to XFree86 and Sun is currently studying how to get Render support
into their servers.  I don't know what other Unix workstation vendors are
planning on doing, but I think XFree86, Sun and HP together represent the vast
majority of current X-based desktops.

Client-side fonts is a lot more than just getting anti-aliasing into
applications.  It provides significantly better i18n support than the core fonts
could ever manage, including BIDI and GSUB/GPOS layout, and will make it
possible to print using precisely the same fonts as appear on the screen.
> Please correct me if I am wrong:
> - This will make font rendering awfull slow 

When I wrote the direct FreeType2 code using XGetImage/XPutImage I checked 
performance on a 266MHz Pentium running Redhat 6.2 and it was okay. The Mozilla 
page load performance numbers showed about a 13% slow down which is significant 
and it did feel a little bit slower. However, it definitely did not feel really 
slow. 
> > This sounds like a change in the virtual layer; eg: the API. How exactly 
> > does this require a change in which file the code lives in?
> 
> No, there are no api changes here.

Okay, then I still do not see an advantage im moving the code to a new file.
The patch is about 16,000 lines long of which about 10,000 lines are mostly
moving the code around.

What do we specifically gain by doing this? Is there a concrete example?
IMO "losing" the CVS history is not a concern. 

The full CVS history is still there, just in a deprecated filename. Standard
practice is to mention the original file's name in the initial commit message so
that this linkage is clear. 

One additional level of indirection, in the relatively unlikely event that
someone wants the obseleted code, is not much of a price to pay for better
functional separation.
> 
> Okay, then I still do not see an advantage im moving the code to a new file.
> The patch is about 16,000 lines long of which about 10,000 lines are mostly
> moving the code around.
> 
> What do we specifically gain by doing this? Is there a concrete example?
> 

I just gave you a set of concrete examples in terms of modularity.

Plus, it basically means that you can build without core support or without xft
support it you want.  Long term, it means the code base(s) are much easier to
support and it's much harder to step on each other's toes.  Why would you not
love that?

I certainly wouldn't have spent weeks working on this patch if I didn't think
there was concrete value involved.  Otherwise, I would have just used the
original patch that Keith and I worked out.
> >> For example, sometimes when you draw instead of the code paths looking 
> >> something like:
> >> 
> >> rendering context -> font metrics -> xfont -> rendering context
> > 
> > Would you please point out this/these specific code path(s)?
> 
> I'm not going to spend a huge amount of time on this but you can see it if you
> follow the usage of nsRenderingContextGTK::my_gdk_draw_text().

First, do you know why this code is there?

Second, would you please show me how/where the call stack could be:

 rendering context -> font metrics -> xfont -> rendering context

I do not see anywhere in nsRenderingContextGTK::my_gdk_draw_text() that 
calls back to the rendering context.


> I just gave you a set of concrete examples in terms of modularity.

How about something in the line of:

  I moved this function or block-of-code from this file to this file
  to achieve ...

The current patch is huge, 16628 lines, of which it looks like at least
10,000 lines are just moving code around with no clear functional change. The 
sheer size of the moves obscures the actual functional changes. If there is 
really some better code reorganization then it should be discussed and done in 
a separate step so that the Xft code changes can be reviewed. Right now the 
functional changes for the Xft code is buried in the code reorganization.

I thought you knew this code really really well? :)

> rendering context -> font metrics -> xfont -> rendering context

nsRenderingContextGTK::DrawString() ->

govels around in the internals of nsFontMetricsGTK, calls
nsFontGTK::DrawString() which in some cases calls:

nsXFontNormal::DrawText8()

which looks like this:

void
nsXFontNormal::DrawText8(GdkDrawable *aDrawable, GdkGC *aGC,
                         PRInt32 aX, PRInt32 aY,
                        const char *aString, PRUint32 aLength)
{
  nsRenderingContextGTK::my_gdk_draw_text(aDrawable, mGdkFont, aGC,
                                          aX, aY, aString, aLength);
}

...and the circle is complete.  Strange, eh?  Especially amusing considering
it's in x11shared and contains gdk specific calls. :)
> 
> How about something in the line of:
> 
>   I moved this function or block-of-code from this file to this file
>   to achieve ...
> 
> The current patch is huge, 16628 lines, of which it looks like at least
> 10,000 lines are just moving code around with no clear functional change. The 
> sheer size of the moves obscures the actual functional changes. If there is 
> really some better code reorganization then it should be discussed and done in 
> a separate step so that the Xft code changes can be reviewed. Right now the 
> functional changes for the Xft code is buried in the code reorganization.

The code that deals with Xft is clearly seperated from the core code.  There's
nothing obscure about it _at all_.  All the code is in a single file, which is
new: nsFontInstanceXft.cpp.  I couldn't possibly make it any easier to review.
I've opened bug 169695 to move nsRenderingContextGTK::my_gdk_draw_text() out
of nsRenderingContextGTK.cpp
the nsFontInstanceXft.cpp/h (1414 lines in the middle of a 16,628 line patch)
must have hooks in other parts. Would the hooks be say 1,000 lines or less?
The would leave 14,000 line of the patch that are not related to the Xft code.

Is there any specific reason why the review/super-review of these 14,000 lines 
must to be part of the Xft review/super-review?

Mixing non-functional changes with functional changes make the patch 
dramatically larger which makes the review/super-review much harder.
Blocks: 169695
No longer blocks: 169695
Depends on: 169695
Uh, excuse me of butting in...(I propably shouldn't be opening my mouth here)
I'm just a bit frustrated with the process of this bug (who isn't?)

A bit of benevolent proactiveness could help perhaps... eg. in Mr Stell's
(latest) questions I'm seeing a proposal for going further. Perhaps that should
have been verbalized already, instead of left to the guesswork of the reader...

So, is it that Mr Stell wants the huge patch split into phases along the lines of
1) rename/move files/contents without changing/adding any functionality
2) add hooks for Xft (if there are such)
3) add Xft (the two "independent" files).

These 3 steps could be done also slightly parallel or smth., and
reasons/justifications of these 3 discussed bit more separately of each other
(not that they are unrelated, but...)

I could also be totally wrong, but that's... well, human :)
Alias: xft
Hi,

I came here to see if I could get a simple question answered, but instead found
one of the most sordid and longest running streak of silly arguments I have seen
in a long time. Too bad to see so much time and energy wasted on what is
essentially a done deal. Xft is what we need, without much delay. Reading this
bug, at one point I actually had tea shooting out of my nose when an "ugly font"
blacklist was proposed. As it stands today, mozilla is (much to my dismay) one
big collection of ugly font display. If I had to choose between 3 more new and
useful features such as gestures/typeahead etc., and good, solid, intuitive, and
pretty looking font support, I'll settle for font support any day. Fonts on
mozilla are nothing to be proud of as-is.

Anyway, as there is little I can do about this, other then vote on the bug and
voice my support for an Xft enabled mozilla, here is my question (yes, I know I
probably ticked of most people likely to help me, but please take pity on me):

I have, sitting on my hard-disk, a shiny new 1.2a tarball, just waiting to be
compiled. I also have, sitting on my harddisk, an Xft library that was assaulted
  with david chester's xft slight hinting hack. I am rather pleased with the
resulting display of this particular configuration, and would be delighted if
the mozilla 1.2a source could be persuaded to make use of these facilities
already present on my system. Is a patch/hack/workaround in existance that will
enable me to do this? If so, where could find one? I appreciate your assistance
in helping me switch from konqueror to mozilla.
To answer your question, I think that you might be able to use keithp's patch
with 1.2a and get away with it.
Hi Chris, thanks! would that be the patch on fontconfig.org (for 2.0), or any of
the patches with this bug? thanks.
I'm talking about the one on fontconfig.org.
This is unfortunately not the case. Keithp's patches old and new are made
against the 1.0 branch (and happen  to work with the 1.1 branch) but the font
spacing calculation code added after 1.1 branched breaks his patch on 1.2 and
trunk. Updating keith's patch to support the new interface is non trivial,
though I think it wouldn't be difficult for someone who is familar with font
code; but I know it's rather beyond me. My attempt at kludging it resulted in a
mozilla that would choke and fail to render non ascii characters. whoops.
Thanks again. I applied it, but gfx/src/gtk/nsFontMetricsGTK.cpp was not too
happy with some of it, and spat hunk 4 back in my face. I'll watch the compile
break, though, and try and work it out from there.

Cheers,

Martijn
Oh well, thanks, Phillip - saved me a few hours there. I am certainly not the
right person to try and fix this. It's a pity we don't have decent xft support
in mozilla after all this time, while the vast majority of applications do this
natively, or with some slight prodding.

Unfortunately, my development skills are not on the same level as most others
here, so I must make myself useful in other ways, such as testing.hence my
desire to run 1.2a - an area where I can contribute. However, since I use my
browser an average of 12 to 14 hours per day, I can state it is my major
productivity application, and as such, I will refuse to use something that looks
plain ugly. The freetype extensions for mozilla just allows me to view ugly
truetype fonts, with many weird artifacts. 

I really wish that this will be sorted out soon, and if there is any area where
i can contribute in helping to make this happen, please let me know.

Cheers,

Martijn
Don't worry, I'll have a patch here soon that will work with the tip.
Chris, Red Hat 8.0 is coming out soon, so many of us want to know: Is this going
to be included in Red Hat's Mozilla RPMs, regardless of how this (endless)
discussion actually ends? 

I certainly hope the answer is yes.
Nope, but if I get a nice working approved patch here, I might do an errata that
includes it down the road since all the infastructure is there and it's quite
possibly our most requested feature for Mozilla.
Attached file checkpointing changes (obsolete) — 
Checkpointing changes.	Fix FP exception if avecharwidth is zero (oops.)
Attachment #99562 - Attachment is obsolete: true
G'd day,

some thing that I don't understand well is that if I want to build Mozilla with
Xft support, supposedly I must download and install first Fontconfig 2.0 from
Keith's web site, then his Mozilla patch, then patch Mozilla source code
(supposedly only Mozilla 1.1, despite we are in 1.2a). Sooooo...my question is,
what must I do with Blizzard's patches presented with this bug??

I'm only a fan of Xft support, with less than a few coding practice, trying
desperatly to build the lizard with a good-looking fonts, sorry for my
inexperience and doubts, but is there anyone that would want to illuminate me?
Is it somewhere a quick & dirty tutorial to build *any* Mozilla branch with Xft
support?

Thank you very much in advance.
Keep up the good work.

        Juanan Pereira (Chessy)
This work is all against the tip at this point.  Sorry, there are no
instructions so far.
Attached file checkpointing changes (obsolete) — 
Fix problems when drawing text with spacing, fix bug with reversed options to
FcFontRenderPrepare() (oops.)  Move nsFontGTK into its own header file so that
the freetype and the Xlib font code can effectively share it.  Re-merge all of
the freetype code back into nsFontInstanceCore.

Core code should be back to where it is normally on the tip.  Only a couple of
things left to clean up on the Xft code.
Attachment #100108 - Attachment is obsolete: true
With the patch from the 21st of September it's not possible to compile with GTK2
toolkit, the patch uses GdkRegionPrivate structs that are not defined in GTK2.

There was a patch kicking around at one point to get xft working with gtk2.  I
wonder where it went...
some gtk2-xft patches can be found from
http://www.istitutocolli.org/prosos/moz_xft/
Attached file checkpointing changes (obsolete) — 
Checkpointing changes.	Unknown glyph rendering and measurement works,
pango-style.  Check for a pref works.  Problems with GetHints() fixed.	Cleaned
up what happens with a glyph is not found and for some reason the minifont can
not be loaded.	Remerge with the tip as well.
Attachment #100126 - Attachment is obsolete: true
Attached file patch (obsolete) — 
Oops.  Helps if I include all of the files.
Attachment #101016 - Attachment is obsolete: true
This is a rework of the old gtk2 patch for kiethp's xft patch so that it
applies to blizzard's xft patch. Well, rather you apply it to mozilla *after*
applying blizzard's xft patch. Assuming the gtk2 impl is correct, it should be
merged into the main patch and then gtk2 stops being left behind.
Attached file checkpointing changes (obsolete) — 
Add gtk2 support (untested, but different than the patch here.)  Fix problem
that owen pointed out with the math for the unknown glyph placement, add
support for enumerating fonts and reading prefs if they are set.  Not a lot
left to do here.
Attachment #101072 - Attachment is obsolete: true
OK.  There are some builds up that are built with the most recent patch here:

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/xft/2002-09-30/

There are two directories there; the RH7 directory contains a build with the
patch included that does not have xft enabled:

./configure  --enable-optimize --disable-debug --disable-tests
--enable-extensions=default,irc --enable-crypto --without-system-nspr

The RH8 directory contains a build built on RH8 with the patch with Xft enabled
and gcc296 so that the usual plugins work.

CC=gcc296 CXX=g++296 ./configure  --enable-optimize --disable-debug
--disable-tests --enable-extensions=default,irc --enable-crypto
--without-system-nspr --disable-elf-dynstr-gc --enable-xft

Please take them out for a spin.  I'm most interested in any functionality that
might be broken in the RH7 build that doesn't have xft included.
Status: NEW → ASSIGNED
Here's a quick description of the way the new code is laid out:

The cast of characters:

nsIFontMetrics, implemented by nsFontMetricsGTK:

  Deals with font what are essentially font groups.  Returns basic metrics for
  that font collection, also handles font loading andm in the current impl,
  matching for languages and characters.

nsIRenderingContext, implemented by nsRenderingContextGTK:

  Deals with the mechanics of rendering onto the screen.  The current
  implementation does a lot of grovelling around inside of nsFontMetricsGTK,
  loading fonts when they are not found and whatnot.  This is also where
  text measurement takes place.

nsIDrawingSurface, impemented by nsDrawingSurfaceGTK:

  Essentially the only thing that we care about is that the clip region is
  stored here.

These are the toplevel interfaces that are used by external consumers.  There
are no changes to these interfaces, just how the internal implementations
interact a little bit.

Here's a diagram that describes how all the major bits interact:

        |------ nsIRenderingContext::SetFont() ------>|
--------------------            -----------------------
| nsFontMetricsGTK |            |nsRenderingContextGTK|
--------------------            -----------------------
         |     |<----- nsFontHandle (interface) ------|
         |
         |
         V
   nsFontInstance (interface) ----------------------
      |                                            |
      |                                            |
      |                                            |
      |                                            |
      V                                            |
  nsFontInstanceCore -------------------           |
   |                 |                 |           |
   |                 |                 |           |
   V                 V                 V           |
  Xlib            FreeType         XGetImage/      |
   |                 |             XPutImage       |
   |                 |                 |           |
 .pcf fonts        TT fonts            |           |
 (many encodings)  and others          V           |
                                    Xserver        |
                                                   |
                                            nsFontInstanceXft
                                                   |
                                   -------------- Xft ------------
                                   |                 |           |
                                   V                 V           V
                                FreeType           XRender      XGetImage/
                                  |                   |         XPutImage
                                  |                   |           |
                                  V                   V           V
                             ISO10646 .pcf fonts   Xserver      Xserver
                             TT fonts and others

That's a pretty good start.  Here's the highlights for reviewers:

o nsFontMetricsGTK is pretty scaled down.  That is, it contains some elements
that are common to the two font instance classes, like the family of fonts that
were part of the original font query, the language group, pixel size, and that's
about it.  It's pretty much been turned into a dispatch tree to send requests to
the right font instance.  A traffic cop, if you will.

It also contains the implementation of the nsFontHandle class.  This class is
what is handed to the nsRenderingContextGTK class via its users.  It hides the
internals of the font metrics class from the rendering context.  If we didn't
have this, we would have to teach the rendering context about the font instances
which is messy.

It dispatches calls (like draw, measure, etc) to the individual font instance
classes.

o nsFontGtk has been moved to its own header since it's shared in between the
freetype code and the Xlib code.  This is done so that we don't need to teach
everyone about the nsFontInstanceCore code, just to get the structure of the
nsFontGtk class.

o All of the code that was in nsFontMetricsGTK has been moved wholesale to
nsFontInstanceCore.  There were no real code changes here, except to clean up
indendation and to declare functions that are actually static, static.  Also,
the order of the functions has been cleaned up so that they follow declaration
order.  Other than that, there aren't any real functional changes.  I'm pretty
sure that most of the function names are even the same.  Because of this, there
isn't much to say here about the structure of this since it's largely the same.

o nsFontInstanceXft is pretty new code, partially taken from the code that Keith
and I sat down and wrote together many months ago.  It's not 100% yet - there
are a couple of things that I would like to add - but it's certainly good enough
for day-to-day usage.  I think the biggest things that are missing are that
GetTextDimensions()(s) and GetBoundingMetrics()(s) aren't impelemeted yet.  I'll
get to those soon enough.

I'd like to see some reviews here.  I think that the architecture is pretty
sound and acheives the goals set out earlier in this bug and is non-disruptive
as I can be to the current code.
I just downloaded the rh 8.0 build you linked to in comment #163.  It looks
great, but I've noticed 1 problem:  long urls overflow the urlbar.  so do
ctrl-shift-l, and type in a pretty long url, and hit enter.  notice that the url
overwrites the stuff to the right of the url bar.

i'm actually running redhat (null) fully updated using up2date, but I bet this
isn't null's problem.
I had some difficulty using the builds mentioned in <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=126919#c163">Comment 163</a>

I'm using debian/unstable, which lacks GLIBC2.3, so the RH8 version doesn't work.

The RH7 version segfaults(though TestGtkEmbed works).

I've been using the 1.0 Xft build and loving it.

Don't know if you want to bother supporting other platforms with these
experimental builds, but I thought you might be interested.
Attached file patch (obsolete) — 
Fix crash on startup with just xlib, support GDK_USE_XFT, fix a memory leak or
two.  Should be good to go here, folks.
Attachment #101086 - Attachment is obsolete: true
OK.  Ready for reviews.
Comment on attachment 101375 [details]
patch

This patch, attachment 101375 [details], is exteremly large, 17,432 lines and mixes new 
features, function changes, and general code reorganization.

Please open a separate bug so we can address the code reorganization
separately.

The current patch has too many different issues all muddled together.
Attachment #101375 - Flags: needs-work+
Attached file patch (obsolete) — 
Actually make clip regions work in Xft again (duh, bad ifdef name.)
Attachment #101375 - Attachment is obsolete: true
I can check in the unbuilt parts instead.  That would make it really easy to review.
Comment on attachment 101405 [details]
patch

The class design in http://bugzilla.mozilla.org/show_bug.cgi?id=126919#c164
needs work.

For example the direct FreeType2 code is not related to the core X fonts
and should not be subclassed under them or used by them.

The XGetImage/XPutImage code is a rendering method not a font. It is used
by the AASB code and the Direct FreeType2 code and should not be directly
called by a virtual font layer.

There has been a long standing request to support 3rd party font
renderers. Any major rewrite should at the bare minimum define the API
that would support this.
Attachment #101405 - Flags: needs-work+
The patch appears to make the decision at to use/not-use Xft at configure
time. If this is correct then this means that trying to run a Xft enabled build 
on a system without Xft will fail to even start and complain that Xft is 
missing. This needs to be changed otherwise we will have a great deal of user 
confusion as people find some versions of Mozilla fail to start up on some 
systems.

Any reviewer should come from the list of developers who regularily work on
the font code:

The primary active developers are:

  shanjian@netscape.com
  bstell@ix.netcom.com
  roland.mainz@informatik.med.uni-giessen.de

Developers who work on the font code on a semi regular basis are:

  katakai@japan.sun.com
  rbs@maths.uq.edu.au
  smontagu@netscape.com
  prabhat.hegde@sun.com
  pete.zha@sun.com
  Louie.Zhao@sun.com

There needs to be a plan to address Xft's deployment especially with regard
to bug fixes.

Xft is a relatively new piece of code and as is common for all new code there
will be bugs. Lets assume that when bugs in Xft are identified they are
rapidly fixed. The issue is how will users get the fixed version on their
systems?

The current model seems to be that the bug will be fixed in the Xft cvs
repository. At some later point the OSes that ship Xft will deploy the new
version. At some point after that users will get and install a new version 
from the vendor or upgrade their OS. This is very likely to introduce months
of delays to getting a fix out.

Another way would be to tell users to pull the source from the Xft cvs 
repository, build it, and install it. This seems like an unacceptable burden 
for a non-technical user.

Another way would be to have code in Xft checked for a new version and
then automatically downloaded and installed the new version. This is a
workable solution but I do not know where the infrastructure will come from.

Another way would be to put a Xft into the Mozilla cvs repository and distribute
it with Mozilla. This will put a burden on the build and distribution systems
and will make the Mozilla release even bigger. However, assuming we could get
people in Mozilla and Netscape to approve this it would be a way to address
the deployment issue and would immediately make Xft/FontConfig available on
all the Linux/Unix platforms that Mozilla ships on.

>The patch appears to make the decision at to use/not-use Xft at configure
>time. If this is correct then this means that trying to run a Xft enabled build 
>on a system without Xft will fail to even start and complain that Xft is 
>missing. This needs to be changed otherwise we will have a great deal of user 
>confusion as people find some versions of Mozilla fail to start up on some 
>systems.

But which is really worse? There is the other side of the coin as well:

If Xft is "silently disabled" when appropriate libraries are not found, it is
very well possible that many hours are spent on "why my mozilla does not look
the same as the other guys?".

As I understand it, the goal, at the moment, is not to make Xft be built on
default, but just to get it into the tree, so that further development can take
place. Thus, if Xft-enabled versions are built, they are supposed to only work
on the certain environment it was built for (eg. RedHat8), not for "all
environments" (eg. RH7 et al.). The "default build" situation may change in the
future, but in my opinion now is not the time for that.

Isn't the GTK2 in similar situation to Xft? That a mozilla build for GTK2 does
not work in GTK1.x or QT or MacOSX and thus fails to even start ?
I would handle XFT the same way it's handled for GTK. (?)

I wish being told to download CVS was unnaceptable, but as a Linux user, I can
say that Linux just isn't at that point yet.  I have had to pull CVS for
everything from X windows (DVI flat panel support in Nvidia driver) to GTK+2
(xinerama).

What happened to the whole "Mozilla isn't an end user product" thing anyhow? 
Distribution is a problem for distributors (RedHat).

> For example the direct FreeType2 code is not related to the core X fonts
> and should not be subclassed under them or used by them.

They are blitted to the screen using the core X protocols.  I could have called
the class "Legacy" or "Old" or "Obsolete" or something else, but I was trying
not to offend.  I don't think the name matters all that much.

The way that the Core X fonts and the freetype code currently work is so closely
linked, it's silly.  There's no way that I'm going to touch that at this time
since it's not a problem that I'm trying to solve.

> 
> The XGetImage/XPutImage code is a rendering method not a font. It is used
> by the AASB code and the Direct FreeType2 code and should not be directly
> called by a virtual font layer.

Great, when we add a virtual font layer, we'll worry about it.

> 
> There has been a long standing request to support 3rd party font
> renderers. Any major rewrite should at the bare minimum define the API
> that would support this.
> 

Problem is, I'm not just adding a renderer here.  I'm adding a complete drop in
replacement for the font querying, loading and rendering.  This requires hooking
in at a higher level than the code that's there now - that's what all this code
is all about.

If you want to drop in another type of renderer into the Core code, that's fine.
 Xft already supports all the renderers in production right now, so I don't need
to worry about adding anything like that at the moment to the Xft code.

As is the case with refactoring, we do it in small steps.  I've refactored what
I need in order to drop in a new querying and rendering system and not much
else.  I'm trying my best to avoid breakage in other people's code as best I
can, as I should be.

> The patch appears to make the decision at to use/not-use Xft at configure
> time. If this is correct then this means that trying to run a Xft enabled build 
> on a system without Xft will fail to even start and complain that Xft is 
> missing. This needs to be changed otherwise we will have a great deal of user 
> confusion as people find some versions of Mozilla fail to start up on some 
> systems.
> 

Yep.  I haven't written the vtable code yet because the current code that's in
there is probably going to change a little bit, but not much.  Once it's stable
and well tested, I'll add the vtable stuff.  I would like to have that in there
very much.  But, as I've said, I'm taking baby steps here.  One thing at a time.
>There needs to be a plan to address Xft's deployment especially with regard
>to bug fixes.

While a plan wouldn't hurt, the idea of the Xft is (as I understand it) to be a
OS-wide font system. As such, it's not anymore "just a library", but a library
that many parts of the OS use, not just mozilla. It sure gives a warm,
comfortable feeling if everything is controlled "in-house", but especially in
open-source world, there definately is a limit to it.

In my world view, I put Xft along with glibc, libstd etc. that mozilla also
depends on, but does not ship "in-house". Sure, it's new piece of code, but
since its the main element in GNOME2 of RedHat8 (and other future distros), I'm
sure it's not just mozilla that suffers bugs, if (when) such are found. And then
(eg. RedHat) probably issues an update, as with other similar things.

It is always possible to add a line to release notes that "due to bugs in xxxx a
 Xft-library version xxxx is required." and then provide a link to that library
(even custom-built, if necessary).

There are of course exceptions, like freetype, which I'd rather not have
'in-house', but I don't know the details...
> 
> Xft is a relatively new piece of code and as is common for all new code there
> will be bugs. Lets assume that when bugs in Xft are identified they are
> rapidly fixed. The issue is how will users get the fixed version on their
> systems?

I'm amazed that you didn't use this logic to try to write your own freetype font
loader.  :)

Seriously, I don't think this is a problem that we need to figure out in the
short term.  Right now there are so few systems that include real fontconfig and
Xft support (although Red Hat's flooded OC3s are an indication of how many
people are about to have support, not to mention the dozens of mirrors.)  I'm
perfect happy adding a vtable system the fact that will allow it to be loaded at
runtime without linking hard to it.

Of course, having it installed properly and having it _configured_ properly are
two seperate issues.

Anyway, this isn't something that I think we need to worry about for the time being.
bstell:
it's full disclosure time:  do you have some interest in preventing linux from
having a uniform nice looking desktop?  do you have some interest in some other
competing font system?  do you have some personal problem with the developers of
this font system?  is there anything else we should know that would prevent you
from reviewing this code on its technical merits alone?

I only ask because it seems like there might be some non-technical issues
interfering with getting this code into the mozilla tree, and I really like the
effects of this code.

BTW, when you listed the people you were going to allow to review this code,
were you subverting the mozilla review process or implying that blizzard was
going to do so?  shouldn't a reviewer either know he is qualified to review font
code or know that he isn't, without you telling him one way or another?
Bstell: I think that maybe you should remove yourself from this bug. 
You obviously have issues beyond the technical merits of this patch. 
It is very obvious that you take this patch personally. For what reasons
I don't know. But your comments accomplish nothing but wasting
everyones time. You are making a fool out of yourself as everyone can see.
Please save yourself from looking even more foolish and stop commenting
on this patch. Your suggestions for this patch are ridiculas, we better not
make mozilla depend on glibc because what if there is a bug in glibc and
people download mozilla and it doesn't run. Lets keep extending this
to the whole linux kernel. You are only hurting yourself.
ash and John M.: That kind of angry fingerpointing gets nowhere.

This isn't a black&white issue, there isn't one "right" solution. Several are
better, some other less so. The idea is to discuss about the merits of different
approaches and then agree on the one that _feels_ like the right one, at the
moment. Blizzard has written a huge amount of code, it's no wonder it is hard to
understand, especially since it mixes changes to the current code and adds new
things.

As for emotions, I'd be surprised, if there weren't any. After all, this is
open-source, in which most people (as I understand) work because of a motivation
other than money (or at least money is not the main/only motivator). Also, the
code is often one's own work and thus something to be proud of, or attached to.

The bottom line is just to try to get along with many very different people, in
co-operation for a common goal. The difficult part is that not all are able to
put as much effort as each wants. Since this work is "voluntary", it is not
possible to transfer "wishes" for others to work on, unless that is felt
reasonable or is also according to the others "wishes".

Oh, and there are no stupid questions.
Attached file patch (obsolete) — 
Fix memory leak in the xft code.  No config changes here since they are checked
into the tree, now.
Attachment #101405 - Attachment is obsolete: true
You should be able to pick up builds from here:

http://komodo.mozilla.org/pub/mozilla/nightly/experimental/xft/2002-10-03/

There's a vanilla build there, for reference, a build on rh7 with the patch
without xft, for testing and an rh8 build with xft and xlib in it.  Please take
them out for a spin.
> As I understand it, the goal, at the moment, is not to make Xft be
> built on default, but just to get it into the tree

If requires a explicitly set config setting to enable this then
we can postpone the requirement until the time when this changes.

> I wish being told to download CVS was unacceptable

Is any serious gecko embedder going to want to use a library if they
have to tell their users to use cvs/build/install when the users
has a problem?


> The way that the Core X fonts and the freetype code currently work is
> so closely linked, it's silly.  There's no way that I'm going to
> touch that at this time since it's not a problem that I'm trying to
> solve.

About 15,000+ lines are unnecessarily changing code in this are.
Please remove this from the patch.

> In my world view, I put Xft along with glibc, libstd etc. that
> mozilla also depends on, but does not ship "in-house".

The significant difference is these have had plenty of time to work
their most serious bug out.

> I'm amazed that you didn't use this logic to try to write your own
> freetype font loader.  :)

Its the other way around. Although I dearly wanted to I did not add
FreeType to mozilla 4 years ago because it was not ready.

> it's full disclosure time:
> do you have some interest in preventing linux from having a uniform
> nice looking desktop?

No, of course not. What I do care about is having a product that works
well for all users including international users. Everyone wants better
text support in the Unix/Linux world. However, making a tradeoff that
make western users happier and international users unhappier is not
attractive. There has and continues to be a long discussion where in I
ask that international features not be regressed and the reply is that
they are not important. I know that most western users do not feel that
international feature are important but I do.

> do you have some interest in some other competing font system?

I have interest in many things.

Xft is interesting but is just now developing.

Sun's STSF is interesting and seems to have better international text
support and has printing support.

ICU's layout library seems to have the most extensive support for
complex text languages (CTL) such as Indic, Thai, Vietnamese, Arabic,
etc., but I am still evaluating this and it only appears to provide a
part of the needs for international text support.

> do you have some personal problem with the developers of this font
> system?

No, Keith is a very smart person but I often find I have the same
problem that Jim Gettys complained of: getting Keith to do some
things, even if they are relatively simple, can be extremely
difficult and slow. It makes me reticent to use a library that has
not had time to mature and where there has been difficulty getting
issues addressed.

Chris and I agree in some areas and disagree in others. The big issue
for this bug is 1st there has been no evaluation of readyness of Xft,
2nd there has been no evaluation of other solutions, 3rd this solution
being proposed is basically a Redhat only solution, 4th there is no
discussion of how to address the fact that getting fixes out will slower
and more difficult, 5th the patches affect a lot of code unnecessarily,
6th international requirements are being brushed aside.

> is there anything else we should know that would prevent you from
> reviewing this code on its technical merits alone?

no

> ... we better not make mozilla depend on glibc because what if there
> is a bug in glibc and people download mozilla and it doesn't run.

A significant difference being that glibc is fairly mature.

Its been a long time but my recollection is that switching to glibc (or
was it glib?) took a very long time and required multiple build 
parallel builds. It was a mess for users trying to figure out which one
they needed to use.


Comment on attachment 101500 [details]
patch

This patch is 17,222 lines long of which is appears that about 15,000 
lines are not directly necessary.
Attachment #101500 - Flags: needs-work+
Comment on attachment 101500 [details]
patch

This patch is 17,222 lines long of which it appears that about 15,000 
lines are not directly necessary.
<q class=offtopic>we switched from nmake to gmake, but that's a build tool not a
runtime issue. and yes it took a very long time. In fact it's not done, because
the 1.0 branch happened before the landing so 1.0 still uses nmake.  It's
actually likely that until the 1.0 branch is retired (and it's a long lived
branch, so that won't be for a while) that nmake will still be around.

the mozilla runtime doesn't use glib. part of the build system is xpidl, a
standalone tool like gmake, and it uses glib. its glib requirement is actually
annoying for some of us who build on strange ports.
> If requires a explicitly set config setting to enable this then
> we can postpone the requirement until the time when this changes.
> 

Yes, --enable-xft.

> There has and continues to be a long discussion where in I
> ask that international features not be regressed and the reply is that
> they are not important. I know that most western users do not feel that
> international feature are important but I do.

They aren't being regressed.  I am not removing the X core code.  Period.  It
still exists.  People that have specific need of it can still use it.  However,
if they already have options as part of an Xft-based desktop, then they can use
the xft code.  Simple as that.  If they had problems with Xft then they wouldn't
be using it in the first place.  They can always disable it with a pref or an
environmental variable.

Second, it's all new code.  You can't have regressions in code that doesn't even
exist yet.  What a bunch of hooey.

Third, pango does all of the language layouts that you are talking about using
Xft and fontconfig.  So I know it works.  It just might take a little more
coding, that's all.  This is certainly not a reason to keep this first pass at
the code out of the tree and certianly not a reason to keep the infastructure
out that will allow you choose between xft and the core code that's in the tree
right now.

> Xft is interesting but is just now developing.

Xft has been shipping in Red Hat since the early 7.x days.  The rendering bits
are in pretty good shape.

fontconfig, which you say is still very immature, is being shipped in the
thousands (tens or hundreds if you want to count,) so it's good enough for
people to use right now.  In terms of the quality of what we've shipped in the
past, this is the best so far - especially in terms if i18n support, text
rendering, performance and stability.  It works, basically.

> Chris and I agree in some areas and disagree in others. The big issue
> for this bug is 1st there has been no evaluation of readyness of Xft,

It's pretty good - good enough for sure.

> 2nd there has been no evaluation of other solutions,

There are no other solutions in production.

> 3rd this solution
> being proposed is basically a Redhat only solution,

Chicken and egg.  Red Hat has always shipped things before other people.  This
will not remain this way for very long.  Others will be shipping this, and soon.
 I'm sure the Ximian folks will pop up and say that they are shipping this as well.

> 4th there is no
> discussion of how to address the fact that getting fixes out will slower
> and more difficult

I'm not sure who has to get the fixes out but I imagine that nothing will change
in this area.  The distributors (including Red Hat and Netscape) can make their
own support decisions.

> 5th the patches affect a lot of code unnecessarily,

Incorrect.  The infastructure has to change in order to get this code cleanly
into the tree.

> 6th international requirements are being brushed aside.

No, they aren't.  This is new code and I _do_ care, but I'm trying to solve one
problem at a time.  This code doesn't do everything that the current X code does
and it might not, unless things in freetype change, but it doesn't mean that I
don't have an interest in making sure they are supported, and well.

Basically, if this code doesn't work for you, don't freaking use it.  This is
open source and I've been pretty damn careful to make sure the impact on the
current code (besides moving it) is minimal.  Please don't stand in the way of
other people using it and improving it for their situtation.  It's just being
obstructionist.
> The big issue for this bug is 1st there has been no evaluation of readyness of
> Xft, 2nd there has been no evaluation of other solutions, 3rd this solution
> being proposed is basically a Redhat only solution, 4th there is no
> discussion of how to address the fact that getting fixes out will slower
> and more difficult, 5th the patches affect a lot of code unnecessarily,
> 6th international requirements are being brushed aside.

Regarding issues #1 and #2: GNOME/GTK and KDE/Qt have chosen Xft, therefore Xft
IS the future of X fonts for the vast majority of Unix desktops, whether you or
I like it or not and regardless of what Mozilla does. Therefore if Mozilla is to
buck the trend and not use Xft on GNOME2 or KDE3 systems, the burden of proof is
on you to explain why we must not use it.

Regarding issue #3: That is simply false. Find me a major Linux vendor who isn't
planning to base their desktop on GNOME2 or KDE3.

Regarding issue #4: The vendors who distribute Xft are responsible for
distributing Xft updates. It is not our problem. The comparison with glibc is
perfectly accurate in this respect.

Regarding issue #6: I18n requirements are secondary to getting actual working
code into the tree where people can test and hack on it. I doubt we'll ever make
Xft mandatory, but if we ever do, then I promise you it won't happen until it
meets all the i18n requirements.

Maybe there's something to issue #5, I'll have to read the code for myself.
> No, of course not. What I do care about is having a product that works
> well for all users including international users. Everyone wants better
> text support in the Unix/Linux world. However, making a tradeoff that
> make western users happier and international users unhappier is not
> attractive. There has and continues to be a long discussion where in I
> ask that international features not be regressed and the reply is that
> they are not important. I know that most western users do not feel that
> international feature are important but I do.

I think this is a completely unfair characterization of the discussion. First,
no one is talking about regressing *anything* but about adding an alternative
renderer that will be *disabled* by default but which many users or distributors
may chose to enable if it is appropriate for them. It seems that initially, this
renderer may not be appropriate for people using certain Asian fonts; then again
it will also not be an appropriate choice for people using old X servers and
libraries. But for all these people, the existing, default code will continue to
work fine. Their experience will not regress. Meanwhile, in response to your
repeatedly voiced concerns about these issues, no one claimed they are not
important, but instead the patch developers offered a step by step case for how
all of the suggested features could be incorporated fairly easily into the Xft
framework. The question is only, must all these changes be made before the patch
can progress?

As for tradeoffs, the fact is that for many users, these patches do indeed
already represent an excellent tradeoff; judging from the comments here,
blizzard's binary builds are fantastically popular. This is the ultimate proof
of the real utility of these patches for many people, and this is why people are
frustrated that you seem to be attempting to block their progress on largely
procedural grounds. Moreover, many features to Xft have been added seemingly in
direct response to your objections dating all the way back to the time that your
freetype rendering patches were under discussion in other bugzilla bugs (these
included removing XRender dependence, adding glyph substitution, etc.) and it is
hard to escape the conclusion that you just keep moving the goalposts. Indeed,
most of your suggestions have been helpful and several have already moved the
project forward, but at this point it's hard to see why further ones can't be
addressed after the patch is committed.

Finally, I strongly resent you characterizing this as a "redhat-only solution,"
simply because redhat are the first to ship the Xft infrastructure (which will,
in any case, be part of the next release of XFree86 for everybody). Just in
these comment pages, we have seen Ximian voice their intention to support the
patches on all of their platforms, we have seen slackware binary packages and
debian users and on and on.
For what it's worth, I just downloaded the xft-enabled nightly Chris just posted
a link to, and the visual difference (from anti-aliasing, I presume) is
stunning.  Looking at the standard build is now too painful to bear.

While I'm not using GNOME or KDE on my desktop, legions of RH8.0 users are (or
will soon be) using those.  They default to antialiased fonts with Xft support
built in, so mozilla without that support will stick out like a sore thumb. 
Getting some option in there which can fix this will deflect a lot of criticism. 

PS - having simple font names in the preferences font chooser (instead of the
complete font spec) is a very nice side effect, too.

It is truly unfortunate to read this page, and to see this stuff going on. 
Mozilla looks like ****, and it has *always* looked like ****. This is sad. 
Mozilla is, in every respect, a superiour browser and platform to anything 
else out there, bar none. However, looking at jagged fonts, or the half-baked 
attempt of a FreeType-only rendering solutions is simply painful on the eyes. 
 
Not to say that Brian Stell does not raise some valid concerns, he certainly 
does in some cases. However, I don't think I am alone in thinking that there 
are other, more personal issues at play here. The work that Keith and Chris 
have shown so-far is simply excellent. To raise the issue that "Keith does not 
respond fast enough" is simply not on. Is he on Netscape's payroll or 
something? Didn't think so.  
 
A big deal is made of the fact that code is moved between files, and that 
infrastructure is added. If this were a core, showstopping issue, without 
which mozilla would not start, yes, then a very conservative approach to 
change management would be prudent. But it seems that this is optional code, 
to be enabled either at compile time by someone who can actually get far 
enough to go ahead an compile mozilla, and thus can be expected to know what 
is going on, or it is enabled by a distributor, who hopefully has the skill 
and know-how to make sure it all works nicely. Not to mention that they would 
likely have some sort of support process in place. 
 
The obstructionst attitude shown in this issue is unprofessional, and 
unbecoming to the the community as whole.  
 
Brian, the code Chris has produced seems to work, at least it seems to do so 
for me and many others following this issue. This is open source. As far as I 
can tell, that leaves you with 3 options: 
 
- Rapidly code something that meets or exceeds the functionality that Chris 
was so kind to give as a gift for all our benefit; 
- Come to the conclusion that the work Chris has provided works, and is more 
or less good to go. The shuffeling of code seems to be your biggest issue - is 
it *really* all that probelematic as you make it out to be? What current 
functionality did Chris *break* with his code? 
- Try and prioritise the issues you see with the code, in order of 
"showstopper", "critical", "important", and "nice to have", and try to work 
with Chris to come to some sort of common ground. 
 
Whichever way, today there is working code providing real funcionality that 
many people want to have. Try and put your ego aside for 10 minutes, and 
really rationalise this issue. What broke? What will break? How is this *bad* 
for Mozilla? What is the worst-case scenario, and is that worst-case scenario 
as bad as the current horrible look and feel that make this product so truly 
unpleasant an experience to use on *nix? 
I have installed and am sucessfully using the XFT ebabled build under Redhat 8
Psyche.

I LOVE it!!!  Mozilla doesn't look like it fell out of the ugly tree any more!
Finally!

And as a fantastic side effect it gets rid of bug 136362 for me, which locked up
my browser almost hourly until now!

Thanks so much!

I did a bit of testing of the build blizzard provided (comment 185), and I also
applied his patch to my main development tree and have thus been using it (with
xft) for most of today.  I also did a build (separate objtree) with the default
xft-related build options.

In my own xft build, I disabled (after hacking configure.in to make it a proper
build option, basically by using what was in attachment 101375 [details], except with the
corexfonts vs. core-xfonts typo fixed -- I'd like to see this checked in,
although it was cls who requested the change in the other direction) the core
xfonts code to make sure that what I was seeing was actually Xft being used.  I
am seeing it, and it looks quite good.  I like the antialiasing, and I did hit a
bunch of Chinese and Hebrew sites to see that they displayed OK.  (I admit I'm
not an expert, but the sites are certainly readable.)  That said, I have my font
configuration pointed at the TT fonts on my Windows partition (via directories
in /usr/share/fonts/ with symlinks, and running of fc-cache), so I probably have
some additional fonts that I'm using in addition to the ones that ship with RH8.

While testing the default-options build, I did notice one regression that needs
to be fixed before this lands.  The intrinsic sizing of text inputs is broken. 
This is very visible on http://bugzilla.mozilla.org/query.cgi .  I think inputs
are one of the few users of one of the more obscure font API methods (I suspect
either GetAveCharWidth or GetMaxAdvance), which probably isn't hooked up quite
right to the old code in nsFontInstanceCore code or related code.  This needs to
be fixed before the patch lands.

A problem that I'm seeing with the xft build is that there are certain cases
where fonts don't resize correctly (and are displayed at a very small size). 
This happens, for example, on the pages:
  http://www.people.fas.harvard.edu/~dbaron/css/test/sec1803  (see test
    item that uses a larger font)
  http://www.0xdeadbeef.com/html/2002/09/ (use text zoom)
The line spacing changes, but the glyphs stay very small.  I think this bug
would need to be fixed before an Xft-enabled build would be usable for dogfood,
but not necessarily before the Xft code lands disabled.

I'm seeing roughly the same size small glyphs in the fonts that are derived from
system fonts, and thus that are being used for Menus, etc.  I suspect this is
the same problem as the one mentioned in the previous paragraph.

Another more subtle issue that may (although may not) need to be addressed at
some poin in the future is that font family prefs don't persist well between the
old code and the Xft code since the font names have changed.  This may just a
cost we pay, although it could end up being a problem for users who have an Xft
build in their distro but download non-Xft nightlies from mozilla.org.

One other issue with prefs is that the defaults for the prefs aren't very
sensibly initialized -- I think they were in the core code.  This is also
something that can be address with later changes.
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attachment #101017 - Attachment is obsolete: true
Attachment #101500 - Attachment is obsolete: true
Attachment #101578 - Attachment is obsolete: true
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Attached file patch (obsolete) — 
Patch that includes dbaron's find (I was just going to look for that exact
problem myself. :))
Uhh.  I'm not sure what happened there.
I've seen lots of confusion over why I split this up in the way that I did,
resulting in a huge patch.  Here are the reasons:

1.  It's important that the fonts guys are able to work unimpeded.  Adding
another layer that they need to worry about is going to hinder their ability to
fix bugs and judge impact.

2.  It's important that I am able to work unimpeded as well in the Xft code. 
(Remember, this is replacement for the current matching and rendering code - not
just "a different rendering method")

3.  You can now build without xft or without core font support.  This is just a
nice side effect and wasn't the primary goal.  But, since I was there...

4.  I don't want to have to load all of the core code support in order to use
Xft, since most users on the next generation desktops are going to be using Xft
as their only rendering method.
One note, since dbaron mentioned default config - the 
default sans/serif/monospace fonts in the UI (menus, etc.) should always be the
aliases fontconfig defines for that purpose (sans-serif, serif, monospace, I
believe). These aliases are defined by convention to be the preferred local UI
fonts. So e.g. Qt and GTK+ will also be defaulting to "sans-serif" as the
primary UI font. These aliases are also the only font names that are 
guaranteed to exist on a given system.
Yeah, there's still some things that I need to do wtf prefs and even probably
the prefs UI to get things working right (we need a "use system font" setting of
some kind.)  Once again, though, not important enough to prevent people from not
checking this in.
blizzard wrote:
> 3.  You can now build without xft or without core font support.  This is just 
> a nice side effect and wasn't the primary goal.  But, since I was there...

You are aware that removing "core" X font support will add _major_
interoperability problems between Unix/Linux, right ?
Xtf's emulation code does not count here since it (AFAIK) doesn't make use of
the fonts provided by the "core" X font system and regresses text rendering
performance a lot on non-Xrender-aware Xservers.
s/wtf/wrt/

:)
Roland, people who want to use core fonts shouldn't disable core fonts.  This
patch doesn't do anything to affect the current core font support, unless the
builder chooses explicitly to exclude it.  And I expect that people who are
planning to distribute to environments that don't have RENDER-capable servers --
poor folk -- won't so choose.  But that's just a hunch. :-))))))

(RH8 only uses core fonts for some legacy gtk 1.2 apps, like Mozilla, and I
expect those apps to be dropped or evolved in the next release or two.  I don't
expect Sun and HP to be far behind, honestly.)
Ok, the XFT Mozilla up and dissapears (*poof*) on me shortly after (briefly)
displaying some serious weirdness at: http://www.zdnet.com/zdnn/

RH8 Psyche, XFT build, and a "whole whack" of Windows True Type fonts installed.

It doesn't fold /every/ time.  I made it through twice.  The other 5 times it
crashed.  It slows down a whole lot before it goes, feels like heavy CPU load. 
I managed to grab a screenshot before it dissapeared:

http://www.hubick.com/temp/bugtests/mozfont1.png

I don't know if this attachment helps you (?).
Hrm.  I seem to be having some difficulty reading the news :)

Clicking on the following link also folds the browser:
http://www.internetwk.com/breakingNews/INW20021003S0005

I've isolated the font size problem in comment 196 to being associated with the
helvetica font.  If a web page, or our chrome (which defaults, on my system, to
"Adobe-Helvetica-ISO8859-1,Helvetica" as the name value in
nsSystemFontsGTK::GetSystemFontInfo) asks for Helvetica, it gets a tiny font
that can only be displayed at one size (about 8px, I'd say).
> I've isolated the font size problem in comment 196 to being associated with the
> helvetica font.  If a web page, or our chrome (which defaults, on my system, to
> "Adobe-Helvetica-ISO8859-1,Helvetica" as the name value in
> nsSystemFontsGTK::GetSystemFontInfo) asks for Helvetica, it gets a tiny font
> that can only be displayed at one size (about 8px, I'd say).

Yeah, that's exactly what I found when I looked at it.  It needs either a more
sane default or needs some fixing in the xft code - we'll see.
I'm not seeing crashes on either of those sites.  Are you sure you're not
stumbling across a flash or java problem?

Are you seeing an X error or a segfault or something else?  A stack trace (those
builds might have simple symbols in them) is way more helpful than strace output.
I don't have either Java or Flash installed.

I get no error message at all.  The browser goes to the site, "slows down",
possibly displays it for a short <= 10 second period of time, the *poof* the
process dissapears with no messages or anything.  I grabbed the screen shot I
linked to during that short time before it folds - just so I can show you I
wasn't imagining it :-)

What's the exit status on the browser?  (From a shell echo $? will tell you.)
[hubick@CHWorkstation hubick]$ /usr/local/mozilla/mozilla
[hubick@CHWorkstation hubick]$ echo $?
11

After I installed RH8, I was trying to figure out how to configure my fonts. 
After reading the RedHat reference guide fonts section, I went looking in 
/etc/X11/fs/config, and saw that it pointed at a directory for TTF fonts.  So I
copied all my TTF fonts to /usr/X11R6/lib/X11/fonts/TTF.  I then ran the lower
casing script on them, created a fonts.scale and fonts.dir file.  After finding
out that this wasn't taking effect for xft applications, I scrounged around some
more on the RH mailing lists, and found out about /etc/fonts.conf.  Which did
not reference the same directory.  I then filed:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=74952
and
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=74953

I added the fonts dir where I put my TTF fonts to fonts.conf and restarted. 
This worked as expected.

Owen then closed the latter of those two bugs saying I put the fonts in the
wrong place (should be /usr/share/fonts).  I figured it wouldn't really matter,
and since they were already there, I just left them.

Just now I tried removing my TTF folder from fonts.conf which I had added, and
tried the ZDNN site, and I no longer see the lockup.

I have a bunch of fonts in that dir - over a hundred that came with my Windows
and CorelDraw distributions, etc.

So, what does that say?

Most likely freetype or possibly a higher layer is crashing on a particular
font. We fixed several bugs along those lines during the Red Hat Linux 8 beta
cycle, but there may be more. A backtrace from gdb ought to turn up the problem.
Or if you can isolate which font is causing the issue.
Just for the record, Mandrake will switch to Xft2/fontconfig for Mdk Linux 9.1..

About the crashes, check patch at
http://www.freetype.org/pipermail/devel/2002-September/004015.html for freetype
2.1.2, it fixes a common crashes with some fonts (like BlueHighway..)
I've installed fontconfig/Xft2/Xft1.1 and I tried to use the build provided by
blizzard and also the build on keithp's page and none of them use xft on my
system (ie they render using core fonts).. But the rest of my gnome2 desktop is
xft'ed... I tried using FC_DEBUG=1 and I get zero output from it for moz (but it
works from gtk2 apps) .. As if moz was not able to find my fontconfig/xft
installation...
Attachment #101579 - Attachment is obsolete: true
Attachment #101581 - Attachment is obsolete: true
Attachment #101583 - Attachment is obsolete: true
Attachment #101584 - Attachment is obsolete: true
Attachment #101585 - Attachment is obsolete: true
Attachment #101586 - Attachment is obsolete: true
Attachment #101588 - Attachment is obsolete: true
> I've installed fontconfig/Xft2/Xft1.1 and I tried to use the build provided by
> blizzard and also the build on keithp's page and none of them use xft on my
> system (ie they render using core fonts).. But the rest of my gnome2 desktop is
> xft'ed... I tried using FC_DEBUG=1 and I get zero output from it for moz (but it
> works from gtk2 apps) .. As if moz was not able to find my fontconfig/xft
> installation...

I'm assuming you pulled down the rh7-xft-patch build?  If so, that's built with
the patch, but doesn't actually have xft turned on since rh7 doesn't have
fontconfig on it.  That's just to test the patch's effect on the core code. (I
guess the name is kind of misleading - sorry.)

Anyway, new rh7 builds should be appearing shortly at:

http://komodo.mozilla.org/pub/mozilla/nightly/experimental/xft/2002-10-04/

that include the ifdef fix.
I've tried compiling from Mozilla CVS several times--if I don't apply Chris's
patch, it works fine, but of course, then it has the core fonts.  If I patch it
before compiling, it will exit with a message the libgfx.so has an undefined
reference to NcFontPattern.  I'm using Slackware 8.1 with the standard XFree86
4.2 packages, and FreeType 2.1.3-RC2.  I'll try it again later with Freetype
2.1.2 and post the exact error message if the problem doesn't go away.
The rendering code, now separated into Xft, is a straight forward task.
Part of this has been maturing for a while and part of it is fairly new; eg:
the support for existing X servers was only put in this spring. Even though
there is a relatively new part in the rendering code, rendering is a relatively
well defined task so Xft is probably ready enough to use in Mozilla. I do want
to note that since the there does not appear to have been any discussion of
printing, the APIs may well require changes when printing is finally considered.

The FontConfig code is fairly new and fonts are a somewhat messy business.
Fonts are about 90-95% order and 5-10% chaos. This makes it much harder
to work out some bugs. As best I can tell, FontConfig uses Unicode ranges to
map languages which works fairly well for some languages but quite poorly for
others (especially Han disambiguation). FontConfig will take some time to
mature. However, some people seem bound and determined to put it in regardless
of its maturity.

If this must be put in before it has had time to mature then put it in with
a pref so *users* can choose.

The current patches are huge and their extreme size make analyzing and
discussing the parts unnecessarily difficult.

These  patches appear to have 3 parts:

1) changes to the API between the rendering layer, metrics layer, and font
   layer

2) code to use Xft

3) moving code to different files

Please separate out the virtual layer changes into a separate bug so this can
be more readily discussed. It should have stubs for the Xft code so the
details of how that Xft/FontConfig is integrated can be clearly seen.

> As best I can tell, FontConfig uses Unicode ranges to
> map languages which works fairly well for some languages but quite poorly for
> others (especially Han disambiguation).

Fontconfig advertises "language" support using RFC 3066 compatible identifiers
which include both ISO 639 language codes and ISO 3166 country codes.  This
allows unambiguous identification of the desired Han glyph shapes.

To compute coverage, Fontconfig uses Unicode coverage tables for each language
generated from various sources.  Many languages have official orthographies
making the task relatively easy.  The Han orthographies were generated by
analysing legacy encoding transformations to Unicode.  This effectively
separates most Han fonts into their respective languages.  The only fonts
presenting difficulty here were the newer chinese encoding (GB18030) which
happen to cover the older simplified chinese encoding (GB2312) as well as the
traditional chinese encoding (Big5).

To disambiguate these fonts, fontconfig looks at an OS/2 code page range table
if present.  If the table indicates support for only one Han language, then
languages mapping to other Han OS/2 code page range bits will not be included in
the list of supported languages.

Using the Unicode coverage also effectively separates fonts with support for the
Hong Kong supplimentary character set from those with only Big5 support.  It
also permits the identification of language support for fonts without OS/2 code
page range information, something provided only by relatively new fonts in
TrueType format.

Fontconfig currently has orthographic information for 122 of the 139 languages
named by ISO 639-1 along with 30 additional languages named only in ISO 639-2.
For Chinese, fontconfig has separate orthographies for China, Hong Kong and
Taiwan.  It uses the orthography from Taiwan for Macau and the orthography from
China for Singapore.

I'm very interested in finding orthographic information for the remaining
languages in ISO 639-1.
> If this must be put in before it has had time to mature then put it in with
> a pref so *users* can choose.

+    rv = prefService->GetBoolPref("fonts.xft.enabled", &enabled);

+        char *val = PR_GetEnv("GDK_USE_XFT");

Lots of way to choose in this patch.

> The current patches are huge and their extreme size make analyzing and
> discussing the parts unnecessarily difficult.

I gave you an easy to use guide in comment 164 that should make it super-easy to
 look at the patch.

> 
> These  patches appear to have 3 parts:
> 
> 1) changes to the API between the rendering layer, metrics layer, and font
>    layer

There are no changes to the APIs of the rendering layer, metrics layer or the
font layer.  All of the calls from the rendering layer through to the fonts
layer are in nsFontHandleGtk.h.

> 
> 2) code to use Xft

The only code that uses Xft is in nsFontInstanceXft.h/cpp.

> 
> 3) moving code to different files
> 
> Please separate out the virtual layer changes into a separate bug so this can
> be more readily discussed. It should have stubs for the Xft code so the
> details of how that Xft/FontConfig is integrated can be clearly seen.

If you want, I can break up the current patch since this is apparently too much
for you to handle.  However, outside of the scope of Xft there isn't a lot of
value talking about changes since they are driven by Xft.

Really, I don't think the layers need much discussion - certainly not enough to
waste time with opening a seperate bug.  If you have a problem with the way that
the nsFontHandleGtk interface works, feel free to discuss it here.  That's why
we have this bug!
re: comment #227
todd: i had a similar problem, i was going to report it as well but didn't get
to it.  it seems that $(MOZ_XFT_LIBS) isn't getting set properly somewhere, as
my libgfx_gtk.so wasn't being linked (at compile time) to libXft or libfreetype
(and i suspect libfontconfig as well, but i don't remember).  i ended up opening
up gfx/src/gtk/Makefile.in and replacing all instances of $(MOZ_XFT_LIBS) with
`xft-config --libs`.  then doing 'make' in that directory should fix it up. 
this isn't a real fix of course, but it should get you going.

blizzard:  any idea why this problem is occurring (tho apparently not all the
time)?  i'm running a generally-fully-updated gentoo 1.2 system with the
Xft2/fontconfig package self-compiled that i got from fontconfig.org.
re: comment #227
same problem with slack8.1 here.
I sort of solved adding in gfx/src/gtk/Makefile:
-lfontconfig -lXft
in line 137 (148 if you build with gtk2).
Have you run autoconf after applying the patch ? This seemed necessary here
> 
> blizzard:  any idea why this problem is occurring (tho apparently not all the
> time)?  i'm running a generally-fully-updated gentoo 1.2 system with the
> Xft2/fontconfig package self-compiled that i got from fontconfig.org.

You have an up-to-date tree, right?  The toplevel configure contains the Xft
changes in it already and should set the proper variables, assuming you have
fontconfig and pkgconfig installed even somewhat properly.
I've read much of the patch in attachment 101589 [details] by this point.  I've
done some cross-tree diffing of moved code, but haven't done enough
quite yet.  However, I *think* I've read most of the patch by this
point, so I'll post my comments so far.  I'll do some more cross-diffing
and such tomorrow, but I probably won't have too many more comments
(although I might have some general ones after having more time to think
about it and stare at it).

Some of these are nits, and not all of the nits really need to be
addressed to land the patch.  However, many of them are pretty easy to
fix.  (And, for that matter, some of the more serious issues, like my
comment about |mWesternFont|, probably don't need to be fixed
immediately, although they do need to be fixed sometime.)

And I haven't really proofread these comments either, so I may have written
some things that make absolutely no sense.


Comments on this patch:

Many of the new files have an sentence with no verb in the license,
which looks like it was copied from some of the old ones:

  nsFontGTK.h: * The Original Code mozilla.org code.
  nsFontHandleGtk.h: * The Original Code mozilla.org code.
  nsFontInstanceCore.cpp: * The Original Code mozilla.org code.
  nsFontInstanceCore.h: * The Original Code mozilla.org code.
  nsFontInstanceFactory.h: * The Original Code mozilla.org code.
  nsFontInstance.h: * The Original Code mozilla.org code.
  nsFontInstanceXft.cpp: * The Original Code mozilla.org code.
  nsFontInstanceXft.h: * The Original Code mozilla.org code.
  nsFontMetricsGTK.cpp: * The Original Code mozilla.org code.
  nsFontMetricsGTK.h: * The Original Code mozilla.org code.

I think the new files should also be copyright 2002, rather than 2001.

Also, shouldn't the files that contain moved code (nsFontInstanceCore)
have the same license as the files the code was moved from (triple-NPL
rather than triple-MPL)?  Or has Netscape agreed that files can be
changed from NPL to MPL?  I never remember.

gfx/src/gtk/Makefile.in:
  Should the addition of XFT build flags to CFLAGS, CXXFLAGS, and
  EXTRA_DSO_LDOPTS be conditional on MOZ_ENABLE_XFT?  (It might help
  ensure people don't add Xft code outside the ifdefs.)

  This still builds the stuff in x11shared even with core x fonts
  disabled, which causes linker warnings when disabling core X fonts.

widget/src/gtk/Makefile.in:
  Why are these changes needed?

nsDrawingSurfaceGTK.h:
  Should the typedef of XftDraw be |#ifdef|ed?  (It might help ensure
  people don't add Xft code outside the ifdefs.)

nsFontInstance.h:

  For the various metrics-getting methods, I think would it be better
  (i.e., faster) to have all these virtual method getters be non-virtual
  inline methods on nsFontInstance that return the values from protected
  member variables of nsFontInstance that are filled in by the derived
  class (just as they are now, but into members of the derived class
  rather than the base class)?  This would reduce a bit of the overhead
  of the reorganization.  (Virtual function calls are bad, especially on
  heavily pipelined processors.)  It looks like the member variables and
  virtual functions are almost, if not completely, identical already.

nsFontInstanceCore.cpp:

  It's a little hard to follow exactly what was moved into this file.
  (diff isn't too helpful since it's not all in the same order as it was
  in nsFontMetricsGTK.cpp).

  XXX I need to comment more on this file.

nsFontInstanceXft.cpp:

  All of the casts to |FcChar8*|, throughout the file, should use
  NS_REINTERPRET_CAST so that you don't unnecessarily cast away |const|
  (which you do in many many places -- hopefully all the function
  signatures put it right back, but I didn't check all of them).  The
  same goes for |FcChar16*|.

  | static const struct XftLangGroup* FindFCLangGroup (nsACString &aLangGroup);
  In C++ rather than C, this can (and should) be just:
  | static const XftLangGroup* FindFCLangGroup (nsACString &aLangGroup);
  The same goes for the definition of |XftLangGroups[]|.

  And while I'm at it, the definition of XftLangGroup should probably
  have additional |const|s in it so that we don't cast string literals
  (in the text section, or wherever to non-const |char| or non-const
  |FcChar8|.  (This would require the addition of |const|s in the casts
  in the definition of |XftLangGroups[]| as well.)

  Should XftLangGroup have a name that makes it's clear it's a Mozilla
  struct rathen than an Xft struct?  (nsXftLangGroup?)

  [ Ignore this paragraph if you want. ]  If I want add a bunch of style
  nits, I'd say that I prefer member initializers to all the assignments
  in the nsFontInstanceXft constructor (but see my comment on
  nsFontInstance.h), and I'd also like to cite bug 78032 comment 1
  (prefer ++i over i++) for the benefit of any future code you might
  write, but I don't see a need to bother cleaning up this patch for
  either.

  In the destructor of nsFontInstanceXft, you can simplify these two
  lines:
    | int end = mLoadedFonts.Count() - 1;
    | for (int i=end; i >= 0; i--) {
  to
    | for (PRInt32 i=mLoadedFonts.Count() - 1; i >= 0; --i) {
  (and I think PRInt32 is probably preferred since that's what the
  parameter to nsVoidArray::ElementAt is).

  You could also pull the RemoveElementAt out of that loop and make it a
  mLoadedFonts.Clear() after the loop, or not bother at all since you're
  about to be destroyed anyway.

  gNumInstances and all its uses should be |#ifdef DEBUG_XFT_MEMORY|

  There are a few odd mixes of coding style in this file -- in
  particular, some function calls have a space between the function name
  and opening parenthesis, which is not Mozilla style.  (e.g., one of
  the calls to AddLangGroup).  I think I've noticed this in some other
  files as well.

  Do we need to load a separate "MiniFont" for every font that's
  requested?  Would one font suffice?  If not, could we delay loading it
  until it's known to be necessary?  (How do other implementations
  handle filling numbers into unknown glyphs?  Is there a better way?)

  I wonder if all the uses of |mWesternFont| are legitimate (such as the
  direct uses in |GetWidth| taking |char*|, etc.).  Is it possible for a
  font to have some but not all ISO-8859-1 characters?  I'd think it
  would be (especially if it were an ISO-8859-x font, for x != 1).  I'm
  seeing a bug that may be related.  In the testcase (you probably don't
  have this font):

  <p style="font-family: 'glyph systems-david-iso8859-8', serif;">
    &Aacute;B&Ccedil;D&Eacute;FGH&Iacute;JKLMN&Ouml;
    &aacute;b&ccedil;d&eacute;fgh&iacute;jklmn&ouml;
  </p>

  I see Hebrew characters in place of the capital A-acute and the
  capital I-acute.  That's bad.  I'm not sure if it's related to this
  particular problem.  (The character positions don't make sense.)

  The printf at the start of nsFontInstanceXft::DrawString should be an
  NS_ASSERTION (or, if that's not what you meant, at the very least
  |#ifdef DEBUG|).  But if that's the case, why do you need the code
  after the early return |if (!aSpacing)|?

  There's so much in common between the two DrawString methods that I
  wish there were a nice way to reduce the amount of copied code, but I
  don't really see a good one (aside, perhaps, from putting the color
  conversion into its own function).

  In the DrawString functions, it would be much nicer to use new[] and
  delete[] than the multi-line arguments to nsMemory::Alloc plus casts.

  In the second DrawString and in GetTextDimensions and in RawGetWidth,
  the |while (j < end)| loop should probably be condensed into a |for|
  loop (no need to declare j so early, or, for that matter |end|, which
  is declared and then assigned into two lines below in a rather odd
  fashion), and again |j| should probably be PRInt32 for consistency.

  There are probably some significant performance improvements that
  could be made to the second DrawString, but we can worry about that
  later.

  The |printf("GetBoundingMetrics")| should be a NS_NOTREACHED.

  nsFontInstanceXft::FindFont calls |mLoadedFonts.Count()| every time
  through the loop.  This needs an |end|.

  In GetTextDimensions, it might be good to save double-calls to
  GetMaxAscent / GetMaxDescent.  You can also pull the declaration of
  |i| into the for-expression.

  The unimplemented GetTextDimensions methods should have an
  NS_NOTREACHED.

  nsFontInstanceXft::CacheFontMetrics uses the expression |(mMaxAscent +
  mMaxDescent)| tons of times -- you could use a variable called
  |lineSpacing| (like nsFontInstanceCore), or just set |mMaxHeight| a
  little earlier and then use it more.

  I think you need a minus sign for the backup code for initializing
  |mUnderlineOffset|.

  How often is nsFontInstanceXft::SetupFCPattern called?  I'd think it's
  a lot, and I'd think you'd want to cache the results of the pref
  lookups (although maybe they're not as slow as I think).

  The comment about adding the language group before the generic seems
  to belong above |AddLangGroup| rather than 30 lines down below all the
  pref code.

  How about an NS_ASSERTION if nsFontInstanceXft::DoMatch enters the
  "loser:" code?  (I ask for things this obvious because I've spent too
  much time finding bugs in really old layout code that would have been
  obvious if people had done things like this.)

  The "FoundFont:" sections of GetTextDimensions and RawGetWidth could be
  simplified a bit by switching the nesting of the |if (prevFont)| and
  the |if (currFont != prevFont)| conditions.

  In SetupMiniFont, I think
    | char str[2] = "\0";
    | str[0] = c;
  would be more clearly (and perhaps faster) written as:
    | char str[2];
    | str[0] = c;
    | str[1] = '\0';

  In SetupMiniFont, shouldn't you use PR_MAX instead of MAX (which may
  not be defined on all platforms)?

  The constructor and destructor of nsXftFont have more of the space
  before "(" in function calls.  Also GetXftFont.

  In nsFontXft::GetXftFont, should you have additional error handling:
    |         mXftFont = XftFontOpenPattern (GDK_DISPLAY(), pat);
    |+
    |+        if (!mXftFont)
    |+            FcPatternDestroy(pat);
    |     }
  or do we assume that never happens?  (Some of the internal Xft code
  does it.)

  The nsFontXft::GetBoundingMetrics{8,16} should use NS_NOTREACHED
  instead of printf.

  The way you use GetXftFont is inconsistent between
  GetMaxAscent/GetMaxDescent (which I think I prefer) and
  GetWidth{8,16}.  The latter seems to be the only place you null-check
  the result.  If you need to, then you should elsewhere.  If you don't
  need to, then don't.  And why have a long comment about getting the
  font that escribes the obvious only in |GetMaxDescent|?  (I'd prefer
  none.)

  I think NS_FamilyExistsXft needs to call FcPatternDestroy.

  The two tests leading to |goto end;| in NS_FamilyExistsXft could be
  combined into a single |if (!set || !set->nfont)|.

  (Eek, the signature of FcPatternGetString really should have a |const|
  for the out parameter!)  You might want to use a |const| for
  |tmpname| anyway, since the source of it makes it look like that's
  what is meant.

  In |NS_EnumFontsXft|, there are some cases (nfont == 0) where you
  return a success result without initializing the out parameters.  You
  might want to initialize them in all cases, but you certainly should
  when returning success.

  |CalculateSlant| might be clearer if the default were a "default:".

  In the following in |CalculateWeight|:
    |    /* Map from weight value to fcWeights index */
    |    static int fcWeightLookup[10] = {
    |        0, 0, 0, 0, 1, 1, 2, 3, 3, 4,
    |    };
  I think you may want to change the second of the 3's to a 4, so that
  an 800 font-weight maps to BLACK rather than BOLD.  This seems more in
  conformance with http://www.w3.org/TR/REC-CSS2/fonts.html#q46 .  (Or
  was there some other reason that led you to pick what you did?)

  In |AddLangGroup|, you can save a string copy (and an nsAutoString
  constructor, which is far worse) by changing the first 4 lines to:

    const PRUnichar *name;
    aLangGroup->GetUnicode(&name);
    nsCAutoString cname;
    cname.AssignWithConversion(nsDependentString(name));

  |AddFFRE| -- another space before (.
    Also, are you doing anything evil here when casting away const?
    (See one of my first comments on this file.)

  In FFREToFamily, you can use
    oFamily.Append(Substring(aFFREName, familyHyphen,
                             registryHyphen-familyHyphen));
  instead of the long thing that does an extra string copy.

  I'd tell you to use |CountCharInReadable| instead of writing your own
  |FFRECountHyphens|, except |CountCharInReadable| is slower than the
  way you wrote it (unless you have a multifragment string of many
  fragments and high density of the target character).  Aargh.  I should
  file a bug.

XXX I need to comment more on the files below, and on the files I
haven't mentioned.

nsRenderingContextGTK.cpp:

  This patch adds an unused member variable, mFontCachedOffset, which
  should be removed.

nsFontHandleGtk.h:

  I don't really understand why this layer is needed.  It seems to me
  that you could avoid a good bit of extra layers while keeping the
  abstration by adding an nsIFontMetricsGTK between nsIFontMetrics and
  nsFontMetricsGTK, and putting the stuff that's on nsFontHandleGtk
  there rather than having a separate object.

nsFontMetricsGTK.{h,cpp}:

  I don't see any reason you need separate variables for
  |mFontInstanceCore| and |mFontInstanceXft|.  Can't you just make this
  a single |mFontInstance|?  I think this would allow removing of some
  null checks.  (Or is there still a possibility of these methods being
  called after failure?  If so, can it be eliminated?)


An addendum to some of my previous comments, which probably doesn't need
to be addressed on the Mozilla end:

The Helvetica problem, which I mentioned in comment 196 and comment 216,
seems to be an Xft bug -- I can reproduce it in other programs by
selecting Helvetica as my Application Font.  (I need to figure out where
my Helvetica font comes from and file an Xft bug in RedHat bugzilla.)


Comments on the moved code that should probably be addressed by its
owner:

nsFontGTK.h

    typedef struct nsFontCharSetInfo nsFontCharSetInfo;
  That's C, not C++.  There are many other things like this.  For C++,
    struct nsFontCharSetInfo;
  is sufficient and preferred (at least in my opinion).

nsFontInstanceCore.cpp:

  No need for PR_{BEGIN,END}_EXTERN_C -- that's for headers that can be
  included by either C or C++.  Here you can just use |extern "C" {| and
  |}|.
> Or has Netscape agreed that files can be changed from NPL to MPL?  I never 
> remember.

They have, yes.  We can tri-MPL-license Netscape-copyrighted code at will, and
should.

>   In SetupMiniFont, I think
>     | char str[2] = "\0";
>     | str[0] = c;
>   would be more clearly (and perhaps faster) written as:
>     | char str[2];
>     | str[0] = c;
>     | str[1] = '\0';

char str[2] = { c, '\0' }; // ?

>     typedef struct nsFontCharSetInfo nsFontCharSetInfo;
>   That's C, not C++.  There are many other things like this.  For C++,
>     struct nsFontCharSetInfo;
>  is sufficient and preferred (at least in my opinion).

It's definitely better C++ and Mozilla style, but I don't think we need the
forward declaration at all; the typedef doesn't provide that on its own, unless
my C-brain is blearier than I think at this hour.

Thanks for the great review, dbaron.  I think you've made blizzard's weekend. =)
i didn't run autoconf, could that actually be it?  i thought configure should
rebuild all the Makefiles from the Makefile.ins.  too tired to play with it now...

blizzard:  yeah, tree was freshly updated before i patched it.  actually i did
make distclean && cd .. && cvs update mozilla/client.mk && make -f
mozilla/client.mk checkout && cd mozilla && zcat
../mozilla-xft-blizzard.diff.gz|patch -p0 && configure && make
or something along those lines.  i'll do a little more playing in the next
couple days to see if i can figure it out myself.

otherwise, moz looks beautiful, other than the location bar text writing over
past the end of the box and onto the buttons to the right (don't know if you
fixed that yet, i'm running i think the last 'checkpointing changes' patch. 
will try the latest soon.  great work; i'm very pleased.

random OT question: what happened to the native gtk widgets?  i noticed native
widgets in i think either 1.2a or something recent, but nothing on the tip now,
just the moz xpwidgets for scrollbars, buttons, etc.
> Have you run autoconf after applying the patch ? This seemed necessary here

Actually, I found that a "sh allmakefiles.sh" was what I needed.  It works great
now :]
For the record, the problem I was having with Helvetica was because I have had
(for a long time) the Mathematica fonts from
http://support.wolfram.com/mathematica/systems/unix/interface/pcf.html installed
in /usr/share/fonts/Mathematica/.  These fonts have a Helvetica
(wri-helvetica-iso8859-1) that is only available at 2 sizes.  I had two other
Helvetica fonts, both bitmap fonts, one available in a wider range of sizes, but
they were both in /usr/X11R6/lib/X11/fonts/.  So I moved the Mathematica fonts
to /usr/X11R6/lib/X11/fonts/ and made the appropriate configuration changes and
the problem went away (since Xft no longer finds any font called Helvetica). 
The lesson, I guess, is that it's probably not a good idea to install bitmap
fonts with common names like Helvetica in /usr/share/fonts/ when Xft is
searching there.

On another note, the Hebrew mixing with ISO-8859-1 problem that I mentioned
yesterday doesn't seem to be related to mWesternFont overuse, but rather some
more general problem.  I see the problem even if I put &#x9000; within the same
run of characters, which I would expect to trigger the 16-bit character codepath
for the whole bunch (although I could be wrong about that, in which case it
could be related to overuse of mWesternFont).
>> bstell wrote:
>>
>> Please separate out the virtual layer changes into a separate bug so this can
>> be more readily discussed. It should have stubs for the Xft code so the
>> details of how that Xft/FontConfig is integrated can be clearly seen.

> blizzard wrote:
>
>If you want, I can break up the current patch since this is apparently too much
>for you to handle.  However, outside of the scope of Xft there isn't a lot of
>value talking about changes since they are driven by Xft.

Blizzard, I hope you that you do understand that it's quite reasonable to ask
for a patch/diff that shows the _actual_ differences.

The diff (tool) ceases to be the useful tool it is, when the code is moved or
re-organized. Here's an example:

1) When moving the code around, you get a diff like this:

Index: xxx.cpp
---------------
- blaa blaa whole 
- license text
- blaa blaa
-
- lots of code
- with (almost) all 
- the stuff in the file
- int fontclass :: somefunc() {
-      stuff...
-      if (fontid == 1) {
(and so on...)

Index: yyy.cpp
---------------
+ blaa blaa whole 
+ license text
+ blaa blaa
+
+ lots of code
+ with (almost) all 
+ the stuff in the file
+ int fontclass :: somefunc() {
+      stuff...
+      if (fontid == 1 && xftfontid ==2) {
(and so on...)

2) without moving/reorg we get only the interesting part, i.e.:

Index: xxx.cpp
---------------
-      if (fontid == 1) {
+      if (fontid == 1 && xftfontid ==2) {


So, it's so much easier for the reviewers to do their work. In the case 1), each
and every reviewer has to do some sort of de-moving and de-ordering in order to
be able to diff the diffs and to get into the case 2).

You would have made yourself a huge favour if you had in some way or another
kept the move/reorganization, changes and additions as separate patches in the
beginning, or at least split the patch later, when realizing this is toying
around lots of code.

The ability to easily review this stuff also contributes better quality; one can
spend more time on finding bugs/typos (which are already easier to spot) and
thinking what would be the best way to organize/integrate/interface the old/new
code.
A few more comments:

nsFontMetricsGTK.cpp:

  You should cache the result of IsXftEnabled so you don't have to do a
  getenv and a pref lookup so often.

nsXFont.h:
nsFontFreetype.cpp:

  The include changes here seem like they're adding gdk dependencies from
  things that shouldn't have them.

nsXFontAAScaledBitmap.cpp:

  Was there a reason you removed that little chunk of code?
Attached file patch (obsolete) — 
Patch after dbaron's first set of comments.
Attachment #101589 - Attachment is obsolete: true
Testing the 2002-10-04 rh8-xft binary and using Verdana as my Serif font and
LucidaTypewriter as my monospace font, the only problem I have seen so far is
the rendering of the MARC mailing list archives, in particular the underline
seems to be raised slightly higher and eats into the text. I don't see this
behaviour if I switch to other fixed width fonts such as Andale
Mono,Courier,Luxi Mono

URL for the linux-kernel archive at MARC is

http://marc.theaimsgroup.com/?l=linux-kernel&r=1&b=200210&w=2

I will attach a screenshot also
For the record, underlined text also looks weird on
http://www.python.org/doc/current/modindex.html
screenshot showing underline being raised slightly and cutting into the text
Fonts configured in Mozilla-xft at the time of screenshot
Serif->Verdana
Monospace->LucidaTypewriter
Perhaps the underline offset issue is related to my comment in comment 235 about
the backup underline offset being the negative of what it should be.  Or maybe
the non-backup one is?  (That would surprise me, though, unless Xft is worse at
getting underline metrics than xfsft was, since many of my fonts have an
underline offset that clearly does have the correct sign.)
Yes, I just tested the minus sign change in my tree, and it does fix
http://www.python.org/doc/current/modindex.html , which did display badly for
me.  Since that minus sign fix is in attachment 101865 [details], the problem is fixed in
the latest version of the patch.
Comment on attachment 101865 [details]
patch

A few comments on this patch (in addition to the ones in coment 241), on the
things that changed since the previous patch:

You can save 4 bytes (an extra vtable pointer) on the size of nsFontMetricsGTK
objects by making nsIFontMetricsGTK inherit from nsIFontMetrics and then making
nsFontMetricsGTK inherit only from nsIFontMetricsGTK.

The nsFontMetricsGTK methods that forward to mFontInstance are inconsistent
about null-checking mFontInstance.  I would hope that now that you only have
one, there wouldn't be a need to null-check it in most places, since the font
metrics Init method would have returned failure and the font metrics object
wouldn't be used.

MozXftLangGroup::mozLangGroup should be |const char*|, not just |char*|.

Once I get jprof working again I'll try to profile and see which of my
performance comments are more important and file bugs on those (depending on
bug 172768).
I'm using the build of this chris put up on october 4th on RedHat 8, and I'm
seeing a symptom similar to that in comment #47, except for me, the browser
doesn't crash, it just hangs forever.  the problem happened to me when viewing
CNET url:
http://news.com.com/2100-1023-959537.html although I've seen it on several
other pages as well.  I've attached the stack trace.  Note, hang is in
malloc...
sorry for spamming, my problem seems to have been related to a plugin...I
disabled all plugins and it went away.  I suspect either the java or flash plugin
I tested blizzard's experimental builds at
ftp://ftp.mozilla.org/pub/mozilla/nightly/experimental/xft/2002-10-04/ and found
that there was no impact at all to existing code paths comparing the vanilla
build and the build with the xft patch.  The scores were 560ms and 561ms,
respectively, on my 1.5GHz RH 7.3 machine with 512 MB RAM.
[ Comments from the first review.  If something isn't listed here it's because I
went ahead and change it after the review. ]

> Also, shouldn't the files that contain moved code (nsFontInstanceCore)
> have the same license as the files the code was moved from (triple-NPL
> rather than triple-MPL)?  Or has Netscape agreed that files can be
> changed from NPL to MPL?  I never remember.

Yes, they agreed to change the license a long time ago.

> gfx/src/gtk/Makefile.in:
>   Should the addition of XFT build flags to CFLAGS, CXXFLAGS, and
>   EXTRA_DSO_LDOPTS be conditional on MOZ_ENABLE_XFT?  (It might help
>   ensure people don't add Xft code outside the ifdefs.)

If Xft isn't enabled, those are empty so I didn't think that's a big
deal.

> widget/src/gtk/Makefile.in:
>   Why are these changes needed?

They aren't (left over from an earlier version of the patch.)

> nsFontInstance.h:
> 
>   For the various metrics-getting methods, I think would it be better
>   (i.e., faster) to have all these virtual method getters be non-virtual
>   inline methods on nsFontInstance that return the values from protected
>   member variables of nsFontInstance that are filled in by the derived
>   class (just as they are now, but into members of the derived class
>   rather than the base class)?  This would reduce a bit of the overhead
>   of the reorganization.  (Virtual function calls are bad, especially on
>   heavily pipelined processors.)  It looks like the member variables and
>   virtual functions are almost, if not completely, identical already.

Yeah, I originally did that seperation for other reasons and they just
ended up the same.  All of those getters are now merged into the
nsFontInstance class.

> nsFontInstanceXft.cpp:
> 
>   All of the casts to |FcChar8*|, throughout the file, should use
>   NS_REINTERPRET_CAST so that you don't unnecessarily cast away |const|
>   (which you do in many many places -- hopefully all the function
>   signatures put it right back, but I didn't check all of them).  The
>   same goes for |FcChar16*|.

We talked about this.  It should be safe to cast away const here, even
though the functions in question don't take const.  Plus, it's all
critical path code so adding the extra code and possible allocations
(even with a fixed size static buffer) might add some serious pain.

I've asked Keith about the safeness of doing this since he knows the
internals of the functions more than I do - I'll fix it if I need to.

>   There are a few odd mixes of coding style in this file -- in
>   particular, some function calls have a space between the function name
>   and opening parenthesis, which is not Mozilla style.  (e.g., one of
>   the calls to AddLangGroup).  I think I've noticed this in some other
>   files as well.

Yeah, as a programmer I still haven't figured out what I'm happy with
here and the results are that I don't have a consistent style yet. :P
I'm not going to try to fix all of these yet.

>   Do we need to load a separate "MiniFont" for every font that's
>   requested?  Would one font suffice?  If not, could we delay loading it
>   until it's known to be necessary?  (How do other implementations
>   handle filling numbers into unknown glyphs?  Is there a better way?)

No, it's needed for each instances since the size of the bounding box
is based on the size of the font.  I don't think that other
implementations fill in the unknown box with the number of the missing
glyph.  This is the pango style, which I'm duplicating since it gives
a little better information than just drawing an empty box.

>   There's so much in common between the two DrawString methods that I
>   wish there were a nice way to reduce the amount of copied code, but I
>   don't really see a good one (aside, perhaps, from putting the color
>   conversion into its own function).

Yeah, there are are lot of similarities.  For that matter there's a
lot of similarities between DrawString() and GetTextDimensions().

>   There are probably some significant performance improvements that
>   could be made to the second DrawString, but we can worry about that
>   later.

Yeah, I thought about it for a bit, but there didn't seem an obvious
way so I just left it as something that "just works."

>   In GetTextDimensions, it might be good to save double-calls to
>   GetMaxAscent / GetMaxDescent.  You can also pull the declaration of
>   |i| into the for-expression.

No, the i is used later after the for loop.  I fixed the double calls,
though.

>   I think you need a minus sign for the backup code for initializing
>   |mUnderlineOffset|.

Yep.  Good catch.

>   How often is nsFontInstanceXft::SetupFCPattern called?  I'd think it's
>   a lot, and I'd think you'd want to cache the results of the pref
>   lookups (although maybe they're not as slow as I think).

It's called once per the lifetime of the nsFontInstanceXft object,
which isn't often at all.  I'm not worried about it.

>   The "FoundFont:" sections of GetTextDimensions and RawGetWidth could be
>   simplified a bit by switching the nesting of the |if (prevFont)| and
>   the |if (currFont != prevFont)| conditions.

I don't think so.  I looked at this for a long time when I was writing
this code and didn't see a way to express it such that it was clearer.
The problem is that for the first character, currFont != prevFont so
you would always enter into the wrong block.  Maybe I'm
mis-understanding what you're saying here - I'm not sure.

>   I think NS_FamilyExistsXft needs to call FcPatternDestroy.

Yeah, I did.  Plusm there was another chance to leak an object in that
code as well that I just fixed.

>   In the following in |CalculateWeight|:
>     |    /* Map from weight value to fcWeights index */
>     |    static int fcWeightLookup[10] = {
>     |        0, 0, 0, 0, 1, 1, 2, 3, 3, 4,
>     |    };
>   I think you may want to change the second of the 3's to a 4, so that
>   an 800 font-weight maps to BLACK rather than BOLD.  This seems more in
>   conformance with http://www.w3.org/TR/REC-CSS2/fonts.html#q46 .  (Or
>   was there some other reason that led you to pick what you did?)

Let's file a seperate bug to look at that.  There are a couple of
other weight-related issues that I need to deal with, too - might as
well kill them all at once.

> nsFontHandleGtk.h:
> 
>   I don't really understand why this layer is needed.  It seems to me
>   that you could avoid a good bit of extra layers while keeping the
>   abstration by adding an nsIFontMetricsGTK between nsIFontMetrics and
>   nsFontMetricsGTK, and putting the stuff that's on nsFontHandleGtk
>   there rather than having a separate object.

I totally agree.  The handle started out as doing more but it's just
forwarding calls now.  Renamed to nsIFontMetricsGTK and pulled
directly into nsFontMetricsGTK.

> 
> nsFontMetricsGTK.{h,cpp}:
> 
>   I don't see any reason you need separate variables for
>   |mFontInstanceCore| and |mFontInstanceXft|.  Can't you just make this
>   a single |mFontInstance|?  I think this would allow removing of some
>   null checks.  (Or is there still a possibility of these methods being
>   called after failure?  If so, can it be eliminated?)
> 
> 

Agreed.  Once upon a time they were slightly different, but in the end
they ended up not being different.
> nsFontMetricsGTK.cpp:
> 
>   You should cache the result of IsXftEnabled so you don't have to do a
>   getenv and a pref lookup so often.

Added in a patch that I've got in my tree now - will upload soon.

> nsXFont.h:
> nsFontFreetype.cpp:
> 
>   The include changes here seem like they're adding gdk dependencies from
>   things that shouldn't have them.

Those files already have gdk dependencies.  Now they're just self-hosting.

> nsXFontAAScaledBitmap.cpp:
> 
>   Was there a reason you removed that little chunk of code?

Organic patch.  Didn't need it.  It's removed now.
> You can save 4 bytes (an extra vtable pointer) on the size of nsFontMetricsGTK
> objects by making nsIFontMetricsGTK inherit from nsIFontMetrics and then making
> nsFontMetricsGTK inherit only from nsIFontMetricsGTK.
> 
> The nsFontMetricsGTK methods that forward to mFontInstance are inconsistent
> about null-checking mFontInstance.  I would hope that now that you only have
> one, there wouldn't be a need to null-check it in most places, since the font
> metrics Init method would have returned failure and the font metrics object
> wouldn't be used.
> 
> MozXftLangGroup::mozLangGroup should be |const char*|, not just |char*|.
> 

All fixed in the patch I'm about to upload.
Attached file patch (obsolete) — 
Patch with all of dbaron's comments addressed.
Attachment #101865 - Attachment is obsolete: true
blizzard asked me to act as reviewer for his patch. I have been travelling and
it is just now that I am having a chance to dig into the patch. Reading the
numerous comments ate plenty of the time. It is however unfortunate that most of
these weren't constructive comments. There were some points that bstell made and
which blizzard responded adequately (comment 94), and this seemed to have
essentially provided a point of consensus: separate Xft support although bstell
isn't totally agreable with all the nitty-gritty details -- without
unfortunately being as constructive as he was in bug 132759. bstell, since you
obviously have a vested interest on fonts, and Xft is poised to supersede what
you did (at some stage), you could have iterated on the proposed patch (as you
did in bug 132759) to contribute your desired alterations for others to chew
upon. Only blizzard's patch is here for us to review.

Unfortunately it is long past midnight here at the moment, and I can only
continue tomorrow (local time).

Something this large has usually been an early milestone material. So it is
probably more realistic to target the upcoming one where less restricted
checkins would allow rapid resolution of issues that my arise, thereby allowing
people to rejoice with a more stable support when the target milestone is
released. In the meantime, s/nsFontInstance/nsFontMetrics/g would make the patch
stay closer to its memory lane.
> In the meantime, s/nsFontInstance/nsFontMetrics/g would make the patch
> stay closer to its memory lane.

Would you really want to give two different layers the same name?


I don't see a large risk to existing codepaths to landing this for 1.2b, and
drivers will be willing to accept changes to the Xft-only (not part of the
default build) code during the closure so that it can be improved quickly. 
Linux distributions need this code a few months ago, or now, or very soon,
depending on the distribution, and I'd rather see the development happen in the
Mozilla tree than force a fork that might be long-lasting.
> bstell, ... you could have iterated on the proposed patch 

The patches have around 2,500 patch lines of real change and about 14,000+ patch 
lines which bascially move functions unchanged from one file to another.  These 
14,000+ lines appear to be not directly related to the Xft changes and obscure it. 
I have been requesting a patch without the file moves for 2 months. I still think
this separation should be done for the reasons listed in comment 240.
Compare the code to check if Xft is enabled. The "A:" lines are from the current 
patch and the "B:" lines are what a simplified patch could look like.

A: -nsFontGTK*
A: -nsFontMetricsGTK::FindLangGroupFont(nsIAtom* aLangGroup, PRUnichar aChar,
nsCString *aName)
A: +/* static */
A: +PRBool
A: +IsXftEnabled(void)
A:  {
A: -  nsFontGTK* font;
A: -
A: -  FIND_FONT_PRINTF(("      lang group = %s", atomToName(aLangGroup)));
A:  
A: -  //  scan gCharSetMap for encodings with matching lang groups
A: -  nsFontCharSetMap* charSetMap;
A: -  for (charSetMap=gCharSetMap; charSetMap->mName; charSetMap++) {
A: -    nsFontLangGroup* mFontLangGroup = charSetMap->mFontLangGroup;
A: +    static PRBool been_here = PR_FALSE;
A: +    static PRBool cachedXftSetting = PR_TRUE;
A:  
A: -    if ((!mFontLangGroup) || (!mFontLangGroup->mFontLangGroupName)) {
A: -      continue;
A: -    }
A: +    if (!been_here) {
A: +        been_here = PR_TRUE;
A: +        nsCOMPtr<nsIPref> prefService;
A: +        prefService = do_GetService(NS_PREF_CONTRACTID);
A: +        if (!prefService)
A: +            return PR_TRUE;
A:  
A: -    if (!charSetMap->mInfo->mLangGroup) {
A: -      SetCharsetLangGroup(charSetMap->mInfo);
A: -    }
A: +        nsresult rv;
A:  
A: -    if (!mFontLangGroup->mFontLangGroupAtom) {
A: -      SetFontLangGroupInfo(charSetMap);
A: -    }
A: +        rv = prefService->GetBoolPref("fonts.xft.enabled", &cachedXftSetting);
A:  
A: -    if ((aLangGroup != mFontLangGroup->mFontLangGroupAtom) 
A: -       && (aLangGroup != charSetMap->mInfo->mLangGroup)) {
A: -      continue;
A: -    }
A: -    // look for a font with this charset (registry-encoding) & char
A: -    //
A: -    nsCAutoString ffreName;
A: -    if(aName) {
A: -      // if aName was specified so call TryNode() not TryNodes()
A: -      ffreName.Assign(*aName);
A: -      FFRESubstituteCharset(ffreName, charSetMap->mName); 
A: -      FIND_FONT_PRINTF(("      %s ffre = %s", charSetMap->mName,
ffreName.get()));
A: -      if(aName->First() == '*') {
A: -         // called from TryFamily()
A: -         font = TryNodes(ffreName, aChar);
A: -      } else {
A: -         font = TryNode(&ffreName, aChar);
A: -      }
A: -      NS_ASSERTION(font ? font->SupportsChar(aChar) : 1, "font supposed to
support this char");
A: -    } else {
A: -      // no name was specified so call TryNodes() for this charset
A: -      ffreName.Assign("*-*-*-*");
A: -      FFRESubstituteCharset(ffreName, charSetMap->mName); 
A: -      FIND_FONT_PRINTF(("      %s ffre = %s", charSetMap->mName,
ffreName.get()));
A: -      font = TryNodes(ffreName, aChar);
A: -      NS_ASSERTION(font ? font->SupportsChar(aChar) : 1, "font supposed to
support this char");
A: -    }
A: -    if (font) {
A: -      NS_ASSERTION(font->SupportsChar(aChar), "font supposed to support this
char");
A: -      return font;
A: -    }
A: -  }
A: +        // Yes, this makes sense.  If xft is compiled in and there's no
A: +        // pref, it's automatically enabled.  If there's no pref, check
A: +        // the environment.
A: +        if (NS_FAILED(rv)) {
A: +            char *val = PR_GetEnv("GDK_USE_XFT");
A:  
A: -  return nsnull;
A: -}
A: -
A: -/*
A: - * First we try to load the user-defined font, if the user-defined charset
A: - * has been selected in the menu.
A: - *
A: - * Next, we try the fonts listed in the font-family property
(FindStyleSheetSpecificFont).
A: - *
A: - * Next, we try any CSS generic font encountered in the font-family list and
A: - * all of the fonts specified by the user for the generic
(FindStyleSheetGenericFont).
A: - *
A: - * Next, we try all of the fonts on the system (FindAnyFont). This is
A: - * expensive on some Unixes.
A: - *
A: - * Finally, we try to create a substitute font that offers substitute glyphs
A: - * for the characters (FindSubstituteFont).
A: - */
A: -nsFontGTK*
A: -nsFontMetricsGTK::FindFont(PRUnichar aChar)
A: -{
A: -  FIND_FONT_PRINTF(("\nFindFont(%c/0x%04x)", aChar, aChar));
A: +            if (val && val[0] == '0') {
A: +                cachedXftSetting = PR_FALSE;
A: +                goto end;
A: +            }
A:  
A: -  nsFontGTK* font = FindUserDefinedFont(aChar);
A: -  if (!font) {
A: -    font = FindStyleSheetSpecificFont(aChar);
A: -    if (!font) {
A: -      font = FindStyleSheetGenericFont(aChar);
A: -      if (!font) {
A: -        font = FindAnyFont(aChar);
A: -        if (!font) {
A: -          font = FindSubstituteFont(aChar);
A: +            cachedXftSetting = PR_TRUE;
A:          }
A: -      }
A: -    }
A: -  }
A: -
A: -#ifdef NS_FONT_DEBUG_CALL_TRACE
A: -  if (gFontDebug & NS_FONT_DEBUG_CALL_TRACE) {
A: -    printf("FindFont(%04X)[", aChar);
A: -    for (PRInt32 i = 0; i < mFonts.Count(); i++) {
A: -      printf("%s, ", mFonts.CStringAt(i)->get());
A: -    }
A: -    printf("]\nreturns ");
A: -    if (font) {
A: -      printf("%s\n", font->mName ? font->mName : "(substitute)");
A: -    }
A: -    else {
A: -      printf("NULL\n");
A: -    }
A: -  }
A: -#endif
A: -
A: -  return font;
A: -}
A: -
A: -
A: -// The Font Enumerator
A: -
A: -nsFontEnumeratorGTK::nsFontEnumeratorGTK()
A: -{
A: -  NS_INIT_ISUPPORTS();
A: -}
A: -
A: -NS_IMPL_ISUPPORTS1(nsFontEnumeratorGTK, nsIFontEnumerator)
A: -
A: -typedef struct EnumerateNodeInfo
A: -{
A: -  PRUnichar** mArray;
A: -  int         mIndex;
A: -  nsIAtom*    mLangGroup;
A: -} EnumerateNodeInfo;
A: -
A: -static PRIntn
A: -EnumerateNode(void* aElement, void* aData)
A: -{
A: -  nsFontNode* node = (nsFontNode*) aElement;
A: -  EnumerateNodeInfo* info = (EnumerateNodeInfo*) aData;
A: -  if (info->mLangGroup != gUserDefined) {
A: -    if (node->mCharSetInfo == &Unknown) {
A: -      return PR_TRUE; // continue
A: -    }
A: -    else if (info->mLangGroup != gUnicode) {
A: -      if (node->mCharSetInfo->mLangGroup != info->mLangGroup) {
A: -        return PR_TRUE; // continue
A: -      }
A: -    }
A: -    // else {
A: -    //   if (lang == add-style-field) {
A: -    //     consider it part of the lang group
A: -    //   }
A: -    //   else if (a Unicode font reports its lang group) {
A: -    //     consider it part of the lang group
A: -    //   }
A: -    //   else if (lang's ranges in list of ranges) {
A: -    //     consider it part of the lang group
A: -    //     // Note: at present we have no way to do this test but we 
A: -    //     // could in the future and this would be the place to enable
A: -    //     // to make the font show up in the preferences dialog
A: -    //   }
A: -    // }
A: -
A: -  }
A: -  PRUnichar** array = info->mArray;
A: -  int j = info->mIndex;
A: -  PRUnichar* str = ToNewUnicode(node->mName);
A: -  if (!str) {
A: -    for (j = j - 1; j >= 0; j--) {
A: -      nsMemory::Free(array[j]);
A:      }
A: -    info->mIndex = 0;
A: -    return PR_FALSE; // stop
A: -  }
A: -  array[j] = str;
A: -  info->mIndex++;
A: -
A: -  return PR_TRUE; // continue
A: -}
A: -
A: -PR_BEGIN_EXTERN_C
A: -static int
A: -CompareFontNames(const void* aArg1, const void* aArg2, void* aClosure)
A: -{
A: -  const PRUnichar* str1 = *((const PRUnichar**) aArg1);
A: -  const PRUnichar* str2 = *((const PRUnichar**) aArg2);
A: -
A: -  // XXX add nsICollation stuff
A: -
A: -  return nsCRT::strcmp(str1, str2);
A: -}
A: -PR_END_EXTERN_C
A: -
A: -static nsresult
A: -EnumFonts(nsIAtom* aLangGroup, const char* aGeneric, PRUint32* aCount,
A: -  PRUnichar*** aResult)
A: -{
A: -  nsresult res = GetAllFontNames();
A: -  if (NS_FAILED(res)) {
A: -    return res;
A: -  }
A: -
A: -  PRUnichar** array =
A: -    (PRUnichar**) nsMemory::Alloc(gGlobalList->Count() * sizeof(PRUnichar*));
A: -  if (!array) {
A: -    return NS_ERROR_OUT_OF_MEMORY;
A: -  }
A: -  EnumerateNodeInfo info = { array, 0, aLangGroup };
A: -  if (!gGlobalList->EnumerateForwards(EnumerateNode, &info)) {
A: -    nsMemory::Free(array);
A: -    return NS_ERROR_OUT_OF_MEMORY;
A: -  }
A: -
A: -  NS_QuickSort(array, info.mIndex, sizeof(PRUnichar*), CompareFontNames,
A: -               nsnull);
A: -
A: -  *aCount = info.mIndex;
A: -  if (*aCount) {
A: -    *aResult = array;
A: -  }
A: -  else {
A: -    nsMemory::Free(array);
A: -  }
A:  
A: -  return NS_OK;
A: -}
A: -
A: -NS_IMETHODIMP
A: -nsFontEnumeratorGTK::EnumerateAllFonts(PRUint32* aCount, PRUnichar*** aResult)
A: -{
A: -  NS_ENSURE_ARG_POINTER(aResult);
A: -  *aResult = nsnull;
A: -  NS_ENSURE_ARG_POINTER(aCount);
A: -  *aCount = 0;
A: + end:
A:  
A: -  return EnumFonts(nsnull, nsnull, aCount, aResult);
A: +    return cachedXftSetting;
A:  }
A:  
A: 

========================================================================

B: +/* static */
B: +PRBool
B: +IsXftEnabled(void)
B: + {
B: + 
B: +    static PRBool been_here = PR_FALSE;
B: +    static PRBool cachedXftSetting = PR_TRUE;
B: + 
B: +    if (!been_here) {
B: +        been_here = PR_TRUE;
B: +        nsCOMPtr<nsIPref> prefService;
B: +        prefService = do_GetService(NS_PREF_CONTRACTID);
B: +        if (!prefService)
B: +            return PR_TRUE;
B: + 
B: +        nsresult rv;
B: + 
B: +        rv = prefService->GetBoolPref("fonts.xft.enabled", &cachedXftSetting);
B: + 
B: +        // Yes, this makes sense.  If xft is compiled in and there's no
B: +        // pref, it's automatically enabled.  If there's no pref, check
B: +        // the environment.
B: +        if (NS_FAILED(rv)) {
B: +            char *val = PR_GetEnv("GDK_USE_XFT");
B: +
B: +            if (val && val[0] == '0') {
B: +                cachedXftSetting = PR_FALSE;
B: +                goto end;
B: +            }
B: + 
B: + 
B: + end:
B: + 
B: +
B: +    return cachedXftSetting;
B: +}
B: + 
B: +


How is this patch any more complicated than the reorganization patch, that you
insist should be a separate (and necessarily first) patch, would be?  Yes, the
"hook" code in this patch hooks into both the Core and the Xft font instance
code, which is an additional few lines of code, but it doesn't make the diff any
harder to read.  For either patch, you'd need to apply it to make sense of some
parts of it.
Why would the reorganization need to be "necessarily first"?

Regardless of which comes first there are 2 issues in this example:

 The functional change is 38 new lines but the "A:" patch is 268 lines.

 The functional change adds one new function but the "A:" patch mixes in
 7 unlreated functions, 1 typedef, and 1 XPCOM declaration.

Would the name "xftIsEnabled" be more descriptive than "cachedXftSetting"?

How does this pref work?

I see IsXftEnabled called in LoadFont, GetHints, EnumerateAllFonts, and
EnumerateFonts. In the current code these are all after the font is
already selected.

>  The functional change is 38 new lines but the "A:" patch is 268 lines.
> 
>  The functional change adds one new function but the "A:" patch mixes in
>  7 unlreated functions, 1 typedef, and 1 XPCOM declaration.
> 

It sounds to me like your complaint is with the author of patch, not with me. 
If you're really that concerned, apply this to your tree.  It's much easier to
read the code that way.

Like I've said, if you really want me to pull out the code for
nsFontInstanceXft.h/cpp, I can, but it's going to buy you exactly nothing.

> Would the name "xftIsEnabled" be more descriptive than "cachedXftSetting"?

You call IsXftEnabled, you don't check it from outside of the function. 
cachedXftSetting is an internal static to that function and is a cached value.

> 
> How does this pref work?

It checks the pref:

rv = prefService->GetBoolPref("fonts.xft.enabled", &cachedXftSetting);

If the pref doesn't exist, it checks the environment.  GDK_USE_XFT=0 turns it
off, GDK_USE_XFT=1 turns it on.  This is the same setting that all other
Xft-enabled gtk apps use.

> 
> I see IsXftEnabled called in LoadFont, GetHints, EnumerateAllFonts, and
> EnumerateFonts. In the current code these are all after the font is
> already selected.

That's fine.  This is just to select the correct code path (whether or not you
use the Core code path or the Xft code path.)
Blocks: 1.2b
davidb & blizzard:

why is it so hard to see that (at this point) bstell is only interested in the
_real_ differences, not all the fluff that comes with re-organisation?

The real differences are possible to see in a patch, if:
a) no re-organisation is done and the new xft-hooks and files are just "added"
   against current mozilla tree
b) re-organisation is separated to another patch (which is not that interesting)  
   and then the real changes can be examined in the much smaller patch.

Either a) or b) is the way I think it should have been done (I'm pretty sure in
similar situation Linus wouldn't accept anything else to his kernel-tree...).

HOWEVER, since this already has been a titanic operation and super-reviewed and
since blizzard doesn't seem to want to do either a) or b), I think it would be
best to just to go through the whole patch (either as diff or as applied to
personal tree). The point has been made already.
I see you're moving lots of stuff around.  Some of my comments will apply to
stuff you didn't actually produce as new code for this patch.  You may feel like
addressing some of these `old code' comments, others not.  Use your own
judgement.  I don't feel I caught as much as the others, the only must fixes are
the |printf|s, and possible some string optimizations.


You might want to wrap

  +#define NS_TO_GDK_RGB(ns) \
  +  (ns & 0xff) << 16 | (ns & 0xff00) | ((ns >> 16) & 0xff)
  
in an extra set of parentheses to avoid unexpected precedence problems, i.e.,

  +#define NS_TO_GDK_RGB(ns) \
  +  ((ns & 0xff) << 16 | (ns & 0xff00) | ((ns >> 16) & 0xff))
  


  +// *wretch* I love it when classes contain elements that are
  +// superclasses

I was slightly confused by this comment, since |nsFontGTK| doesn't actually
contain a |nsFontGTKUserDefined|; rather it has a pointer to one.


  [[comments about nsFontInstanceCore.cpp, .h elided]]

  [[my usual babbling about ++i vs i++]]


wrt |nsFontInstanceXft|, T::T() mX(0) {} is preferred over T::T() { mx = 0; },
though for Plain-Old-Data members (that is, in this case, members that have no
constructor of their own) this might be considered only a style issue


  +    if (aLength == 0) {

  +    if (nsnull != aFontID)


  if ( !aLength )
  
  if ( !aFontID )


  +nsresult
  +nsFontInstanceXft::DrawString(const char *aString, PRUint32 aLength,
  +                              nscoord aX, nscoord aY,
  +                              const nscoord* aSpacing,
  +                              nsRenderingContextGTK *aContext,
  +                              nsDrawingSurfaceGTK *aSurface)
  +{
  +    if (aSpacing) {
  +        printf("XXX DrawString(8) spacing!\n");
  +    }
  +

Yikes!  What's a |printf| doing in there?  Maybe this was in


  +        specBuffer =
  +            (XftCharFontSpec *)nsMemory::Alloc(sizeof(XftCharFontSpec) *
  +                                               aLength);

C-style casts always compile, even when you don't want them to.  Using modern
casts through our macros catches errors and saves lives.  Here you probably want

  NS_STATIC_CAST(XftCharFontSpec*, nsMemory::Alloc(...))


hmmm, more |printf|s


examining the |goto|s and |continue| in the big loop in
|nsFontInstanceXft::GetTextDimensions|, (and several other close to identical
loops) I feel like I want to say something... but a convincingly better solution
is not immediately apparent to me



A lot of these getters and setters look like they would have been good
candidates for inlining.  They're probably all virtual, though, huh.  Still,
does the pattern in which they are used lend itself to inlining?  That is, where
they are called, could the compiler know for certain that the static type of the
object was an |nsFontInstanceXft|?  If so, they could be both inline and
virtual, perhaps for a speed win.


  +        nsCString name;
  +        name += "font.name.";
  +        name += mFontMetrics->mGenericFont->get();
  +        name += ".";
  +
  +        nsString langGroup;
  +        mFontMetrics->mLangGroup->ToString(langGroup);
  +

prefer

  nsString langGroup;
  mFontMetrics->mLangGroup->ToString(langGroup);

  nsCString name = NS_LITERAL_CSTRING("font.name.")
                 + mFontMetrics->mGenericFont->get()
                 + NS_LITERAL_CSTRING(".")
                 + NS_ConvertUCS2toUTF8(langGroup);


more old-style casts, maybe a search for /\*\)/ will help find them all

extremely minor nit: inconsistent forms of labels

  goto end;
  goto loser;
  goto FoundFont;


|nsFontInstanceXft::DrawUnknownGlyph| --- magic numbers 5, 3, and maybe 2
4 is OK, I see the // Draw the four characters
comments for the other numbers would help


hmmm, more |printf|s


In |NS_FamilyExistsXft|, your work with |name| may be way too high in the
function.  I guess if you _almost_always_ expect the search to find a match,
it's OK to do this work at the top.  But if not, you may want to defer this till
you know you need it.  Perhaps you could even avoid it by using a case
insensitive comparison, e.g.,

  if ( !Compare(nsDependentCString(tmpname), name,
nsCaseInsensitiveCStringComparator) )

If you expect to hit this comparison only once, use the above.  Else stick with
your version, but in any case, I probably would have declared |name| as

  NS_ConvertUCS2toUTF8 name(aName);

(if I recall the name correctly) since that is a kind of |nsAutoCString| and
saves a heap allocation.  And saved that conversion and the down-casing till the
point when I new I wasn't going to jump past its use.

alright ... well, that's the stuff that caught my attention.

Hope this helps

Comment on attachment 101942 [details]
patch

r=scc
Attachment #101942 - Flags: review+
> > I see IsXftEnabled called in LoadFont, GetHints, EnumerateAllFonts, and
> > EnumerateFonts. In the current code these are all after the font is
> > already selected.
> 
> That's fine.  This is just to select the correct code path (whether or not
> you use the Core code path or the Xft code path.)

Where does the decision to use / not use Xft happen?

GetHints doesn't seem like the place.

EnumerateAllFonts and EnumerateFonts are (at least were) used when
generating the list of fonts for displaying in the font prefs dialog.

LoadFont used to happen after the font had been selected but before it was
used for display. This point seems too late to control the font selection.

rbs: FYI, as I understand it when Xft is enabled MathML be disabled.
There is a pref to disable Xft but if the MOZ_ENABLE_COREXFONTS is not
set then it seems unlikely that it would have any effect.

blizzard: will redhat have MOZ_ENABLE_COREXFONTS enabled or disabled?


>   +#define NS_TO_GDK_RGB(ns) \
>   +  (ns & 0xff) << 16 | (ns & 0xff00) | ((ns >> 16) & 0xff)
>   
> in an extra set of parentheses to avoid unexpected precedence problems, i.e.,

Fixed.

> wrt |nsFontInstanceXft|, T::T() mX(0) {} is preferred over T::T() { mx = 0; },
> though for Plain-Old-Data members (that is, in this case, members that have no
> constructor of their own) this might be considered only a style issue

Are you sure you're looking at the right version of the patch?  The most recent
version in the bug has:

nsFontInstanceXft::nsFontInstanceXft() :
    mFontMetrics(nsnull),
    mWesternFont(nsnull),
    [...]

or are you talking about something else?

>   +    if (aLength == 0) {
> 
>   +    if (nsnull != aFontID)
> 
> 
>   if ( !aLength )
>   
>   if ( !aFontID )

Fixed.

> 
> 
>   +nsresult
>   +nsFontInstanceXft::DrawString(const char *aString, PRUint32 aLength,
>   +                              nscoord aX, nscoord aY,
>   +                              const nscoord* aSpacing,
>   +                              nsRenderingContextGTK *aContext,
>   +                              nsDrawingSurfaceGTK *aSurface)
>   +{
>   +    if (aSpacing) {
>   +        printf("XXX DrawString(8) spacing!\n");
>   +    }
>   +
> 
> Yikes!  What's a |printf| doing in there?  Maybe this was in

I'm pretty sure you're looking at an older version of the patch.  This has
already been removed.

>   +        specBuffer =
>   +            (XftCharFontSpec *)nsMemory::Alloc(sizeof(XftCharFontSpec) *
>   +                                               aLength);
> 
> C-style casts always compile, even when you don't want them to.  Using modern
> casts through our macros catches errors and saves lives.  Here you probably want
> 
>   NS_STATIC_CAST(XftCharFontSpec*, nsMemory::Alloc(...))

This bit of code isn't in the patch anymore, but I updated the other
nsMemory::Alloc() in the patch.

> 
> 
> hmmm, more |printf|s

I think you're looking at an old version.  I had a couple of commented out
|printf|s that I just removed.

> examining the |goto|s and |continue| in the big loop in
> |nsFontInstanceXft::GetTextDimensions|, (and several other close to identical
> loops) I feel like I want to say something... but a convincingly better solution
> is not immediately apparent to me
> 

Yeah.  It's a well tested algorithm and reasonably fast and I'd rather not touch
it at this point.  I hope you don't have a visceral and unreasonable hatred of
gotos. :)

> A lot of these getters and setters look like they would have been good
> candidates for inlining.  They're probably all virtual, though, huh.  Still,
> does the pattern in which they are used lend itself to inlining?  That is, where
> they are called, could the compiler know for certain that the static type of the
> object was an |nsFontInstanceXft|?  If so, they could be both inline and
> virtual, perhaps for a speed win.

I think the most recent patches has the getters you're looking at all as
non-virtual inlines.  I'm not sure, though.

> 
> 
>   +        nsCString name;
>   +        name += "font.name.";
>   +        name += mFontMetrics->mGenericFont->get();
>   +        name += ".";
>   +
>   +        nsString langGroup;
>   +        mFontMetrics->mLangGroup->ToString(langGroup);
>   +
> 
> prefer
> 
>   nsString langGroup;
>   mFontMetrics->mLangGroup->ToString(langGroup);
> 
>   nsCString name = NS_LITERAL_CSTRING("font.name.")
>                  + mFontMetrics->mGenericFont->get()
>                  + NS_LITERAL_CSTRING(".")
>                  + NS_ConvertUCS2toUTF8(langGroup);

That doesn't even come close to compiling.

nsFontInstanceXft.cpp:849: no match for `nsDependentCString + const char*' 
   operator
../../../dist/include/string/nsDependentConcatenation.h:219: candidates are: 
   const nsDependentConcatenation operator+(const nsDependentConcatenation&, 
   const nsAString&)
../../../dist/include/string/nsDependentConcatenation.h:226:                 
   const nsDependentCConcatenation operator+(const nsDependentCConcatenation&, 
   const nsACString&)
../../../dist/include/string/nsDependentConcatenation.h:233:                 
   const nsDependentConcatenation operator+(const nsAString&, const nsAString&)
../../../dist/include/string/nsDependentConcatenation.h:240:                 
   const nsDependentCConcatenation operator+(const nsACString&, const 
   nsACString&)

> extremely minor nit: inconsistent forms of labels
> 
>   goto end;

Normal exit.

>   goto loser;

Loser exit (usually memory allocation failure.)

>   goto FoundFont;

History. :)

> |nsFontInstanceXft::DrawUnknownGlyph| --- magic numbers 5, 3, and maybe 2
> 4 is OK, I see the // Draw the four characters
> comments for the other numbers would help
> 

I added some comments there.

> 
> hmmm, more |printf|s

I don't think that I have any left in the file now...

> In |NS_FamilyExistsXft|, your work with |name| may be way too high in the
> function.  I guess if you _almost_always_ expect the search to find a match,
> it's OK to do this work at the top.  But if not, you may want to defer this till
> you know you need it.  Perhaps you could even avoid it by using a case
> insensitive comparison, e.g.,
>
>   if ( !Compare(nsDependentCString(tmpname), name,
> nsCaseInsensitiveCStringComparator) )

Close, but fixed. :)

> If you expect to hit this comparison only once, use the above.  Else stick with
> your version, but in any case, I probably would have declared |name| as
> 
>   NS_ConvertUCS2toUTF8 name(aName);
> 
> (if I recall the name correctly) since that is a kind of |nsAutoCString| and
> saves a heap allocation.  And saved that conversion and the down-casing till the
> point when I new I wasn't going to jump past its use.

Fixed.
> why is it so hard to see that (at this point) bstell is only interested in the
> _real_ differences, not all the fluff that comes with re-organisation?
> 

OK.  The bulk of this patch _is_ the reorganization.  If you don't want to see
the Xft bits, ignore nsFontInstanceXft.*.  There's a pretty clear seperation,
here, and it's easy to seperate the parts.  patch is messy, but new files are
clearly defined in the patch.

> Where does the decision to use / not use Xft happen?
> 
> GetHints doesn't seem like the place.

Are you wondering why IsXftEnabled() is called from the GetHints() call?  The
answer is pretty simple.  GetHints() is sometimes called in the rendering
context before the font is set on that rendering context, so you need to call
into a global symbol instead of into any specific font instance.  I didn't feel
like trying to fix that in this run since it would further obscure the changes
in question.

> EnumerateAllFonts and EnumerateFonts are (at least were) used when
> generating the list of fonts for displaying in the font prefs dialog.

Yep, I know that.

> 
> LoadFont used to happen after the font had been selected but before it was
> used for display. This point seems too late to control the font selection.

LoadFont() in nsFontMetricsGTK is called after the font information is stored in
the font metrics class.  Once you save that information, you need to know
whether or not you need to create an Xft object or a Core object as the font
instance for that metrics object.  That's why it's called from there.

> rbs: FYI, as I understand it when Xft is enabled MathML be disabled.
> There is a pref to disable Xft but if the MOZ_ENABLE_COREXFONTS is not
> set then it seems unlikely that it would have any effect.

Only because I haven't added support for it yet.

> 
> blizzard: will redhat have MOZ_ENABLE_COREXFONTS enabled or disabled?

I don't expect to disable it for quite a while.  We're talking years here, not
months.  Certainly not for the 8.x release(s), anyway.  It depends on how long
it takes the Xft code in Mozilla to come up to spec and when Red Hat and XFree86
decide to drop support for core fonts, as they exist today.*

* We will probably see a drop in replacement for the Xlib fonts apis that will
use fontconfig under the covers and won't use any of the old X font paths. 
That's just rumor, though.  We'll see how it goes.
Attached file patch (obsolete) — 
Patch after scc's comments.
Attachment #101942 - Attachment is obsolete: true
Comment on attachment 102108 [details]
patch

carrying forward reviews
Attachment #102108 - Flags: superreview+
Attachment #102108 - Flags: review+
> * We will probably see a drop in replacement for the Xlib fonts apis that will
> use fontconfig under the covers and won't use any of the old X font paths. 
> That's just rumor, though.  We'll see how it goes.

I can't imagine admiting a patch into XFree86 which would change the Xlib APIs
or the protocol generated thereby.  Compatibility with legacy applications and X
servers can best be served by not disturbing the foundations they rest upon.

I do plan to eliminate the current bitmap font file format in favor of TrueType
files which include only bitmaps and no outlines.  Early experiments with that
change see some significant space savings over the compressed .pcf files as we
can store multiple sizes and encodings in the same file.  This
change would also permit Xft applications to share these bitmap fonts without
needing to provide full support for the existing .pcf format in FreeType2.

Another likely project is to permit the X server to use fontconfig to discover
available fonts as that would eliminate yet another custom font configuration
mechanism.  I haven't figured out exactly how that would work as XLFD names and
properties contain some data not provided by TrueType fonts.

> I don't expect to disable it for quite a while.  We're talking years here, not
> months.  Certainly not for the 8.x release(s), anyway.  It depends on how long
> it takes the Xft code in Mozilla to come up to spec and when Red Hat and XFree86
> decide to drop support for core fonts, as they exist today.*

X is a network protocol with semantics which include the existing core fonts. 
Changing Xlib would have no effect on the requirements within the X server, and
so would not reduce the ongoing support burden for XFree86.  However, migrating
the server to build upon FreeType2 and fontconfig instead of custom libraries
will, while at the same time preserving required functionality.
rbs@maths.uq.edu.au wrote:
> Something this large has usually been an early milestone material. So it is
> probably more realistic to target the upcoming one where less restricted
> checkins would allow rapid resolution of issues that my arise, thereby 
> allowing people to rejoice with a more stable support when the target 
> milestone is released.

I agree here with rbs. IMHO we should wait until the 1.2beta freeze is over
before checking the patch "in". The patch should be able to cook at _least_ a
month in "trunk" before we should ship a "final" milestone with that - we're
having enougth problem reports in this bug which indicates that there is still
lots of work to-do...

... I'll take a closer look at the patch tomorrow evening (MET; I am now too
tired to do a usefull review tonight), doing quick look shows (at least) lots of
small nits which will turn half Seamonkey-Ports into flames... ;-(
The problem reports in this bug are almost entirely from those who are enabling
the Xft code, and it will not be enabled by default.  (Most of the problems have
been fixed already, too.)  I thus still stand by my comment 258.
David Baron wrote:
> The problem reports in this bug are almost entirely from those who are 
> enabling the Xft code, and it will not be enabled by default. 
> (Most of the problems have been fixed already, too.) 
> I thus still stand by my comment 258.

Again, I don't think this is a wise idea to ship a "final" milestone with an
all-new, unfinished code. For example - did anyone do tests with Rational Purify
yet and check for problems ? The current code in "trunk" has been extensively
tested since _years_ and is known to work without problems - and doing the
testing+fixing with the requirement of an a= for each single patch would be a
pain.
Without problems?  As I already mentioned, the current code actually works less
well for me than does this new code, thanks to my using Xinerama.  For a project
that professes to be so concerned with stability, bug 136362 hasn't had so much
as an acknowledgement in 6 months.   (yet another blatant plug to wave font
developers in that direction :)

>> Mox:
>> why is it so hard to see that (at this point) bstell is only interested in 
>> the _real_ differences, not all the fluff that comes with re-organisation?
>> 
>Blizzard:
>OK.  The bulk of this patch _is_ the reorganization.  If you don't want to see
>the Xft bits, ignore nsFontInstanceXft.*.  There's a pretty clear seperation,
>here, and it's easy to seperate the parts.  patch is messy, but new files are
>clearly defined in the patch.

Well, the interesting parts are the actual changes to the old ("corefonts")
code. I.e. the stuff that changes the code in current tree. The re-organisation
of that code is not interesting, and it effectively hides the actual changes.

This patching issue is very similar to indentation; although one could argue
that amounts of whitespaces are irrelevant (no compile or runtime differences),
it does improve code readability and is thus generally part of CodingStyle.

Similarly, keeping re-organisation and (functional) code changes separate
improves patch readability, but apparently that rule is not part of mozilla
Coding/PatchingStyle.

The milestones raised yet another reason for PatchingStyle. It's very
understandable people don't want to have 14k lines patch to the upcoming moz1.2
for the fear of regressions (and not that start of 1.3alpha is so far
anyways...). If you had just the small patch making the changes to the old code
(either before or after reorganization), it would be very easy to convince that
the impact is minimal. With a single large patch the reviewers have to do either
huge amount of review work or have faith in you when you say "really, almost
nothing has changed."
Just a couple of anecdotal comments, and a possible bug report:

This all refers to the 08Oct2002 rh8 build, under rh8.

1) First, this looks great.  Even looks tons better than the old Xft patches
from 0.9.9/1.0rc days.  Definately in the same league as Mozilla with Quartz
rendering on OSX.  

2) Generally, it has worked very well for me, relatively stable, good choices
for fonts, etc (but I'm no artist).  Printing has also worked very well, on the
2 documents I printed.  They both came out laid-out well, with crisp fonts, etc.

3) Some things are definately slower.  I haven't done any benchmarks on page
load times, but they might be a bit slower.  Doesn't bother me, though.  

4) The one place where there is an annoyance related to speed (I think) is if
you hit (and hold) the down-arrow in a long document, the scrolling can't keep
up with the keyboard repeat-rate, so you always end up overshooting.  Maybe
there's some tuning that can be done here to ignore scrolling requests while the
browser is working on a current scroll request -- the overshoot is very annoying
(and oddly, it doesn't happen on _this_ page).

5) I've gotten some odd crashes, which I don't seem to be able to repeat, but
just a few minutes ago, here's what I got in stdout or err:

Warning:
    Name: HorScrollBar
    Class: XmScrollBar
    The specified scrollbar value is greater than the maximum
    scrollbar value minus the scrollbar slider size.

Gdk-ERROR **: BadLength (poly request too large or internal Xlib length erro
  serial 1382940 error_code 16 request_code 76 minor_code 0
Gdk-ERROR **: BadRequest (invalid request code or no such operation)
  serial 1382941 error_code 1 request_code 0 minor_code 0

I got this while mouse-wheel scrolling in a bugzilla page.
Attached file patch (obsolete) — 
Re-merge with the tip after changes last night.
Attachment #102108 - Attachment is obsolete: true
Comment on attachment 102206 [details]
patch

Carrying forward reviews.
Attachment #102206 - Flags: superreview+
Attachment #102206 - Flags: review+
Attached file patch (obsolete) — 
Re-merge with the tip.	Again.
Attachment #102206 - Attachment is obsolete: true
Comment on attachment 102273 [details]
patch

...
Attachment #102273 - Flags: superreview+
Attachment #102273 - Flags: review+
OK, I have read the patch and see the motivation of what the patch is doing
(comment 125 and comment 164). Other reviewers have caught most of the edgy
stuff, but there is remaining architectural point which might greatly simplify
the patch (and ease maintainance of both codes and the the final switch over to
Xft). 

> ------- Additional Comment #258 From David Baron  2002-10-07 08:52 -------
>
> > In the meantime, s/nsFontInstance/nsFontMetrics/g would make the patch
> > stay closer to its memory lane.
>
> Would you really want to give two different layers the same name?

That's because of the approach of the patch. I think there might be a simpler
(and maybe less contentious approach). It would avoid the need for
nsFontInstanceFactory, etc.

There can be a dispatch mechanism in gfx/src/gtk/nsGfxFactoryGTK.cpp.
With that, it would be possible to setup nsFontMetricsXftGTK (or some other
name), and override the default behavior in nsGfxFactoryGTK.cpp, e.g., by doing:

// objects that just require generic constructors

+//override the generic constructor
+//NS_GENERIC_FACTORY_CONSTRUCTOR(nsFontMetricsGTK)

static nsresult
nsFontMetricsGTKConstructor(nsISupports* aOuter, REFNSIID aIID, void** aResult)
{
  *aResult = nsnull;
  if (aOuter)
    return NS_ERROR_NO_AGGREGATION;
  result = IsXftEnabled() ? new nsXftFontMetricsGTK() : new nsFontMetricsGTK();
  if (!result)
    return NS_ERROR_OUT_OF_MEMORY;

  NS_ADDREF(result);
  nsresult rv = result->QueryInterface(aIID, aResult);
  NS_RELEASE(result);
  return rv;
}

This would allow getting Xft on demand, while retaining nsFontMetricsGTK pretty
much as it is now (modulo some cleanup to ease the Xft integration). It seems to
me that this might appeal to all parties involved since the Xft codepath would
have a life of its own, without the need for a big moving patch for the old code.
Just naming nsFontMetricsXft is OK. s/ns[Xft]FontMetrics[Xft]Gtk/nsFontMetricsXft/g
Such a design wouldn't allow the code to be changed so that a failure of Xft to
find a font would allow falling back to the Core font code.  Although the
current code doesn't do that, I *think* blizzard wants it to be able to do that
(I remember reading a comment somewhere saying that it should do that).  If
things were done that way, there wouldn't be any point to distributions building
the core code once Xft was enabled, would there?  (unless they wanted their
users to be able to use the pref or environment variable)

Is that an important goal?
> This would allow getting Xft on demand, while retaining nsFontMetricsGTK pretty
> much as it is now (modulo some cleanup to ease the Xft integration). It seems to
> me that this might appeal to all parties involved since the Xft codepath would
> have a life of its own, without the need for a big moving patch for the old code.
> 

I was trying my best to avoid a total fork of the code.  There's still a little
bit of code that is shared - some of the attributes, etc.

> Such a design wouldn't allow the code to be changed so that a failure of Xft to
> find a font would allow falling back to the Core font code.

No, that wasn't really one of my design goals.  fontconfig almost always returns
some font.
Falling back to core fonts if a fontconfig font is not available is 
highly undesirable - it would not produce consistent behavior with 
the other applications on the operating system, and would mean that 
/etc/fonts/fonts.conf did not 100% control the font configuration, producing
weirdness.

If we have a problem in fontconfig configuration, we want to know about it, 
we don't want to start loading a big hunk o' legacy code and 
getting ugly fonts.

So basically mozilla should pick a font backend on startup and stick to it.
In fontconfig mode we want fontconfig fonts _only_.
> > Such a design wouldn't allow the code to be changed so that a failure of Xft
> > to find a font would allow falling back to the Core font code.
>
> No, that wasn't really one of my design goals.  fontconfig almost always
> returns some font.
[...]
> If we have a problem in fontconfig configuration, we want to know about it, 
> we don't want to start loading a big hunk o' legacy code and 
> getting ugly fonts.
>
> So basically mozilla should pick a font backend on startup and stick to it.
> In fontconfig mode we want fontconfig fonts _only_.

Indeed, that's how I understood (and saw) it. Since Xft and the legacy code are
meant to be mutually exclusive, you want to mess with it as little as possible.
If there are too much entanglements between the two, you will have to maintain
both rather than concentrating on improving the Xft code. With the proposed
design, the old code may phase out (if deemed necessary when Xft is in parity
with the features currently offered by the legacy code - comment 82).
If its true that this patch won't affect Mozilla until the end user takes action
to enable xft, I think it would be really desireable to see it go in for
1.2beta.  Its perfect timing with Redhat 8.0's release for early adopter types
to play with and shake out any bugs, and if it goes well, allowing a little
blurb in 1.2final's release notes about how to enable this for those 8.0 users
and others willing to install the correct bits.

If this is felt to be "too risky" for inclusion during the 1.2beta freeze, it
seems unlikely it'll be included until after 1.2 branches.
Not everybody (myself included) is convinced that the patch is ready yet in its
present form. An alternative way of moving the Xft integration forward has been
made. Please do not flood the bug with further advocacy comments. It makes
wading through the bug extremely difficult. Everytime I click, I seem to wait
forever to get the page. It has already been made clear that the Xft support is
important and should be integrated sooner or later.
Quick bug report against the current (Oct 8) rh8 build: the browser
chooses correct defaults when started without a user configuration,
but the font properties dialog fails to recognize these.  Instead, it
shows the first Xft font found (In my case, a nasty cursive font
called Amaze) in each of the combo boxes.

Additionally, the Xft default font names (serif, mono, sans, I
believe) fail to appear in the dialogs.  Since these are intended to
be settable via the user's theme, they really should be choosable via
browser configuration.  I'm fuzzy on the details here...
I'm working on an alternative patch.
> Additionally, the Xft default font names (serif, mono, sans, I
> believe) fail to appear in the dialogs.

For the current code, the fonts on the dialog are queried from
nsFontEnumeratorGTK (currently implemented in nsFontMetricsGTK). So the Xft
integration would need its own nsFontEnumeratorXft that will talk with the Xft
engine to figure out what to say for the Fonts preference dialog, etc. But that
can wait. Anyway, there is some way to go before matching the legacy code. Only
the hooks are important at this stage.
Attached patch patch (obsolete) — — Splinter Review 
Patch that implements rbs' suggestion.	Lots less churn here.  Seems to work
really well here.
Attachment #102273 - Attachment is obsolete: true
Comment on attachment 102418 [details] [diff] [review]
patch

Looking much neater and less scary indeed. Some code-review comments:

+PRBool
+nsFontMetricsGTK::GetHints(void)
+{
+  PRUint32 result = 0;
[...]
+  return result;
 }

Should be |PRUint32| GetHints(void).

+// Class nsFontXft is an actual instance of a font.  The
+// 'nsFontInstance' class is made up of a collection of these little
+// fonts, really.

Outdated comments now that nsFontInstance is gone.

+struct MozXftLangGroup {
+static const MozXftLangGroup MozXftLangGroups[] = {

Some places inside these structs have a weird indentation.

 nsFontMetricsGTK::nsFontMetricsGTK()
-  : mFonts() // I'm not sure what the common size is here - I generally
+  : mFonts(), mCurrentFont(nsnull) // I'm not sure what the common
[...]
+nsFontMetricsXft::nsFontMetricsXft() : mDeviceContext(nsnull),
+					mGenericFont(nsnull),
+					mFont(nsnull),
+					mPixelSize(0),
[...]

These explicit initialization to zero aren't needed since the classes
are using the much cleaner NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW which
will take care of zeroing the whole object at its creation.

+nsFontMetricsXft::~nsFontMetricsXft()
+{
[...]
+	 if (mPattern)
+	 FcPatternDestroy(mPattern);

Another weird indentation.

 NS_IMETHODIMP	nsFontMetricsGTK::GetFontHandle(nsFontHandle &aHandle)
 {
-  aHandle = (nsFontHandle)mWesternFont;
+  aHandle = (nsFontHandle)(nsIFontMetricsGTK *)this;
   return NS_OK;
 }
[...]
+NS_IMETHODIMP
+nsFontMetricsXft::GetFontHandle(nsFontHandle &aHandle)
+{
+    aHandle = (nsFontHandle)(nsIFontMetricsGTK *)this;
+    return NS_OK;
+}
[...]
-  nsFontGTK		 *mCurrentFont;
+  nsIFontMetricsGTK	 *mFontHandle;

I think these are incorrect. You want to return the _native_ handle of the 
mWesternFont here (nsFonGTK or nsFonXft as in the old code shown above).
If somebody wanted the "this" -- they would have done the cast themselves
straightaway rather than using this_font_metrics->GetFontHandle() to re-get 
this_font_metrics).

With that...
+NS_IMETHODIMP
+nsRenderingContextGTK::GetTextDimensions(const PRUnichar* aString,
+					  PRUint32 aLength,
+					  nsTextDimensions& aDimensions, 
+					  PRInt32* aFontID)
+{
+  return mFontHandle->GetTextDimensions(aString, aLength, aDimensions,
+					 aFontID, this);
 }
becomes...
+  return mFontMetrics->GetTextDimensions(), etc. Since mFontMetrics
is nsIFontMetricsGTK by design.

(Note: the 'true' font hande itself, i.e., mWesternFont, is used somewhere
higher up the UI for certain shortcuts, c.f. nsWidget::SetFont. I think
this old design should be cleaned up since it doesn't seem to be used
anymore, but that's another story for another time...).

+nsresult
+nsFontMetricsXft::DrawString(const char *aString, PRUint32 aLength,
+			      nscoord aX, nscoord aY,
+			      const nscoord* aSpacing,
+			      nsRenderingContextGTK *aContext,
+			      nsDrawingSurfaceGTK *aSurface)
+{
+    // Set our color
+    XftColor color;
+    nscolor rccolor;
+
+    aContext->GetColor(rccolor);
+
+    color.pixel = gdk_rgb_xpixel_from_rgb(NS_TO_GDK_RGB(rccolor));
+    color.color.red = (NS_GET_R(rccolor) << 8) | NS_GET_R(rccolor);
+    color.color.green = (NS_GET_G(rccolor) << 8) | NS_GET_G(rccolor);
+    color.color.blue = (NS_GET_B(rccolor) << 8) | NS_GET_B(rccolor);
+    color.color.alpha = 0xffff;
+
+    XftDraw *draw = aSurface->GetXftDraw();
+
+    nsCOMPtr<nsIRegion> lastRegion;
+    nsCOMPtr<nsIRegion> clipRegion;
+
+    aSurface->GetLastXftClip(getter_AddRefs(lastRegion));
+    aContext->GetClipRegion(getter_AddRefs(clipRegion));
+
+    // avoid setting the clip, if possible
+    if (!lastRegion || !clipRegion || !lastRegion->IsEqual(*clipRegion)) {
+	 aSurface->SetLastXftClip(clipRegion);
+
+	 GdkRegion *rgn = nsnull;
+	 clipRegion->GetNativeRegion((void *&)rgn);
+
+#ifdef MOZ_WIDGET_GTK
+	 GdkRegionPrivate  *priv = (GdkRegionPrivate *)rgn;
+	 XftDrawSetClip(draw, priv->xregion);
+#endif
+
+#ifdef MOZ_WIDGET_GTK2
+	 GdkRegionSetXftClip(rgn, draw);
+#endif
+    }
+
[...]
+    return NS_OK;
+}

====================

+nsresult
+nsFontMetricsXft::DrawString(const PRUnichar* aString, PRUint32 aLength,
+			      nscoord aX, nscoord aY,
+			      PRInt32 aFontID,
+			      const nscoord* aSpacing,
+			      nsRenderingContextGTK *aContext,
+			      nsDrawingSurfaceGTK *aSurface)
+{
+    // Set our color
+    XftColor color;
+    nscolor rccolor;
+
+    aContext->GetColor(rccolor);
+
+    color.pixel = gdk_rgb_xpixel_from_rgb(NS_TO_GDK_RGB(rccolor));
+    color.color.red = (NS_GET_R(rccolor) << 8) | NS_GET_R(rccolor);
+    color.color.green = (NS_GET_G(rccolor) << 8) | NS_GET_G(rccolor);
+    color.color.blue = (NS_GET_B(rccolor) << 8) | NS_GET_B(rccolor);
+    color.color.alpha = 0xffff;
+
+    XftDraw *draw = aSurface->GetXftDraw();
+
+    nsCOMPtr<nsIRegion> lastRegion;
+    nsCOMPtr<nsIRegion> clipRegion;
+
+    aSurface->GetLastXftClip(getter_AddRefs(lastRegion));
+    aContext->GetClipRegion(getter_AddRefs(clipRegion));
+
+    // avoid setting the clip, if possible
+    if (!lastRegion || !clipRegion || !lastRegion->IsEqual(*clipRegion)) {
+	 aSurface->SetLastXftClip(clipRegion);
+
+	 GdkRegion *rgn = nsnull;
+	 clipRegion->GetNativeRegion((void *&)rgn);
+
+#ifdef MOZ_WIDGET_GTK
+	 GdkRegionPrivate  *priv = (GdkRegionPrivate *)rgn;
+	 XftDrawSetClip(draw, priv->xregion);
+#endif
+
+#ifdef MOZ_WIDGET_GTK2
+	 GdkRegionSetXftClip(rgn, draw);
+#endif
+    }
+
[...]
+    return NS_OK;
+}

====================

Common big chunk of code. Need a re-usable helper function, e.g.,
nsFontMetricsXft::PrepareToDraw(aContext, aSurface).
Index: gfx/src/gtk/nsFontMetricsUtils.h
===================================================================
+#ifndef __nsIFontMetricsUtils_h
+#define __nsIFontMetricsUtils_h
[...]
+#endif /* __nsIFontMetricsUtils_h */

s/nsIFontMetricsUtils_h/nsFontMetricsUtils_h/g
   ^^^
Just one note:

-EXTRA_DSO_LDOPTS += $(MOZ_GTK_LDFLAGS) $(MOZ_GTK2_LIBS)
-CXXFLAGS	+= $(MOZ_GTK_CFLAGS) $(MOZ_GTK2_CFLAGS)
-CFLAGS		+= $(MOZ_GTK_CFLAGS) $(MOZ_GTK2_CFLAGS)
+EXTRA_DSO_LDOPTS += $(MOZ_GTK_LDFLAGS) $(MOZ_GTK2_LIBS) $(MOZ_XFT_LIBS)
+CXXFLAGS	+= $(MOZ_GTK_CFLAGS) $(MOZ_GTK2_CFLAGS) $(MOZ_XFT_CFLAGS)
+CFLAGS		+= $(MOZ_GTK_CFLAGS) $(MOZ_GTK2_CFLAGS) $(MOZ_XFT_CFLAGS)
 
WHen you do this, it is breaking on my machine on the compile

since GTK_CFLAGS are first, -I/usr/X11R6/include is first, with
-I/usr/include/Xft2 to be later

This is causing the build to find the xft1 headers in XF4.2 before the new XFt2
headers. If you put MOZ_XFT_CFLAGS first, this won't happen.


> Should be |PRUint32| GetHints(void).

Fixed.

> +struct MozXftLangGroup {
> +static const MozXftLangGroup MozXftLangGroups[] = {
> 
> Some places inside these structs have a weird indentation.

I'm not sure what you're talking about here.  They look OK here.

> These explicit initialization to zero aren't needed since the classes
> are using the much cleaner NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW which
> will take care of zeroing the whole object at its creation.

Fixed.

> +	 FcPatternDestroy(mPattern);
> 
> Another weird indentation.
> 

Cut-n-paste.  Fixed.

> I think these are incorrect. You want to return the _native_ handle of the 
> mWesternFont here (nsFonGTK or nsFonXft as in the old code shown above).
> If somebody wanted the "this" -- they would have done the cast themselves
> straightaway rather than using this_font_metrics->GetFontHandle() to re-get 
> this_font_metrics).
> 
> With that...
> +NS_IMETHODIMP
> +nsRenderingContextGTK::GetTextDimensions(const PRUnichar* aString,
> +					  PRUint32 aLength,
> +					  nsTextDimensions& aDimensions, 
> +					  PRInt32* aFontID)
> +{
> +  return mFontHandle->GetTextDimensions(aString, aLength, aDimensions,
> +					 aFontID, this);
>  }
> becomes...
> +  return mFontMetrics->GetTextDimensions(), etc. Since mFontMetrics
> is nsIFontMetricsGTK by design.

Actually, mFontMetrics is an nsIFontMetrics pointer, not an nsIFontMetricsGTK
object.

> (Note: the 'true' font hande itself, i.e., mWesternFont, is used somewhere
> higher up the UI for certain shortcuts, c.f. nsWidget::SetFont. I think
> this old design should be cleaned up since it doesn't seem to be used
> anymore, but that's another story for another time...).

Yeah, I removed that bit of code from nsWidget.cpp since it's not used anywhere
in the entire tree.

I'd rather not allow nsFontGtk or nsFontXft get passed back through
GetFontHandle() since that can add all kinds of nasty dependencies throughout
calling code.  Instead, I'd rather stick with the interface defined in
nsFontInstanceGTK.  This is why I moved all of those functions into
nsFontMetricsGTK, anyway.  Otherwise I could have just left them out and
peppered them with ifdefs.  That starts to get _really_ ugly, though.  This is
much cleaner and easier to understand.

I should really rename nsIFontMetricsGTK to something a little more different
than nsIFontMetrics and nsFontMetricsGTK.  Maybe I can use nsIFontHandleGTK! :)

> 
> Common big chunk of code. Need a re-usable helper function, e.g.,
> nsFontMetricsXft::PrepareToDraw(aContext, aSurface).

Fixed.

> 
> s/nsIFontMetricsUtils_h/nsFontMetricsUtils_h/g
>    ^^^

Fixed.

> This is causing the build to find the xft1 headers in XF4.2 before the new XFt2
> headers. If you put MOZ_XFT_CFLAGS first, this won't happen.

Oh.  That problem.  I changed it.  (Of course, RH8 only has one Xft.h on the
whole system.)
Attached patch patch (obsolete) — — Splinter Review 
Lucky number 16.
Attachment #102418 - Attachment is obsolete: true
Comment on attachment 102460 [details] [diff] [review]
patch

A few nits:

>Index: gfx/src/gtk/nsFontMetricsXft.cpp
>+    // hang onto the pref service

You're not really hanging on to it, and there's no need for the comment anyway.

>Index: gfx/src/gtk/nsRenderingContextGTK.h
>+  // Get the pointer to twip conversion for this rendering context

*Pixels* to twips.

I didn't look very closely at the stuff that looked the same as previous
versions of the patch, but the new stuff looks fine.  sr=dbaron still stands.
Attachment #102460 - Flags: superreview+
Those are fixed in my local tree.  Not worth uploading a whole patch for it. 
I'll see what scc and rbs come back with first.
Comment on attachment 102460 [details] [diff] [review]
patch

 nsFontMetricsGTK::nsFontMetricsGTK()
-  : mFonts() // I'm not sure what the common size is here - I generally
-  // see 2-5 entries.	For now, punt and let it be allocated later.  We can't
-  // make it an nsAutoVoidArray since it's a cString array.
-  // XXX mFontIsGeneric will generally need to be the same size; right now
-  // it's an nsAutoVoidArray.	If the average is under 8, that's ok.
+  : mFonts() // I'm not sure what the common
+  // size is here - I generally see 2-5 entries.  For now, punt and
+  // let it be allocated later.  We can't make it an nsAutoVoidArray
+  // since it's a cString array.  XXX mFontIsGeneric will generally
+  // need to be the same size; right now it's an nsAutoVoidArray.  If
+  // the average is under 8, that's ok.

What about just copy-pasting the old comment to save CVS from generating
a diff block that isn't meaningful.

> > Some places inside these structs have a weird indentation.
>
> I'm not sure what you're talking about here.  They look OK here.

Below is what I see from my end:

+struct MozXftLangGroup {
+    const char   *mozLangGroup;
+    FcChar32	    character;
+    const FcChar8 *XftLang;
+};
+
+static const MozXftLangGroup MozXftLangGroups[] = {
+    { "x-western",	 0x0041,	(const FcChar8 *)"en" },
+    { "x-central-euro", 0x0100,	(const FcChar8 *)"pl" },
+    { "x-cyrillic",	 0x0411, (const FcChar8 *)"ru" },
+    { "x-baltic",	 0x0104, (const FcChar8 *)"lv" },
+    { "x-unicode",	 0x0000,		  0    },
+    { "x-user-def",	 0x0000,			0    },
+};

> Since mFontMetrics is nsIFontMetricsGTK by design.
>
> Actually, mFontMetrics is an nsIFontMetrics pointer, not an nsIFontMetricsGTK
> object.

Actually, it is an nsIFontMetricsGTK object because the rendering context's
mFontMetrics is created with the nsGfxFactoryGTK's nsFontMetricsConstructor
which hands back an nsFontMetricsGTK (or Xft) -- which is nsIFontMetricsGTK and 
nsIFontMetrics by design:

+class nsIFontMetricsGTK : public nsIFontMetrics {
+class nsFontMetricsGTK : public nsIFontMetricsGTK
+class nsFontMetricsXft : public nsIFontMetricsGTK

> Yeah, I removed that bit of code from nsWidget.cpp since it's not used anywhere
> in the entire tree.

Since you have already double-checked that it isn't used at all, I suggest
changing

 NS_IMETHODIMP	nsFontMetricsGTK::GetFontHandle(nsFontHandle &aHandle)
 {
-  aHandle = (nsFontHandle)mWesternFont;
+  aHandle = (nsFontHandle)(nsIFontMetricsGTK *)this;
   return NS_OK;
 }

to

 NS_IMETHODIMP	nsFontMetricsGTK::GetFontHandle(nsFontHandle &aHandle)
 {
  return NS_ERROR_NOT_IMPLEMENTED;
 }

and having another bug to remove the API nsIFontMetrics::GetFontHandle() 
altogether. The platform-specific nsWidget::SetFont() are the sole consumers
of nsIFontMetrics::GetFontHandle(), e.g.,
widget/src/gtk/nsWidget::SetFont(const nsFont &aFont)
widget/src/windows/nsWindow::SetFont(const nsFont &aFont)

If nobody uses nsWidget::SetFont(), then it would be good to remove
nsIFontMetrics::GetFontHandle() since it introduces nasty dependencies
as you pointed out. On the other hand, I don't like your current return
of the 'this' pointer either, as I pointed out earlier.

> I should really rename nsIFontMetricsGTK to something a little more different
> than nsIFontMetrics and nsFontMetricsGTK.  Maybe I can use nsIFontHandleGTK! :)

Purists will say nsPI...something :-) But nsIFontHandleGTK would be a
misnomer. The mFontHandle that you added isn't needed since it is redundant
with mFontMetrics as I mentioned earlier. Simply change the declaration 
of the nsRenderingContextGTK's mFontMetrics to nsIFontMetricsGTK.

+  // Get the pointer to twip conversion for this rendering context
+  float GetP2T() {
+    return mP2T;
+  }

This function isn't needed since you can get the P2T factor by just using an
existing function to that effect in nsIDeviceContext:

  float p2t;
  deviceContext->GetDevUnitsToAppUnits(p2t);
  (and you already have a hold to the device context within the font metrics)


+    // Get a GdkFont for this handle, if there is one.  This can
+    // return 0, which means there is no GdkFont associated with this
+    // particular handle.
+    virtual GdkFont* GetGDKFont(void) = 0;

Suggesting to rename it as GetCurrentGDKFont() since it will reflect better
what the function is doing.
> This function isn't needed since you can get the P2T factor by just using an
> existing function to that effect in nsIDeviceContext:

Do we really need to insist on making a virtual function call just to get a float?
Do you instead want to pollute the other classes and defeat the purpose of other
APIs. Yo can remove nsIDeviceContext::* if you don't like that instead. 
> What about just copy-pasting the old comment to save CVS from generating
> a diff block that isn't meaningful.

Fixed.  Silly mistake.

> 
> +struct MozXftLangGroup {
> +    const char   *mozLangGroup;
> +    FcChar32	    character;
> +    const FcChar8 *XftLang;
> +};
> 
> Some k

Some hidden tabs in there.  Fixed.

> Actually, it is an nsIFontMetricsGTK object because the rendering context's
> mFontMetrics is created with the nsGfxFactoryGTK's nsFontMetricsConstructor
> which hands back an nsFontMetricsGTK (or Xft) -- which is nsIFontMetricsGTK and 
> nsIFontMetrics by design:
> 

All right. You sold me.  Fixed.  We can file a new bug once this is checked in
to remove that interface since it doesn't seem to be needed since there are no
users now.

> 
> This function isn't needed since you can get the P2T factor by just using an
> existing function to that effect in nsIDeviceContext:
> 

Fixed.

> Suggesting to rename it as GetCurrentGDKFont() since it will reflect better
> what the function is doing.

Fixed.
Attached patch patch — — Splinter Review 
Fix rbs' latest suggestions.
Attachment #102460 - Attachment is obsolete: true
Comment on attachment 102543 [details] [diff] [review]
patch

r=rbs
Attachment #102543 - Flags: superreview+
Attachment #102543 - Flags: review+
Comment on attachment 102543 [details] [diff] [review]
patch

a=roc,shaver for 1.2b.	Lux fiat.
Attachment #102543 - Flags: approval+
Checked in.  No apparent affect on leaks, page load times or other automated tests.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Big thanks to everybody who worked on this :)
Halleluja!
Usually I do not want to spam here, but I've got to say, too:
THANKS A LOT !!! THIS IS ABSOLUTELY AWESOME !!!
Thanks for the wonderful work. I've been using Xft-enabled build for a week and
it's great. 

I've got a question or two. I considered just emailing Chris off-line or
opening a new bug, but
decided to write here(before opening a new bug) because some other people here
may be interested in what I'm gonna write. 

My question is whether it's possible to provide a 'dumb' TTF 
with some intelligence. What do I mean by this? It's kinda like 'font-hack'
for X11 core fonts with (non-)standard/custom encoding. (see comment #112).

There are some Korean TTFs included in Old Korean tools from Microsoft
for pre-1933 orthography text. They're just
plain TTFs with no intelligence in the sense that they don't have any
opentype tables (gsub, gpos, etc) for the proper rendering of Hangul Jamos
(U+1100). However, they sorta have more or less all the necessary glyphs 
in PUA (and some bad ones have glyphs assigned code space of BMP).
What's missing is a *font-specific* mapping from a sequence of Unicode 
code points to a sequence of glyphs in PUA. I added these mappings
to Yudit(http://www.yudit.org) and Lambda(Unicode-enabled LaTeX) and
have been working on adding them to Pango
1.1(http://bugzilla.gnome.org/show_bug.cgi?id=95708). 

With X11 core fonts,
it's relatively easy to support this custom-encoding and it can be done
pretty quickly (as is done for 'johab*-1' fonts mentioned in comment #112).
Now, I'm wondering whether it's doable with Xft and how hard it would be and
where I have to look if possible. 

The reason I thought this might be of interest to others is:  
until OTF support is in place and working OTFs are available,
the proper support of Indic script is hard to come by. However,
recently a set of free fonts for various Indic scripts were released
under GPL. Unfortunately, they're not OTFs but plain TTFs with
custome-encoding. Still they have necessary glyphs for rendering
of the target script, but without mapping from a seq. of
Unicode char. to a seq. of glyphs, they're useless. The situation seems to be
similar to Korean situation aforementioned. 

Any idea or comment would be appreciated. 
 

comment #87 by Owen gave a generic answer (ans. to q. 4, for example) and
it reminded me that Mathml is another area in which some sort of 'font-specific'
hack is needed (at least for now).
 

> If fonts are encoded using some sort of standard,it should just be
> a layout level issue. If it involves recognizing specific fonts, and
> knowing how they are encoded, it will be a little bit difficult to
> fit that sort of hackery on top o fthe fontconifg framework, 

> though Mozilla 
> could certainly just ask for those fonts specifically.

  Suppose a specific font with custom encoding (i.e. the mapping
from a seq. of Unicode characters to a seq. of glyphs in the font
is not trivial one-to-one, but the font is dumb and is not an
OTF but a plain TTF) is explicitly specified via CSS font-family 
in a html document. fontconfig would just hand in that font to
Mozilla.. then, taking care of this font-specific mapping/encoding
is  the responsibility of what you're refering to as layout... 
I've gotta figure out how it's done with hopefully some help ...
 
one little thing: it would be nice if the menus/dialogs used the Gnome
application font instead of the mozilla default font. right now, my mozilla
menus don't quite match the rest of my system because i use a bold font for
them, which cannot be specified by the mozilla font preferences. should i open
an enhancement request, or can we just reuse this bug, since the CC: people may
be interested?

please give yourselves a pat on the back; you guys do a great job, but we users
are never satisfied! :)

Alvin, I believe you are interested in bug 175025. Please, no more comments in
this bug.
Just installed the 1.1 beta build with xft support for RedHat 8.0 from the RPM
files provided at the mozilla ftp site. The xft support is OK. I only have the
following remark.

When I go to select Mozilla fonts in Appearance / Fonts I can't chose any other
font than "Palatino Linotype" for Cyrillic and Greek encodings. In other
encodings I can select all the fonts. And - yes I'we got some Cyrillic and Greek
fonts instaled and properly configured - including the MS fonts from Windows. 

The cyrillic fonts are working fine with other xft2 applications like GNOME and
KDE from RH8. Also the Cyrillic fonts are working fine with the very same
mozilla in the main window when you surf - so I can see Cyrillic Verdana on
www.mksoftver.org or www.bagra.net.mk so I suppose this is a tiny bug.

Hope I hellped - have fun.


EVERYONE READ THIS:

If you have a bug report, FILE A NEW BUG. Don't add more comments to an 
already-closed bug. You need to create a new bug so it gets tracked.

If you have support requests, mail the mailing lists or newsgroups or 
ask on IRC.

No more comments!
Depends on: xft_triage
Depends on: 209626
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: