146622 - PrefEnumCallback does not work as expected

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Masaki Katakai]

I found serious problem on nsFontMetricsGTK.cpp. If font can not be
found for the default language in preferece, we look for all fonts prefs
by using PrefEnumCallback(), but it does not work. It always looks for
'*' for search. So it returns unexpected result.

I found this problem on Solaris. Korean user prefers "ksc5601.1992-3"
for Korean language so they defined the font in preference. It works
fine on ko locale. But on UTF-8 locale, the font exists but it can not
be picked up. It always uses "ksc5601.1987-0" by this problem.

Look at the log below, the first matched pref is "font.name.monospace.tr"
and it's not defined in user prefs. So "" is returned and the
current codes is trying to find the font by "" in TryNode().
TryNode() looks for fonts by "aFFREName = *". If user pref
is not defined, this always trys "*" and stops. We should
avoid "" search at TryNode().

       PrefEnumCallback, nsFontMetricsGTK.cpp 4380
        TryNode aName = , nsFontMetricsGTK.cpp 4165
        TrySubplane: wild-card the encoding, nsFontMetricsGTK.cpp 4216
        TryNodes aFFREName = *, nsFontMetricsGTK.cpp 4137
        load font *-symbol-adobe-fontspecific, nsFontMetricsGTK.cpp 3460
        load font *-gothic-ksc5601.1987-0, nsFontMetricsGTK.cpp 3460

Comment 1

[Masaki Katakai]

Attachment: proposed patch

Comment 2

[Masaki Katakai]

Attachment: Sorry I put wrong file. here is correct one.

Comment 3

[Masaki Katakai]

I've put proposed patch.
 
1. If len=0, TryNode() should return.
2. I changed if(value) to if(value.get()) according with other codes.
3. The following codes should be in if (value.get()) { }

    s->mFont = s->mMetrics->TryLangGroup(s->mMetrics->mLangGroup, &name,
s->mChar);
    if (s->mFont) {
      NS_ASSERTION(s->mFont->SupportsChar(s->mChar), "font supposed to support
this char");
      return;
    }

Comment 4

[Masaki Katakai]

Brian, what do you think? 

Comment 5

[kill this account]

This seems okay.

Is there something other than a whitespace change here:

-  s->mFont = s->mMetrics->TryLangGroup(s->mMetrics->mLangGroup, &name, 
s->mChar);
-  if (s->mFont) {
-    NS_ASSERTION(s->mFont->SupportsChar(s->mChar), "font supposed to support 
this char");
-    return;
+    s->mFont = s->mMetrics->TryLangGroup(s->mMetrics->mLangGroup, &name, 
s->mChar);
+    if (s->mFont) {
+      NS_ASSERTION(s->mFont->SupportsChar(s->mChar), "font supposed to support 
this char");
+      return;
+    }

Comment 6

[Masaki Katakai]

I've just moved those codes into inside of if (value.get()) {}.

Comment 7

[kill this account]

Comment on attachment 84883 [details] [diff] [review]
Sorry I put wrong file. here is correct one.

looks okay to me
r=[e-mail]

Comment 8

[Masaki Katakai]

This bug does not happen very often. This problem happens
if users change font preference but the font is not available
in the user environment. At that case, prefs.js will contain,

user_pref("font.name.monospace.tr", "");

Comment 9

[Masaki Katakai]

Attachment: check also needs in TryLangGroup()

Comment 10

[Masaki Katakai]

Brian,

Sorry for dup work but the check is needed also in
TryLangGroup().

Could you r= please?

Comment 11

[kill this account]

r=[e-mail]

Katakai: would you ask Jag to sr this?

Comment 12

[jag (Peter Annema)]

Comment on attachment 85136 [details] [diff] [review]
check also needs in TryLangGroup()

One of the trunk builds has been crashing for me when I tried to submit patch
comments through this form, I didn't realize it also happened for this bug.

sr=jag, provided you fix the comments below:

>Index: nsFontMetricsGTK.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp,v
>retrieving revision 1.209
>diff -u -r1.209 nsFontMetricsGTK.cpp
>--- nsFontMetricsGTK.cpp	24 May 2002 00:00:28 -0000	1.209
>+++ nsFontMetricsGTK.cpp	27 May 2002 02:41:06 -0000
>@@ -4175,6 +4175,9 @@
>   //
>   // check the specified font (foundry-family-registry-encoding)
>   //
>+  if (!(*aName).Length()) {
>+    return nsnull;
>+  }

if (aName->IsEmpty())
  return nsnull;

>@@ -4236,6 +4239,9 @@
>   //
>   FIND_FONT_PRINTF(("      TryLangGroup lang group = %s, aName = %s", 
>                             atomToName(aLangGroup), (*aName).get()));
>+  if (!(*aName).Length()) {
>+    return nsnull;
>+  }

if (aName->IsEmpty())
  return nsnull;

Comment 13

[Roland Mainz]

Comment on attachment 85136 [details] [diff] [review]
check also needs in TryLangGroup()

The patch needs to be ported to Xlib gfx, too - I'll file a new patch...

Comment 14

[Roland Mainz]

Attachment: Patch for both GTK+ and Xlib gfx

Comment 15

[Roland Mainz]

Stealing from katakai...

Comment 16

[Masaki Katakai]

Thank you Roland for your kindness,

I need to put this to both Trunk and Branch.

But could you please look at Jag's comments of
aName->IsEmpty() ?

Thank you.

Comment 17

[Roland Mainz]

Masaki Katakai wrote:
> Thank you Roland for your kindness,

No problem... :)

> But could you please look at Jag's comments of
> aName->IsEmpty() ?

Eeeeeekks... ;-(

I forgot that... sorry... new patch in a few secs...

Comment 18

[Roland Mainz]

Attachment: New patch per jag's comments

Comment 19

[Scott Collins]

Comment on attachment 86577 [details] [diff] [review]
New patch per jag's comments

sr=scc

Comment 20

[Roland Mainz]

katakai:
Can you r= the new patch, please ?

Comment 21

[Masaki Katakai]

Sure. I've verified the fix on GTK. The same codes
in Xlib. Looks good for me.

r=katakai

but I'm sorry I could not mar has-review for the attachment.

Comment 22

[Roland Mainz]

Masaki Katakai wrote:
> Sure. I've verified the fix on GTK. The same codes
> in Xlib. Looks good for me.
>
> r=katakai

Thanks!

> but I'm sorry I could not mar has-review for the attachment.

Uhm... what went wrong ? Which error did you get ?

Comment 23

[Roland Mainz]

Comment on attachment 86577 [details] [diff] [review]
New patch per jag's comments

Marking r=katakai per comment #21 ...

Comment 24

[Roland Mainz]

Requesting a= for trunk and 1.0.1-branch...

Comment 25

[Simon Montagu :smontagu]

Checked in to trunk for Roland

Comment 26

[Yuying Long]

I'm not sure if I'm the right person to verify this code change.  I'm change QA
contact to Frank right now.  Please provide the test case or steps if need me
verify, thanks!

Comment 27

[Masaki Katakai]

I'll try to verify.

Comment 28

[Roland Mainz]

Requesting a= for 1.0.1-branch...

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 86577 [details] [diff] [review]
New patch per jag's comments

Approved for 1.0 branch.  Please remove mozilla1.0.1+ when it's fixed and add
fixed1.0.1

Comment 30

[Simon Montagu :smontagu]

checked in to branch

Comment 31

[lchiang]

[e-mail] - can you verify on branch and trunk?

Comment 32

[Masaki Katakai]

verified on both Trunk and 1.0.1 branch.