106488 - Make window CMAP code readable

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P4)

Description

[Frank Tang]

When I try to fix 90804 I copy the GetCMAP code from Window to mac. I got some
review comment which suggest we make the code readable. Therefore, I create this
bug to make sure we also fix the window code.
Following comment copy from bug 90804, which also apply to window code:

------- Additional Comments From Mike Pinkerton 2001-08-21 16:46 -------
- I don't really care for macros. Use inline functions or c++ const decls 
instead. Macros don't give you the benefit of type-checking and they're a pain to 
wade through when there are compiler errors.
- you shouldn't be using PR_Malloc(). Use nsMemory::Alloc()/Free() instead.

------- Additional Comments From Mike Pinkerton 2001-10-23 10:11 -------
a couple of comments:

- there are several places where you increment by a constant. I'm assuming those
are sizes of characters or longs. use sizeof(PRUnichar) or sizeof(long) (or
whatever is correct) instead of the bare constants. If not, then comment why
you're adding the constant.
    PRUint8* p = buf + 2;
    ...
    p += 2;
    PRUint16 encodingID = GET_SHORT(p);
    p += 2;
    offset = GET_LONG(p);
    p += 4;

- use enums/constants for the format and platformID codes rather than just using
the comment to tell what they mean in FillFontInfoFromCMap()


------- Additional Comments From Mike Pinkerton 2001-10-23 15:09 -------

> See nsFontMetricsWin.cpp . The code is copy from there. 

well then perhaps we should fix them both, or add comments. Why are you adding
2? What's the significance of 2?

> There are no meaning enums or constants for the format ID

There might not be apple-defined constants, but that doesn't stop you from
creating your own. makes the code much more readable.

------ Additional Comments From Jean-Marc Desperrier 2001-10-24 07:20 -------

I checked how this is handled in other Mozilla module.

There's a IS_BIG_ENDIAN and a IS_LITTLE_ENDIAN constant defined.

Suggested code replacement for both nsMacUnicodeFontInfo.cpp and  
nsFontMetricsWin.cpp so they will be synchronized :

#ifdef IS_BIG_ENDIAN 
# undef GET_SHORT
# define GET_SHORT(p) (*((PRUint16*)p))
# undef GET_LONG
# define GET_LONG(p)  (*((PRUint32*)p))
#else
# ifdef IS_LITTLE_ENDIAN 
#  undef GET_SHORT
#  define GET_SHORT(p) (((p)[0] << 8) | (p)[1])
#  undef GET_LONG
#  define GET_LONG(p) (((p)[0] << 24) | ((p)[1] << 16) | ((p)[2] << 8) | (p)[3])
# endif
#endif

Comment 1

[Frank Tang]

Mark it m.0.96 P4.

Comment 2

[Frank Tang]

Attachment: patch

Comment 3

[Frank Tang]

Attachment: patch v2, also change GetGlyphIndecies and change malloc and free

Comment 4

[Frank Tang]

Jean-Marc Desperrier: can you r= this ?

Comment 5

[Jean-Marc Desperrier]

After some more thinking, I'm afraid the current IS_BIG_ENDIAN code is only 
garanteed to work on PPC, they allow unaligned reads in BIG_ENDIAN, but most 
BIG_ENDIAN processors will not.

If the format does not garantee all the reads will be aligned, we should add a 
comment, or switch to a variant that will never do unaligned reads.

Comment 6

[Frank Tang]

read http://developer.apple.com/fonts/TTRefMan/RM06/Chap6.html#Types
>TrueType Font files: an overview
>
>A TrueType font file consists of a sequence of concatenated tables. A table is
>a sequence of words. Each table must be long aligned and padded with zeroes if 
>necessary.

Does that answer your question? Also, I am not intend to put these MACRO to a xp 
direction, but only put under Mac and Window. 

For Linux, we will depend on FreeType to do most of the job for us in the futre. 
CC bstell.

Comment 7

[Jean-Marc Desperrier]

> Does that answer your question?
Yes.

Comment 8

[Frank Tang]

Jean-Marc Desperrier :
can you r= this ?

Comment 9

[Frank Tang]

shanjian- can you r= this one?

Comment 10

[Shanjian Li]

frank, in block 1229, 1262, can you replace 
   } // if (platformID == eTTPlatformIDMicrosoft) { 
with 
   } // if (platformID == eTTPlatformIDMicrosoft)
It does no harm, but it might confuse editor when searching corresponding pair 
of "{" and "}". No need to resubmit your patch. r=shanjian

Comment 11

[Frank Tang]

ok, I remove two { inside the // comment
in my local tree.

Comment 12

[Frank Tang]

sfraser, can you sr= this ?

Comment 13

[Simon Fraser [no longer active]]

-  mSubsets = (nsFontSubset**) PR_Calloc(mSubsetsCount, sizeof(nsFontSubset*));
+  mSubsets = (nsFontSubset**) nsMemory::Alloc(mSubsetsCount* 
sizeof(nsFontSubset*));

-        font->mSubsets = (nsFontSubset**)PR_Calloc(1, sizeof(nsFontSubset*));
+        font->mSubsets = (nsFontSubset**)nsMemory::Alloc(sizeof(nsFontSubset*));

Calloc will clear, nsMemory::Alloc might not. Does this matter?

Comment 14

[Jean-Marc Desperrier]

I don't know if you still care, but AFAIC :
r=jmd
BTW, Afternoon in West Coast => sleep in Europe.

Comment 15

[Frank Tang]

ok, I change
-  mSubsets = (nsFontSubset**) PR_Calloc(mSubsetsCount, sizeof(nsFontSubset*));
+  mSubsets = (nsFontSubset**) nsMemory::Alloc(mSubsetsCount* 
sizeof(nsFontSubset*));
   if (!mSubsets) {

to
-  mSubsets = (nsFontSubset**) PR_Calloc(mSubsetsCount, sizeof(nsFontSubset*));
+  mSubsets = (nsFontSubset**) nsMemory::Alloc(mSubsetsCount* 
sizeof(nsFontSubset*));
   if (!mSubsets) {
+  memset(mSubsets, 0, mSubsetsCount* sizeof(nsFontSubset*));

and change 
-        font->mSubsets = (nsFontSubset**)PR_Calloc(1, sizeof(nsFontSubset*));
+        font->mSubsets = 
(nsFontSubset**)nsMemory::Alloc(sizeof(nsFontSubset*));
         if (font->mSubsets) {
to
-        font->mSubsets = (nsFontSubset**)PR_Calloc(1, sizeof(nsFontSubset*));
+        font->mSubsets = 
(nsFontSubset**)nsMemory::Alloc(sizeof(nsFontSubset*));
         if (font->mSubsets) {
+          font->mSubsets[0] = nsnull;

see patch v3.

Comment 16

[Frank Tang]

sorry, it is not
-  mSubsets = (nsFontSubset**) PR_Calloc(mSubsetsCount, sizeof(nsFontSubset*));
+  mSubsets = (nsFontSubset**) nsMemory::Alloc(mSubsetsCount* 
sizeof(nsFontSubset*));
   if (!mSubsets) {
+  memset(mSubsets, 0, mSubsetsCount* sizeof(nsFontSubset*));

I add that 
+  memset(mSubsets, 0, mSubsetsCount* sizeof(nsFontSubset*));

after that block of if (!mSubsets) {
not IN the block

see the new attachment

Comment 17

[Frank Tang]

Attachment: v3 of patch

Comment 18

[Frank Tang]

sfraser- can you sr= it again?

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 55267 [details] [diff] [review]
v3 of patch

sr=sfrasre

Comment 20

[Frank Tang]

fix and check in