170854 - Roman script of UI should be rendered with Lucida Grande on all system locales

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Yuying Long]

Build: 09-24 trunk build / Mac 10.1.5

Steps:
1. Set the OS locale to Japanese in System Preferences.
2. Load some web page that don't have font difined in CSS, or create a Japanese
document in Composer and open it in browser window.
3. Switch system locale to some other languages - English, SimpChinese...etc.

Result:
The font is displayed differently between in Japanese locale and other language
locales.

Since the Japanese font setting are same in Preferences, so I expected there
should not any difference.

Comment 1

[Yuying Long]

Attachment: screen shot under Japanese locale

Comment 2

[Yuying Long]

Attachment: screen shot with English locale

Some other locale like SimpChinese display same as this.

Comment 3

[Aaron Madlon-Kay]

Here is another screenshot showing the same issue in Pinstripe on Firefox 0.8
official release:
http://homepage.mac.com/amake/pinstripe.png

Comment 4

[YAMASHITA Makoto]

Attachment: Always use Lucida as "System" font

The immediate cause of Hiragino picked as the UI font is GetThemeFont called with the "system script" in the nsDeviceContextMac.cpp of gfxmac module.
Korean and Chinese locales are not affected because they get special casing to fix the bug 129188 (this bug is a "variant" of that bug).
The patch changes it to always call GetThemeFont for Roman script (to get Lucida). a Japanese font will be chosen at drawing process as a fallback font when needed.

Comment 5

[Roy Yokoyama]

Comment on attachment 204765 [details] [diff] [review]
Always use Lucida as "System" font

Changing sysScript to smRoman for all the language may be over-kill and impact other languages.

Instead, I'd like to see if we can add another *if* for 
Japanese.

eg. 
-        if ((smKorean == sysScript) ||
-            (smTradChinese == sysScript) ||
-            (smSimpChinese == sysScript))
-            (smJapanese == sysScript) || // <== here ==
-          sysScript = smRoman;
-          

Simon/Jungshik: It's been a while since I looked at the mozilla code; so I appreciate your input as well.

Comment 6

[Roy Yokoyama]

Oops, I hate the small edit text windown for review page......
Here is the correction.

-        if ((smKorean == sysScript) ||
-            (smTradChinese == sysScript) ||
-            (smSimpChinese == sysScript) || 
-            (smJapanese == sysScript)) // <== here ==
-          sysScript = smRoman;
-     

Comment 7

[YAMASHITA Makoto]

(In reply to comment #5)
> Changing sysScript to smRoman for all the language may be over-kill and impact
> other languages.
If some language is affected by this, it should be so because Lucida Grande should be used in the UI wherever possible independent of the user's preferred language, as explained in the bug 233781. (these are dupes)

Comment 8

[Roy Yokoyama]

Comment on attachment 204765 [details] [diff] [review]
Always use Lucida as "System" font

Ok, then I don't have problem with your patch.
I'd like Mac super-reviewers to assess the impact.

(may be Kevin Gerich, Simon Fraser, Mike Pinkerton or Blake Ross)

Comment 9

[Roy Yokoyama]

(In reply to comment #8)
> (may be Kevin Gerich, Simon Fraser, Mike Pinkerton or Blake Ross)
> 
or any super-reviewers feel comfortable :)

Comment 10

[Brian Ryner (not reading)]

Please ask pinkerton for super-review.  Thanks.

Comment 11

[Hiro]

FYI.

In bugzilla-jp, a similar patch has already been tried. 
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=4057

Because a Japanese font is different when the case where it doesn't specify Lucida Grande and the font is compared, this patch is canceled. 

Test patch
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2424&action=view

Test case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2421&action=view

Screen shot of Test case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2425&action=view

Comment 12

[YAMASHITA Makoto]

I prefer good layout + "slightly bad" glyph shape by the patch. After all, the glyph quality difference is so subtle compared to the layout improvement (the most annoying was that ugly URL field as explained in bug 233781). Moreover, Japanese labels affected by this problem are relatively few, because the menu items have been immune to this bug from the first.

Comment 13

[Mike Pinkerton (not reading bugmail)]

it doesn't seem like we have much consensus here on what we really want in this case, or am i misreading the comments?

Comment 14

[Hiro]

(In reply to comment #12)
> (the most annoying was that ugly URL field as explained in bug 233781).

Yes, this problem was a very annoying problem in a Japanese environment of Mac for a long long time. 
And, to deal with this problem, the following work around is given in the Japanese version package. 
Now, if the same work around is given to userChrome.css, it is possible to evade also in other language packages. 
http://lxr.mozilla.org/l10n/source/ja-JP-mac/toolkit/chrome/global/intl.css#13

UI improves certainly by patch of attachment 204765 [details] [diff] [review] compared with now. 
However, even if this patch does the check-in, the work around cannot be detached because Japanese used with UI is different from other Mac applications...

Comment 15

[HARUNAGA Hirotoshi]

I think this bug should be marked as WFM or wontfix since it seems to
me the problem such as attachment 100668 [details] can't be reproduced on OS
10.3.9.
The patch should be submitted to bug 233781.

Comment 16

[HARUNAGA Hirotoshi]

The patch introduces a regression which makes Japanese UI font ugly
(something like Osaka without anti-alias, narrow letter spacing)
in Japanese language environment as same as in Engllish language.
Layout problem described in bug 233781 is not major since Bug 109176
has been already fixed.
I think this patch shouldn't be checked in.

Comment 17

[YAMASHITA Makoto]

(In reply to comment #16)
> The patch introduces a regression which makes Japanese UI font ugly
> (something like Osaka without anti-alias, narrow letter spacing)
> in Japanese language environment as same as in Engllish language.
I'm not sure where this happens. My build's screenshot (essentially done by the patch here + the tweak suggested in the comment #14 + the patch at the bug 121540) 
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3026&action=view
shows Japanese bookmark entries rendered in Hiragino. Could you provide testcases/screenshots to isolate this problem?

> Layout problem described in bug 233781 is not major since Bug 109176
> has been already fixed.
Hiragino is still used in the UI (for JP sys + EN build combination) and its ugly Roman glyphs are annoying anyway.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3026&action=view
> shows Japanese bookmark entries rendered in Hiragino. Could you provide
> testcases/screenshots to isolate this problem?

Ah... O.K.
We might need to change default button caption style on Mac.
See test case's first button's caption in your screenshot.
It's not rendered with Hiragino. But bookmark toolbar buttons isn't rendered so.

Maybe,
http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsDeviceContextMac.cpp#275
275         case eSystemFont_Button:      fontID = kThemePushButtonFont; break;

I think this mapping is wrong actually.(But I think this is correct on logic.)
::GetThemeFont might return different font name for native button widgets if fontID is kThemePushButtonFont.
Please check it.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops...

Sorry. Your screenshot is including intl.css hacks.
Please ignore previous comment.

# Maybe, we need to update intl.css for button captions.
http://lxr.mozilla.org/l10n/source/ja-JP-mac/toolkit/chrome/global/intl.css#13
See forms.css, we are having some bugs in intl.css.

Comment 20

[HARUNAGA Hirotoshi]

Attachment: screenshot with attachment 204765

(In reply to comment #17)
> > The patch introduces a regression which makes Japanese UI font ugly
> > (something like Osaka without anti-alias, narrow letter spacing)
> > in Japanese language environment as same as in Engllish language.
> I'm not sure where this happens.

Please see the first button and font:caption, icon, menu, message-box,
small-caption, status-bar in your screenshot http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3025&action=view
or my old screenshot http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2425&action=view
by the patch http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2424&action=view.

And see this attachment.  The regression by your patch is not only
in buttons but also in <input>, <option> and bookmark toolbar.

These patches make Japanese UI font from Hiragino to some ugly font
like as Osaka (without anti-alias, narrow letter spacing).

> Hiragino is still used in the UI (for JP sys + EN build combination)

Of course I know it.

> and its ugly Roman glyphs are annoying anyway.

No, Hiragino's Roman glyphs are not ugly.  The problem here is only
that Roman UI font in Japanese locale is displayed as Hiragino instead
of as Lucida Grande.

I don't believe Japanese users prefer this change by your patch.

And please note intl.css for Japanese Mac build is only a workaround
for Japanese Mac build.

Comment 21

[YAMASHITA Makoto]

(In reply to comment #20)
> Please see the first button and font:caption, icon, menu, message-box,
> small-caption, status-bar in your screenshot
> http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3025&action=view
> or my old screenshot
> http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2425&action=view
> by the patch http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2424&action=view.
Hmm... Yours is different from mine above (please see bookmarks without layout problems) and 
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3023&action=view
(form labels) apparently with Hiragino. Then should we make this dependent of bug 121540?

> And please note intl.css for Japanese Mac build is only a workaround
> for Japanese Mac build.
I'm not including that intl.css hack in part of my patch here, as it would give an unnecessary preference over System fonts of other scripts. Without that the English build will use Osaka for Japanese characters, but it is less a problem than the current situation. I apply it to produce the screenshots only because we've got to see whether my patch makes regressions for Japanese build, as you are concerned.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

YAMASHITA-san:

I have an idea.
nsFont can have plural font names by comma separated value.

See http://lxr.mozilla.org/mozilla/source/layout/style/nsRuleNode.cpp#1705

> 1705   if (eCSSUnit_String == aFontData.mFamily.GetUnit()) {
> 1706     // set the correct font if we are using DocumentFonts OR we are overriding for XUL
> 1707     // MJA: bug 31816
> 1708     if (aUseDocumentFonts) {
> 1709       if (!aIsGeneric) {
> 1710         // only bother appending fallback fonts if this isn't a fallback generic font itself
> 1711         aFont->mFont.name.Append((PRUnichar)',');
> 1712         aFont->mFont.name.Append(aDefaultFont.name);
> 1713       }
> 1714     }

You should return two font names if the |sysScript| is not |smRoman|.
I.e., if |sysScript| is |smJapanese|, you should set "fontNameForRoman,fontNameForJapanese" to aFont->name.
Of course, fontNameForRoman is gotten by |GetThemeFont| with |smRoman|.
fontNameForJapanese is gotten by |GetThemeFont| with |smJapanese|.

And for other members of aFont(I.e., size and style), You should use the return value of |GetThemeFont| with |smJapanese|.

Please check this approach. I think we can remove intl.css by this approach.

Comment 23

[YAMASHITA Makoto]

Attachment: Make a list of system fonts (roman sys font, local sys font)

This incorporates Nakano-san's suggestion (#22). The program is doing what we really want, without that intl.css hack. Thanks.
Harunaga-sam, could you try this version to see whether this one works for you without the bug 121540 works?
This one is totally different from the prev one, and I now recognize bug 233781 is a more adequate place to address this issue. So I don't mind moving & resubmitting this patch there to get more assessments. Shall we? Yokoyama-san, thanks for your review here anyway.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205667 [details] [diff] [review]
Make a list of system fonts (roman sys font, local sys font)

Don't use nsString for local variables. You should use nsAutoString instead.

> +        nsString localSysFontName = GetSystemFontForScript(fontID, sysScript, fontSize, fontStyle);
> +        nsString romanSysFontName = GetSystemFontForScript(fontID, smRoman, fontSize, fontStyle);
> +        nsString sysFonts = romanSysFontName + NS_LITERAL_STRING(",") + localSysFontName;

You don't need to get sysScript if that is smRoman. And I think that you should use font size and font style these are gotten with sysScript.

I.e,
nsAutoString fontName = GetSystemFontForScript(fontID, smRoman, fontSize, fontStyle);
if (sysScript != smRoman) {
  SInt16 tmpFontSize;
  Style tmpFontStyle;
  nsAutoString sysFontName = GetSystemFontForScript(fontID, sysScript, tmpFontSize, tmpFontStyle);
  if (!sysFontName.IsEmpty()) {
    fontSize = tmpFontSize;
    fontStyle = tmpFontStyle;
    if (!fontName.IsEmpty())
      fontName += NS_LITERAL_STRING",";
    fontName += sysFontName;
}
aFont->name = sysFontName;
if (aFont->name.IsEmpty())
  aFont->name = NS_LITERAL_STRING("Lucida Grande");

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> if (!sysFontName.IsEmpty()) {

Oops, sorry.
This should be...

if (!sysFontName.IsEmpty() && fontName != sysFontName) {

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

See
http://developer.mozilla.org/en/docs/XPCOM:Strings
http://www.mozilla-japan.org/projects/xpcom/string-guide.html

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205667 [details] [diff] [review]
Make a list of system fonts (roman sys font, local sys font)

> +nsString
> +nsDeviceContextMac::GetSystemFontForScript(ThemeFontID fontID, ScriptCode scriptCode, SInt16& fontSize, Style& fontStyle) const

I think you should get result font name by param instead of return value.
And this function should be inline. And function's param's name should have 'a' prefix.
I.e.,

inline void
GetSystemFontForScript(ThemeFontID aFontID, ScriptCode aScriptCode, nsAFlatString & aFontName, SInt16& aFontSize, Style& aFontStyle) const

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

See also our coding rules.
http://www.mozilla-japan.org/hacking/mozilla-style-guide.html

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> NS_WARNING("Could not create the converter.\n");

You don't need to append "\n" for NS_WARING param.

Comment 30

[YAMASHITA Makoto]

Attachment: Listing of sys fonts, polished

> And I think that you should use font size and font style these are gotten with sysScript.
What is the point in doing so, while we give preference to the Roman sys font (Lucida) for font family?

I'm not sure about this: does a string comparison like "sysFont != localSysFontName" mean what we really mean (not a comparison of pointers)?

Comment 31

[HARUNAGA Hirotoshi]

The problem originally reported with English locale in comment 0
is only in UI fonts (form controls, tab's title, tooltips, etc.),
which are displayed as somthing like Osaka instead of Hiragino
as I wrote before.

Adding the current discussion to the summary in the meantime.
Bug 210167 depends on this.

Comment 32

[HARUNAGA Hirotoshi]

*** Bug 233781 has been marked as a duplicate of this bug. ***

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205763 [details] [diff] [review]
Listing of sys fonts, polished

>> And I think that you should use font size and font style these are gotten with sysScript.
> What is the point in doing so, while we give preference to the Roman sys font
> (Lucida) for font family?
You should use current locale font's style and size for result nsFont.
Don't use smRoman's.
I.e.,
+ if (!localSysFontName.IsEmpty() && sysFont != localSysFontName) {
+   sysFont += NS_LITERAL_STRING(",") + localSysFontName;
    fontSize = lclFontSize;
    fontStyle = lclFontStyle;
+ }

In a locale, we should use  the locale's font data. But we need smRoman's font family only for better rendering. So we need only for smRoman's font name. The style and size are not so.

> I'm not sure about this: does a string comparison like "sysFont !=
> localSysFontName" mean what we really mean (not a comparison of pointers)?

This means comparing these text, not for these pointers.
If sysFont and localSysFontName are "Lucida Grande", we should not return "Lucida Grande,Lucida Grande".

And you should unify variable naming rule.

I think these better names are:
nsAutoString sysFont; -> sysFontName
SInt16 lclFontSize; -> localFontSize
Style lclFontStyle; -> localFontStyle

> +inline nsresult nsDeviceContextMac::GetSystemFontForScript(ThemeFontID aFontID,
> +ScriptCode aScriptCode, nsAFlatString& aFontName, SInt16& aFontSize,
> +Style& aFontStyle) const

Please use following format.(like nsDeviceContextMac::BeginDocument)
# we have 80 characters limit per line.

+inline nsresult
 nsDeviceContextMac::GetSystemFontForScript(ThemeFontID aFontID,
+                                                                      ScriptCode aScriptCode,
                                                                        nsAFlatString& aFontName,
                                                                       SInt16& aFontSize,
+                                                                      Style& aFontStyle) const

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry. I hate the editor of edit attachment...

> Please use following format.(like nsDeviceContextMac::BeginDocument)
> # we have 80 characters limit per line.

inline nsresult
nsDeviceContextMac::GetSystemFontForScript(ThemeFontID    aFontID,
                                           ScriptCode     aScriptCode,
                                           nsAFlatString& aFontName,
                                           SInt16&        aFontSize,
                                           Style&         aFontStyle) const

And I think this function should not be a member of nsDeviceContextMac class...

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I'm not sure about this: does a string comparison like "sysFont !=
> localSysFontName" mean what we really mean (not a comparison of pointers)?

See also:
http://lxr.mozilla.org/mozilla/source/xpcom/string/public/nsTSubstring.h#661

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Here is one more comment.

> +          if (!localSysFontName.IsEmpty() && sysFont != localSysFontName) {
> +            sysFont += NS_LITERAL_STRING(",") + localSysFontName;
> +          }

if sysFont is empty, we will create ".localSysFontNameValue". This is not good.
Please check smRoman's font is empty or not for appending ",".
# or following lines should be before "if (sysScript != smRoman) {"
# If so, of course, |aFont->name| will be changed to |sysFont|.

> +        NS_ASSERTION(!sysFont.IsEmpty(), "empty font name");
> +        aFont->name = sysFont.IsEmpty() ? NS_LITERAL_STRING("Lucida Grande") :
> +          sysFont;

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, for the spam..

> nsAutoString sysFont; -> sysFontName

nsAutoString sysFont; -> fontName

Comment 38

[YAMASHITA Makoto]

Attachment: Listing of sys fonts, mo' polished

Besides your suggestions, it's now using the return value of GetSystemFontForScript.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205897 [details] [diff] [review]
Listing of sys fonts, mo' polished

looks good!

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205897 [details] [diff] [review]
Listing of sys fonts, mo' polished

Roy or jshin:

I'm not formal reviewer. Please check this patch.

Comment 41

[HARUNAGA Hirotoshi]

The patch works for me using SeaMonkey in Japanese locale on OS 10.3.9,
but doesn't work in English locale...
Should we reopen Bug 233781 for this patch?

Comment 42

[YAMASHITA Makoto]

That's because the patch produces "Lucida Grande" (on Western locales) or "Lucida Grade,LOCAL_SYS_FONT" (on non-Western ones) for system fonts. So you get a contrary problem of bug 233781 for a Firefox/Seamonkey (w/o intl.css + forms.css hacks) when a non-Japanese language is primary on the OS. I think non-Japanese distributions have been doing what you see, on non-Jp systems. In addition I don't know whether the Chinese sys font is used for Chinese UI labels for non-Chinese build on non-Chinese systems, ditto for Korean, etc. The Japanese build can keep on those CSS hacks just in case it's launched in non-Japanese systems.
I thought this behaviour is less a problem because non-Japanese users rarely have Japanese labels on UI/care about the font for them, while non-English users naturally encounter English labels on forms, bookmarks or use English builds on non-English systems (e.g. in development cycle, like me).
If you do find it annoying, please open a new bug rather than reopening 233781, since the problem is the other way round of that bug.

Comment 43

[HARUNAGA Hirotoshi]

I have been saying that the problem originally reported in this bug is
"Japanese font face in English (or non-Japanese) locale".
Your patch solves bug 233781, but doesn't affect English locale.

Comment 44

[Hiro]

(In reply to comment #42)
> That's because the patch produces "Lucida Grande" (on Western locales) or
> "Lucida Grade,LOCAL_SYS_FONT" (on non-Western ones) for system fonts.

I think it is useless if the font list is not made as much as workaround of intl.css. 

Non-Japanese OS locale: "Lucida Grade,LOCAL_SYS_FONT,Font obtained with smJapanese"
Japanese OS locale: "Lucida Grade,LOCAL_SYS_FONT"

For examples, to the following:

+        if (sysScript != smRoman) {
+          SInt16 localFontSize;
+          Style localFontStyle;
+          nsAutoString localSysFontName;
+          rv = GetSystemFontForScript(fontID, sysScript,
+                                      localSysFontName,
+                                      localFontSize, localFontStyle);
+          if (NS_SUCCEEDED(rv) && fontName != localSysFontName) {
+            fontName += NS_LITERAL_STRING(",") + localSysFontName;
+            fontSize = localFontSize;
+            fontStyle = localFontStyle;
+          }

           if (sysScript != smJapanese) {
//           The font name of smJapanese is added just like the above-mentioned. 
             SInt16 japaneseFontSize;
             Style japaneseFontStyle;
             nsAutoString japaneseSysFontName;
             rv = GetSystemFontForScript(fontID, smJapanese,
                                         japaneseSysFontName,
                                         japaneseFontSize, japaneseFontStyle);
            if (NS_SUCCEEDED(rv)) {
            fontName += NS_LITERAL_STRING(",") + japaneseSysFontName;
          }

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #41)
> The patch works for me using SeaMonkey in Japanese locale on OS 10.3.9,
> but doesn't work in English locale...
> Should we reopen Bug 233781 for this patch?

This issue is existing on all OS. Because font switching mechanism cannot always work as expected for all users. If we will do so, we need all list up all locales default fonts. But this is not good way for performance. And on non-CJKT locale, we cannot know what order these fonts be listed. This is a issue of Unicode...

(In reply to comment #44)
> Non-Japanese OS locale: "Lucida Grade,LOCAL_SYS_FONT,Font obtained with
> smJapanese"
> Japanese OS locale: "Lucida Grade,LOCAL_SYS_FONT"

I cannot accept your approach. This problem might not be only occured on Japanese characters. And if we do so, Gecko always use Japanese font for rendering the Kanji(Hanja) on CKT locales.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Should we reopen Bug 233781 for this patch?

This bug is using for fixing bug 233781, now...
I think if you think this is not good, please file a new bug.
Because this bug has many comments for fixing bug 233781.
So, this bug is not readable for original reported issue.

I'm changing summary. Sorry.

Comment 47

[Masatoshi Kimura [:emk]]

Why are you bothered to simulate OS fallback list?
IMO we should resolve bug 121540 first as comment #21 suggests.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 205897 [details] [diff] [review]
Listing of sys fonts, mo' polished

Changing the reviewer Roy to Jshin. Because I don't have any replies from him.

Comment 49

[Jungshik Shin]

Comment on attachment 205897 [details] [diff] [review]
Listing of sys fonts, mo' polished

>+// helper function to get the system font for a specific script
>+inline nsresult 
>+GetSystemFontForScript(ThemeFontID aFontID, ScriptCode aScriptCode,
>+                       nsAFlatString& aFontName, SInt16& aFontSize,
>+                       Style& aFontStyle)

I think it's better to declare this explicitly as 'static'

I thought I had reviewed this, but apparently I didn't press 'submit' button. (I even remember asking sfraser for sr).

Comment 50

[Simon Fraser [no longer active]]

Comment on attachment 205897 [details] [diff] [review]
Listing of sys fonts, mo' polished

>Index: nsDeviceContextMac.cpp
>===================================================================

> 
>+// helper function to get the system font for a specific script
>+inline nsresult 
>+GetSystemFontForScript(ThemeFontID aFontID, ScriptCode aScriptCode,
>+                       nsAFlatString& aFontName, SInt16& aFontSize,
>+                       Style& aFontStyle)

Don't use inline for a function this big.

>+{
>+  Str255 fontName255;
>+
>+  ::GetThemeFont(aFontID, aScriptCode, fontName255, &aFontSize, &aFontStyle);
>+  fontName255[fontName255[0]+1] = 0;

This could potentially write off the end of the array if the font name is 255 chars long.

>+  OSStatus err;
>+  // the theme font could contains font name in different encoding. 
>+  // we need ot covert them to unicode according to the font's text encoding.

"to convert"

>+  TECObjectRef converter = 0;
>+  TextEncoding unicodeEncoding = 
>+    ::CreateTextEncoding(kTextEncodingUnicodeDefault, 
>+                         kTextEncodingDefaultVariant,
>+                         kTextEncodingDefaultFormat);
>+                                                              
>+  FMFontFamily fontFamily;
>+  TextEncoding fontEncoding = 0;
>+  fontFamily = ::FMGetFontFamilyFromName(fontName255);
>+  err = ::FMGetFontFamilyTextEncoding(fontFamily, &fontEncoding);
>+
>+  if (err != noErr) {
>+    NS_WARNING("Could not get the encoding for the font.");
>+    return NS_ERROR_FAILURE;
>+  }
>+  err = ::TECCreateConverter(&converter, fontEncoding, unicodeEncoding);
>+  if (err != noErr) {
>+    NS_WARNING("Could not create the converter.");
>+    return NS_ERROR_FAILURE;
>+  }
>+  PRUnichar unicodeFontName[sizeof(fontName255)];

Will this always be big enough? Could you require more unicode chars that the number of bytes in the native encoding?

>+  ByteCount actualInputLength, actualOutputLength;
>+  err = ::TECConvertText(converter, &fontName255[1], fontName255[0], 
>+                         &actualInputLength, 
>+                         (TextPtr)unicodeFontName, sizeof(unicodeFontName),
>+                         &actualOutputLength);  
>+  unicodeFontName[actualOutputLength / sizeof(PRUnichar)] = PRUnichar('\0');
>+
>+  ::TECDisposeConverter(converter);

I sigh every time I see this code; we have it in 2 places in gfx, and I think it's time that we had a little C++ helper class to wrap the TEC.
There are lots of errors that we don't deal with here (output buffer too small etc).



>+        nsresult rv;
>+        rv = GetSystemFontForScript(fontID, smRoman,
>+                                    fontName, fontSize, fontStyle);
>+        if (NS_FAILED(rv))
>+          fontName = NS_LITERAL_STRING("Lucida Grande");
>+
>+        if (sysScript != smRoman) {
>+          SInt16 localFontSize;
>+          Style localFontStyle;
>+          nsAutoString localSysFontName;
>+          rv = GetSystemFontForScript(fontID, sysScript,
>+                                      localSysFontName,
>+                                      localFontSize, localFontStyle);
>+          if (NS_SUCCEEDED(rv) && fontName != localSysFontName) {

Is there an operator != for strings? Might be clearer to spell that out.

>+            fontName += NS_LITERAL_STRING(",") + localSysFontName;
>+            fontSize = localFontSize;
>+            fontStyle = localFontStyle;
>+          }
>         }
>+        aFont->name = fontName;        
>         aFont->size = NSToCoordRound(float(fontSize) * dev2app);
>

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> >+        if (sysScript != smRoman) {
> >+          SInt16 localFontSize;
> >+          Style localFontStyle;
> >+          nsAutoString localSysFontName;
> >+          rv = GetSystemFontForScript(fontID, sysScript,
> >+                                      localSysFontName,
> >+                                      localFontSize, localFontStyle);
> >+          if (NS_SUCCEEDED(rv) && fontName != localSysFontName) {
> 
> Is there an operator != for strings? Might be clearer to spell that out.

Do you say that we should use |nsString::Equals()| insted of operator on Mac code? On XP code, we are using operator for comparison of string.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops. Simon, please check my previous comment.(comment 51)

Comment 53

[Simon Fraser [no longer active]]

(In reply to comment #51)

> > >+          if (NS_SUCCEEDED(rv) && fontName != localSysFontName) {
> > 
> > Is there an operator != for strings? Might be clearer to spell that out.
> 
> Do you say that we should use |nsString::Equals()| insted of operator on Mac
> code? On XP code, we are using operator for comparison of string.

I find Equals has a lower brain-print (when I see use of the operator, I have to always think "did the author mean to do a pointer comparison, or do they actually want a string compare".

Comment 54

[YAMASHITA Makoto]

Attachment: coding fixes

should we be more explicit on the flatness of fontName? Looks like we need this (I don't know why).

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 209089 [details] [diff] [review]
coding fixes

YAMASHITA-san:

Please mark review flag to '+' yourself in sr process.
r=me+jshin

# Of course, if new patch concept/logic is large changed from previous patch, you need review.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Simon:

Please continue sr.

Comment 57

[Simon Fraser [no longer active]]

Comment on attachment 209089 [details] [diff] [review]
coding fixes

>Index: nsDeviceContextMac.cpp
>===================================================================

>+// helper function to get the system font for a specific script
>+#define FONTNAME_MAX_UNICHRS sizeof(fontName255) * 2
>+nsresult 
>+GetSystemFontForScript(ThemeFontID aFontID, ScriptCode aScriptCode,
>+                       nsAFlatString& aFontName, SInt16& aFontSize,
>+                       Style& aFontStyle)
>+{
>+  Str255 fontName255;
>+  ::GetThemeFont(aFontID, aScriptCode, fontName255, &aFontSize, &aFontStyle);
>+  if (fontName255[0] == 255) {
>+    NS_WARNING("Too long fong name (> 254 chrs)");
>+    return NS_ERROR_FAILURE;
>+  }
>+  fontName255[fontName255[0]+1] = 0;

You could just have used an unsigned char[257], but this is good enough.

>+  ::TECDisposeConverter(converter);
>+
>+ unicodeFontName[actualOutputLength / sizeof(PRUnichar)] = PRUnichar('\0');
>+ aFontName = nsDependentString(unicodeFontName);

Weird spacing here.

>+        nsresult rv;
>+        rv = GetSystemFontForScript(fontID, smRoman,
>+                                    fontName, fontSize, fontStyle);
>+        if (NS_FAILED(rv))
>+          fontName = NS_LITERAL_STRING("Lucida Grande");
>+
>+        if (sysScript != smRoman) {
>+          SInt16 localFontSize;
>+          Style localFontStyle;
>+          nsAutoString localSysFontName;
>+          rv = GetSystemFontForScript(fontID, sysScript,
>+                                      localSysFontName,
>+                                      localFontSize, localFontStyle);
>+          if (NS_SUCCEEDED(rv) && !fontName.Equals(localSysFontName)) {
>+            fontName += NS_LITERAL_STRING(",") + localSysFontName;
>+            fontSize = localFontSize;
>+            fontStyle = localFontStyle;
>+          }
>         }
>+        aFont->name = fontName;        
>         aFont->size = NSToCoordRound(float(fontSize) * dev2app);

I presume it's OK for the font name to contain spaces in the aFont->name string, and not require quotes (as CSS would)?

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #57)
> I presume it's OK for the font name to contain spaces in the aFont->name
> string, and not require quotes (as CSS would)?

Oh, I forgot this issue. However, in many cases, this doesn't make the problem.

http://www.w3.org/TR/CSS21/fonts.html#font-family-prop
> If an unquoted font family name contains parentheses, brackets, and/or braces, they must still be escaped per CSS grammar rules. Similarly, quotation marks (both single and double), semicolons, exclamation marks, commas, and leading slashes within unquoted font family names must be escaped. Font names containing any such characters or whitespace should be quoted:

On Windows, the method returns unquoted value. But we don't have any bugs.

Comment 59

[YAMASHITA Makoto]

jshin, could you check the patch in with fixes for this bad indentation?:
>+  ::TECDisposeConverter(converter);
>+
>+ unicodeFontName[actualOutputLength / sizeof(PRUnichar)] = PRUnichar('\0');
>+ aFontName = nsDependentString(unicodeFontName);
>+  return NS_OK;

Comment 60

[Daniel Veditz [:dveditz]]

blocking 1.8.0.2 denied, not a regression, no trunk-baked patch.

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk.

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for check-in

Comment 63

[Hiro]

VERIFIED.

Mac OS X 10.3.9
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060215 Firefox/1.6a1

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for 1.8 branch

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.8 branch.

Comment 66

[Hiro]

VERIFIED 1.8branch.

Comment 67

[Darin Fisher]

How confident do you guys feel about this patch for 1.8.0.3?

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #67)
> How confident do you guys feel about this patch for 1.8.0.3?
> 

I'll get Mac soon. I'll test the patch on 1.8.0 branch and I'll decide it.

Comment 69

[Daniel Veditz [:dveditz]]

Please ask for branch approval on the patch when you're confident in it for 1.8.0.3 

Comment 70

[Daniel Veditz [:dveditz]]

Too late for a non-approved patch, bumping to next release.

Comment 71

[Daniel Veditz [:dveditz]]

Not blocking the 1.8.0.5 releases, but if you're confident in the patch you can request approval and we'll consider it.