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

Mark it m.0.96 P4.

Jean-Marc Desperrier: can you r= this ?

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.

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.

> Does that answer your question? Yes.

Jean-Marc Desperrier : can you r= this ?

shanjian- can you r= this one?

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

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

sfraser, can you sr= this ?

- 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?

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

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.

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

sfraser- can you sr= it again?

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

fix and check in