1.36 KB, patch
|Details | Diff | Splinter Review|
1.40 KB, patch
|Details | Diff | Splinter Review|
we currently load langGroups.properties at startup time, we should find a way to remove the need of loading it at startup time to speed up startup performance we could 1. build in common used pair in the cpp code, and/or 2. cache the resolved result in registry.
per offline discussion with ftang - tentatively reassigning to myself
Assignee: ftang → jbetak
Status: ASSIGNED → NEW
Stack trace for when this gets loaded: LookupLanguage("en-US"); en-US should be probably hardcoded into the app. (gdb) bt #0 stat ( __path=0x82255d8 "/home/dp/moz/mozilla/dist/bin/res/langGroups.properties", __statbuf=0x8225714) at /usr/include/sys/stat.h:346 #1 0x401d5e2c in nsLocalFile::FillStatCache (this=0x8225708) at nsLocalFileUnix.h:100 #2 0x40177949 in nsLocalFile::IsDirectory (this=0x8225708, _retval=0xbfffdef8) at nsLocalFileUnix.cpp:1226 #3 0x40980ce0 in nsFileIO::GetInputStream (this=0x822b590, aInputStream=0xbfffdfa8) at nsFileStreams.cpp:238 #4 0x40982be8 in nsFileTransport::OpenInputStream (this=0x822b4f0, aTransferOffset=0, aTransferCount=4294967295, aFlags=0, aResult=0xbfffdfa8) at nsFileTransport.cpp:457 #5 0x409d7233 in nsFileChannel::Open (this=0x8225630, result=0xbfffdfa8) at nsFileChannel.cpp:245 #6 0x415b4962 in NS_OpenURI (result=0xbfffe020, uri=0x82265b0, ioService=0x0, loadGroup=0x0, notificationCallbacks=0x0, loadAttributes=0) at ../../../dist/include/nsNetUtil.h:177 #7 0x415aed88 in nsLanguageAtomService::InitLangGroupTable (this=0x8212500) at nsLanguageAtomService.cpp:130 #8 0x415af282 in nsLanguageAtomService::LookupLanguage (this=0x8212500, aLanguage=0x8101f20, aResult=0xbfffe380) at nsLanguageAtomService.cpp:177 #9 0x415aff15 in nsLanguageAtomService::GetLocaleLanguageGroup ( this=0x8212500, aResult=0xbfffe5b0) at nsLanguageAtomService.cpp:285 #10 0x415af81c in nsLanguageAtomService::LookupCharSet (this=0x8212500, aCharSet=0xbfffe698, aResult=0x820dbac) at nsLanguageAtomService.cpp:230
dp, I apologize for dragging my feet with this. It seems like a small change, but I'll have to verify the platform locale strings for all major platforms and possibly make the ifs a little smarter. Dumping all language-language group pairs seems to be not a real option at this point, as i18n desires run-time extensibility. Looks like the goal is to remove the largest performance penalty and if that proves not to be enough continue with another push at the expense of runtime extensibility.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
I would expect these to be: en_US ja_JP.eucJP de_DE not what is in the patch.
Brian, the locale string is already being converted to lowercase by the existing code, which makes only "ja_JP.eucJP" disputable. Japanese NT is returning ja-jp and I haven't checked Japanes Linux and Mac yet. Is it reasonably safe to assume that the locale string will be uniform across platforms. I'd like to avoid parsing the language ID out to minimize the performance penalty we are introducing for other language groups. Cc'ing nhotta for the question on uniformness of platform locale strings.
oh, and I verified on German Linux (SuSe distribution) that the separator is in fact a hyphen and not an underscore. Similarly based on the existing code I think it's reasonable to expect an hyphen: http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLanguageAtomService.cpp#197
Platform locale is platform specific. For Macintosh, platform locale is not a string but a collection of numbers (scriptCode, langCode, regionCode). XP locale is cross platform, it is lang and country separated by a hyphen.
If this is going into cross platform code then the current patches leave me very uncomfortable. What if the locale is de_DE@Euro or de_DE.UTF8 or de_DE.UTF@EURO ?
bstell the function jbetak is changing is 157 nsLanguageAtomService::LookupLanguage(const PRUnichar* aLanguage, 158 nsILanguageAtom** aResult) The input is NOT a locale string, but a Language String, which should be in the format of "xx-yy" where "xx" is the two letter language code and "yy" is the two letter region code. This part is ensure by the platform specific locale code. If the input is not in that format, then it mean our platform specific locale code (which convert from platform locale name to xp language name) have bug and we should fix it there. The patch simply shortcut our tier one language to by passed those operation. The risk is later we may introduce bug in the non shortcut part without knowing them since most of the time we won't hit those code.
Comment on attachment 51981 [details] [diff] [review] correcting my "guesstimate" for the Japanese locale string - Japanese NT is returning ja-jp r=ftang I know you are using -w -b to show the diff so I won't see the indent differences please make sure while you check in, the else part is iindent correctly (which you probably already did in your tree)
Attachment #51981 - Flags: review+
thanks for explaining this Frank. I was looking into the locale code to understand fully how it works, my feeling was that in LookupLanguage() we are expecting a language-locale pair. Brian here is the only caller of LookupLanguage(): http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLanguageAtomService.cpp#300 and here is nsLocale.cpp, which I was trying to understand better: http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLocale.cpp#121 I apologize for all the confusion.
cc'ing Chris Blizzard for potential i18n sr.
change to m0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Use foo.Assign(NS_LITERAL_STRING("bar")) instead of foo.AssignWithConversion("bar"). Make those changes, and sr=waterson.
Attachment #51981 - Attachment is obsolete: true
fix checked in - thanks everyone!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Switching qa cantact to ftang for now. Frank, can this one be verified within development or can you provide QA with a test case? Thanks.
QA Contact: andreasb → ftang
You need to log in before you can comment on or make changes to this bug.