make nsFreeType a xpcom service and move from shared lib to static lib

VERIFIED FIXED

Status

()

Core
Internationalization
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: kill this account, Assigned: kill this account)

Tracking

Trunk
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
Created attachment 106706 [details] [diff] [review]
patch; makes libgfxft2 a static lib, makes nsFreeType a xpcom service
(Assignee)

Updated

16 years ago
Attachment #106706 - Flags: review?(Louie.Zhao)
(Assignee)

Updated

16 years ago
Attachment #106706 - Flags: superreview?(alecf)

Comment 2

16 years ago
Created attachment 106779 [details]
comment on patch 106706

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

Comment 3

16 years ago
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)
(Assignee)

Comment 4

16 years ago
Created attachment 107030 [details] [diff] [review]
patch; with changes for comments

> +#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
(Assignee)

Updated

16 years ago
Attachment #106706 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107030 - Flags: review?(Louie.Zhao)
(Assignee)

Comment 5

16 years ago
> - 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
(Assignee)

Comment 6

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

Comment 7

16 years ago
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..

(Assignee)

Updated

16 years ago
Blocks: 144669

Comment 8

16 years ago
Comment on attachment 107030 [details] [diff] [review]
patch; with changes for comments

This patch has fixed all issues and is quite well. 
seeking sr=
Attachment #107030 - Flags: review?(Louie.Zhao) → review+

Updated

16 years ago
Attachment #107030 - Flags: superreview?(alecf)
(Assignee)

Updated

16 years ago
Attachment #106706 - Flags: superreview?(alecf)

Comment 9

15 years ago
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)
(Assignee)

Comment 10

15 years ago
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?
(Assignee)

Comment 11

15 years ago
Created attachment 107983 [details] [diff] [review]
patch with revised method name and return values
Attachment #107030 - Attachment is obsolete: true
(Assignee)

Comment 12

15 years ago
Created attachment 107984 [details]
diff between attachment 107030 [details] [diff] [review] and attachment 107983 [details] [diff] [review]

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
(Assignee)

Comment 13

15 years ago
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=
Attachment #107983 - Flags: superreview?(alecf)
Attachment #107983 - Flags: review+
(Assignee)

Comment 14

15 years ago
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
(Assignee)

Updated

15 years ago
Attachment #107030 - Flags: superreview?(alecf)

Comment 15

15 years ago
Comment on attachment 107983 [details] [diff] [review]
patch with revised method name and return values

sr=alecf

Comment 16

15 years ago
Comment on attachment 107983 [details] [diff] [review]
patch with revised method name and return values

oops, actually mark that.
Attachment #107983 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 17

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Attachment #106706 - Flags: review?(Louie.Zhao)

Comment 18

15 years ago
Change QA contact to Brian to verify this change.  Otherwise please provide test
case then I can do the verification.
QA Contact: ylong → bstell
(Assignee)

Comment 19

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

Comment 20

15 years ago
Verified the true type font is displayed fine in 12-04 trunk build / RH7.2.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Blocks: 296439
You need to log in before you can comment on or make changes to this bug.