149692 - Japanese font is displayed ugly on linux RH7.2

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Shanjian Li]

Copied from bugscape 12828.

Build: N6.2.1-JA and N6.2.2-JA on linux RH7.2

The Japanese font is dispplayed ugly with XUL on linux RH7.2, but with fonts for
web page contains inside the browser window are displayed fine. - see a followed
screen shot.

Linux RH7.1 and 6.2 don't have same problem.

This bug is filed for potentially might cause the problem on later linux RH
version with our future release JA product.

------- Additional Comment #4 From Brian Stell 2002-03-26 16:32 -------

This looks like moz is getting outline scaled glyphs from the X font server.

------- Additional Comment #13 From Shanjian Li 2002-06-04 14:30 -------

I spent more time on this and now I have a different understanding. 

Menu font (and together with other system fonts) is retrieved from GDK default
system font in nsDeviceContextGTK.cpp. The fontset for japaneese include
following 3 fonts:
-adobe-helvetica-medium-r-normal--14-100-100-100-p-76-iso8859-1
-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-0
-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1976-0

The code retrieve only font family and reconstruct font later using our own
routine. For above fonts set, we got "helvetica, fixed". Font resolution in our
code lead to loaded font of:
  -adobe-helvetica-medium-r-normal--14-140-75-75-p-77-iso8859-1
  -alias-helvetica-medium-r-normal--14-*-0-0-c-*-jisx0208.1983-0

So we end up with a scale font for japanese as bstell guessed. 

I have no good idea about how to fix this problem. Should we have a preference
for system font? That might be a solution. bstell?

Comment 1

[Shanjian Li]

I just figured out a solution. In our unix font code, we can take either font
family name or FFRE name. The later one contains more information and thus has
more binding power. patch will follow.

Comment 2

[Shanjian Li]

Attachment: patch

Comment 3

[Shanjian Li]

brian, what do you think of my patch? Could you give r=?

Comment 4

[R.K.Aa.]

Wasn't this a RedHat 7.2 bug in the fonts.alias that came with the ghostscript
fonts pacage for japanese fonts?

I believe it was fixed long ago with an RH errata for 7.2, thus should not need
any Mozilla specific workaround.
Similar: bug 120643

See RedHat:
http://rhn.redhat.com/errata/RHBA-2001-145.html
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=58154

Comment 5

[rbs]

Patch looks good to me, especially considering that the loss of information in
the reconstuction was the cause of a smoketest blocker a while ago (bug 102208).
By retaining the fully qualified as the patch is doing, it will probably provide
a safe and robust solution to the reported problem once for all.

Comment 6

[rbs]

Since a faithful reconstruction (to guarantee consistent XUL appearance in
various settings) has priority over anything else, why don't you replace the
other AppendFontName() with AppendFontFFREName() and remove AppendFontName()
altogether?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Wow, I didn't realize we supported FFRE names.  Perhaps we should use them for
the non-fontset case too?

Comment 8

[Shanjian Li]

Attachment: patch v2

Comment 9

[Shanjian Li]

Attachment: patch v2

Comment 10

[Shanjian Li]

I didn't use FFRE name for single font in the beginning because I was afraid
that this might cause undesired result for character outside font encoding. In
patch v2, I put both FFRE name and family name there. This should avoid the
possible problem by using FFRE only.

Comment 11

[rbs]

Comment on attachment 86813 [details] [diff] [review]
patch v2

r=rbs

+AppendFontFFREName(nsString& aString, const char* aFontName)

nit (if you like it):
s/aFontName/aXLFDName/

Comment 12

[Shanjian Li]

chris, could you sr?

Comment 13

[Shanjian Li]

I will rename the variable as suggested by rbs before check in. 

Comment 14

[Chris Waterson]

Comment on attachment 86813 [details] [diff] [review]
patch v2

>Index: nsDeviceContextGTK.cpp

When writing parsing routines like this, it is very helpful if you provide
an example of the content that you're expecting to parse (e.g., in a
comment). That makes it easier for people to understand what you're doing. :-)

>+AppendFontFFREName(nsString& aString, const char* aFontName)
>+{
>+  // convert fontname from XFLD to FFRE and append
>+  nsCAutoString nameStr;
>+  nameStr.Append(aFontName);
>+  PRInt32 pos1, pos2;
>+  pos1 = nameStr.FindChar('-');

Why do you need to do the above FindChar? The value isn't used before
it's clobbered by the FindChar, below.

>+  nameStr.Cut(0, 1);
>+  pos1 = nameStr.FindChar('-');

Are you sure you're always going to find a '-'? If not, pos1 == -1.

>+  pos1 = nameStr.FindChar('-', pos1+1);
>+  pos2 = pos1;
>+  for (PRInt32 i=0; i < 10; i++) {
>+    pos2 = nameStr.FindChar('-', pos2+1);

pos2 will become -1 if you run out of '-' characters. Ought you check for that?

>+  }
>+  nameStr.Cut(pos1, pos2-pos1);

What happens if you run out of '-', and pos2 is l.t. zero? Does Cut work
with a negative length?

>+  aString.AppendWithConversion(nameStr.get());
>+}
>+
>+static void
> AppendFontName(XFontStruct* aFontStruct, nsString& aString, Display *aDisplay)
> {
>   unsigned long pr = 0;
>+  // we first append the FFRE name to reconstruct font more faithfully
>+  unsigned long font_atom = gdk_atom_intern("FONT", FALSE);

What does this do? Is this a leak?

>+  ::XGetFontProperty(aFontStruct, font_atom, &pr);
>+  if(pr) {
>+    char* xlfdName = ::XGetAtomName(aDisplay, pr);
>+    AppendFontFFREName(aString, xlfdName);
>+    ::XFree(xlfdName);
>+  }
>+ 
>+  aString.Append(NS_LITERAL_STRING(","));

Does Append(PRUnichar(',')) work here?

Comment 15

[rbs]

Comment on attachment 86813 [details] [diff] [review]
patch v2

BTW, if you didn't do it, generously scatter the good old printf() while coding
to double-check that the names that you get is alright.

Comment 16

[Shanjian Li]

Attachment: patch v3

Comment 17

[Shanjian Li]

Comment on attachment 87446 [details] [diff] [review]
patch v3

carry over review

Comment 18

[Shanjian Li]

>When writing parsing routines like this, it is very helpful if you provide
>an example of the content that you're expecting to parse (e.g., in a
>comment). That makes it easier for people to understand what you're doing. :-)
Done.

>Why do you need to do the above FindChar? The value isn't used before
>it's clobbered by the FindChar, below.
That's a problem. Fixed.

>Are you sure you're always going to find a '-'? If not, pos1 == -1.
It should, since xfld name is strict in format. I added error handling anyway.
>+  unsigned long font_atom = gdk_atom_intern("FONT", FALSE);
>What does this do? Is this a leak?
What makes you think so? It does not look like a leak to me.

>Does Append(PRUnichar(',')) work here?
Yes, I changed it. 

Comment 19

[rbs]

Comment on attachment 87446 [details] [diff] [review]
patch v3

+  nsCAutoString nameStr;
+  nameStr.Append(aXLFDName);

What about: nsCAutoString nameStr(aXLFDName);

> >Are you sure you're always going to find a '-'? If not, pos1 == -1.
> It should, since xfld name is strict in format. I added error handling anyway.

Since it "should", you might want to loose the tests -- which is what waterson
was wondering (presently, the tests look overkill and make reading difficult.
Besides, the font code will fallback to another layer if something weird
happens and causes an erroneous name to be appended).

+  aString.Append(NS_LITERAL_STRING(","));

a left-over awaiting to migrate to aString.Append(PRUnichar(','));

+      //fontName.Append(NS_LITERAL_STRING(","));
+      fontName.Append(PRUnichar(','));

not necessary to keep the comment.

Comment 20

[Frank Tang]

are we working on this for machv rtm? or just for the trunk. IF you think we
need this for rtm, then mark this as nsbeta1 please

Comment 21

[Ying-Lin Xia]

We like to see this to be fixed in rtm.
CCing Michele...

Comment 22

[Shanjian Li]

Attachment: patch v4

improve readability.

Comment 23

[rbs]

+  aString.Append(NS_LITERAL_STRING(','));

Still there -- missing PRUnichar.

I will loose the macros and remove the |if| tests since the XLFD name is coming
from a system call and so the likehood of a bad format in this context is nearly
inexistent, plus there is another fallback if the name is bad.

Comment 24

[rbs]

I will be travelling but r=rbs still applies on what you end up pleasing the
super-reviewer with.

Comment 25

[Shanjian Li]

Comment on attachment 88044 [details] [diff] [review]
patch v4

carry over review

Comment 26

[Shanjian Li]

chris waterson, could you sr?

Comment 27

[Chris Waterson]

Have you considered using string iterators for this? (Ask jag to help you, if 
you need help.) This code ends up doing several memmove operations 
that appear to be unnecessary. (Or is performance of this code 
completely irrelevant?)

Comment 28

[Shanjian Li]

chris, the code will only be executed several times in a browser life span. Once
system font is decided, this piece of code will never be executed again. So
performance is not a concern here.

Comment 29

[Chris Waterson]

Comment on attachment 88044 [details] [diff] [review]
patch v4

sr=waterson

Comment 30

[Shanjian Li]

fix checked in. 

Comment 31

[Roland Mainz]

Reopening, the patch does not cover the Xlib toolkit... ;-(

Comment 32

[Roland Mainz]

Taking myself...

Comment 33

[Roland Mainz]

I have to fix this later, the current Xlib toolkit in CVS "trunk" tree does not
have support for toolkit-specific system fonts... ;-(

... marking bug as FIXED (for the GTK+ toolkit), I'll integrate this fix into my
tree for Xlib toolkit's system font patch...

Comment 34

[Yuying Long]

Verified fixed on 06-24 trunk build / Linux RH7.2 with Japanese personal toolbar
names and bookmarks.

Can we get this into branch? add nsbeta1 keyword.

Comment 35

[Masaki Katakai]

Can anyone try to check in this fix to 1.0.1 branch?

Solaris also has the same problem in many locales with the
previous implementation, which will be serious serious for
our Netscape 7 release.

Comment 36

[Shanjian Li]

The patch for this bug used FFRE (foundry-family-registry-encoding) name to
transfer system font information instead of just family name. On a typical unix
system, there are many font in some popular font family, like helvetica and fixed.
Using FFRE name enable us to better restore system font.

Risk? logically, there should be no risk at all. The FFRE name is got from a
existing font returned by system font query. Font metrics code will recovery
even if something with the new code. 

Comment 37

[Yuying Long]

Attachment: screen shot before patch checked in

Comment 38

[Yuying Long]

Attachment: screen shot after patch checked-in

On US build with Japanese bookmarks name.

Comment 39

[nhottanscp]

Shanjian, why the problem appears on RH7.2 but not for earlier versions?
Any workaround for l10n available if this does not get in?

For Solaris, is this a new problem or existed with earlier versions of Netscape?

Comment 40

[Shanjian Li]

see #13.

Comment 41

[kill this account]

First, Let me say that I think that using more information to select system 
specified fonts is a good idea.

I do want to say a word about trying to fix font alias problems with code. I 
think that this is a problematic idea. 

First: real fonts tend to change slowly therefore the code can stay relatively 
correct over time. Aliases change very rapidly and code cannot even hope change 
fast enough to keep track. 

Second: people who develope aliases do so based on what the code does. If we 
then try to change the code to "fix" alias problems we can then be in an 
unstable situation where each side is trying to "fix" the behavior of the 
other.

Third: the font selection code is already complex. I doubt anyone wants more 
complexity there.

Fourth: moz needs to be careful to follow CSS font selection rules. If the
alias says that a font is helvetica and the CSS font style says helvetica is
the first choice, then moz should look at all the helvetica fonts before
looking at any other fonts.

Comment 42

[Masaki Katakai]

Again, can anyone try to check in this fix to 1.0.1 branch?

Comment 43

[Masaki Katakai]

Any objection when I ask a= for 1.0.1/OEM BRANCH?

The exact problem is that we can not pick up fonts that are
defined in gtkrc for some fonts. 

When we put font name into gtkrc as XLFD, there are
some cases that family name of XLFD and value of
XA_FAMILY_NAME are not the same.

  unsigned long pr = 0;
  ::XGetFontProperty(aFontStruct, XA_FAMILY_NAME, &pr);
  if (!pr)
    ::XGetFontProperty(aFontStruct, XA_FULL_NAME, &pr);
  if (pr) {
    char *fontName = ::XGetAtomName(aDisplay, pr);
    aString.AppendWithConversion(fontName);
    ::XFree(fontName);
  }

For example, "-sun-sung-medium-r-normal--14-120-75-75-c-120-cns11643-14"
defines "MingUnusual" as family name. So Mozilla tries to pick up
"-*-MingUnusual-*" but the font is not defined in system. Yes, actually
it may be problem of fonts.dir/fonts.alias on the system.

But this patch can solve this issue because family name is picked up
from XLFD name. I belive this is correct way.

Comment 44

[kill this account]

Do we know if there was any performance hit?

Comment 45

[Masaki Katakai]

Hi Brian,

Sorry I don't understand what you mean. You mean performance hit before the patch?
or performance hit by this patch?

I don't have the exact data about performance but I think the performance has been
improved slightly by this patch because nsDeviceContextGTK.cpp can pass FFRE name to
FindStyleSheetSpecificFont() which can call TryNode() directly with the FFRE name,
not call TryFamily().

Comment 46

[kill this account]

I was curious if anyone checked if this patch had any significant negative
performance hit.

Comment 47

[Masaki Katakai]

I'm sorry I don't have the exact number of performance regression,
because it's too difficult to measure the performance for generic
because fonts much depend on user platform.

The change of this patch has changed the css from
"family" to "foundry-family-registry-encoding" form.
For example, "helvetica" to "adobe-helvetica-iso8859-1"
which means TryNode() is now called by the patch,
not TryFamily() to find UI fonts.

nsFontMetricsGTK::FindStyleSheetSpecificFont()

    if (hyphens == 3) {
      font = TryNode(familyName, aChar);
    ...
    else {
      font = TryFamily(familyName, aChar);
    ...

I believe this change will be good for performance because
we're calling XListFonts() with foundry, family, registry
and encoding fields.

  -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-iso8859-1 

In the case of TryFamily(), we're using only family field,

  -*-helvetica-*-*-*-*-*-*-*-*-*-*-*-*

Here are the results of XListFonts() at start up of Mozilla
(with not default page) in English locale. You can find only
one XListFonts() is called after the patch.

before
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-*-* ret=184
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-iso8859-1 ret=184

after
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-iso8859-1 ret=72

Of course, usually users start Mozilla with default start up
page that may have CSS style. If the page requires "helvetica",
*after* case will call TryFamily() and need to find the
following pattern, which was already checked and cached
in *before* case,

 -*-helvetica-*-*-*-*-*-*-*-*-*-*-*-* ret=184

However, the page contents with no CSS style will look for
"font.name" preference which is always "foundry-family-registry-encoding"
form, so the font might have checked and cached in
*after* case.

What will happen when UI contains localized string?

Before the patch, Mozilla will check "helvetica" then "fixed".
After the patch, that will be "-adobe-helvetica-iso8859-1"
then "-misc-fixed-jisx0208.1983-0". The number of calling
XListFonts() was 10 in *before*, 12 in *after* the patch.
But I'm not sure this causes significant regression for
performance.

Here are the results when Mozilla is invoked in Japanese
locale with Japanese UI,

before
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-*-* ret=184
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-0 ret=0
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-1 ret=0
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1983-0 ret=0
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1990-0 ret=0
 -*-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0212.1990-0 ret=0
 -*-fixed-*-*-*-*-*-*-*-*-*-*-*-* ret=78
 -*-fixed-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-0 ret=9
 -*-fixed-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-1 ret=0
 -*-fixed-*-*-*-*-*-*-*-*-*-*-jisx0208.1983-0 ret=9

after
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-iso8859-1 ret=72
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-iso8859-* ret=72
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-0 ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-* ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0201.1976-1 ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1983-0 ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1983-* ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1990-0 ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0208.1990-* ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0212.1990-0 ret=0
 -adobe-helvetica-*-*-*-*-*-*-*-*-*-*-jisx0212.1990-* ret=0
 -misc-fixed-*-*-*-*-*-*-*-*-*-*-jisx0208.1983-0 ret=3

I understand that XListFonts() is not only problem
for performance. I can not say the patch has improved
the performance but I can not say that has caused performance
regression... But I believe there is no case when
the patch causes significant impact against performance.

Comment 48

[Masaki Katakai]

Brian, any comments if I request this to OEM branch?

Comment 49

[Frank Tang]

add jdunn: it looks like sun want this bug land into oem branch. 

Comment 50

[kill this account]

> Brian, any comments if I request this to OEM branch?

If this has been on the trunk and no performance issue has surfaced then please
do so.

Comment 51

[Masaki Katakai]

Thanks, I'll check in the patch to OEM branch.

Comment 52

[Masaki Katakai]

patch checked into OEM branch.

Thank you very much for help.

Comment 53

[kill this account]

Thank you Katakai for working on this.