Closed
Bug 170854
Opened 22 years ago
Closed 19 years ago
Roman script of UI should be rendered with Lucida Grande on all system locales
Categories
(Core :: Internationalization, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.8.1
People
(Reporter: amyy, Assigned: makotoy)
References
(Blocks 1 open bug)
Details
(Keywords: intl, verified1.8.1, Whiteboard: [pending to request approval 1.8.0 branch after 1.5.0.2])
Attachments
(5 files, 5 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
Comment 3•21 years ago
|
||
Here is another screenshot showing the same issue in Pinstripe on Firefox 0.8 official release: http://homepage.mac.com/amake/pinstripe.png
Assignee | ||
Comment 4•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #204765 -
Flags: review?(tetsuroy)
Comment 5•19 years ago
|
||
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.
Attachment #204765 -
Flags: review?(tetsuroy) → review-
Comment 6•19 years ago
|
||
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; -
Assignee | ||
Comment 7•19 years ago
|
||
(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•19 years ago
|
||
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)
Attachment #204765 -
Flags: review- → review+
Comment 9•19 years ago
|
||
(In reply to comment #8) > (may be Kevin Gerich, Simon Fraser, Mike Pinkerton or Blake Ross) > or any super-reviewers feel comfortable :)
Assignee | ||
Updated•19 years ago
|
Attachment #204765 -
Flags: superreview?(bryner)
Assignee | ||
Updated•19 years ago
|
Attachment #204765 -
Flags: superreview?(bryner) → superreview?(mikepinkerton)
Comment 11•19 years ago
|
||
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
Assignee | ||
Comment 12•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 17•19 years ago
|
||
(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•19 years ago
|
||
> 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•19 years ago
|
||
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•19 years ago
|
||
(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.
Assignee | ||
Comment 21•19 years ago
|
||
(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•19 years ago
|
||
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.
Assignee | ||
Comment 23•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #204765 -
Attachment is obsolete: true
Attachment #204765 -
Flags: superreview?(mikepinkerton)
Comment 24•19 years ago
|
||
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");
Attachment #205667 -
Flags: review-
Comment 25•19 years ago
|
||
> if (!sysFontName.IsEmpty()) {
Oops, sorry.
This should be...
if (!sysFontName.IsEmpty() && fontName != sysFontName) {
Comment 26•19 years ago
|
||
FYI: See http://developer.mozilla.org/en/docs/XPCOM:Strings http://www.mozilla-japan.org/projects/xpcom/string-guide.html
Comment 27•19 years ago
|
||
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•19 years ago
|
||
FYI: See also our coding rules. http://www.mozilla-japan.org/hacking/mozilla-style-guide.html
Comment 29•19 years ago
|
||
> NS_WARNING("Could not create the converter.\n");
You don't need to append "\n" for NS_WARING param.
Updated•19 years ago
|
Assignee: tetsuroy → makotoy
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 30•19 years ago
|
||
> 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)?
Attachment #205667 -
Attachment is obsolete: true
Comment 31•19 years ago
|
||
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.
Blocks: 210167
Summary: Japanese font face is displayed different depends on OS locale → Japanese font face is displayed different depends on OS locale (Roman UI font in Japanese locale should be displayed as Lucida Grande)
Comment 33•19 years ago
|
||
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
Attachment #205763 -
Flags: review-
Comment 34•19 years ago
|
||
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•19 years ago
|
||
> 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•19 years ago
|
||
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•19 years ago
|
||
Sorry, for the spam..
> nsAutoString sysFont; -> sysFontName
nsAutoString sysFont; -> fontName
Assignee | ||
Comment 38•19 years ago
|
||
Besides your suggestions, it's now using the return value of GetSystemFontForScript.
Attachment #205763 -
Attachment is obsolete: true
Comment 39•19 years ago
|
||
Comment on attachment 205897 [details] [diff] [review] Listing of sys fonts, mo' polished looks good!
Attachment #205897 -
Flags: review+
Comment 40•19 years ago
|
||
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.
Attachment #205897 -
Flags: review?(tetsuroy)
Comment 41•19 years ago
|
||
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?
Assignee | ||
Comment 42•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
> 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.
Summary: Japanese font face is displayed different depends on OS locale (Roman UI font in Japanese locale should be displayed as Lucida Grande) → Roman script of UI should be rendered with Lucida Grande on all OS locale
Updated•19 years ago
|
Summary: Roman script of UI should be rendered with Lucida Grande on all OS locale → Roman script of UI should be rendered with Lucida Grande on all system locales
Comment 47•19 years ago
|
||
Why are you bothered to simulate OS fallback list? IMO we should resolve bug 121540 first as comment #21 suggests.
Comment 48•19 years ago
|
||
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.
Attachment #205897 -
Flags: review?(tetsuroy) → review?(jshin1987)
Comment 49•19 years ago
|
||
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).
Attachment #205897 -
Flags: superreview?(sfraser_bugs)
Attachment #205897 -
Flags: review?(jshin1987)
Attachment #205897 -
Flags: review+
Comment 50•19 years ago
|
||
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); >
Attachment #205897 -
Flags: superreview?(sfraser_bugs) → superreview-
Comment 51•19 years ago
|
||
(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 53•19 years ago
|
||
(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".
Assignee | ||
Comment 54•19 years ago
|
||
should we be more explicit on the flatness of fontName? Looks like we need this (I don't know why).
Attachment #205897 -
Attachment is obsolete: true
Attachment #209089 -
Flags: superreview?(sfraser_bugs)
Attachment #209089 -
Flags: review?
Comment 55•19 years ago
|
||
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.
Attachment #209089 -
Flags: review? → review+
Updated•19 years ago
|
Attachment #209089 -
Flags: branch-1.8.1?(sfraser_bugs)
Updated•19 years ago
|
Flags: blocking1.8.0.2?
Comment 57•19 years ago
|
||
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)?
Attachment #209089 -
Flags: superreview?(sfraser_bugs)
Attachment #209089 -
Flags: superreview+
Attachment #209089 -
Flags: branch-1.8.1?(sfraser_bugs)
Attachment #209089 -
Flags: branch-1.8.1+
Comment 58•19 years ago
|
||
(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.
Assignee | ||
Comment 59•19 years ago
|
||
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•19 years ago
|
||
blocking 1.8.0.2 denied, not a regression, no trunk-baked patch.
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Comment 61•19 years ago
|
||
checked-in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1
Comment 63•19 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•19 years ago
|
Flags: blocking1.8.1?
Comment 64•19 years ago
|
||
Keywords: verified1.8.1
Updated•19 years ago
|
Keywords: fixed1.8.1
Updated•19 years ago
|
Flags: blocking1.8.0.3?
Whiteboard: [pending to request approval 1.8.0 branch after 1.5.0.2]
Comment 68•19 years ago
|
||
(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•19 years ago
|
||
Please ask for branch approval on the patch when you're confident in it for 1.8.0.3
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment 70•19 years ago
|
||
Too late for a non-approved patch, bumping to next release.
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.4-
Flags: blocking1.8.0.4+
Comment 71•19 years ago
|
||
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.
Flags: blocking1.8.0.5? → blocking1.8.0.5-
You need to log in
before you can comment on or make changes to this bug.
Description
•