144666 - Glyph Fill In and Font Fallback

Status: VERIFIED FIXED

(Core :: Internationalization, enhancement)

Description

[kill this account]

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.

Comment 1

[Pete Zha]

taking

Comment 2

[kill this account]

Attachment: patch that adds font fill in

Comment 3

[kill this account]

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 4

[kill this account]

Attachment: patch with some fixes

Comment 5

[kill this account]

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.

Comment 6

[kill this account]

Attachment: a bit missing from the patch

Comment 7

[kill this account]

Attachment: combine 2 pieces and a minor fix

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 8

[Pete Zha]

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

Comment 9

[kill this account]

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

[kill this account]

Attachment: revised patch

Comment 11

[Pete Zha]

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 12

[Louie Zhao]

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.

Comment 13

[kill this account]

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

Comment 14

[Louie Zhao]

I mean your patch is quite ok.

Comment 15

[Pete Zha]

->Brian

Comment 16

[kill this account]

Attachment: make diff easier to apply; no functional changes

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

Comment 17

[kill this account]

Comment on attachment 111685 [details] [diff] [review]
make diff easier to apply; no functional changes

carry forward Louie's r=

Comment 18

[Johnny Stenback (:jst)]

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

Comment 19

[kill this account]

checked in

Comment 20

[Yuying Long]

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