Glyph Fill In and Font Fallback

VERIFIED FIXED

Status

()

Core
Internationalization
--
enhancement
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: kill this account, Assigned: kill this account)

Tracking

({intl})

Trunk
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
Blocks: 144663

Updated

16 years ago
Keywords: intl
QA Contact: ruixu → ylong

Updated

16 years ago
QA Contact: ylong → kasumi

Comment 1

15 years ago
taking
Assignee: bstell → pete.zha
(Assignee)

Updated

15 years ago
Depends on: 144668
(Assignee)

Comment 2

15 years ago
Created attachment 110292 [details] [diff] [review]
patch that adds font fill in
(Assignee)

Comment 3

15 years ago
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.
Attachment #110292 - Attachment description: patch that add font fill in → patch that adds font fill in
Attachment #110292 - Flags: review?(pete.zha)
(Assignee)

Comment 4

15 years ago
Created attachment 110431 [details] [diff] [review]
patch with some fixes
Attachment #110292 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110292 - Flags: review?(pete.zha)
(Assignee)

Updated

15 years ago
Attachment #110431 - Flags: review?(pete.zha)
(Assignee)

Comment 5

15 years ago
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.
(Assignee)

Comment 6

15 years ago
Created attachment 110527 [details] [diff] [review]
a bit missing from the patch
(Assignee)

Comment 7

15 years ago
Created attachment 110564 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #110431 - Attachment is obsolete: true
Attachment #110431 - Flags: review?(pete.zha)
(Assignee)

Updated

15 years ago
Attachment #110527 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110564 - Flags: review?(pete.zha)

Comment 8

15 years ago
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
Attachment #110564 - Flags: review?(pete.zha) → review?(Louie.Zhao)
(Assignee)

Comment 9

15 years ago
> 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.
(Assignee)

Comment 10

15 years ago
Created attachment 110593 [details] [diff] [review]
revised patch
Attachment #110564 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110564 - Flags: review?(Louie.Zhao)
(Assignee)

Updated

15 years ago
Attachment #110593 - Flags: review?(Louie.Zhao)

Comment 11

15 years ago
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

15 years ago
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.
Attachment #110593 - Flags: review?(Louie.Zhao) → review+
(Assignee)

Comment 13

15 years ago
> (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

15 years ago
I mean your patch is quite ok.
(Assignee)

Updated

15 years ago
Attachment #110593 - Flags: superreview?(jst)

Comment 15

15 years ago
->Brian
Assignee: pete.zha → bstell
(Assignee)

Comment 16

15 years ago
Created attachment 111685 [details] [diff] [review]
make diff easier to apply; no functional changes

now that bug 144668 has been checked in make this a standard patch
Attachment #110593 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110593 - Flags: superreview?(jst)
(Assignee)

Comment 17

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

carry forward Louie's r=
Attachment #111685 - Flags: superreview?(jst)
Attachment #111685 - Flags: review+
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
Attachment #111685 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 19

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 20

15 years ago
True type printing is working on 01-21 trunk build / Linux RH7.2, mark this as
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.