nsFreeType will to be shared by the gtk module and the ps module so it needs to be a xpcom service (In a separate aim discussion) Alec said it is okay for the ps module to be dependent on the gtk module for the nsFreeType code. So the nsFreeType code can be put in the libgfx_gtk lib. So there is no reason to have it in a separate shared lib.

Most of the patch is excellent. Only several things need more discussion (I mark them by bold comments in attachment).

Wow, before looking at the patches I read Louie's review. It is a fantastic review, and I'd like to see all his issues addressed before I actually start the super-review. especially important points are: - rely on the service manager to ensure a singleton instance, don't try to do singleton management yourself - rely on getService() failing to determine if the service is available or not - if you want your object to return an error, you should use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT() which allows you to call a nsresult-returning function. This allows your object to return an error during object creation - such as the ft2 library not being available. How would this work? define a method in your class nsresult nsFreeType2::Init() { if (!library = PR_LoadLibrary("...")) return NS_ERROR_FAILURE; } the nsFreeType2 object will be destroyed, and the failure will be propagated up to the caller to do_GetService(). - don't use static objects (including static nsCOMPtrs)

> +#include "nsISupports.idl" > +%{C++ > +#include <ft2build.h> > +#include FT_FREETYPE_H > > ... Should "FT_GLYPH_H" be wrapped in idl file? > ... should the idl include all ft2 data type and function used in mozilla I would prefer to keep the idl focused on the interface it defines. As such I feel it should only have the includes it needs and and define the data types it needs. > these macro are define in nsFontFreeType and not only used in > nsFontFreeType.cpp, but also used in ps module in future, can put it in IDL > file? > #define FT_CEIL(x) (((x) + 63) & -64) > #define FT_FLOOR(x) ((x) & -64) > #define FT_TRUNC(x) ((x) >> 6) > #define FT_16_16_TO_REG(x) ((x)>>16) I feel they should be in a *.h file not a *.idl file. > EXPORT_LIBRARY = 1 > MODULE_NAME = nsGfxFT2Module > This line can be deleted, shared library or static library don't need > modulename, only xpcom library use it. done > -char* nsFreeType::gFreeType2SharedLibraryName = nsnull; > -PRBool nsFreeType::gEnableFreeType2 = PR_FALSE; > -PRBool nsFreeType::gFreeType2Autohinted = PR_FALSE; > -PRBool nsFreeType::gFreeType2Unhinted = PR_TRUE; > -PRUint8 nsFreeType::gAATTDarkTextMinValue = 64; > -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; > +char* nsFreeType2::gFreeType2SharedLibraryName = nsnull; > +PRBool nsFreeType2::gEnableFreeType2 = PR_FALSE; > +PRBool nsFreeType2::gFreeType2Autohinted = PR_FALSE; > +PRBool nsFreeType2::gFreeType2Unhinted = PR_TRUE; > +PRUint8 nsFreeType2::gAATTDarkTextMinValue = 64; > +double nsFreeType2::gAATTDarkTextGain = 0.8; > +PRInt32 nsFreeType2::gAntiAliasMinimum = 8; > +PRInt32 nsFreeType2::gEmbeddedBitmapMaximumHeight = 1000000; > +nsHashtable* nsFreeType2::sFontFamilies = nsnull; > +nsHashtable* nsFreeType2::sRange1CharSetNames = nsnull; > +nsHashtable* nsFreeType2::sRange2CharSetNames = nsnull; Most of these could/should be moved to nsFT2FontCatalog or nsFontFreeType but since they are both linked into the gtk module it is not required. Since this patch is already over 1K lines I'd like to limit the changes to the more important issues for the xpcom work. > -nsICharsetConverterManager2* nsFreeType::sCharSetManager = nsnull; > +nsICharsetConverterManager2* nsFreeType2::sCharSetManager = nsnull; > why use nsFreeType2, in this patch, many changes is from nsFreeType to > nsFreeType2 ? If leave class name "nsFreeType" unchanged , the patch will > be much smaller. Is there special reason to use nsFreeType2? I am moving the naming in line with FreeType. There is a FreeType (version 1) and a FreeType2. This is not critical but I would like to get this in. > -FTC_Manager nsFreeType::sFTCacheManager; > -FTC_Image_Cache nsFreeType::sImageCache; > +nsCOMPtr<nsIFreeType2> nsFreeType2::sFt2; > I remember it's not recommended to use static nsCOMPtr before, though I am > not sure why. mark here to get further consideration. I changed all the static members to non static except those that should be addresed / moved to nsFT2FontCatalog/nsFontFreeType. (I think the issue was having a static nsCOMPtr at global scope. I think even that issue (some compiliers had trouble initializing them) has been resolved. A quick search of lxr found 62 static nsCOMPtr lines in the source.) > void > -nsFreeType::ClearFunctions() > +nsFreeType2::ClearFunctions() > { > FtFuncList *p; > + void *ptr = sFt2; > for (p=FtFuncs; p->FuncName; p++) { > - *p->FuncPtr = (PRFuncPtr)&nsFreeType__DummyFunc; > + *((PRFuncPtr*)((char*)ptr+p->FuncOffset)) = > + (PRFuncPtr)&nsFreeType2__DummyFunc; > } > } I detect that you use offset to indication function's postion instead > of function pointer. Why not use function pointer like before, since it's > more readable. In the past I stored the function pointers in static globals (hence the pointers). In this patch I'm storing them in the object (hence the offsets to the position in the object). > +#include "nsIFreeType2.h" > #include <ft2build.h> > #include FT_FREETYPE_H > #include FT_CACHE_H > can delete the last 3 lines since "nsIFreeType2.h" already include them. Yes, I thought about this but I felt that just having the FreeType2 include macros standing by themselves would likely be confusing to other developers looking at this code. > -typedef FT_Error (*FT_Get_Sfnt_Table_t)(FT_Face, FT_Sfnt_Tag); > +typedef void* (*FT_Get_Sfnt_Table_t)(FT_Face, FT_Sfnt_Tag); > why change the function prototype, has the ft2 function changed in ft2 lib? The previous declaration was incorrect. This is actually the correct prototype. > // class nsFreeType class definition > -class nsFreeType { > +class nsFreeType2 : nsIFreeType2 { > + NS_DECL_ISUPPORTS > + > public: > - inline PRBool FreeTypeAvailable() { return sAvailable; }; > maybe using NS_DECL_IFREETYPE2 to replace the list of declaration of > function is better. excellent suggestion. done > + protected: > + static nsCOMPtr<nsIFreeType2> sFt2; > I have question on this. Since nsFreeType2 already become xpcom service, > there is no reason to maintain a static instance of nsFreeType2. Service > manager will do this job. and the initializing and destruction of > nsFreeType2 should also managed by service-manager (now, the initializing > of nsFreeType2 is also invoked by constructor of nsFT2FontCatalog, > meanwhile not by "GetService", but by calling "nsFreeType2::InitGlobals" > -- see below). done > +public: > + // these belong in the nsFontFreeType code > static PRBool gFreeType2Autohinted; > static PRBool gFreeType2Unhinted; > static PRUint8 gAATTDarkTextMinValue; > ps module will act like nsFontFreeType (they all get font metrics. so ps > module maybe still need these value -- I am not sure about this, need > further discussion). If don't put here, ps module will read them again. The ps module and gtk module differ sharply in this area. The gtk module must render to a screen which is a relatively low resolution device. It is the low resolution that causes the gtk to anti-alisa. The ps module is rendering to postscript with will internally handle the resolution issues. Thus these values, hinting and sharpening, are only needed for screen (low res) display. > +ifdef MOZ_ENABLE_FREETYPE2 > +# the SHARED_LIBRARY_LIBS line must be before the rules.mk include > +SHARED_LIBRARY_LIBS += $(DIST)/lib/$(LIB_PREFIX)gfxft2_s.$(LIB_SUFFIX) > +endif > gfxshared_s is also a static library, using it in gtk module is > through EXTRA_DSO_LDOPTS += -lgfxshared_s. It will link correctly if added via EXTRA_DSO_LDOPTS but the makefile will not recognize changes to the libgfxft2_s.a library. When changes are made to files that are part of the static library it will rebuild but the dll will not. Thus checkins that cause the static lib to rebuild will not be seen by other developers using cvs unless/until they manually delete the dll or do a full rebuild. > #if (defined(MOZ_ENABLE_FREETYPE2)) > nsresult rv; > mAvailableFontCatalogService = PR_FALSE; > - rv = nsFreeType::InitGlobals(); > + rv = nsFreeType2::InitGlobals(); > > shouldn't invoke nsFreeType2::InitGlobals() here. Initializing code > should be placed in constructor of nsFreeType2. -- see above. done > @@ -153,7 +160,7 @@ > /* destructor code */ > FreeGlobals(); > // assumption: no one else use nsFreeType > - nsFreeType::FreeGlobals(); > + nsFreeType2::FreeGlobals(); > shouldn't invoke nsFreeType2::FreeGlobals() here. Freeing code should be > placed in destructor of nsFreeType2. -- see above. done > + if (!InitGlobals(nsFreeType2::GetLibrary())) { > now, not all function of nsFreeType2 is wrapped by xpcom interface. Gtk > module uses both interface of nsIFreeType2 and public function of > nsFreeType2. Function such as GetLibrary, GetFTCacheManager can't be seen > by other module. Till now, GetFTCacheManager seem to be used when > accessing FT_Manager_Lookup_Size in ps module. This doesn't matter. So > can wrap GetFTCacheManager also in interface? done > - ffei = nsFreeType::GetCustomEncoderInfo(face->family_name); > + ffei = nsFreeType2::GetCustomEncoderInfo(face->family_name); > These function is also unseen outside gtk module. Since it seems not used > by ps module now, don't need wrap it in interface. This is ok. Okay, no change planned for this. > #define FT_CEIL(x) (((x) + 63) & -64) > @@ -137,6 +138,12 @@ > nsFreeTypeFont::NewFont(nsITrueTypeFontCatalogEntry *aFaceID, > PRUint16 aPixelSize, const char *aName) > { > + // Make sure FreeType is available > + if (!nsFreeType2::FreeTypeAvailable()) { > + NS_ERROR("FreeType2 routines not available"); > + return nsnull; > + } > + FreeTypeAailable is judge whether freetype2 exits. This work can be > done when GetService, if getting service succeed, it mean "FreeType2 > Available"; if "FreeType2 is not Available", getting service should fail > and won't use it afterwards. So "nsFreeType2::FreeTypeAvailable" should be > deleted. done

> - rely on the service manager to ensure a singleton instance, don't try to do > singleton management yourself done > - rely on getService() failing to determine if the service is available or not - > if you want your object to return an error, you should use > NS_GENERIC_FACTORY_CONSTRUCTOR_INIT() which allows you to call a > nsresult-returning function. This allows your object to return an error during > object creation - such as the ft2 library not being available. done > - don't use static objects (including static nsCOMPtrs) done except for the parts that need to be separated to nsFontFreeType

I've opened bug 181310 "move the nsFontFreeType variables from nsFreeType" to address the issue of moving the nsFontFreeType variables from nsFreeType to nsFontFreeType

Just one comment on the static nsCOMPtrs and such: just because something is in the tree doesn't justify adding more of that sort of thing (nor can you use it as a justification for not fixing something easy & obvious) - as an example, there are memory leaks in our code, but that doesn't justify writing leaky code. static nsCOMPtrs are banned, even though there are pleanty of them in the codebase..

Comment on attachment 107030 [details] [diff] [review] patch; with changes for comments This patch has fixed all issues and is quite well. seeking sr=

Comment on attachment 107030 [details] [diff] [review] patch; with changes for comments I'm really not a fan of the methods in the nsIFreeType2 interface - if you're going to make an XPCOM interface, then at minimum you should use XPCOM conventions in the method names. It looks more like there are 2-3 different interfaces that you really need here, such as nsIFreeType2Service and nsIFreeType2Cache, which both would be implemented on the same object (i.e. nsFreeType2) then things like FT_Outline_Decompose() should be called "outlineDecompose" and will become nsIFreeType2Service::OutlineDecompose() in C++. For instance, look at this line: + mFt2->FT_Set_Charmap(face, face->charmaps[i], &fterror); yuck! that would look much nicer as mFt2->SetCharmap(...) Also, this is more of a "would be nice", but wouldn't it be cleaner to do conversions between fterror and nsresult, so you don't have to keep passing around "out" pointers, and could instead call rv = ft2->SetCharmap(..); and check the rv? I dunno, just a thought. Also, there are a few already defined types such as voidPtr to mean "void*" - use them when they're available (look at other IDL if you're not sure)

I really don't have a strong opinion on this. The question is: when using XPCOM to wrap a 3rd party library should the methods more or less closely reflect the method names of the library that is being wrapped vs the XPCOM naming conventions? I can certianly add additional interfaces. Could you expound on the advantages of doing so for the cache methods?

Alec: this is a diff between attachment 107030 [details] [diff] [review] and attachment 107983 [details] [diff] [review] it shows the differences but is somewhat hard to read so it may not be that useful

Comment on attachment 107983 [details] [diff] [review] patch with revised method name and return values since there were only API changes and no functional changes I'm carrying forward Louie's r=

this is the core of the change diff -N gfx/idl/nsIFreeType2.idl -+[ptr] native const_char_p(const char); ++ ++[ptr] native constCharPtr(const char); -+[ptr] native void_p(void); -+ FT_Error FT_Done_Face(in FT_Face face); -+ FT_Error FT_Done_FreeType(in FT_Library lib); -+ void FT_Done_Glyph(in FT_Glyph glyph); -+ FT_UInt FT_Get_Char_Index(in FT_Face face, in FT_ULong charcode); -+ FT_Error FT_Get_Glyph(in FT_GlyphSlot slot, out FT_Glyph glyph); -+ void_p FT_Get_Sfnt_Table(in FT_Face face, in FT_Sfnt_Tag tag); -+ void FT_Glyph_Get_CBox(in FT_Glyph glyph, in FT_UInt mode, -+ out FT_BBox box); -+ FT_Error FT_Init_FreeType(out FT_Library lib); -+ FT_Error FT_Load_Glyph(in FT_Face face, in FT_UInt gindex, in FT_Int flags); -+ FT_Error FT_New_Face(in FT_Library lib, in const_char_p filename, -+ in FT_Long face_num, out FT_Face face); -+ FT_Error FT_Outline_Decompose(in FT_Outline_p outline, -+ in const_FT_Outline_Funcs_p funcs, -+ in void_p p); -+ FT_Error FT_Set_Charmap(in FT_Face face, in FT_CharMap charmap); -+ FT_Error FTC_Image_Cache_Lookup(in FTC_Image_Cache cache, -+ in FTC_Image_Desc_p desc, -+ in FT_UInt gindex, out FT_Glyph glyph); -+ FT_Error FTC_Manager_Lookup_Size(in FTC_Manager manager, in FTC_Font font, -+ out FT_Face face, out FT_Size size); -+ void FTC_Manager_Done(in FTC_Manager manager); -+ FT_Error FTC_Manager_New(in FT_Library lib, in FT_UInt max_faces, -+ in FT_UInt max_sizes, in FT_ULong max_bytes, -+ in FTC_Face_Requester requester, -+ in FT_Pointer req_data, out FTC_Manager manager); -+ FT_Error FTC_Image_Cache_New(in FTC_Manager manager, -+ out FTC_Image_Cache cache); ++ void doneFace(in FT_Face face); ++ void doneFreeType(in FT_Library lib); ++ void doneGlyph(in FT_Glyph glyph); ++ FT_UInt getCharIndex(in FT_Face face, in FT_ULong charcode); ++ void getGlyph(in FT_GlyphSlot slot, out FT_Glyph glyph); ++ voidPtr getSfntTable(in FT_Face face, in FT_Sfnt_Tag tag); ++ void glyphGetCBox(in FT_Glyph glyph, in FT_UInt mode, out FT_BBox box); ++ void initFreeType(out FT_Library lib); ++ void loadGlyph(in FT_Face face, in FT_UInt gindex, in FT_Int flags); ++ void newFace(in FT_Library lib, in constCharPtr filename, ++ in FT_Long face_num, out FT_Face face); ++ void outlineDecompose(in FT_Outline_p outline, ++ in const_FT_Outline_Funcs_p funcs, in voidPtr p); ++ void setCharmap(in FT_Face face, in FT_CharMap charmap); ++ void imageCacheLookup(in FTC_Image_Cache cache, in FTC_Image_Desc_p desc, ++ in FT_UInt gindex, out FT_Glyph glyph); ++ void managerLookupSize(in FTC_Manager manager, in FTC_Font font, ++ out FT_Face face, out FT_Size size); ++ void managerDone(in FTC_Manager manager); ++ void managerNew(in FT_Library lib, in FT_UInt max_faces, ++ in FT_UInt max_sizes, in FT_ULong max_bytes, ++ in FTC_Face_Requester requester, in FT_Pointer req_data, ++ out FTC_Manager manager); ++ void imageCacheNew(in FTC_Manager manager, out FTC_Image_Cache cache); the rest is just what was need to accomadate this

Comment on attachment 107983 [details] [diff] [review] patch with revised method name and return values sr=alecf

Comment on attachment 107983 [details] [diff] [review] patch with revised method name and return values oops, actually mark that.

checked in

Change QA contact to Brian to verify this change. Otherwise please provide test case then I can do the verification.

Yuying: to verify this would you enable truetype and check that it still works? http://www.mozilla.org/projects/fonts/unix/enabling_truetype.html

Verified the true type font is displayed fine in 12-04 trunk build / RH7.2.