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.

Some other locale like SimpChinese display same as this.

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

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

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

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

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

Please ask pinkerton for super-review. Thanks.

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

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.

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

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

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.

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.

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

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

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.

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

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

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.

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 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");

> if (!sysFontName.IsEmpty()) { Oops, sorry. This should be... if (!sysFontName.IsEmpty() && fontName != sysFontName) {

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

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

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

> NS_WARNING("Could not create the converter.\n"); You don't need to append "\n" for NS_WARING param.

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

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.

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

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

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

> 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

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;

Sorry, for the spam.. > nsAutoString sysFont; -> sysFontName nsAutoString sysFont; -> fontName

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

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

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.

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?

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.

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.

(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; }

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

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

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

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 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 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); >

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

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

(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".

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

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.

Simon: Please continue sr.

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

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

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;

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

checked-in to trunk.

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

checked-in to 1.8 branch.

VERIFIED 1.8branch.

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

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

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

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

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.