Build ID: CVS build, pulled on 2001-10-27 after the Mach-O changes landed on the trunk Steps to reproduce: 1) Build FizzillaMach with --enable-mathml (I can't try FizzillaCFM, but I imagine it has the same problem.) 2) Install the Mathematica fonts and MT-Extra 3) Start Mozilla 4) Examine the tests at http://www.mozilla.org/projects/mathml/fonts/encoding/ Actual results: The special math chars from the math fonts don't get used. The chars either show up as question marks or get rendered using Lucida Grande (or Symbol?). The math fonts get treated as normal MacRoman fonts. Expected results: Expected the math fonts to work the way they do on Windows and on GTK platforms. Additional information: ucvmath get registered as a component. Also, the mathfont property files get loaded. The converter code does not appear to run. See also the thread titled "Font tests? (MathML on Mac OS X)" in n.p.m.mathml.

I have no clue what is Mach-O build.

FizzillaMach is a version of Mozilla for Mac OS X that uses the Unix back end and the Carbon front end and is built using gcc/make instead of CodeWarrior. http://www.mozilla.org/ports/fizzilla/Mach.html My guess is that the problem with ucvmath also shows up in the CodeWarrior/CFM build and the Mac Classic build, but I am unable to verify, because I can't build those.

This seems a Mac MATHML issue. you are welcome to fix it but I won't spend time there for now. Sorry. rbs may help you

Marking helpwanted. If someone wants to take this from me, please go ahead.

Mac OS X 10.1.2 appears to have built-in knowledge about the Wolfram and Design Science fonts not being MacRoman fonts. I haven't yet been able to figure out how to use the fonts with OS X's own Unicode implementation, but TextEdit now falls back to Lucida Grande if I try to apply Math1 to a-z letters.

ATSUI font rendering might help here.

Should this bug be marked dependent on bug 121540?

here's a patch to enable ucvmath on mac platforms. i've only tested it on osx, but i have no reason to believe it wouldn't work on os9. i've also only tested it with the mathematica fonts and not the tex fonts. to make this work you'll need to copy "gfx/src/windows/fontEncoding.properties" into "MozillaDebug.app/Contents/MacOS/res/fonts/". in addition to ucvmath support this patch also enables faster functions to get bounding metrics than those i submitted for bug #74821 (http://bugzilla.mozilla.org/show_bug.cgi?id=74821). so mathml mostly works now. there are still (at least) 2 problems which may require separate bug reports: 1. characters with large descents (and possible ascents too) get clipped improperly when the page scrolls. 2. some spanning characters have gaps in them. (p.s. i apologize for submitting this patch with ie so it doesn't work for windows users, but mozilla gives me errors when i try to upload the file)

this is the orignal patch plus fixes for two bounding metrics issues. the first was a problem with the atsui fallback where two characters measured sequentially with the same font/style/size would have the same bounding metrics even though they shouldn't have. atsui was not properly invalidating it's internal caches, so i explicitly invalidated them. the second issue was with the default mechanism where the right side bearing was not being calculated properly and was off by a few pixels in some cases. now it is calculated properly and matches the behavior of other platforms.

Comment on attachment 92003 [details] [diff] [review] original patch + fixes for a couple of bounding metrics issues =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsATSUIUtils.cpp,v + // ATSUSetTextPointerLocation won't invalidate atsui's internal cache if aCharPt is + // the same address it already has. therefore, since we are definitely changing the + // text here, we should explicitly invalidate any existing caches + ::ATSUClearLayoutCache(oLayout, kATSUFromTextBeginning); Shouldn't you clear the cache *before* doing the setup, i.e, + ::ATSUClearLayoutCache(oLayout, kATSUFromTextBeginning); err = ATSUSetTextPointerLocation( oLayout, (ConstUniCharArrayPtr)aCharPt, 0, 1, 1); Index: nsUnicodeFontMappingMac.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeFontMappingMac.cpp,v +#ifdef MOZ_MATHML [...] +static NS_DEFINE_CID(kCharsetConverterManagerCID, NS_ICHARSETCONVERTERMANAGER_CID); + +static nsIPersistentProperties* gFontEncodingProperties = nsnull; +static nsICharsetConverterManager2* gCharsetManager = nsnull; +static class nsFontMappingCleanupObserver *gFontCleanupObserver = nsnull; +GetEncoding(const char* aFontName, nsString& aValue) +GetConverter(const char* aFontName, nsIUnicodeEncoder** aConverter) +GetCCMapThroughConverter(nsIUnicodeEncoder *converter) [...] +#endif // MOZ_MATHML You can migrate all these changes (and perhaps the other following converter-related changes) here: http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsMacUnicodeFontInfo.cpp Some of what you added already exist over there, so it will be less duplication. That file was a partial attempt to re-use some of the capabilities that we have for Windows. And since the chunk of code being added here also comes from the Windows version, we may as well merge over there. If so, the #idef MOZ_MATHML could be removed since that code has stood the test of time, and might be used for other purposes (e.g., on Windows, it is enabled by default and is used to support fonts such as Webdings, and people such as ftang & shanjian are familiar with it too). @@ -83,19 +266,23 @@ - MyFontEnumData(nsIDeviceContext* aDC, PRInt8* pMap, short* pFonts) : mContext(aDC) { + MyFontEnumData(nsUnicodeFontMappingMac* fontMapping, nsIDeviceContext* aDC) : mContext(aDC) { Keep "nsIDeviceContext* aDC" as the first argument. @@ -123,22 +310,62 @@ - data->mContext->GetLocalFontName(aFamily, realFace, aliased); +#ifdef MOZ_MATHML + char buffer[256]; + if(NS_SUCCEEDED(GetConverter(aFamily.ToCString(buffer, 256), + getter_AddRefs(mSymbolicFontConverter)))) + { + mSymbolicFontNum = fontNum; + } +#endif // MOZ_MATHML Use |realFace|, and you can change the signature of GetConverter() to pass nsString& if it works better that way. But... apart from these notes, there is a fundamental architectural that is missing from your patch. (Sorry, part of this is my fault since I didn't highlighted it when I was giving the indications... I didn't want to overwhelm you when you were still trying to get the patch to work at all.). The statement |mSymbolicFontNum = fontNum| assumes that there is only one symbolic font. In reality, there can be (and usually there is) a list of fonts, e.g., "Math1, CMEX10, Math3". So you need to keep a list of fonts, so that you can switch over them (in that CSS order) when looking for glyphs, while knowing that not all fonts in the list are necessarily symbolic. With your current patch, only the first symbolic font will interact. Another thing is that the character maps (mSymbolicFontCCMap) need to be cached across fonts/windows/tabs, otherwise they may consume an inordinate amount of memory (especially in MathML where rendering involves numerous fonts wich may only change by sizes in sup/subscripts, etc). The maps don't change when the font sizes change. So having a hashtable is necessary. For this, you need a global nsHashTable, or to be more precise nsObjectHashtable in this case (there are several flavors of hashtable in Mozilla but these are the easiest to use and are okay here), e.g, ============= static nsObjectHashtable* gFontMaps = nsnull; // to be new'ed in InitGlobals() and deleted in FreeGlobals() // callback function to free the entries when you are done static PRBool PR_CALLBACK freeCCMap(nsHashKey *aKey, void *aData, void *closure) { FreeCCMap((PRUInt16*)aData); return PR_TRUE; } // create gFontMaps = new nsObjectHashtable(nsnull, nsnull, freeCCMap, nsnull); // add PRUInt16* fontCCmap = GetCCMap(fontName); nsStringKey hashkey(fontName); gFontMaps->Put(&hashkey, fontCCmap); // retrieve nsStringKey hashkey(fontName); PRUInt16* fontCCmap = (PRUInt16*)gFontMaps->Get(&hashkey); // destroy (will loop over the entries and invoke your callback) delete gFontMaps; ============= The converters (nsIUnicodeEncoder) are already cached by the Charset Converter Manager. So no special action needed here. I will let you have another iteration over the patch before making further comments I may have. It is coming along very well.

here's a revised patch that incorporates the previous suggestions. as far as the atsui cache clearing goes the order of those two functions does not matter and produces the same results either way. ATSUSetTextPointerLocation will never do any layout and therefore will never validate any internal atsui caches. i can change it if you want me to, but it does work correctly as is.

Comment on attachment 92250 [details] [diff] [review] revised patch > i can change it if you want me to, but it does work correctly as is. yes, I feel more comfortable with clearing something before operating on it. (tip: since you tested the two ways, reviewers like it when you retain the one preferred by the reviewer rather than undoing what you experimented to revert to what you had). + if(!gCharsetManager) + { + nsServiceManager::GetService(kCharsetConverterManagerCID, + NS_GET_IID(nsICharsetConverterManager2), (nsISupports**) &gCharsetManager); + } + + nsCOMPtr<nsIAtom> charset; + rv = gCharsetManager->GetCharsetAtom(value.get(), getter_AddRefs(charset)); Missing null-check after trying to initialize gCharsetManager + if (mapper) + return MapperToCCMap(mapper); + else + return nsnull; Not all people like an 'else' after 'return', simply use: return mapper ? MapperToCCMap(mapper) : nsnull; + // make sure we have the hashtable + if(!gFontMaps) + gFontMaps = new nsObjectHashtable(nsnull, nsnull, freeCCMap, nsnull); Missing null-check, and return NS_ERROR_OUT_OF_MEMORY if the |new| failed + // if it's not already in the hashtable, create it and add it to the hashtable + *aCCMap = GetCCMapThroughConverter(*aConverter); + gFontMaps->Put(&hashKey, *aCCMap); Missing null-check before adding to the hashtable +class nsSymbolicFontMac I think there are issues with your patch re:CSS order. Looking at the code below, your always look for a glyph in the symbolic fonts. So if somebody wants to render <font face="Times, Symbol">Hello Plus: +</font>, the "+" sign is going to come from the "Symbol" font, whereas the intention is that the "+" sign should come from the "Times" font. In general, your patch will have troubles when characters exist in several fonts (overlapping). + short getFontID(PRUnichar aChar) + { + if(mCCMap && CCMAP_HAS_CHAR(mCCMap, aChar)) + return mFontNum; + else if(mNextFont) + return mNextFont->getFontID(aChar); + else + return BAD_FONT_NUM; + } + + PRBool convert(const PRUnichar *aString, PRInt32 *aStringLength, char *aBuffer, PRInt32 *aBufferLength) + { + if(mConverter && NS_SUCCEEDED(mConverter->Convert(aString, aStringLength, + aBuffer, aBufferLength)) && *aBufferLength) + { + return PR_TRUE; + } + else if(mNextFont) + return mNextFont->convert(aString, aStringLength, aBuffer, aBufferLength); + else + return PR_FALSE; =========================================================== Here is what you might do to consolidate your patch: 1.a Declare a font ================== class nsFontMac { public: nsFontMac(nsIUnicodeEncoder *aConverter, PRUint16 *aCCMap, short aFontNum, short aScriptNum) { // Note: either the font is symbolic (i.e., non-null converter) // or it is non-symbolic (i.e, valid script value) NS_ASSERTION(aConverter || aScriptNum != BAD_SCRIPT, "internal error"); mConverter = aConverter; mCCMap = aCCMap; mFontNum = aFontNum; mScriptNum = aScriptNum; } // the font whose number equals aFontNum does the conversion PRBool Convert(const PRUnichar *aString, PRInt32 *aStringLength, char *aBuffer, PRInt32 *aBufferLength) { if (mConverter) { // non-null means this is a symbolic font return NS_SUCCEEDED(mConverter->Convert(aString, aStringLength, aBuffer, aBufferLength)) && *aBufferLength); } //...add code to do the conversion with mScriptNum NS_ASSERTION(mScriptNum != BAD_SCRIPT, "internal error"); UnicodeToTextInfo converter = GetConverterByScript(mScriptNum); if (converter) { ... etc ... } return PR_FALSE; } // Note: use Caps (AddFont, GetFontID, Convert) in C++. The practice in // Mozilla is generally to reserve interCaps for IDL or JavaScript functions. public: nsCOMPtr<nsIUnicodeEncoder> mConverter; PRUint16 *mCCMap; // we don't own this buffer, so don't free it short mFontNum; short mScriptNum; }; 1.b Declare a font list ======================= class nsUnicodeFontMappingMac { [...] private: +#ifdef MOZ_MATHML + nsAutoVoidArray mFontList; #endif PRInt8 mPrivBlockToScript [kUnicodeBlockVarScriptMax] ; short mScriptFallbackFontIDs [smPseudoTotalScripts] ; static nsUnicodeMappingUtil* gUtil; }; 2. Change ConvertUnicodeToGlyphs() to pass aFontNum instead =========================================================== +PRBool nsUnicodeFontMappingMac::ConvertUnicodeToGlyphs(aFontNum, + ConstUniCharArrayPtr aString, ByteCount aStringLength, + char *aBuffer, ByteCount aBufferLength, ByteCount& oActualLength, + ByteCount& oBytesRead, OptionBits opts) +{ [...] +#ifdef MOZ_MATHML for (PRint32 i = 0; i < mFontList.Count(); i++) { nsFontMac* font = (nsFontMac*)mFontList[i]; if (aFontNum == font->FontNum) return font->Convert(...); } +#endif // MOZ_MATHML ++++ with the fix to GetConverterByScript() to accept aFontNum and to do ++++ the Right Thing (tm) with it... + UnicodeToTextInfo converter = GetConverterByScript(aScript); + if(converter) + { + OSStatus err = ::ConvertFromUnicodeToText(converter, 2 * aStringLength, aString, + opts, 0, NULL, 0, NULL, + aBufferLength, &oBytesRead, &oActualLength, + (LogicalAddress) aBuffer); + + return (oActualLength > 0 ? PR_TRUE : PR_FALSE); + } + return PR_FALSE; +} 3. nsUnicodeFontMappingMac::GetFontID(aChar) =========================================== This needs care. There seems to be some voodoo going on in the existing code using the 'script' of the font. To recover the current behavior while allowing symbolic fonts to participate in their corresponding CSS order, I think you can do: a) get the first symbolic font that can represent the char b) get the first non-symbolic font that can represent the char b) the winner is the one that is encountered first in the CSS list. Programmatically, this means doing the following tweaks: short nsUnicodeFontMappingMac::GetFontID(PRUnichar aChar) { #ifdef MOZ_MATHML short i = BAD_FONT_NUM; PRInt32 k = -1; for (PRint32 f = 0; f < mFontList.Count(); f++) { nsFontMac* font = (nsFontMac*)mFontList[f]; if (font->mCCMap && CCMAP_HAS_CHAR(font->mCCMap, aChar)) { i = font->mFontNum; k = f; break; } } nsUnicodeBlock block = GetBlock(aChar); if (block < kUnicodeBlockFixedScriptMax) { j = mScriptFallbackFontIDs[gUtil->BlockToScript(block)]; if (i != BAD_FONT_NUM) { // there is also a symbolic font with the char for (PRint32 f = 0; f < mFontList.Count(); f++) { nsFontMac* font = (nsFontMac*)mFontList[f]; if (j == font->mFontNum) { return k < f ? i : j; // winner is the first in the list } } } } if (i == BAD_FONT_NUM) i = mScriptFallbackFontIDs[ mPrivBlockToScript[ block - kUnicodeBlockFixedScriptMax] ]; return i; #else // old code nsUnicodeBlock block = GetBlock(aChar); if(block < kUnicodeBlockFixedScriptMax) return mScriptFallbackFontIDs[gUtil->BlockToScript(block)]; else return mScriptFallbackFontIDs[ mPrivBlockToScript[ block - kUnicodeBlockFixedScriptMax] ]; #endif } 4. Initialization: all fonts (symbolic or non-symbolic) go in the list (substitute/mSymbolicFont/mFontList/) to allow honoring the CSS order ====================================================================== i.e., rewrite this part of the patch accordingly. Non-symbolic will just have a null converter and ccmap. ScriptCode script = BAD_SCRIPT; nsCOMPtr<nsIUnicodeEncoder> converter; PRUint16 *ccmap = nsnull; res = nsMacUnicodeFontInfo::GetConverterAndCCMap(realFace, getter_AddRefs(converter), &ccmap))) if (NS_FAILED(res)) { script = ::FontToScript(fontNum); } nsFontMac *font = new nsFontMac(converter, ccmap, fontNum, script); if (font) mFontList.Append(font); === part of the old patch to rewrite === + aDeviceContext->GetLocalFontName(aFamily, realFace, aliased); + if (aliased || (NS_OK == aDeviceContext->CheckFontExistence(realFace))) { short fontNum; nsDeviceContextMac::GetMacFontNumber(realFace, fontNum); if(0 != fontNum) { +#ifdef MOZ_MATHML + nsCOMPtr<nsIUnicodeEncoder> aConverter; + PRUint16 *aCCMap; + + if(NS_SUCCEEDED(nsMacUnicodeFontInfo::GetConverterAndCCMap(realFace, + getter_AddRefs(aConverter), &aCCMap))) + { + nsSymbolicFontMac *aSymbolicFont = new nsSymbolicFontMac(aConverter, aCCMap, fontNum); + if(mSymbolicFont) + mSymbolicFont->addFont(aSymbolicFont); + else + mSymbolicFont = aSymbolicFont; + } +#endif // MOZ_MATHML ScriptCode script = ::FontToScript(fontNum); - if(BAD_FONT_NUM == data->mFont[ script ]) - data->mFont[ script ] = fontNum; - FillVarBlockToScript( script, data->mMap); + if(BAD_FONT_NUM == mScriptFallbackFontIDs[ script ]) + mScriptFallbackFontIDs[ script ] = fontNum; + FillVarBlockToScript( script, mPrivBlockToScript); } } 5. HasGlyphFor ============== in nsUnicodeRenderingToolkit, there is code to do ||info.HasGlyphFor(*aCharPt))) before doing anything else. It looks like this function is excluding symbolic fonts (or always pushing them to the fallback code-path). Double-check here. It seems that the code of HasGlyphFor() has to be revised to account for symbolic fonts. ===================================================================== +void nsUnicodeRenderingToolkit::GetScriptTextBoundingMetrics( + const char* buf, + ByteCount aLen, + ScriptCode aScript, + nsBoundingMetrics& oBoundingMetrics) +{ + Point frac = { 1, 1 }; + Fixed *widths = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed)); + Fixed *lefts = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed)); + Rect *rects = (Rect*) nsMemory::Alloc(aLen * sizeof(Rect)); + OSStatus err; It is often the case there is only a single character passed to the function, and you will be allocating all the time. So for efficiency, you can use stack variables and only allocate/free on the heap if the length exceeds what is on the stack. + nsBoundingMetrics bounds; + bounds.Clear(); No need to clear. The constructor is already doing it. + bounds.leftBearing = rects[glyphIndex].left + FixRound(lefts[glyphIndex]); + bounds.rightBearing = rects[glyphIndex].right + FixRound(lefts[glyphIndex]); + bounds.ascent = rects[glyphIndex].bottom; + bounds.descent = -rects[glyphIndex].top; + bounds.width = FixRound(widths[glyphIndex]); Any reason to round the numbers here. Doing so might amplify misalignments, and leave gaps when positionning glyphs === Awaiting next iteration.

typo... I missed an early return in the pseudo-code: nsUnicodeBlock block = GetBlock(aChar); if (block < kUnicodeBlockFixedScriptMax) { j = mScriptFallbackFontIDs[gUtil->BlockToScript(block)]; if (i != BAD_FONT_NUM) { // there is also a symbolic font with the char for (PRint32 f = 0; f < mFontList.Count(); f++) { nsFontMac* font = (nsFontMac*)mFontList[f]; if (j == font->mFontNum) { return k < f ? i : j; // winner is the first in the list } } } + return j; }

>5. HasGlyphFor >============== >in nsUnicodeRenderingToolkit, there is code to do >||info.HasGlyphFor(*aCharPt))) >before doing anything else. > >It looks like this function is excluding symbolic fonts (or always pushing >them to the fallback code-path). Double-check here. It seems that the >code of HasGlyphFor() has to be revised to account for symbolic fonts. this is only used in the fallback case where we're not using any special mappings for symbolic characters. >Any reason to round the numbers here. Doing so might amplify >misalignments, and leave gaps when positionning glyphs the numbers that were being rounded were of type Fixed, which is a type that has 16 bits of integer data and 16 bits of decimal data. the FixRound() function was convertering them to integers. i see your point though, so i did scale all the values by mP2T so we get sub-pixel accuracy. >Awaiting next iteration. here you go.

sorry, added: + return firstNonSymbolicFont; to comply with your second message

Is there any connection between these various patches for MathML on Fizzilla and the ATSUI work on bug 121540 (that will be resurrected, eventually)?

it seems to me that they are fairly independent of one another.

Comment on attachment 92453 [details] [diff] [review] 2002.07.23 patch #2 Only a few remaining nits now. Index: nsUnicodeFontMappingMac.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeFontMappingMac.cpp,v +#ifdef MOZ_MATHML +#include "nsMacUnicodeFontInfo.h" +#endif // MOZ_MATHML You might loose the #ifdef here. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> + PRBool HasChar(PRUnichar aChar) + { + return (mCCMap && CCMAP_HAS_CHAR(mCCMap, aChar)); + } This might causes confusion later, as font->HasChar() is undefined for a non-symbolic font in the list. I suggest having a GetCCMap(), and ket the caller use CCMAP_HAS_CHAR() on the result. BTW, mind using GetFontNum() rather than just FontNum(). >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -static PRBool FontEnumCallback(const nsString& aFamily, PRBool aGeneric, void *aData) +static PRBool RealFontEnumCallback(const nsString& aFamily, PRBool aGeneric, void *aData) +{ + MyFontEnumData* data = (MyFontEnumData*) aData; + return data->mFontMapping->FontEnumCallback(aFamily, aGeneric, data->mContext); +} + +PRBool nsUnicodeFontMappingMac::FontEnumCallback( + const nsString& aFamily, PRBool aGeneric, nsIDeviceContext *aDeviceContext) Why bother with an extra hop. I think retaining/upgrading the earlier callaback is fine. Plus, keeping it static makes it more robust against future changes in the class/params, since it forces people to take a closer look. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> + nsCOMPtr<nsIUnicodeEncoder> aConverter; + PRUint16 *aCCMap = nsnull; + ScriptCode script = ::FontToScript(fontNum); + +#ifdef MOZ_MATHML + // if this fails, aConverter & aCCMap will be nsnull + nsMacUnicodeFontInfo::GetConverterAndCCMap(realFace, getter_AddRefs(aConverter), &aCCMap); +#endif // MOZ_MATHML Use |converter|, |ccmap|. The 'a' prefix is conventionnally used to denote 'a'rguments of functions >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> + PRInt32 firstSymbolicFontIndex = mFontList.Count(); Simply use firstSymbolicFontIndex = -1; (or, if you like, = 0, and use it as the runnng index -- but that will make the code a bit more verbose...). Index: nsUnicodeFontMappingMac.h =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeFontMappingMac.h,v +#ifdef MOZ_MATHML +#include "nsIUnicodeEncoder.h" +#endif // MOZ_MATHML Do you need this include? The .ccp file is including the header. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> + if(aLen > STACK_TREASHOLD) + { + widths = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed)); + lefts = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed)); + rects = (Rect*) nsMemory::Alloc(aLen * sizeof(Fixed)); Missing null-checks: if (!widths || !lefts || !rects) { if (widths) nsMemory::Free(widths); if (lefts) nsMemory::Free(lefts); if (rects) nsMemory::Free(rects); return NS_ERROR_OUT_OF_MEMORY; } + } >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Did you try to compile with MathML turned off to verify that it doesn't the build, and is back to the previous behavior?

-> re-assigning to schofield@wolfram.com who is fixing the bug.

Little extra... + ScriptCode script = ::FontToScript(fontNum); + +#ifdef MOZ_MATHML + // if this fails, aConverter & aCCMap will be nsnull + nsMacUnicodeFontInfo::GetConverterAndCCMap(realFace, getter_AddRefs(aConverter), &aCCMap); +#endif // MOZ_MATHML Should be: +#ifdef MOZ_MATHML + // if this fails, aConverter & aCCMap will be nsnull + nsMacUnicodeFontInfo::GetConverterAndCCMap(realFace, getter_AddRefs(converter), &ccmap); +#endif // MOZ_MATHML + ScriptCode script = (converter)? BAD_SCRIPT : ::FontToScript(fontNum); (to enforce the invariant that the script of a symbolic font is undefined)

>Why bother with an extra hop. I think retaining/upgrading the earlier >callaback is fine. Plus, keeping it static makes it more robust >against future changes in the class/params, since it forces people to >take a closer look. it's nice to have access to the private instance variables. i changed it to a static class function, is that acceptable? >Did you try to compile with MathML turned off to verify that it doesn't >the build, and is back to the previous behavior? i'll get back to you on this.

>>Did you try to compile with MathML turned off to verify that it doesn't >>the build, and is back to the previous behavior? > >i'll get back to you on this. i have verified that this code still builds and functions properly with mathml disabled (via - -disable-mathml flag to configure script).

Comment on attachment 92573 [details] [diff] [review] 2002.07.24 patch > i changed it to a static class function, is that acceptable? Yes. Patch looks good and is ready to go IMO, now that you have also verified that non-MathML builds don't break. Could you do the following tweak to bullet-proof the function and ensure that when the function succeeds, it indeed means that both 'converter' *and* 'ccmap' are there. +nsresult +nsMacUnicodeFontInfo::GetConverterAndCCMap(const nsString& aFontName, nsIUnicodeEncoder** aConverter, + PRUint16** aCCMap) +{ + if(NS_SUCCEEDED(GetConverter(aFontName, aConverter)) && *aConverter) + { + // make sure we have the hashtable + if(!gFontMaps) + { + gFontMaps = new nsObjectHashtable(nsnull, nsnull, freeCCMap, nsnull); + if(!gFontMaps) + return NS_ERROR_OUT_OF_MEMORY; + } + + // first try to retrieve the ccmap for this font from the hashtable + nsStringKey hashKey(aFontName); + *aCCMap = (PRUint16*) gFontMaps->Get(&hashKey); + if(!*aCCMap) + { + // if it's not already in the hashtable, create it and add it to the hashtable + *aCCMap = GetCCMapThroughConverter(*aConverter); + if(!*aCCMap) { >>>>> *aConverter = nsnull; >>>>> return NS_ERROR_FAILURE; } gFontMaps->Put(&hashKey, *aCCMap); >>>>>> return NS_OK; + } + } >>>> return NS_ERROR_FAILURE; +}

bullet-proofed.

Oops... misplaced 'return' + // first try to retrieve the ccmap for this font from the hashtable + nsStringKey hashKey(aFontName); + *aCCMap = (PRUint16*) gFontMaps->Get(&hashKey); + if(!*aCCMap) + { + // if it's not already in the hashtable, create it and add it to the hashtable + *aCCMap = GetCCMapThroughConverter(*aConverter); + if(!*aCCMap) + { + *aConverter = nsnull; + return NS_ERROR_FAILURE; + } + gFontMaps->Put(&hashKey, *aCCMap); ------ return NS_OK; + } ++++++ return NS_OK; + } Also, while you are at it, I will just go ahead and merge these two since 'rv' isn't propagated and nobody cares about its precise value from there on. + nsresult rv = GetConverter(aFontName, aConverter); + if(NS_SUCCEEDED(rv) && *aConverter) + if(NS_SUCCEEDED(GetConverter(aFontName, aConverter)) && *aConverter)

done.

Comment on attachment 92636 [details] [diff] [review] 2002.07.24 patch #3 r=rbs

+ ScriptCode script = (converter ? BAD_FONT_NUM : ::FontToScript(fontNum)); Should'nt this be BAD_SCRIPT ?

Awaiting sr=, will ask drivers approval afterwards. (After letting the patch to bake a little while, it might be a good idea to loose some of the #idef MOZ_MATHML to allow everyone to benefit from this support of arbitrary symbolic fonts.)

Comment on attachment 92661 [details] [diff] [review] patch to be checked in rob, I was wondering about this scaling: +void nsUnicodeRenderingToolkit::GetScriptTextBoundingMetrics( [...] + // scale the metrics by mP2T so everything is in twips rather than points + Point numer = { mP2T, mP2T }, denom = { 1, 1 }; mP2T is a |float| and can be a fractional number on certain devices/display resolutions. Since the fields numer.h and number.v are integers, the compiler will round the value when doing the assignment. So rather than trying to do the scaling deep inside the low-level functions, let's just propagate the native values, and then do the scaling on the final result -- as GetTextDimensions() is currently doing. Updated patch coming up.

i don't have strong feelings one way or the other about this issue, though i considered it a fair amount while implementing it. the only issue with not putting in the scaling factor is that if there are fractional values they will be completely lost and rounded to the nearest integer by ::OutlineMetrics(). the integer number of points can then be scaled to app units, but we're not really gaining any resolution by doing so. is it better to always go with integer units that will always be off for fractional values or go with fractional units that may be off by a tiny bit in some cases (when i was testing i think mP2T was 15.0 so it wouldn't have been off)?

The value of mP2T = 15.0 that you saw was particular to your environment. There could be a fractional number on certain devices or display resolutions. If I remember correctly (haven't double-checked now...), it can be set to an arbitrary value when the user plays with the slider of "Display resolution" on the Edit -> Appearance -> Fonts preference dialog. A further iteration could be to try a scale={1000,1000}, or something that isn't necessarily related to this uncertain mP2T, and do a fix-up w.r.t. to that scale, while still keeping the conversion to twips at the last minute. Alternatively, the helper functions don't necessarily have to return an integer nsBoundingMetrics struct. Indeed the Get[TextSegment]BoundingMetrics functions could be changed to return the native |Fixed *aLeftBearing, Fixed *aRightBearing, etc...| and then do the fix-up to twips at the last minute. There is room for exprimentation. Bottom-line is: I am okay with any iteration that you see fit to collapse the sub-pixel gaps later on (i.e., not necessarily in this bug), including reverting to scale=max{mP2T,1}, max{mP2T,1}}.

Does OS X allow sub-pixel positioning of the beginning of a rendered string? That is, are individually rendered chars locked to full pixels anyway? (Does the rounding decision affect printing noticeably?) A big, big thank you for implementing this!

The GDI/QuickDraw/ATSUI/etc are bound to have round-off errors, and there is no way to ensure perfection in all situations. However, round-off errors in a given font-engine can hopefully be expected to be uniform within the same font. Take for example the usual text run. It will use glyphs that are designed to be drawn horizontally, and they will invariably seat neatly on the baseline (as designed) if they are drawn at the same y-coordinate, whether or not y is a fractional number of pixels. Similarly, if two partial glyphs have been designed to be juxtaposed vertically, then drawing them at the same x-coordinate will keep them neatly aligned vertically (as the font designer intended -- except for those broken & poorly designed fonts that are out there and that cause jagged rendering). It can indeed be seen on the screenshot that the gap comes from incrementing the y-coordinate. But to accommodate the Mac, I might possibly further tweak the stretching code so that it picks a delta-y considerably less than the height of the glyph to further tighten the overlap between the parts. So as I said, there is room for experimentation. Let's just avoid premature optimization.

sfraser, did you a chance to follow-up? Cc:ing some drivers as we need traction if the patch is to stand a chance for inclusion in 1.1final, while letting a little space for clearing any dust that might come from the checkin (including possibly fixing the pixel gap in juxtaposing glyphs, as seen e.g. in Torture Test #14 on attachment 92867 [details] [diff] [review]).

s/did you a chance/did you had a chance/

I'll try and get to this tomorrow.

Comments: Index: nsMacUnicodeFontInfo.cpp =================================================================== +static nsIPersistentProperties* gFontEncodingProperties = nsnull; +static nsICharsetConverterManager2* gCharsetManager = nsnull; +static nsObjectHashtable* gFontMaps = nsnull; +static nsFontCleanupObserver *gFontCleanupObserver = nsnull; +static PRUint16* gCCMap = nsnull; Can these statics be pushed inside of factory methods? +static nsresult +GetEncoding(const nsString& aFontName, nsString& aValue) +{ ... + // if we have not init the property yet, init it right now. + if (! gFontEncodingProperties) It would be nice to move the gFontEncodingProperties into a separate function, like EnsureFontEncodingProperties(); + rv = NS_OpenURI(getter_AddRefs(in), uri); I'm a little unhappy using NS_OpenURI here, for the reason that it can block the UI thread (see comments at http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsNetUtil.h#181 ). This might hurt startup time. And doesn't this add a new dependency between gfx and necko? + return (*aConverter)->SetOutputErrorBehavior((*aConverter)->kOnError_Replace, nsnull, '?'); This is a bit ugly. Cleaner to store the converter in a local before returning it. +// This function uses the charset converter manager to fill the map for the +// font whose name is given +static PRUint16* +GetCCMapThroughConverter(nsIUnicodeEncoder *converter) The comment should say who owns the returned data, and who's responsible for deleting it. + if(gFontMaps) + delete gFontMaps; if (gCCMap) - FreeCCMap(gCCMap); You don't need 'if' tests here. C++ specifies that 'delete NULL' is OK. Index: nsMacUnicodeFontInfo.h =================================================================== + static nsresult GetConverterAndCCMap(const nsString& aFontName, nsIUnicodeEncoder** aConverter, PRUint16** aCCMap); Add a comment to say who owns the returned aCCMap. +static UnicodeToTextInfo gConverters[32] = { + nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, + nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, + nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, + nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull, nsnull +}; This could be written as static UnicodeToTextInfo gConverters[32] = {0}. C will zero out members with non-specified initializers. + if ((sc == smArabic) || (sc == smHebrew)) + return nsnull; + NS_PRECONDITION(sc < 32, "illegal script id"); Maybe we need a macro like IS_BIDI_SCRIPT() ? Whitespace before the NS_PRECONDITION(). + if(sc >= 32) Standardize 'if' spacing to: "if (" + TextEncoding scriptEncoding; + err = ::UpgradeScriptInfoToTextEncoding(sc, kTextLanguageDontCare, kTextRegionDontCare, nsnull, &scriptEncoding); + if ( noErr == err ) + err = ::CreateUnicodeToTextInfoByEncoding(scriptEncoding, &gConverters[sc] ); Do these converters ever get freed? + nsUnicodeFontMappingEntry( + nsIUnicodeEncoder *aConverter, + PRUint16 *aCCMap, + short aFontNum, + ScriptCode aScript) + { + NS_ASSERTION(aConverter || aScript != BAD_SCRIPT, "internal error"); + mConverter = aConverter; + mCCMap = aCCMap; + mFontNum = aFontNum; + mScript = aScript; + } Please use C++ initializers: nsUnicodeFontMappingEntry(...) : mConverter(aConverter) , mCCMap(aCCMap) ... + PRBool Convert( + const PRUnichar *aString, + ByteCount aStringLength, + char *aBuffer, + ByteCount aBufferLength, + ByteCount& oActualLength, + ByteCount& oBytesRead, + OptionBits opts) + { +#ifdef MOZ_MATHML + if(mConverter) + { + oActualLength = aBufferLength; + if(NS_SUCCEEDED(mConverter->Convert(aString, (PRInt32*) &aStringLength, aBuffer, + (PRInt32*) &oActualLength)) && oActualLength) Why do we need to do something different from MATHML here? Also, is this a new dependency between gfx and uconv? + oBytesRead = 2 * aStringLength; sizeof(PRUnichar) * aStringLength? + PRUint16* GetCCMap() If the CCMap is not owned, maybe make it a 'const PRUint16* everywhere, or even make a typedef for it. + for(PRInt32 i = 0; i < mFontList.Count(); i++) ... + if(aFontNum == entry->GetFontNum()) Spacing here and in lots of other places. + MyFontEnumData fontData(aDeviceContext, this); + aFont->EnumerateFamilies(nsUnicodeFontMappingMac::FontEnumCallback, &fontData); Whitespace (tabs?) + if(fontMapping.ConvertUnicodeToGlyphs(scriptFallbackFonts[fallbackScript], aCharPt, 1, + buf, STACK_TREASHOLD, outLen, processBytes, kUnicodeLooseMappingsMask)) STACK_TREASHOLD spelled wrong (Yeah, I know it was there already. you might as well fix it). + GetScriptTextBoundingMetrics(buf,1,::FontToScript(fontNum),oBoundingMetrics); Spaces after commas please. + while(byteIndex < aLen) + { + nsBoundingMetrics bounds; + bounds.leftBearing = rects[glyphIndex].left + FixRound(lefts[glyphIndex]); + bounds.rightBearing = rects[glyphIndex].right + FixRound(lefts[glyphIndex]); + bounds.ascent = rects[glyphIndex].bottom; + bounds.descent = -rects[glyphIndex].top; + bounds.width = FixRound(widths[glyphIndex]); + + if(glyphIndex == 0) + oBoundingMetrics = bounds; + else + oBoundingMetrics += bounds; + + // for two byte characters byteIndex will increase by 2 + // while glyph index will only increase by 1 + if(CharacterByteType((Ptr) buf, byteIndex, aScript) == smFirstByte) + byteIndex += 2; + else + byteIndex++; + glyphIndex++; + } This is potentially an O(n^2) operation, since CharacterByteType() has to run through the entire string from start to byteIndex to get the byte type. If aLen is often large, it would be better to call FillParseTable() and then do table lookups.

[To win the time (time difference!), I went ahead and applied the changes]. > Can these statics be pushed inside of factory methods? Not much win in doing that. The variables are not exported. They remain local to that single file. Using factory methods might just add a bunch of functions which are called only once. > It would be nice to move the gFontEncodingProperties into a separate function, > like EnsureFontEncodingProperties(); Done (using InitEncodingProperties as we do on Windows). > > + rv = NS_OpenURI(getter_AddRefs(in), uri); > > I'm a little unhappy using NS_OpenURI here Switched to a string bundle to make you smile again :-) (Added a similar trick to what we do on Windows to avoid a startup hit). > + return (*aConverter)->SetOutputErrorBehavior((*aConverter)->kOnError_Replace, > nsnull, '?'); > > This is a bit ugly. Cleaner to store the converter in a local before returning it. Used a tmp (although I don't mind the other form). > The comment should say who owns the returned data, and who's responsible for > deleting it. Updated. > You don't need 'if' tests here. C++ specifies that 'delete NULL' is OK. Updated. > + static nsresult GetConverterAndCCMap(const nsString& aFontName, > > Add a comment to say who owns the returned aCCMap. Done. > This could be written as > static UnicodeToTextInfo gConverters[32] = {0}. > C will zero out members with non-specified initializers. Done (was inherited from exising code which is just copied/pasted to another location). > Maybe we need a macro like IS_BIDI_SCRIPT() ? Same existing code that is shuffled. Retained what was there. > Whitespace before the NS_PRECONDITION(). Updated (was inherited from exising code). > > + if(sc >= 32) > > Standardize 'if' spacing to: "if (" Apparently, it looks like schofield just followed the local custom over there. So no special action taken. > > + TextEncoding scriptEncoding; > + err = ::UpgradeScriptInfoToTextEncoding(sc, kTextLanguageDontCare, > kTextRegionDontCare, nsnull, &scriptEncoding); > + if ( noErr == err ) > + err = ::CreateUnicodeToTextInfoByEncoding(scriptEncoding, &gConverters[sc] ); > > Do these converters ever get freed? Yes (from the existing code). > Please use C++ initializers: Done. > +#ifdef MOZ_MATHML > + if(mConverter) > + { > + oActualLength = aBufferLength; > + if(NS_SUCCEEDED(mConverter->Convert(aString, (PRInt32*) > &aStringLength, aBuffer, > + (PRInt32*) &oActualLength)) && oActualLength) > > Why do we need to do something different from MATHML here? Have now removed the #ifdef that are secondary. > Also, is this a new dependency between gfx and uconv? There is already a dependency to handle the transliteration. > + PRUint16* GetCCMap() > If the CCMap is not owned, maybe make it a 'const PRUint16* everywhere, or even > make a > typedef for it. Didn't dare to do that as I can't compile to fix the const cast that might now be needed here and there... Suggesting not worrying about this since all consumers are internal utils. > Spacing here and in lots of other places. Same custom from Rome. > > + MyFontEnumData fontData(aDeviceContext, this); > + aFont->EnumerateFamilies(nsUnicodeFontMappingMac::FontEnumCallback, &fontData); > > Whitespace (tabs?) Removed. > STACK_TREASHOLD spelled wrong (Yeah, I know it was there already. you might as well > fix it). Corrected with a s/STACK_TREASHOLD/STACK_TRESHOLD/g. > GetScriptTextBoundingMetrics(buf,1,::FontToScript(fontNum),oBoundingMetrics); > > Spaces after commas please. Added. > This is potentially an O(n^2) operation, since CharacterByteType() has > to run through the entire string from start to byteIndex to get the > byte type. If aLen is often large, it would be better to call > FillParseTable() and then do table lookups. A string that is measured is the textual content of a DOM node, <tag>text</tag>, and such a text is usually a single character ('x', '&alpha') or a function name ('sin', 'cos'), c.f., e.g., the markup at: http://bugzilla.mozilla.org/attachment.cgi?id=79770&action=view So perf issues are not anticipated w.r.t. to the usage of CharacterByteType. (Perf issues instead arise due the rather deep hierarchy involved in MathML markup that lead to repeated calls -- although at ony one time, just a short text is measured).

Comment on attachment 93520 [details] [diff] [review] updated patch following sfraser's comments sr=sfraser (i'm trusting you).

Comment on attachment 93520 [details] [diff] [review] updated patch following sfraser's comments a=asa (on behalf of drivers) for checkin to 1.1

Checked in. Fingers crossed for build bustages...

Using the tests at [http://www.w3.org/Math/testsuite/], this doesn't appear to work using FizzillaCFM/2002080203. For example, the General/Math/Math1 test results in a Section Sign (&#x00a7;) being displayed.

Are the changes already in that build? The place to verify is here: http://www.mozilla.org/projects/mathml/fonts/encoding/ For example, if you click the link to Math1, you can see a screen capture of its native character map here: http://www.mozilla.org/projects/mathml/fonts/encoding/math1.gif And you can test if Mozilla is matching that by looking at this page: http://www.mozilla.org/projects/mathml/fonts/encoding/math1-encoding.html

Using FizzillaCFM/2002080303 to access [http://pear.math.pitt.edu/mathzilla/Examples/markupOftheWeek.mhtml] still results in a dialog warning about the Symbol font and, in fact, the MathML markup lays out correctly, but uses the wrong symbols in the blue-background areas.

Could you open another bug and attach screenshots for what you see (not everybody -- including myself, has a Mac). There is a known bug with GfxMac that schofield and I discussed. In order to fix bug 33258, a public API (CheckFontExistence) was changed on the Mac to special-case the Symbol font and to return that it doesn't exist: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&fi le=nsDeviceContextMac.cpp&root=/cvsroot&subdir=mozilla/gfx/src/mac&command=DIFF_ FRAMESET&rev1=1.71&rev2=1.72 But this breaks everybody that uses the API. You are getting the warning dialog because the Mac's CheckFontExistence() is erroneosuly reporting that the Symbol doesn't exist. We should find a cleaner fix to bug 33258 (perhaps by fixing the source of the bug which seems internal to nsUnicodeFontMappingMac rather than tweaking the public API, or perhaps by just removing the special-case now that symbolic fonts are supported in general). Could you file other bugs rather than reporting issues here? This bug has already served its original purpose which was to hook the math fonts.

Done, as bug 161048.