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: