Closed Bug 144664 Opened 22 years ago Closed 22 years ago

Font Catalog Service

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Platform:
All
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: bstell, Assigned: Louie.Zhao)

References

Details

(Keywords: intl)

Attachments

(3 files, 16 obsolete files)

Interface of FontCatalogService (Change some declaration of variables)
4.97 KB, text/plain
Details
working patch
96.33 KB, patch
bstell
: review+
bryner
: superreview+
Details | Diff | Splinter Review
diff between working patch and old patch (have r/sr)
16.05 KB, text/plain
Details 
Make the Linux/Unix TrueType font catalog a XPCOM service so it can be shared 
by the moz Gtk code and the moz PS code
Blocks: 144663
Keywords: intl
QA Contact: ruixu → kasumi
Currently, these code is in gfx/src/x11share/nsFT2FontCatalog.
We need to simply move them into service or need to add more functions?
It needs to be a service so it can be shared by both the gfx/src/gtk module and
the gfs/src/ps module.

As a starting point one could look at nsISample.
I plan to do it.
I want to add an idl file named nsICataLogService.idl in mozilla/gfx/idl.
And then will implement with nsCatalogService.cpp in gfx/src
Brian, do you think functions in nsFT2FontCatalog is enough?
Here is the public function list of it:
  static void        FreeGlobals();
  static PRBool      InitGlobals(FT_LibraryRec_ *);
  static void        GetFontNames(const char* aPattern, 
                                  nsFontNodeArray* aNodes);
  static PRUint16*   GetCCMap(nsFontCatalogEntry *aFce);
  static nsTTFontFamilyEncoderInfo* GetCustomEncoderInfo(const char *);
  static nsTTFontFamilyEncoderInfo* GetCustomEncoderInfo(nsFontCatalogEntry *);
  static const char* GetFileName(nsFontCatalogEntry *aFce);
  static const char* GetFamilyName(nsFontCatalogEntry *aFce);
  static PRInt32*    GetEmbeddedBitmapHeights(nsFontCatalogEntry *aFce);
  static PRInt32     GetFaceIndex(nsFontCatalogEntry *aFce);
  static PRInt32     GetNumEmbeddedBitmaps(nsFontCatalogEntry *aFce);
I think there will be 2 xpcom interfaces:

  1) get the list of per-font entries

  2) get the information in a particular per-font entry.

>  static void        FreeGlobals();
>  static PRBool      InitGlobals(FT_LibraryRec_ *);

I think these can be handled in the "get the service" call and the implied 
service shutdown.

>  static void        GetFontNames(const char* aPattern, 
>                                  nsFontNodeArray* aNodes);

To get the list of per-font entries I might suggest something like this:

  GetNumFontCatalogEntries();
  GetFontCatalogEntries(name, language, slant, weight, width, spacing, variant);
  GetFontCatalogEntries(xlfd, &fce_p, &num);

(To get exact match specify all the parameters of interest.)
(To get a loose match wildcard the appropiate paramaters and then check
 the appropiate values of each entry to find the best match.)


To get the info in a particular per-font entry your list looks like it
has most of the right functions. Might want to add these.

  GetFoundry(fce);
  GetLanguage(fce);
  GetSpacing(fce);
  GetStyle(fce);
  GetVariant(fce);
  GetWeight(fce);


Pete: can we assign this to you?
ok. ->myself
Assignee: bstell → pete.zha
Attached file Interface of FontCatalogService (obsolete) — 
per discuss with bstell, we worked out the idl file.
Comment on attachment 88267 [details]
Interface of FontCatalogService (Change some declaration of variables)

This certainly seems like a reasonable start; though implementation will lead
you to some interface refinements, I'm sure.
Status: NEW → ASSIGNED
Blocks: 144665
Brian, I am going to implement this interface. There is a question about the
interface. 

Now, "nsFT2FontCatalog.cpp" creates a "nsFontCatalog" and a "nsFontNode"
collection(Hash Table) when "InitGlobals", and the interface "GetFontNames"
actually return "nsFontNode" array. 

But it seems the new interface has no relation to "nsFontNode", it just create
"nsFontCatalog" from font files and return "nsFontCatalogEntry" array according
to searching pattern.

My question is which one should be returned by "GetFontNames",
"nsFontCatalogEntry" or "nsFontNode"? If return "nsFontCatalogEntry", the
gfx/src/gtk module should convert it to nsFontNode, am I right?
Careful.  I'm about to whack this code hard, removing most of the direct free
type interfaces in favor of Xft.
> If return "nsFontCatalogEntry", the gfx/src/gtk module should convert it to 
> nsFontNode, am I right?

Yes

> Careful.  I'm about to whack this code hard, removing most of the direct free
> type interfaces in favor of Xft.

What is the bug number?

Please note that any Xft/FontConfig code should be behind this interface as the
whole point is to make the font access available to both the X display code and
the printing code.

Yeah, it's really easy to get your hands on the raw freetype data once you have
the correct font handle.
Hmm...looking at the interface, though, this is really, really low level. 
Probably far lower than it needs to be.
Blizzard, using Xft was more convenient than using FreeType2 directly, but do
all platforms have Xft? I can't find one for solaris. If Xft is only for
XFree86(Linux), mozilla can't be cross-platform. If other platforms can use Xft,
where can I find one for solaris?

Brian, the "nsFontNode" in "gfx/src/gtk" module has the same structure as the
"nsFontNodeXlib" in "gfx/src/xlib" module. They are all used for  return value
of "GetFontNames" of nsFontCatalog. So I want to do as following: 

1. implement nsFontCatalog Service, it return nsFontCatalogEntry according to
search pattern. It can use FreeType2 directly or use Xft, Anyway, the interface
is the same.

2. write static converter to transfer nsFontCatalogEntry to nsFontNode. we can
define structure "nsFontNode" and converting  function in public area, which can
be accessed by gtk, xlib and ps module. So we don't have to define the same
thing in several places.

what do you think about this?



> Blizzard, using Xft was more convenient than using FreeType2 directly, but do
> all platforms have Xft? I can't find one for solaris. If Xft is only for
> XFree86(Linux), mozilla can't be cross-platform. If other platforms can use Xft,
> where can I find one for solaris?

If you guys are shipping gnome2, you will be shipping Xft in one form or
another.  You should have it available.
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?

Louie: I watch the development on Xft/FontConfig all the time. As far as I can 
tell there it is still developing and is not ready. Until we have determined 
that Xft/FontConfig is ready and there are patches to integrate Xft and/or
FontConfig ready please do not get distracted by it.

Chris: I see that the FontConfig part of Xft is still under serious development. 
When it is ready I look forward to seeing it integrated into moz. Until 
the time it is ready it should not be used to inhibit printing support.

> 1. implement nsFontCatalog Service, it return nsFontCatalogEntry according to
> search pattern. It can use FreeType2 directly or use Xft, Anyway, the 
> interface is the same.

Yes.

> 2. write static converter to transfer nsFontCatalogEntry to nsFontNode. we can
> define structure "nsFontNode" and converting  function in public area, which 
> can be accessed by gtk, xlib and ps module. So we don't have to define the 
> same thing in several places.
> 
> what do you think about this?

I'm not sure what the function will needing to be but in general this sounds 
like a good plan.


> 1. implement nsFontCatalog Service, it return nsFontCatalogEntry according to
> search pattern. It can use FreeType2 directly or use Xft, Anyway, the 
> interface is the same.

Yes.

> 2. write static converter to transfer nsFontCatalogEntry to nsFontNode. we can
> define structure "nsFontNode" and converting  function in public area, which 
> can be accessed by gtk, xlib and ps module. So we don't have to define the 
> same thing in several places.
> 
> what do you think about this?

I'm not sure exactly how this will end up being done but in general this sounds
like a good plan.


This pathc is the implementation of nsIFontCatalogService(I modify the
interface a little). Since gtk/xlib/ps modules share this service, it should
not to be
dependent on anyone of them. The implementation is based on x11shared/* which
dependent on something in gtk module.

The most difference between this implementation and original code is
  1. FontCatalog become a "service" which can be shared
  2. it's independent on gtk/xlib/ps module.

I test the initialization of Font Catalog, the result summary file is identical
to nsFT2FontCatalog generates, which demonstrates we got the right information
from TrueType Font file.But There is also problems:

   1. font-catalog-entry has no "language", "slant" and "spacing", how can I
search entry collection using these patterns?
   2. These codes are used by unix, so it should not locate at "gfx/src", but
where to put these unix-shared codes? 

This is the first patch which implement Font Catalog Service. I am not sure
there is no problem in this patch. More discussion will make the patch better.
Thanks for working on this!

> The implementation is based on x11shared/* which dependent on something in 
> gtk module.

The x11shared code is supposed to be independent from the gfx/src/gtk 
code (Roland, are you working on this?). For the font catalog service I 
would just ignore the current dependency if possible. I believe that this 
would reduce the size of your patch dramatically and make it much simpler 
to get this reviewed / super-reviewed. If you feel that large blocks of 
code must be moved then I would recommend doing the move separately from 
the code changes. This way we can tell the reviewers / super-reviewers 
that the 1st step does not change the functionality which makes it much 
easier to get that part reviewed.

> I test the initialization of Font Catalog, the result summary file is 
> identical to nsFT2FontCatalog generates, which demonstrates we got the 
> right information from TrueType Font file.

Can you move the font catalog initialization out of the gfx/src/src code 
and into the font catalog service? This way the gfx/src/gtk code can
start the font catalog service which then gets the font summaries. This
way the font catalog service hides the details of where the font summaries
come from and could thus use different sources such as Xft/FontConfig.

> 1. font-catalog-entry has no "language", "slant" and "spacing", how can 
> I search entry collection using these patterns?

At the core of Erik's font selection architecture is a big (gigantic) 
table of fonts that moz fills as needed. When a document asks for a font
there is no guarantee that a font with the exact specification is 
available. To find the nearest fit, Erik's design groups all fonts with 
the same name in a group; eg: all courier fonts are grouped so this group
would have regular, bold, bold-italic, condensed, wide, etc. If the doc
asks for wide-bold-italic-courier-6px then the font selection code finds 
the nearest font from the group.

What this means in regard to you question is: when moz asks the font 
supplier it wants all the fonts in that group. The font catalog service 
you are working on will always be supplying a group of fonts. The font 
selection code will then pick the nearest fit. The logic for picking the 
nearest fit belongs in the font selection code not in the font supplier 
code. Figuring out which fonts to select is very difficult and I want
to keep this out of the code that gets the list of available fonts.

> 2. These codes are used by unix, so it should not locate at "gfx/src", 
> but where to put these unix-shared codes? 

One option is to put it in the gfx/src/x11shared dir and have it disabled
from compiling by a configure switch for beos and qt. If someone wants
to add TrueType Postscript printing to beos or qt we could then spend the
effort to make it a separate module.
>The x11shared code is supposed to be independent from the gfx/src/gtk
>code. For the font catalog service I would just ignore the current
>dependency if possible.

x11shared code use some structure and varaible in gfx/src/gtk. The
implementation of FCS is independent. I am not sure whether I can reduce the
size of the patch dramatically.

>Can you move the font catalog initialization out of the gfx/src/src
>code and into the font catalog service? 
Now things are what you expected. Font Catalog initialization is excuted when
starting service.
Brian Stell wrote:
> The x11shared code is supposed to be independent from the gfx/src/gtk 
> code (Roland, are you working on this?).

I can hack a patch to switch-over the GDK/GTK+-calls in gfx/src/x11shared to
plain libX11 calls, but I am not sure whether blizzard will like that change
(the functionality will still be the same) ...
Roland: we want the Gtk and Xlib version to peacefully coexit. How about if we
use virtual functions?

I don't think the windows or mac UI will ever use FreeType so this probably 
should not go in the gfx/src dir.

In the long run the gfx/src/xllshared dir should not be tied to gtk.

Both Xft/FontConfig and the direct FreeType2 code as very tied to X and should
be shared by the gtk and xlib UI.

How about putting the FCS in gfx/src/x11shared/fontcatalog ?


Louie: did you check if moz with the patch will build if FreeType2 is not
on the development system?
OS/2 might use it. http://www.freetype.org/ft_os2/
istr mkaply mentioning that.  so i think gfx/src sounds right.
I would like it to be at least one level below gfx/src so how about 
gfx/src/freetype2/moz/fontcatalog or gfx/src/freetype2/moz_fontcatalog ?

(The extra 'moz' dir is to leave room for gfx/src/freetype2/xft)
I am working on a new patch which
   1. located in gfx/src/freetype2/moz
   2. no compiling error if no freetype2 in system
      (the first patch is not well when no freetype2)
   3. less code

I still have question about how "getFontCatalogEntries" works. As Brian
said-"The font catalog service will always be supplying a group of fonts" and
"Erik's design groups all fonts with the same name in a group",
"getFontCatalogEntries" can use only "font name" as its argument. (a little
confused~~)

But there is many arguments in "getFontCatalogEntries", most of which is the
property of FontCatalogEntry. Now the working mode is going through all
FontCatalogEntry, and add the matching one to array, then return the array. But
there are arguments (language, slant, spacing) which are not the property of
FontCatalogEntry, how can I use them to search?

if these arguments (language, slant, spacing) are the property of group (i
guess), where can i find them? also from TrueType font file?


> But there are arguments (language, slant, spacing) which are not the property 
> of FontCatalogEntry, how can I use them to search?


Language
We will need a table that maps languages to CodePage range bits. Something like
this (but languages instead of encodings):
http://lxr.mozilla.org/seamonkey/source/gfx/src/x11shared/nsFT2FontCatalog.cpp#2673
We may also want a secondary table that maps Unicode Ranges to languages. I think
we can work on that later.

Slant:
 
http://lxr.mozilla.org/seamonkey/source/gfx/src/x11shared/nsFT2FontCatalog.cpp#1444
  1444   if (aFce->mStyleFlags & FT_STYLE_FLAG_ITALIC)
  1445     styleIndex = NS_FONT_STYLE_ITALIC;
  1446   else
  1447     styleIndex = NS_FONT_STYLE_NORMAL;

Spacing:
  fce->mFaceFlags = face->face_flags;
  http://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html
  FT_FACE_FLAG_FIXED_WIDTH

  #define FT_FACE_FLAG_FIXED_WIDTH  4
  A bit-field constant, used to indicate that a given face contains fixed-width 
  characters (like Courier, Lucida, MonoType, etc.).
  
  We probably should have an equivalent like #define NS_FONT_STYLE_MONOSPACED 4

=>Louie
Assignee: pete.zha → Louie.Zhao
Status: ASSIGNED → NEW
Attached patch second patch (obsolete) — — Splinter Review 
************ step to add Font Catalog Service ********************
 1. mkdir mozilla/gfx/src/freetype2
 2. mkdir mozilla/gfx/src/freetype2/moz (mkdir mozilla/gfx/src/freetype2/xft
[this is not neccessary, for xft later])
 3. copy following files to mozilla/gfx/src/freetype2/moz
      Makefile.in
      nsFreeType2.h
      nsFreeType2.cpp
      nsFontCatalogEntry.h
      nsFontCatalogEntry.cpp
      nsFontCatalogService.h
      nsFontCatalogService.cpp
 4. copy following files to mozilla/gfx/src/freetype2
      Makefile.in
 5. copy nsIFontCatalogService.idl to mozilla/gfx/idl
 6. modify following files
      mozilla/gfx/idl/Makefile.in
      mozilla/gfx/src/Makefile.in

************ Font Catalog Service under different system
configuration********************
1. no freetype2 installed:
      can get FontCatalogService, but num of entries returned = 0
2. freetype2 installed, but not enabled in mozilla:
      can get FontCatalogService, but num of entries returned = 0
3. freetype2 installed, and enabled in mozilla
      can get FontCatalogService, num of entries returned > 0

************ something need improved********************
Brian, can you help me to clarify 2 "TODO" at line 963 and 985. I am not very
sure the logic of "slant" & "spacing" selection at the 2 places. Following your
comments, I know how to select "language" :-), only except TT_OS2_CPR1_OEM (at
line 3179). Thank you!

I have purified and tested this patch, it's more concise and works well. If
anything need improve, please inform me, I will do it ASAP.
Attachment #94312 - Attachment is obsolete: true
Could you do a minimal change to just move the unchanged parts of these:

    Index: src/freetype2/moz/nsFreeType2.cpp - 400 lines
    :this looks like it is just being moved

    Index: src/freetype2/moz/nsFreeType2.h - 186 lines
    :this looks like it is just being moved

    Index: src/freetype2/moz/nsFontCatalogService.cpp - 2622 lines
    :just move the unchanged parts of this from 
    gfx/src/x11shared/nsFT2FontCatalog.cpp

    Index: src/freetype2/moz/nsFontCatalogService.h - 285 lines
    :just move the unchanged parts of this from 
    gfx/src/x11shared/nsFT2FontCatalog.h

The new patch should not have any of the new code in it. Thus this should mean 
that about ~3400 of the ~4200 lines you want to change will be a simple move and 
as such will be much easier to review. 

After that is in then your patch with the new code should be in the 500-1000 
line range which is much easier to review that ~4200 lines.
Depends on: 166773
No longer blocks: 144665
Attached patch much more concise patch (obsolete) — — Splinter Review 
Step to test this patch:
1. do this patch
2. mkdir mozilla/gfx/src/freetype
3. move nsFreeType.h nsFreeType.cpp nsFT2FontCatalog.h nsFT2FontCatalog.cpp
nsFontDebug.h under "x11shared" to "freetype"
4. move the new Makefile.in to "freetype"

Brief comments about this patch:
1. After accomplish of bug 166773(rearrange the code of FreeType2 Font Catalog
under mozilla/gfx/src/x11shared), the patch for this bug is much concise.

2.After discussion with Brian, we found there is no need to make "font catalog"
a XPCOM service. The goal of our work is to share "font catalog", and all the
modules(e.g. gfx/src/gtk, gfx/src/ps) can use it. Make "font catalog" a public
library is a much better solution. (Maybe we should change the name of this bug
:-)). Although XPCOM service is a idea form for sharing, it's not suitable for
"font catalog":
  1. "font catalog" is quite low level, and is used frequently, making it
service will lower the performance.
  2. "font catalog" is used only be gtk, xlib, ps module(c++), Javascript never
use it.
  3. "Writing service" will make the code more complex without any new
functionality and better performance.

So, we use shared library to implement sharing "font catalog".
Attachment #97000 - Attachment is obsolete: true
seeking r= & sr=
Depends on: 172477
Comment on attachment 100859 [details] [diff] [review]
much more concise patch 

once bug 172477 (which does the file moves) is in this patch will need to
be redone.
Attachment #100859 - Flags: needs-work+
seeking r= & sr=
After copying files(nsFreeType.* nsFT2FontCatalog.*) from x11shared to
freetype, Only doing this patch can implement FontCatalogService. After
checking in this patch, we can remove nsFreeType.* & nsFT2FontCatalog.* from
x11shared safely. Till then, all the work about FCS will be done.
Attachment #100859 - Attachment is obsolete: true
+nsFT2FontCatalog::GetFontCatalogEntries(const nsACString & aFamilyName,
...
+  const char* familyName = ToNewCString(aFamilyName);
+  const char* language = ToNewCString(aLanguage);

Do we need to alloc a copy? If so, do they need to be freed?

+nsFT2FontCatalog::GetFontNames(const char*    aFamilyName,
...
+  const char *familyName, *language;
...
+  if (aFamilyName) {
+    familyTmp.Assign(aFamilyName);
+    ToLowerCase(familyTmp);
+    familyName = familyTmp.get();
+    if (strlen(familyName) == 0)
+      familyName = nsnull;
+  } else
+    familyName = nsnull;
...
+  FREE_IF(familyName);
+  FREE_IF(language);

is the free here correct?

+//friend class nsFT2FontNode;

is this line necessary? If not please delete it.

-  void   doGetFontNames(const char* aPattern, nsFontCatalog* aFC);
+  void   doGetFontNames(const char*    familyName,
+                        const char*    language,

Is this needed / implemented? If not please delete it.

+  NS_ASSERTION(aFce, "init of nsFreeTypeFace need nsFontCatalogEntry");

Nit: change "need" to "needs"

+  const short FONT_CATALOG_TRUE_TYPE = 1;

Could we change this to FONT_CATALOG_TRUETYPE ?

With these minor changes, r=bstell@ix.netcom.com

seeking sr=
Attachment #102443 - Attachment is obsolete: true
Attachment #102560 - Flags: review+
Comment on attachment 102560 [details] [diff] [review]
FCS implementation following Brian's suggestion

>Index: gfx/src/freetype/nsFT2FontCatalog.cpp

>+nsCOMPtr<nsIPref>  nsFT2FontCatalog::mPref = nsnull;

I seem to recall that global nsCOMPtr were bad. Let me check with someone who
knows this stuff.

>@@ -221,7 +228,106 @@
>   }
>   return ffei;
> }
>+//
>+// Implementation of Interfaces nsIFontCatalogService
>+//
>+NS_IMPL_ISUPPORTS1(nsFT2FontCatalog, nsIFontCatalogService)
>+
>+nsFT2FontCatalog::nsFT2FontCatalog()
>+{
>+  NS_INIT_ISUPPORTS();
>+  
>+#if (defined(MOZ_ENABLE_FREETYPE2))
>+  nsresult rv;
>+  availableFontCatalogService = PR_FALSE;

If |availableFontCatalogService| is a member or global, please prefix it with
|m| or |g| as applicable. Also, we only use the |a| prefix for arguments, so
for local variables don't use prefixes. As a small (contrived) example to
demonstrate all prefix practices:

void* gFoo = 0;

class Bar {
  public:
    void Foopy(int aArg1, int aArg2) {
      int foo = aArg1 + aArg2;
      mFoo = foo;
    }

  private:  
    int mFoo;
};

Could you take a look at the changes you've made and make sure that (at least)
all the new variable names you're adding adhere to this?


>+NS_IMETHODIMP
>+nsFT2FontCatalog::GetFontCatalogEntries(const nsACString & aFamilyName,

...

>+  const char* familyName =  PromiseFlatCString(aFamilyName).get();
>+  const char* language =  PromiseFlatCString(aLanguage).get();

This is unsafe. PromiseFlatCString creates a temporary object which might be
destroyed at the next line (per C++ language spec), which means that the buffer
pointed to by the result of .get() might also be destroyed, in other words you
could end up with a dangling pointer this way. The safe way to do this is:

  GetFontNames(PromiseFlatCString(aFamilyName).get(),
	       PromiseFlatCString(aLanguage).get(),
	       aWeight, aWidth, aSlant, aSpacing, aFC);

The other option is to make GetFontNames accept const nsACString& for
aFamilyName and aLanguage. Then inside GetFontNames instead of copying those
strings into nsCAutoStrings you could do something like:

nsCAutoString familyName, language;
ToLowerCase(aFamilyName, familyName);
ToLowerCase(aLanguage, language);

unsigned long bit1 = GetRangeLanguage(language.get(), CPR1);
unsigned long bit2 = GetRangeLanguage(language.get(), CPR2);

// note, no |a| prefix on |bit1| and |bit2|.

// etc.

Instead of STRMATCH(familyName, X) you could then do familyName.Equals(X).

>@@ -2299,6 +2479,23 @@
>   return 0;
> }
> 
>+unsigned long
>+nsFT2FontCatalog::GetRangeLanguage(const char * language,
>+                                   PRInt16 aRange)
>+{
>+  unsigned long *aBit;

Local variable, no prefix.

>Index: gfx/src/x11shared/nsFT2FontNode.cpp
>===================================================================
>RCS file: /export/src/cvs/mozilla_5/mozilla/gfx/src/x11shared/nsFT2FontNode.cpp,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 nsFT2FontNode.cpp
>--- gfx/src/x11shared/nsFT2FontNode.cpp	2002/10/11 00:12:31	1.1.1.1
>+++ gfx/src/x11shared/nsFT2FontNode.cpp	2002/10/11 09:19:09
>@@ -39,7 +39,7 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsFT2FontNode.h"
>-#include "nsFreeType.h"
>+#include "freetype/nsFreeType.h"

Why can't you just say #include "nsFreeType.h" here? Either this header file is
in one of the public include directories, or it should be mentioned on the
local include lines in the Makefile.
Attachment #102560 - Flags: needs-work+
>+unsigned long
>+nsFT2FontCatalog::GetRangeLanguage(const char * language,

and language -> aLanguage
Attached patch patch following Jag's comments (obsolete) — — Splinter Review 
This is an updating to the previous patch. The changes include:

1. fix the problem mentioned in Jag's and Brian's comments (#41 & #42)
2. make changes (not much) due to the check in of xft patch (bug 165804).
3. move nsFreeType to new library(libgfxft2.so) and implement Font Catalog
Service in gfx/src/gtk instead of moving both to new library.

/-----------------------------------------------------------
A more detailed description of change "3":

In our original plan, nsFreeType(public FT2 function) and
nsFT2FontCatalog(implementation of FCS) are moved to the same library. This
library will be used by XP Component Manager (it contains "Service") and other
library(e.g. libgfx_gtk.so will use FT2 function). This case will cause
problem. There is no need to put FCS in new library because it's "Service" and
can be shared by others even it locates in gfx/src/gtk.

Since the seperation of nsFreeType and nsFT2FontCatalog, I do once more
"rearranging code". That is, moving some static data (and "Get..." function)
and static function from nsFT2FontCatalog to nsFreeType because they will be
shared by others. They include:

1. function: GetCustomEncoderInfo
   related data: sCharSetManager, gFontFamilyEncoderInfo
2. function: GetRange1(2)CharSetNames
   related data: ulCodePageRange1(2)CharSetNames
3. function: ParseXLFD

This work is just moving, no any changing.
/-----------------------------------------------------------
Attachment #102560 - Attachment is obsolete: true
seeking r=
Brian, can you review this path first? Thanks a lot.
Nit: could you remove the new tabs (except where necessary such as in Makefiles
and where surrounding code already uses them.

Do we need to test if these are not null?
+  delete sRange1CharSetNames;
+  delete sRange2CharSetNames;
+  delete sFontFamilies;

Nit: in the nsFreeTypeFace getter routines can you assign default values if 
mFce==null? eg:
  in GetFamilyName => aFamily.Assign("");
  in GetFaceIndex  => *aFaceIndex = 0;

Nit: fix spacing
 +typedef struct nsTTFontFamilyEncoderInfo {
 +  const char                *mFamilyName;
 +  nsTTFontEncoderInfo *mEncodingInfo;

Nit: could you explain why the extra dir is needed in the include statements?
 +#include "x11shared/nsFT2FontCatalog.h"

Do we need to check if "entries" is non-null?
+  NS_NewISupportsArray(getter_AddRefs(entries));
+  ...
+    entries->InsertElementAt(genericFce, 0);
 
Nit: please clean up the spacing:
+nsFT2FontCatalog::GetFontNames(const nsACString & aFamilyName,
+                               const nsACString & aLanguage,
+                               PRUint16       aWeight,
+                               PRUint16       aWidth,
+                               PRUint16       aSlant,
+                               PRUint16       aSpacing,
+                               nsFontCatalog* aFC)

Nit: this can be simplified
+    case kFCSlantRoman:
+      italicBit = 0;
+      break;
+    case kFCSlantItalic:
+      italicBit = 1;
+      break;

to

+    case kFCSlantRoman:
+    case kFCSlantItalic:
+      italicBit = 1;
+      break;

ditto for:
+    case kFCSlantReverseItalic:
+      italicBit = 1;
+      break;
+    case kFCSlantReverseOblique:
+      italicBit = 1;
+      break;

to 

+    case kFCSlantReverseItalic:
+    case kFCSlantReverseOblique:
+      italicBit = 1;
+      break;

(they could all be condensed but it seems odd to have italic and reverse italic
in the same case path)

Nit: could you add a comment stating the words that make up the acronym?
+#define CPR1 1
+#define CPR2 2

clean up these and r=bstell@ix.netcom.com
Attached patch patch following Brian's comments (obsolete) — — Splinter Review 
seek sr=
Attachment #104355 - Attachment is obsolete: true
Attachment #104475 - Flags: review+
Louie, I'm wondering about the nsFT2FontCatalog problem. I wonder if the "can't
start again" problem you were seeing would be fixed by deleting
dist/bin/components/compreg.dat.
As for your patch, I'll need to spend some time to fully understand what's going
on here, and I don't have that time right now. Perhaps you could ask blizzard or
brendan for sr=?
Jag, after deleting dist/bin/components/compreg.dat, mozilla can start. But the
problem still comes out when starting the second time.
Doug, do you have any idea what could cause the problem Louie noted where
(apparently) the component manager crashes on the second start after applying
the patch in attachment 102560 [details] [diff] [review]?

Actually, nevermind.  This is almost surely a side effect of linking one
component  against another component, a definite no-no.
Comment on attachment 104475 [details] [diff] [review]
patch following Brian's comments

>Index: gfx/idl/nsIFontCatalogService.idl
>===================================================================
>RCS file: nsIFontCatalogService.idl
>diff -N nsIFontCatalogService.idl
>--- /dev/null	Tue Oct 29 19:10:10 2002
>+++ nsIFontCatalogService.idl	Tue Oct 29 19:18:47 2002
>@@ -0,0 +1,119 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Netscape Public License
>+ * Version 1.1 (the "License"); you may not use this file except in
>+ * compliance with the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/NPL/
>+ *

New files should be under the MPL/GPL/LGPL, not NPL/GPL/LGPL.  Also make sure
to update the copyright year.

>Index: gfx/src/Makefile.in
>===================================================================
>RCS file: /export/src/cvs/mozilla_1/mozilla/gfx/src/Makefile.in,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 Makefile.in
>--- gfx/src/Makefile.in	2002/10/21 00:16:42	1.1.1.1
>+++ gfx/src/Makefile.in	2002/10/29 11:18:47
>@@ -45,6 +45,8 @@
> 
> DIRS        = shared
> 
>+DIRS        += freetype
>+

This should be ifdef'd, shouldn't it? We don't want to build freetype code on
Mac and Windows.

>Index: gfx/src/freetype/nsFreeType.cpp
>===================================================================
>RCS file: /export/src/cvs/mozilla_1/mozilla/gfx/src/freetype/nsFreeType.cpp,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 nsFreeType.cpp
>--- gfx/src/freetype/nsFreeType.cpp	2002/10/21 00:16:44	1.1.1.1
>+++ gfx/src/freetype/nsFreeType.cpp	2002/10/29 11:18:47
>@@ -74,8 +77,15 @@
> double            nsFreeType::gAATTDarkTextGain = 0.8;
> PRInt32           nsFreeType::gAntiAliasMinimum = 8;
> PRInt32           nsFreeType::gEmbeddedBitmapMaximumHeight = 1000000;
>+nsHashtable*      nsFreeType::sFontFamilies = nsnull;
>+nsHashtable*      nsFreeType::sRange1CharSetNames = nsnull;
>+nsHashtable*      nsFreeType::sRange2CharSetNames = nsnull;
>+nsICharsetConverterManager2* nsFreeType::sCharSetManager = nsnull;

It was my understanding that we were discouraging new use of nsHashtable in
favor of pldhash.  

>+nsTTFontFamilyEncoderInfo*
>+nsFreeType::GetCustomEncoderInfo(const char * aFamilyName)
>+{
>+  if (!sFontFamilies)
>+    return nsnull;
>+
>+  nsTTFontFamilyEncoderInfo *ffei;
>+  nsCAutoString name(aFamilyName);
>+  ToLowerCase(name);
>+  nsCStringKey key(name);
>+  ffei = (nsTTFontFamilyEncoderInfo*)sFontFamilies->Get(&key);
>+  if (!ffei)
>+    return nsnull;
>+
>+  // init the converter
>+  if (!ffei->mEncodingInfo->mConverter) {
>+    nsTTFontEncoderInfo *fei = ffei->mEncodingInfo;
>+    //
>+    // build the converter
>+    //
>+    nsICharsetConverterManager2* charSetManager = GetCharSetManager();
>+    if (!charSetManager)
>+      return nsnull;
>+    nsCOMPtr<nsIAtom> charset(dont_AddRef(NS_NewAtom(fei->mConverterName)));
>+    if (charset) {
>+      nsresult res;

How often is this called? Would it be worth it to cache these atoms?

> ///////////////////////////////////////////////////////////////////////
> //
> // class nsFreeTypeFace data/functions
> //
> ///////////////////////////////////////////////////////////////////////
>+NS_IMPL_ISUPPORTS1(nsFreeTypeFace, nsITrueTypeFontCatalogEntry)
> 
> nsFreeTypeFace::nsFreeTypeFace(nsFontCatalogEntry *aFce)
> {

I'd suggest initializing mCCMap with a constructor initializer, instead of an
assignment.  The bigger problem in the constructor is that we shouldn't be
doing anything there that could fail due to an out of memory error.  Instead,
an Init() method should be added to the interface, then the caller can check
the return value and deal with a failure.  That would also let you safely
eliminate all of the extra checking for mFce in member functions.

>Index: gfx/src/freetype/nsFreeType.h
>===================================================================
>RCS file: /export/src/cvs/mozilla_1/mozilla/gfx/src/freetype/nsFreeType.h,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 nsFreeType.h
>--- gfx/src/freetype/nsFreeType.h	2002/10/21 00:16:44	1.1.1.1
>+++ gfx/src/freetype/nsFreeType.h	2002/10/29 11:18:47
>@@ -27,20 +27,67 @@
> void     nsFreeTypeFreeGlobals(void);
> nsresult nsFreeTypeInitGlobals(void);
> 
>+#include "nsIFontCatalogService.h"
>+
> #if (defined(MOZ_ENABLE_FREETYPE2))
> 
>+#include "nspr.h"

If you need NSPR types, I think prtypes.h is the preferred header to include.
bryner, I have fixed the things you mentioned

1.correct license and copyright date
2.I don't change "GetCustomEncoderInfo" and those nsHashtable and just move it
from nsFT2FontCatalog to nsFreeType. Since it works well before, we can correct
this later if it's really not good.
3.adding Init of nsFreeTypeFace. It's really good idea :).
4.can't use #include "prtypes.h" to replace #include "nspr.h" because I can't
find PR_Library if do so.
Attachment #104475 - Attachment is obsolete: true
Comment on attachment 104838 [details] [diff] [review]
new patch following bryner's comments

sr=bryner
Attachment #104838 - Flags: superreview+
Comment on attachment 104838 [details] [diff] [review]
new patch following bryner's comments

carry Brian's r=
Attachment #104838 - Flags: review+
checked in trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
>>+DIRS        += freetype
>>+
>This should be ifdef'd, shouldn't it? We don't want to build freetype code on
>Mac and Windows.

So... about following review comments....

This has just been backed out because the Windows and mac boxes did not deal
well with being asked to link against freetype.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
nsFT2FontNode.cpp
aCC -ext +p -o nsFT2FontNode.o -c -DNATIVE_THEME_SUPPORT -DOSTYPE=\"HP-UXB.11\"
-DOSARCH=\"HP-UX\" -DOJI -DUSE_POSTSCRIPT -DUSE_XPRINT -DUSE_MOZILLA_TYPES
-DMOZ_ENABLE_FREETYPE2 -I./. -I./.. -I./../shared -I./../freetype
-I./../x11shared  -I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/widget -I../../../dist/include/view
-I../../../dist/include/util -I../../../dist/include/pref
-I../../../dist/include/uconv -I../../../dist/include/unicharutil
-I../../../dist/include/locale -I../../../dist/include/necko
-I../../../dist/include/content -I../../../dist/include/layout
-I../../../dist/include/imglib2 -I../../../dist/include/gfx
-I../../../dist/include
-I/builds/tinderbox/SeaMonkey/HP-UX_B.11.00_Clobber/mozilla/dist/include/nspr  
   -I/opt/freetype/include/freetype2    +Z   -DHPUX11 -Dhpux
+W67,652,728,740,749,829  -DNDEBUG -DTRIMMED  -I/opt/gtk+/include
-I/opt/glib/lib/glib/include -I/opt/glib/include -I/usr/include/X11R6   
-DMOZILLA_CLIENT -D!
NSCAP_DIS
ABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino -DMOZ_OJI_REQUIRE_THREAD_SAFE_ON_STARTUP=1
-DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1
-DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UINT16_T=1
-DHAVE_DIRENT_H=1 -DHAVE_SYS_BYTEORDER_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1
-DHAVE_NL_TYPES_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1
-DHAVE_SYS_VFS_H=1 -DHAVE_SYS_MOUNT_H=1 -DNEW_H=\<new\> -DHAVE_LIBM=1
-DFUNCPROTO=15 -DHAVE_XSHM=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1
-DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_STATVFS=1
-DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_NL_LANGINFO=1 -DHAVE_FLOCKFILE=1
-DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_ICONV=1 -DHAVE_IOS_BINARY=1
-DHAVE_CPP_EXPLICIT=1 -DHAVE_CPP_SPECIALIZATION=1
-DHAVE_CPP_MODERN_SPECIALIZE_TEMPLATE_SYNTAX=1
-DHAVE_CPP_PARTIAL_SPECIALIZATION=1 -DHAVE_CPP_ACCESS_CHANGING_USING=1
-DHAVE_CPP_AMBIGUITY_RESOLVING_USING=1 -DHAVE_CPP_UNAMBIGUOUS_STD_NOTEQUAL=1
-DHAVE_CPP_NEW_CASTS=1 !
-DHAVE_CP
P_DYNAMIC_CAST_TO_VOID_PTR=1 -DNEED_CPP_UNUSED_IMPLEMENTATIONS=1
-DHAVE_I18N_LC_MESSAGES=1 -DMOZ_DEFAULT_TOOLKIT=\"gtk\" -DMOZ_WIDGET_GTK=1
-DMOZ_ENABLE_XREMOTE=1 -DMOZ_X11=1 -DMOZ_ENABLE_COREXFONTS=1 -DIBMBIDI=1
-DACCESSIBILITY=1 -DMOZ_MATHML=1 -DMOZ_SVG=1 -DMOZ_LOGGING=1
-DMOZ_USER_DIR=\".mozilla\" -DCPP_THROW_NEW=throw\(\) -DMOZ_XUL=1
-DINCLUDE_XUL=1 -DNS_MT_SUPPORTED=1 -DUSE_IMG2=1
-DMOZ_BYPASS_PROFILE_AT_STARTUP=1 -DMOZ_DLL_SUFFIX=\".sl\" -DXP_UNIX=1
-DUNIX_ASYNC_DNS=1 -DJS_THREADSAFE=1 -DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 
nsFT2FontNode.cpp
Error 197: "nsFT2FontNode.cpp", line 130 # Jump past initialization of variable 'i'.
        goto cleanup_and_return;
        ^^^^^^^^^^^^^^^^^^^^^^^^
Error 197: "nsFT2FontNode.cpp", line 134 # Jump past initialization of variable 'i'.
        goto cleanup_and_return;
        ^^^^^^^^^^^^^^^^^^^^^^^^
Error 197: "nsFT2FontNode.cpp", line 138 # Jump past initialization of variable 'i'.
        goto cleanup_and_return;
        ^^^^^^^^^^^^^^^^^^^^^^^^

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/x11shared/nsFT2FontNode.cpp&rev=1.2&root=/cvsroot&mark=130,134,138#120
When check in, meet 2 Cross-Platform problems. 

1. gfx/src/freetype compile error under Windows. Since that dir was never used
on Windows and MacOS, I use ifdef MOZ_ENABLE_FREETYPE2 in src/Makefile.in.
Bryner really saw this problem before, but I am sorry to miss it.

2. for(PRUint32 i ... after "goto .." will cause problem on HP-UX. I has fix
all these to PRUint32 i; for (i=.....
Attachment #104838 - Attachment is obsolete: true

    
    

    
  
Comment on attachment 105306 [details] [diff] [review]
diff for the CrossPlatform problem fixing

sr=bzbarsky

        Attachment #105306 -
        Flags: superreview+

    
    

    
  
Comment on attachment 105306 [details] [diff] [review]
diff for the CrossPlatform problem fixing

r=jkeiser

        Attachment #105306 -
        Flags: review+

    
    

    
  


  
In the previous one, code below is to prevent compile freetype dir on Window or
MacOS. use MOZ_ENABLE_COREXFONTS will be more suitable than
MOZ_ENABLE_FREETYPE2. Because xft patch (bug 165804) include some the related
code (nsFontMetricsGTK.cpp and gfx/src/x11shared) in the MOZ_ENABLE_COREXFONTS.


+ifdef MOZ_ENABLE_FREETYPE2
 DIRS	     += freetype
+endif

    
    

    
  


  
correct the previous wrong patch

        Attachment #105420 -
        Attachment is obsolete: true

    
  

        Attachment #105422 -
        Flags: superreview+

    
    

    
  
Comment on attachment 105422 [details] [diff] [review]
use MOZ_ENABLE_COREXFONTS will be more suitable

r=bryner for the makefile and bustage fixes (please note bstell's review for
the rest of it in your checkin comment)

        Attachment #105422 -
        Flags: review+

    
    

    
  
Comment on attachment 105422 [details] [diff] [review]
use MOZ_ENABLE_COREXFONTS will be more suitable

carry sr=bz from IRC discussion

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for bustage (11.7) (obsolete)
        — 
          — Splinter Review
      

    


  
fix the windows and MacOS platform bustage

    
    

    
  


  
I forgot to check in gfx/src/gtk/Makefile.in in the previous the bustage
fixing. This is the patch for that file.

    
    

    
  
it looks like the checkin for this bug caused bug 178888 (which seems to be ok
after the recent backout)

    
    

    
  
Yesterday, when check in the patch, we meet some cross-platform (WINNT and
HP-UX) problems and I have fixed them already (Attachment 105306 [details] [diff] and 105436).
But "Linux comet Dep" still can't run "viewer" after building. I have
investigated the reason.

1.Build Log (Brief) for Linux comet Dep

	Running ViewerAliveTest test ...
	Timeout = 15 seconds.
	Begin: Thu Nov  7 09:40:34 2002
	cmd = viewer
	End:   Thu Nov  7 09:40:36 2002
	----------- Output from ViewerAliveTest ------------- 
	----------- End Output from ViewerAliveTest --------- 
	Error: viewer: received SIGHUP
	Error: viewer: exited with status 16777215
	Error: viewer: dumped core.
	
2.The mozconfig on Linux comet Dep is
	
	ac_add_options --enable-crypto
	ac_add_options --enable-extensions=all,-venkman
	ac_add_options --enable-optimize
	ac_add_options --disable-debug

3.The error is caused by attachement 105436, which moves "gFontDebug" from
libgfxft2.so (gfx/src/freetype) to libgkgfx.so (gfx/src/gtk) (different lib).
"gFontDebug" is a variable shared by 2 library. I defined it in libgfxft2.so
before. After I move it to libgkgfx.so, "Linux comet Dep" core dumped. That's
because libgkgfx.so is "XPComponent" library and it's symbols will be limited
when "-disable-debug". when libgfxft2.so access "gFontDebug", problem will come.
So I think it's rule that never use public varaible and function in a
XPComponent library except Component Interface. That kind of problem never come
without "--disable-debug" because "debug" will not limit the export of symbols.
so I think I have to test all my patch with "--disable-debug" in addition from now.

4.Hope mozilla guys to add comments about this issue. I really learn a lot from
making a XP Component.:)

5. I will put a new patch which will fix all the cross-platform problem and the
issue above.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        working patch (obsolete)
        — 
          — Splinter Review
      

    


  
I have fixed the issue mentioned above and all cross-platform problems in this
patch and have tested on Linux(Debug), Linux(without Debug), Solaris, WINNT
platform.

        Attachment #105304 -
        Attachment is obsolete: true

        Attachment #105306 -
        Attachment is obsolete: true

        Attachment #105422 -
        Attachment is obsolete: true

        Attachment #105436 -
        Attachment is obsolete: true

        Attachment #105437 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        working patch
        — 
          — Splinter Review
      

    


  
update previous one

    
    

    
  


  
This is the diff of working patch and old patch which has got review and
superview. Since the patch is big, hope this diff can help reviewer and
superviewer.

    
    

    
  
Comment on attachment 105947 [details] [diff] [review]
working patch

r=bstell@ix.netcom.com

        Attachment #105947 -
        Flags: review+

        Attachment #105947 -
        Flags: superreview+

    
    

    
  
I'm really disappointed that this was made into another DLL. There is no reason
this should be in a seperate dll.. all this does is impact footprint and
performance even more. Each additional dll has a 32-100k runtime overhead, and
cross-dll calls are as expensive as vtable calls....can we reconsider how this
was implemented and instead bring the freetype stuff into the existing GFX dlls?

    
    

    
  
fcs has been finished, mark it fixed
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED

    
    

    
  
If you add DLLs, you should change packaging so that installed builds urn (bug
180318).  However, given alecf's comments,  should this be backed out instead? 
Why was this turned into another DLL?  (Were there global variables that should
have been |static| but weren't?)

    
    

    
  
I would REALLY like to see this backed out and fixed the right way.

Brian, when we talked in my cube the other day, you said something about
avoiding loading the DLL alltogether if the font catalog service was not
installed. The thing is, if its already built into a DLL, then there is no
additional DLL to load. And you don't have to worry about DLL size, because DLLs
are memory-mapped...relatively speaking, the size of the DLL plays a small role
in performance. number of DLLs and symbols per DLL makes a bigger difference.

    
    

    
  
blizzard is in the process of testing if backing this out fixes the blocker.

(it might just be a simple case of packages-unix is not correct)

but since dbaron and alecf want it backed out, that might happen.

    
    

    
  
see blizzards check in for smoketest blocker bug 180318.

    
    

    
  
I've opened bug 180473 to make the nsFreeType code part of a static lib (linked to
the gtk code) instead of a shared lib.

    
  
Depends on: 180668

    
  
No longer depends on: 180668

    
    

    
  
we discussed this over AIM, I thought nsFreeType was going to be made into a
component? libgkgfx.so (the non-component DSO) is only for base classes that are
derived from, like nsRenderingContext, and so forth.


    
    

    
  
oops, nevermind I commented before I completely understood your comment. The
other bug does the right thing.

    
    

    
  
True type printing is working on 01-21 trunk build / linux RH7.2.  Mark as verified.
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: