166773 - rearrange the code of FreeType2 Font Catalog under mozilla/gfx/src/x11shared

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Louie Zhao]

The code under gfx/src/x11shared are shared by gtk, xlib module. Some of them
are independent on gtk(xlib)module (e.g. nsFreeType & FT2 Font Catalog) and some
of them are not(e.g. nsFreeTypeFont). The two parts should be placed seperately,
but unfortunately, they are messed altogether.

The design should be:

1. class nsFreeType: wrapper of FT2 function and library, no relation to gtk
module, placed in nsFreeType.cpp(h)

2. class nsFT2FontCatalog: implement getting FT2 Font Catalog, return Font
Catalog Entry. no relation to gtk module, placed nsFT2FontCatalog.cpp(h).

3. class nsFT2FontNode: newly created class. Convert Font Catalog Entry
collection to nsFontNode(defined and usedy by gtk-module) array. Originally be
part of nsFT2FontCatalog, placed in nsFT2FontNode.cpp(h). gtk-module related.

4. class nsFreeTypeFont: derived class of nsFontGTK, remove from
nsFreeType.cpp(h) to nsFontFreeType.cpp(h), gtk-module related.

This is a preparing work for bug 144664 (Font Catalog Service)

Comment 1

[Louie Zhao]

Attachment: first proposed patch

Comment 2

[Louie Zhao]

Attachment: Brian's comments

Brian's comments about the first proposed patch.

Comment 3

[Louie Zhao]

Attachment: patch following brian's comments

seeking r=

Comment 4

[Louie Zhao]

This is a brief introduction of this patch, hope it can help review and
super-review.

FreeType2-related part under gfx/src/x11shared mainly use FreeTyp2 function to
get font entry(s) and font glyph(s) from font file. These font entry(s) and
glyph(s) are used by other module, e.g. gfx/src/gtk. Some of these code are
FreeType2-related only, but some is gtk-module-related. So this patch mainly
seperates gtk-module-related part from gtk-module-unrelated part of these code
in mozilla/gfx/src/x11shared.

The motivation of this action is due to bug 144664 (Font Catalog Service). FCS
take search pattern (string) and return matching collection of font catalog
entry (fce).The service has no relation to any other module like gtk. Though the
original code has this functionality, such code has been messed up with other
gtk-module-related part. Rewritting the whole Font Catalog Service will make the
patch large and hard to (super)review. Then our solution is to 

1. rearrange (part of) the gfx/src/x11shared code.
2. using existing code to implement Font Catalog Service.

Rearranging code mainly includes following items:

1.nsFreeType.cpp(h): This file include 2 classes -  nsFreeType and
nsFreeTypeFont. nsFreeType is wrapper of FreeType2 library and function.
nsFreeTyepFont use function provided by nsFreeType to get font glyph.
nsFreeTypeFont is derived from nsFontGTK (belong to gtk module). So
nsFreeTypeFont must be seperated.I put nsFreeTypeFont in new file
nsFontFreeType.cpp(h), and leave gtk-unrelated part - nsFreeType in
nsFreeType.cpp(h).


2.nsFT2FontCatalog.cpp(h): This file include 1 class - nsFT2FontCatalog. It use
searching pattern to look for matching fce collection (what FCS should do), and
then convert fce to nsFontNode and return nsFontNode collection. nsFontNode is
defined in gtk module. I devided nsFT2FontCatalog into 2 parts.
   A.nsFT2FontCatalog (still the name): use searching pattern to look for
matching fce collection
   B.nsFT2FontNode : use nsFT2FontCatalog to get matching fce collection, then
convert to nsFontNode collection.

After these rearranging, nsFreeType and nsFT2FontCatalog are gtk-unrelated, and
can be wrapped to be Font Catalog Service later. nsFT2FontNode and
nsFreeTypeFont will use nsFreeType and nsFT2FontCatalog to provide things needed
by gtk module.

3.About Debug: Debugging code of font is placed in
gfx/src/gtk/nsFontMetricsGTK.h, there debugging code are used by gtk module and
x11shared, so I move them into x11shared/nsFontDebug.h, thus can be shared by
other module.

Comment 5

[kill this account]

Roland: please take a look at this. We are reorganizing as a first step before
starting the truetype printing work.

Comment 6

[Louie Zhao]

Attachment: new patch

refined patch following Brian's suggestion (mainly add "moving some reading
preference code from nsFontMetricsGTK to nsFreeType")

Comment 7

[Louie Zhao]

Attachment: new patch [HTML version]

Comment 8

[Louie Zhao]

Attachment: description of details of arrangement

Hope this detailed description of what the patch had done can help
(super)viewer to look through the patch(attachment 98877 [details] [diff] [review]).

Comment 9

[kill this account]

Attachment: patch for gtk/Makefile.in

Comment 10

[kill this account]

Attachment: patch for gtk/nsFontMetricsGTK.cpp

Comment 11

[kill this account]

Attachment: patch for gtk/nsFontMetricsGTK.h

Comment 12

[kill this account]

Attachment: patch for x11shared/nsFT2FontCatalog.cpp

Comment 13

[kill this account]

Attachment: patch for x11shared/nsFT2FontCatalog.h

Comment 14

[kill this account]

Attachment: patch for x11shared/nsFT2FontNode.cpp

Comment 15

[kill this account]

Attachment: patch for x11shared/nsFT2FontNode.h

Comment 16

[kill this account]

Attachment: patch for x11shared/nsFontDebug.h

Comment 17

[kill this account]

Attachment: patch for x11shared/nsFontFreeType.cpp

Comment 18

[kill this account]

Attachment: patch for x11shared/nsFontFreeType.h

Comment 19

[kill this account]

Attachment: patch for x11shared/nsFreeType.cpp

Comment 20

[kill this account]

Attachment: patch for x11shared/nsFreeType.h

Comment 21

[kill this account]

Because the patch (attachment 98877 [details] [diff] [review]) is long (~3700 lines) I have broken it down
into individual file patches so we can spread the super-review effort over more
than one person.

Comment 22

[kill this account]

r=[e-mail] for attachment 
r=[e-mail] for attachment 99116 [details] [diff] [review]
r=[e-mail] for attachment 99117 [details] [diff] [review]
r=[e-mail] for attachment 99118 [details] [diff] [review]
r=[e-mail] for attachment 99119 [details] [diff] [review]
r=[e-mail] for attachment 99120 [details] [diff] [review]
r=[e-mail] for attachment 99121 [details] [diff] [review]
r=[e-mail] for attachment 99122 [details] [diff] [review]
r=[e-mail] for attachment 99123 [details] [diff] [review]
r=[e-mail] for attachment 99124 [details] [diff] [review]
r=[e-mail] for attachment 99125 [details] [diff] [review]
r=[e-mail] for attachment 99126 [details] [diff] [review]
r=[e-mail] for attachment 99128 [details] [diff] [review]

Comment 23

[kill this account]

the changes are basically:

attachment 99126 [details] [diff] [review] : nsFreeType.cpp
attachment 99128 [details] [diff] [review] : nsFreeType.h
attachment 99124 [details] [diff] [review] : nsFontFreeType.cpp
attachment 99125 [details] [diff] [review] : nsFontFreeType.h
attachment 99117 [details] [diff] [review] : nsFontMetricsGTK.cpp

  The class nsFreeTypeFont is split into the part the interacts with
  the FreeType2 library (nsFreeType) and the part that measures/renders
  TrueType glyphs (nsFontFreeType). The nsFontFreeType code is moved to
  nsFontFreeType.cpp/h

  FreeType pref values are moved from nsFontMetricsGTK.cpp to nsFreeType.cpp

  The weight correction table is moved to nsFT2FontNode

attachment 99116 [details] [diff] [review] : gtk/Makefile.in

  nsFontFreeType.cpp and nsFT2FontNode.cpp are added to list of *.cpp files

attachment 99118 [details] [diff] [review] : nsFontMetricsGTK.h
attachment 99123 [details] [diff] [review] : nsFontDebug.h

  separate the font debug macros from nsFontMetricsGTK.h

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 99117 [details] [diff] [review]
patch for gtk/nsFontMetricsGTK.cpp

sr=jst

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 99116 [details] [diff] [review]
patch for gtk/Makefile.in

+		nsFT2FontNode.cpp \
		nsFT2FontCatalog.cpp \
		 nsX11AlphaBlend.cpp \
		 nsXFontAAScaledBitmap.cpp \

Looks like we have some inconsistent indentation in this makefile, please clean
this up when checking in.

sr=jst

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 99119 [details] [diff] [review]
patch for x11shared/nsFT2FontCatalog.cpp

sr=jst

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 99123 [details] [diff] [review]
patch for x11shared/nsFontDebug.h

...
+#define NS_FONT_DEBUG_FIND_FONT   0x04
+#define NS_FONT_DEBUG_SIZE_FONT   0x08
+#define NS_FONT_DEBUG_SCALED_FONT	 0x10
+#define NS_FONT_DEBUG_BANNED_FONT	 0x20
...

Maybe indent the values before 0x10 to line up with the rest?

sr=jst

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 99125 [details] [diff] [review]
patch for x11shared/nsFontFreeType.h

+#ifndef MAX
+#define MAX(a,b) ((a) > (b) ? (a) : (b))
+#endif
+#ifndef MIN
+#define MIN(a,b) ((a) < (b) ? (a) : (b))
+#endif

Nspr already defines PR_MAX() and PR_MIN(), can't those be used here too, or is
there a reason to duplicate those macros here?

sr=jst

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 99128 [details] [diff] [review]
patch for x11shared/nsFreeType.h

- In nsFreeType.h:

+#ifndef MAX
+#define MAX(a,b) ((a) > (b) ? (a) : (b))
+#endif
+#ifndef MIN
+#define MIN(a,b) ((a) < (b) ? (a) : (b))
+#endif

Same here, PR_MAX()/PR_MIN()?

sr=jst

Comment 30

[Johnny Stenback (:jst)]

With those comments, sr=jst for the whole thing.

Comment 31

[Louie Zhao]

checked in trunk by [e-mail]