Closed Bug 166773 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: Printing: Output, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Louie.Zhao, Assigned: Louie.Zhao)

References

Details

Attachments

(15 files, 3 obsolete files)

11.30 KB, text/plain
Details
120.96 KB, patch
Details | Diff | Splinter Review
5.12 KB, text/html
Details
611 bytes, patch
bstell
: review+
Details | Diff | Splinter Review
4.79 KB, patch
bstell
: review+
Details | Diff | Splinter Review
2.31 KB, patch
bstell
: review+
Details | Diff | Splinter Review
11.60 KB, patch
bstell
: review+
Details | Diff | Splinter Review
3.43 KB, patch
bstell
: review+
Details | Diff | Splinter Review
9.50 KB, patch
bstell
: review+
Details | Diff | Splinter Review
2.76 KB, patch
bstell
: review+
Details | Diff | Splinter Review
3.94 KB, patch
bstell
: review+
Details | Diff | Splinter Review
28.23 KB, patch
bstell
: review+
Details | Diff | Splinter Review
5.92 KB, patch
bstell
: review+
Details | Diff | Splinter Review
43.07 KB, patch
bstell
: review+
Details | Diff | Splinter Review
5.92 KB, patch
bstell
: review+
Details | Diff | Splinter Review
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)
Attached patch first proposed patch (obsolete) — Splinter Review
Blocks: 144664
Attached file Brian's comments
Brian's comments about the first proposed patch.
Attached patch patch following brian's comments (obsolete) — Splinter Review
seeking r=
Attachment #97907 - Attachment is obsolete: true
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.
Roland: please take a look at this. We are reorganizing as a first step before
starting the truetype printing work.
Attached patch new patchSplinter Review
refined patch following Brian's suggestion (mainly add "moving some reading
preference code from nsFontMetricsGTK to nsFreeType")
Attachment #98575 - Attachment is obsolete: true
Attached file new patch [HTML version] (obsolete) —
Hope this detailed description of what the patch had done can help
(super)viewer to look through the patch(attachment 98877 [details] [diff] [review]).
Attachment #99014 - Attachment is obsolete: true
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.
Attachment #99116 - Flags: review+
Attachment #99117 - Flags: review+
Attachment #99118 - Flags: review+
Attachment #99119 - Flags: review+
Attachment #99120 - Flags: review+
Attachment #99121 - Flags: review+
Attachment #99122 - Flags: review+
Attachment #99123 - Flags: review+
Attachment #99125 - Flags: review+
Attachment #99128 - Flags: review+
Attachment #99126 - Flags: review+
Attachment #99124 - Flags: review+
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 on attachment 99117 [details] [diff] [review]
patch for gtk/nsFontMetricsGTK.cpp

sr=jst
Attachment #99117 - Flags: superreview+
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
Attachment #99116 - Flags: superreview+
Attachment #99118 - Flags: superreview+
Comment on attachment 99119 [details] [diff] [review]
patch for x11shared/nsFT2FontCatalog.cpp

sr=jst
Attachment #99119 - Flags: superreview+
Attachment #99120 - Flags: superreview+
Attachment #99121 - Flags: superreview+
Attachment #99122 - Flags: superreview+
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
Attachment #99123 - Flags: superreview+
Attachment #99124 - Flags: superreview+
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
Attachment #99125 - Flags: superreview+
Attachment #99126 - Flags: superreview+
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
Attachment #99128 - Flags: superreview+
With those comments, sr=jst for the whole thing.
checked in trunk by pete.zha@sun.com
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: