The printing code needs to check if the glyph is available in the font list for this element and if not add it to the font list for this elements.

taking

Comment on attachment 110292 [details] [diff] [review] patch that adds font fill in this patch depends on bug 144668 and is intended to be applied after http://bugzilla.mozilla.org/attachment.cgi?id=109613&action=view is applied.

Comment on attachment 110431 [details] [diff] [review] patch with some fixes this patch depends on bug 144668 and is intended to be applied after http://bugzilla.mozilla.org/attachment.cgi?id=109613&action=view is applied.

this patch depends on bug 144668 and is intended to be applied after http://bugzilla.mozilla.org/attachment.cgi?id=109613&action=view is applied.

Comment on attachment 110564 [details] [diff] [review] combine 2 pieces and a minor fix This patch works fine for me. I have a few comments 1) > #include "nsReadableUtils.h" > #ifdef MOZ_ENABLE_FREETYPE2 > #include "nsType8.h" >+#include "nsIFontCatalogService.h" nsIFontCatalogService.h already has been included in nsIFontMetricsPS.h 2) > nscoord > nsFontPSAFM::GetWidth(const char* aString, PRUint32 aLength) > { >- nscoord width; >+ nscoord width = 0; > if (mAFMInfo) { > mAFMInfo->GetStringWidth(aString, width, aLength); function GetStringWidth in nsAFMObject will init the width. We don't need to do it here. 3) >- nsCOMPtr<nsITrueTypeFontCatalogEntry> entry; >- rv = FindFontEntry(aFont, lang, getter_AddRefs(entry)); >- NS_ENSURE_SUCCESS(rv, nsnull); >- NS_ENSURE_TRUE(entry, nsnull); >+ return PR_FALSE; Shall we return PR_TRUE? Otherwise we will always return PR_FALSE >+} 4) >+ if (state == 0) { >+ FIND_FONTPS_PRINTF(("get the CSS specified entries for the element's " >+ "language")); >+ aFont.EnumerateFamilies(nsFontPSFreeType::CSSFontEnumCallback, &fpi); >+ } >+ else if (state == 1) { Can we use switch here? 5) >+nsFontPSFreeType::FindFont(PRUnichar aChar, const nsFont& aFont, >+nsFontPSFreeType::CSSFontEnumCallback(const nsString& aFamily, PRBool aGeneric, String arguments to functions should be declared as nsAString. 6) >+ fontps *fps = new fontps; >+ NS_ENSURE_TRUE(fps, NS_ERROR_FAILURE); Should return NS_ERROR_OUT_OF_MEMORY here Louie, you should take a look at this patch and give your review comments since some codes are written by you. Thanks

> 1) ... > >+#include "nsIFontCatalogService.h" > nsIFontCatalogService.h already has been included in nsIFontMetricsPS.h Done, removed include. > 2) > > nscoord > > nsFontPSAFM::GetWidth(const char* aString, PRUint32 aLength) > > { > >- nscoord width; > >+ nscoord width = 0; > > if (mAFMInfo) { > > mAFMInfo->GetStringWidth(aString, width, aLength); > function GetStringWidth in nsAFMObject will init the width. We don't need to do > it here. The initialization is to cover the case where the GetStringWidth function is not called. If we know that mAFMInfo will always be non-null then we should remove the if test. > 3) > >- nsCOMPtr<nsITrueTypeFontCatalogEntry> entry; > >- rv = FindFontEntry(aFont, lang, getter_AddRefs(entry)); > >- NS_ENSURE_SUCCESS(rv, nsnull); > >- NS_ENSURE_TRUE(entry, nsnull); > >+ return PR_FALSE; > Shall we return PR_TRUE? Otherwise we will always return PR_FALSE Sure, done. > 4) > >+ if (state == 0) { > >+ FIND_FONTPS_PRINTF(("get the CSS specified entries for the element's " > >+ "language")); > >+ aFont.EnumerateFamilies(nsFontPSFreeType::CSSFontEnumCallback, &fpi); > >+ } > >+ else if (state == 1) { > Can we use switch here? This feels gratuitous but I made the change. > 5) > >+nsFontPSFreeType::FindFont(PRUnichar aChar, const nsFont& aFont, I do not see a string in this call. > >+nsFontPSFreeType::CSSFontEnumCallback(const nsString& aFamily, PRBool aGeneric, > String arguments to functions should be declared as nsAString. Changing this would require changing the signature of nsFont::EnumerateFamilies which would require changes in many other files including the Win/Mac code. If you feel that this is important would you mind opening a separate bug for this? > 6) > >+ fontps *fps = new fontps; > >+ NS_ENSURE_TRUE(fps, NS_ERROR_FAILURE); > Should return NS_ERROR_OUT_OF_MEMORY here Done.

Comment on attachment 110593 [details] [diff] [review] revised patch >> >+ else if (state == 1) { >> Can we use switch here? >This feels gratuitous but I made the change. Maybe it's faster than (if/else). >> >+nsFontPSFreeType::FindFont(PRUnichar aChar, const nsFont& aFont, >I do not see a string in this call. Sorry, I made a mistake, I meant another place of CSSFontEnumCallback. You don't need to change it. >> function GetStringWidth in nsAFMObject will init the width. We don't need to do >> it here. >The initialization is to cover the case where the GetStringWidth function is >not called. If we know that mAFMInfo will always be non-null then we should >remove the if test. OK. r=pete

Comment on attachment 110593 [details] [diff] [review] revised patch The patch looks quite good. Finding font for each char instead of finding font using only font Description can ensure each char to display correctly. r = louie (1) > for (; *aString; aString++) { > if (*aString == ' ') >+ *aString = '_'; >+ else if (*aString == '(') >+ *aString = '_'; >+ else if (*aString == ')') > *aString = '_'; > } maybe "if ((*aString == " ") || (*aString == "(") || (*aString == ")"))" is better since the action under all condictions is the same. (2) >- FONT_CATALOG_PRINTF(("%s", "matching")); >+ FONT_CATALOG_PRINTF(("%0x\t%-20s\t%08lx\t%08lx\t%i\t%i\t%08lx\t%08lx", >+ fce->mFlags, >+ fce->mFamilyName, >+ fce->mCodePageRange1, >+ fce->mCodePageRange2, >+ fce->mWeight, >+ fce->mWidth, >+ fce->mStyleFlags, >+ fce->mFaceFlags)); the original code is to display all the font catalog entries and indicate which one is matching. It's for debugging "FontCatalogService". Since "FontCatalogService" code is stable and the output of such debugging is really too much. This change can be applied.

> (2) > >- FONT_CATALOG_PRINTF(("%s", "matching")); > > >+ FONT_CATALOG_PRINTF(("%0x\t%-20s\t%08lx\t%08lx\t%i\t%i\t%08lx\t%08lx", > >+ fce->mFlags, > >+ fce->mFamilyName, > >+ fce->mCodePageRange1, > >+ fce->mCodePageRange2, > >+ fce->mWeight, > >+ fce->mWidth, > >+ fce->mStyleFlags, > >+ fce->mFaceFlags)); > the original code is to display all the font catalog entries and indicate > which one is matching. It's for debugging "FontCatalogService". Since > "FontCatalogService" code is stable and the output of such debugging is > really too much. This change can be applied. I'm unclear on what you would like here. Are you asking that this not be applied?

I mean your patch is quite ok.

->Brian

now that bug 144668 has been checked in make this a standard patch

Comment on attachment 111685 [details] [diff] [review] make diff easier to apply; no functional changes carry forward Louie's r=

Comment on attachment 111685 [details] [diff] [review] make diff easier to apply; no functional changes - In nsRenderingContextPS.cpp: + nsCOMPtr<nsIAtom> langGroup = nsnull; Unnecessary initialization of a nsCOMPtr, they default to null, no need to do it in the code. + mPSObj->setlanggroup(langGroup.get()); No need for the .get() there. Other than that, and the other issues already mentioned in this bug, sr=jst

checked in

True type printing is working on 01-21 trunk build / Linux RH7.2, mark this as verified.