Closed Bug 153586 Opened 22 years ago Closed 20 years ago

Date.prototype.toLocaleString ( ) not implemented correctly

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: mark, Assigned: tor)

References

Details

(Keywords: intl, Whiteboard: [QA note: also verify Regional Settings > Date > Long date style, etc.])

Attachments

(4 files, 16 obsolete files)

(deleted), text/html
Details
(deleted), patch
jst
: review+
brendan
: superreview+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
jst
: review+
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0rc3) Gecko/20020523 BuildID: 2002052306 Quoting from ECMAScript Language Specification Edition 3 24-Mar-00 15.9.5.5 Date.prototype.toLocaleString ( ) This function returns a string value. The contents of the string are implementation-dependent, but are intended to represent the Date in the current time zone in a convenient, human-readable form that corresponds to the conventions of the host environment's current locale. Using the hebrew regional setting I still get US style date strings returned from the method. Reproducible: Always Steps to Reproduce: 1. change regional setting of PC to hebrew 2. run the following script var d=new Date(); alert(d.toLocaleString()); Actual Results: US (english) date string displayed Expected Results: hebrew date string displayed
Reassigning to Kenton; cc'ing Mike and Waldemar. Note that "Regional Settings" is a Control Panel feature on Windows that says, "Many programs support international settings. Changing the Regional Settings affects the way these programs display and sort dates, times, currency, and numbers." The user can choose between English(U.S), French, Spanish, etc. markk5@bezeqint.net: out of curiosity, what does IE show when you run the same script? Thanks -
Assignee: rogerl → khanson
The Date function uses an API that allows the OS (windows) to format the string, so we thought it might work. There might be some sneakier API call required, though. Does it work on IE? Does it work on other applications that format dates for you (not sure what those would be...)
Well, IE returns "‏יום ראשון ‏23 ‏יוני ‏2002 07:59:45 " which is a right to left string of day, day in month, month name, year, time. Outlook express is sensative to the regional settings, you can see the change in the "date" field of a e-mail, and I guess that all other MS products use the settings as well.
Confirming bug with Mozilla trunk binary 2002061707 on WinNT. On WinNT, I go into Control Panel > Regional Settings and change the setting from English(U.S) to French(Standard). I launch Mozilla, run the above script, and get Sunday, June 23, 2002 14:25:15 I launch IE6, and get dimanche 23 juin 2002 14:26:06 I don't have to restart my PC in order to see this change in IE.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Reporter's testcase (deleted) —
I'm getting the correct danish string when I do this...
Bug 83092 is closely related (possibly a dup). On Linux date.toLocaleString returns a localized time, but it is interpreted as a ASCII (iso-8859-1?) string, not as a localized/unicode one, so it is not displayed correctly...
*** Bug 156143 has been marked as a duplicate of this bug. ***
There is no platform specific code in jsdate.c to handle date sensitive locale stuff. It appears that there should be a facility to somehow setup a callback so Javascript can call into the platform specific locale code. Right now this is totally broke.
*** Bug 176785 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2.1) Gecko/20021130 I have a problem getting the correct value for future dates. Try this: javascript:alert(new Date(209e10 ).toLocaleString() ); I see: "Monday, January 01 00:00:00:00 1900" Should be like 2036 or so. Not sure if this is a bug, but it does not seem correct.
Garrett is correct, and the problem occurs on Mac 9.x as well as on OSX. This problem only occurs on Mac, and not on WinNT or Linux. It occurs somewhere between 208e10 and 209e10 milliseconds after the Unix epoch: var dt = new Date(208e10); alert(dt.toString() + '\n' + dt.toLocaleString()); Thu Nov 29 2035 17:46:40 GMT-0800 Thursday November 29 17:46:40 2035 var dt = new Date(209e10); alert(dt.toString() + '\n' + dt.toLocaleString()); Mon Mar 24 2036 11:33:20 GMT-0800 Monday January 01 00:00:00 1900 <<<--------------- ?????????? We already have a Mac-only bug 61184 on the toLocaleString functions: "Date.prototype.toLocale___String() functions : GMT error" However, this issue seems to differ both from that one, and from the current bug, so I will file a separate report for it -
I have filed bug 188211: "(Mac-only): Date.prototype.toLocale___String() error on future dates"
*** Bug 87263 has been marked as a duplicate of this bug. ***
Adding QA note to Status whiteboard to honor the duped bug 87263: [QA note: also verify Regional Settings > Date > Long date style, etc.]
Whiteboard: [QA note: also verify Regional Settings > Date > Long date style, etc.]
cc'ing Brendan, Mike for consideration of this patch from tor@acm.org -
Comment on attachment 132417 [details] [diff] [review] add js callback to convert native string to unicode First scan looks good, and I think the approach is just fine, but I haven't reviewed in depth yet. Busy today, but Brendan might pick up my slack.
Looks good for the js/src changes. Cc'ing DOM folks to review the somewhat meatier dom/src/base changes. /be
Attachment #132417 - Flags: review?(jst)
Comment on attachment 132417 [details] [diff] [review] add js callback to convert native string to unicode - In LocaleToUnicode(): + PRUnichar *aLocaleUnichar = NULL; + nsString aCategory; + nsCString aCharset; The naming convention "aFoo" is typically used in Mozilla to name method arguments, so don't name local's "aFoo", in stead, use "foo". And in Mozilla C++ code, use "nsnull", not "NULL" (all over this method). And could you use nsXPIDLString here instead of using a raw PRUnichar ptr? + nsILocale *appLocale; + res = localeService->GetApplicationLocale(&appLocale); Use nsCOMPtr, i.e.: + nsCOMPtr<nsILocale> appLocale; + res = localeService->GetApplicationLocale(getter_AddRefs(appLocale)); and then there's no need to manually release the ptr. + if (!gDecoder) { + str = JS_NewStringCopyN(cx, src, srcLength); + } else { + PRInt32 unicharLength = srcLength; + PRUnichar *unichars = new PRUnichar[srcLength]; + nsresult rv; + + rv = gDecoder->Convert(src, &srcLength, unichars, &unicharLength); + if (NS_FAILED(rv)) { + delete unichars; + return JS_FALSE; + } + + str = JS_NewUCStringCopyN(cx, unichars, unicharLength); + delete unichars; + } Flip this around, put the success case (i.e. we got a decoder) in the if, and the error case in the else. And use "delete []unichars" since unichars is an array. And never ever return JS_FALSE w/o ensuring that a JS exception has been thrown. In most cases here, you'll need to throw one (use nsDOMClassInfo::ThrowJSException(cx, rv)). Other than those issues, this looks good. Fix that, and I'll have one more look.
Attachment #132417 - Flags: review?(jst) → review-
Attached patch address jst's comments (obsolete) (deleted) — Splinter Review
Attachment #132417 - Attachment is obsolete: true
Attached patch missed one... (obsolete) (deleted) — Splinter Review
Attachment #133520 - Attachment is obsolete: true
Comment on attachment 133521 [details] [diff] [review] missed one... - In LocaleToUnicode() + nsString category; + nsCString charset; Make those nsAuto[C]String. + category.Assign(NS_LITERAL_STRING("NSILOCALE_TIME##PLATFORM")); [...] + res = appLocale->GetCategory(category.get(), getter_Copies(locale)); You could actually eliminate the local "category" string here by inlining the NS_LITERAL_STRING().get() in the above call, or if you like to have the local variable, use NS_NAMED_LITERAL_STRING() to avoid copying the string into the local variable. + if (gDecoder) { + PRInt32 unicharLength = srcLength; + PRUnichar *unichars = new PRUnichar[srcLength]; you should throw NS_ERROR_OUT_OF_MEMORY if the above new fails. + if (!str) + str = JS_NewStringCopyN(cx, src, srcLength); + + if (!str) { + nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED); + return JS_FALSE; + } Move the second if (!str) inside the first one to avoid performing the check twice even when str is non-null. r/sr=jst with the above suggested changes. Thanks for the patch!
Attachment #133521 - Flags: review+
Attached patch polish, polish, polish... (obsolete) (deleted) — Splinter Review
Address latest set of comments. Instead of throwing an error if the unichar alloc fails I made it fall back into the old codepath.
Attachment #133521 - Attachment is obsolete: true
Comment on attachment 133521 [details] [diff] [review] missed one... Drive-by review comments: rv is the canonical name, not res, and there's no need to initialize it given that it is set in straight line code below the declaration (but move the declaration down to first set). Then the block-local rv can be unified with "res". Don't we want to fail if gDecoder can't be bootstrapped? Returning a locale-specific charset encoded string inflated from bytes to uint16s seems like a bad idea. /be
Attached patch lather, rinse, repeat (obsolete) (deleted) — Splinter Review
Address brendan's comments.
Attachment #133528 - Attachment is obsolete: true
Suggested revisions to LocaleToUnicode, made via editing a fragment of the latest patch: +static JSBool JS_DLL_CALLBACK +LocaleToUnicode(JSContext *cx, char *src, jsval *rval) +{ + nsresult rv; + + if (!gDecoder) { + // use app default locale + nsCOMPtr<nsILocaleService> localeService = + do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv); + if (NS_SUCCEEDED(rv)) { + nsCOMPtr<nsILocale> appLocale; + rv = localeService->GetApplicationLocale(getter_AddRefs(appLocale)); + if (NS_SUCCEEDED(rv)) { + nsXPIDLString locale; + rv = appLocale-> + GetCategory(NS_LITERAL_STRING("NSILOCALE_TIME##PLATFORM").get(), + getter_Copies(locale)); + NS_ASSERTION(NS_SUCCEEDED(rv), "failed to get app locale info"); + + nsCOMPtr <nsIPlatformCharset> platformCharset = + do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv); + if (NS_SUCCEEDED(rv)) { + nsCAutoString charset; + rv = platformCharset->GetDefaultCharsetForLocale(locale, charset); + if (NS_SUCCEEDED(rv)) { + // Get or create a unicode decoder for charset. + nsCOMPtr <nsICharsetConverterManager> ccm = + do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv); + if (NS_SUCCEEDED(rv)) + ccm->GetUnicodeDecoder(charset.get(), &gDecoder); + } + } + } + } + } + + JSString *str = nsnull; + PRInt32 srcLength = PL_strlen(src); + + if (gDecoder) { + PRInt32 unicharLength = srcLength; + PRUnichar *unichars = (PRUnichar*) malloc(srcLength * sizeof(PRUnichar)); + if (unichars) { + rv = gDecoder->Convert(src, &srcLength, unichars, &unicharLength); + if (NS_SUCCEEDED(rv)) + str = JS_NewUCString(cx, unichars, unicharLength); + else + free(unichars); + } + } + + if (!str) { + nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED); + return JS_FALSE; + } + + *rval = STRING_TO_JSVAL(str); + return JS_TRUE; +} Comments on this patch-fragment: 0. No point in declaring locale early -- I moved it down to right before it is set by appLocale->GetCategory. 1. No point in declaring charset early, or giving it a default value ("ISO-8869-1") that should never be used, given proper failure result checking. 2. Control flow should avoid retesting failed rv, or overwriting earlier failure code in rv to use the default "ISO-8869-1" charset. Thus no need for mappedCharset, and charset moves down and "in" (indentation-wise) to where it is set and used. 3. Hand off malloc'd memory for the converted string to the JS engine, saving a new[]/delete[] temporary. It appears that the string storage may be overallocated -- this seems to be inherent in the design of the Convert method of nsIUnicodeDecoder. We could realloc to exact (converted) length before handing off to JS_NewUCString, but I didn't make that change in this patch-fragment. I'm not sure it's worth the code. Tor, any idea how much for common charsets we would save? 4. Fixed variant brace style nit after if (!gDecoder). 5. Avoided going over 80 columns. Reassigning to tor, who was kind enough to take this bug in the first place! /be
Assignee: khanson → tor
In my haste, I left out the JS_NewUCString error handling code: + if (gDecoder) { + PRInt32 unicharLength = srcLength, origSrcLength = srcLength; + PRUnichar *unichars = (PRUnichar*) malloc(srcLength * sizeof(PRUnichar)); + if (unichars) { + rv = gDecoder->Convert(src, &srcLength, unichars, &unicharLength); + if (NS_SUCCEEDED(rv)) { + // nsIUnicodeDecoder::Convert may use less than srcLength PRUnichars. + if (unicharLength < origSrcLength) { + PRUnichar *shrunkUnichars = (PRUnichar*) + realloc(unichars, unicharLength * sizeof(PRUnichar)); + if (shrunkUnichar) { + unichars = shrunkUnichars; + } + } + str = JS_NewUCString(cx, unichars, unicharLength); + } + if (!str) { + free(unichars); + } + } + } This is tricky code in part because Convert may change srcLength too (it's an in-out param). From nsNativeUConvService.cpp's IConvAdaptor::ConvertInternal, it seems that only if iconv returns -1, and other conditions after that are true, might we return from Convert without updating unicharLength (or srcLength). I'm assuming, so don't trust me, that in the normal case of a complete, well-formed, charset-encoded string in src, Convert will return with srcLength 0 and unicharLength the exact number of PRUnichars stored in unichar. /be
Notes on last comment: 0. I should have noted that I added the cut-to-fit realloc code. 1. Yes, shrinking realloc may fail, at least on FreeBSD (or so someone demonstrated once, in a JS-related patch). 2. Nit-picking my own comment, I should have written "fewer than srcLength..." not "less than", but what a kook am I to pick on this?! /be
> I'm assuming, so don't trust me, that in the normal case of a complete, > well-formed, charset-encoded string in src, Convert will return with > srcLength 0 and unicharLength the exact number of PRUnichars stored in unichar. s/srcLength 0/srcLength unchanged/ which means the origSrcLength variable is unnecessary. But what a sucky interface method Convert is. Grumble. /be
Attached patch update to latest round of comments (obsolete) (deleted) — Splinter Review
Deeply nested control logic isn't really my style. I tend to agree with Linus' coding style (linux/Documentation/CodingStyle): Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. I think it's worth trying to shorten the string before passing it off to JS. It only costs a single compare if not necessary (for example en_US), and the savings can be significant even for just date strings. A couple random examples: ta_IN: ~50% smaller ko_KR.utf8: ~35% smaller zh_CN: ~25% smaller
Attachment #133685 - Attachment is obsolete: true
Comment on attachment 133703 [details] [diff] [review] update to latest round of comments I'm an old Unix fart -- for years I hacked using 8-space-equivalent tabs for indentation. The discipline of that huge honking indentation unit led, in part, to better control flow via break, continue, and early return (and the more than occasional downward goto! ;-). But here we have a case where we want to fall through to the consolidated if (!src) error return, if anything failed trying to bootstrap gDecoder. The only thought is to move the bootstrapping code out to its own static helper, but why bother? So: two-space indentation is for wimps, yeah ;-) But When In Rome. Which reminds me: that opening brace after if (!gDecoder) is still flouting local style conventions. I leave it to jst to hold the line. Looks good to me, pre-emptive sr=brendan. Thanks again, /be
Attachment #133703 - Flags: superreview+
(FWIW, I never agreed with a low arbitrary limit on indentation, as Linus puts forth. Why? There's no mathematical or algorithmic reason behind it. My SGI kernel code sometimes went over 4 or even 5 tabs! /be)
Attachment #133703 - Flags: review?(jst)
Comment on attachment 133703 [details] [diff] [review] update to latest round of comments A few more nits, and one problem found... - In LocaleToUnicode(): + if (!gDecoder) + { As brendan pointed out, that brace should go to the end of the previous line. + nsCOMPtr <nsIPlatformCharset> platformCharset = + do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv); ... + nsCOMPtr <nsICharsetConverterManager> ccm = + do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv); Loose the space between nsCOMPtr and "<...". + PRUnichar *unichars = (PRUnichar *)malloc(srcLength * sizeof(PRUnichar)); + if (unichars) { + rv = gDecoder->Convert(src, &srcLength, unichars, &unicharLength); + if (NS_SUCCEEDED(rv)) { ... + str = JS_NewUCString(cx, unichars, unicharLength); + } + if (!str) + free(unichars); Only free if !str and unichar is non-null, otherwise you'll crash if the first malloc fails. r=jst with those changes.
Attachment #133703 - Flags: review?(jst) → review+
The "if (!str) free(...)" is inside "if (unichars)", so the situation you mentioned doesn't happen.
Oh, duh. My eyes must not have waken up yet, or something.
Thanks for the feedback, everyone - checked in with the last nits fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Patch breaks building on GCC 3.3.1, AFAIK. http://forums.mozillazine.org/viewtopic.php?t=30746
Build bustage fix checked in (r+sr=jst, a=tor).
*** Bug 83092 has been marked as a duplicate of this bug. ***
Comment on attachment 133703 [details] [diff] [review] update to latest round of comments a=asa (on behalf of drivers) for backout for 1.6 alpha.
Attachment #133703 - Flags: approval1.6a+
Backed out due to this breaking toLocaleString completely on non-unix platforms (Win32, OS/2).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch backout diff (obsolete) (deleted) — Splinter Review
Attached patch updated patch (obsolete) (deleted) — Splinter Review
Changes to get patch working on win2k and unix: * use NSILOCALE_TIME instead of NSILOCALE_TIME##PLATFORM (?) * null-terminate returned string from decoder * call setlocale() at startup to pull us out of the "C" locale If anyone could test this out on win98 or os2 I'd be interested in the results.
Attachment #133703 - Attachment is obsolete: true
Attachment #134253 - Attachment is obsolete: true
Comment on attachment 134276 [details] [diff] [review] updated patch Firebird, Thunderbird, and the embedding test harnesses won't benefit from the xpfe change to call setlocale. Isn't there some generic place to put it, that all embedding apps must call into at startup? /be
Attached patch move setlocale to NS_InitXPCOM2 (obsolete) (deleted) — Splinter Review
Attachment #134276 - Attachment is obsolete: true
As an embeddable browser, I am not sure I would want xpcom to change my default code page. Is setting the code page to ANSI is correct? What if the system has a default code page of UTF8? Sorry, I just don't know enough about how setlocale will effect embedders.
Programs start in the "C" locale (part of the C specification). Setting the locale to "" causes us to switch from that to whatever the system locale is set to.
tor, sorry I wasn't more clear: suppose I am writing app that uses gecko. at some point, I initalize Gecko. However, before that I have modified the locale for my embedding application. Do you really want the init of gecko reverting the system locale back to the default?
Possibly not, but on the other hand do we want to trust embedders/users of gecko to set the locale properly?
If we are counting on a certain locale setting (when using strftime, e.g.), we had better set it ourselves. If it were cheap enough and we could save/restore the old setting in a safe way, then I suppose we could just do that: bracket our uses of C standard functions that depend on locale in save/restore setlocale calls. tor, what do you think? /be
Sure, that would work. I doubt anyone would be using locale specific date output in a performance critical fashion.
Attached patch more updates (obsolete) (deleted) — Splinter Review
Changes this time around: * update to tip (locale changes) * back to NSILOCALE_TIME##PLATFORM, as nsUNIXCharset expects the posix name of the locale instead of the mozilla name (_ vs -) * bracket strftime with setlocale if the locale hasn't already been been set by the calling environment
Attachment #134334 - Attachment is obsolete: true
Comment on attachment 134489 [details] [diff] [review] more updates >@@ -1614,9 +1615,16 @@ > JSString *str; > PRMJTime split; > jsdouble *date = date_getProlog(cx, obj, argv); >+ static JSBool locale_checked = JS_FALSE, need_setlocale; >+ > if (!date) > return JS_FALSE; > >+ if (!locale_checked) { >+ need_setlocale = (strcmp(setlocale(LC_TIME, NULL), "C") == 0); >+ locale_checked = JS_TRUE; >+ } This isn't thread-safe, and it's conceivable that two threads could race in a way that leaves one setting need_setlocale to true; while the second thread, preempted just after it tested if (!locale_checked) and before the first thread set locale_checked to true, runs again only after the first thread has changed the locale temporarily. The result could be need_setlocale being cleared. PR_CallOnce could help, but I think we should do something else instead (see below). BTW, statics are zero-initialized by default, and explicitly initializing puts the static into data instead of bss (black starting storage, which takes no object file space). Small point, but worth doing consistently with the rest of the engine, if the static were safe to use here. >+ > if (!JSDOUBLE_IS_FINITE(*date)) { > JS_snprintf(buf, sizeof buf, js_NaN_date_str); > } else { >@@ -1625,7 +1633,11 @@ > new_explode(local, &split, JS_FALSE); > > /* let PRMJTime format it. */ >+ if (need_setlocale) >+ setlocale(LC_TIME, ""); (the then statement is overindented by two spaces) > result_len = PRMJ_FormatTime(buf, sizeof buf, format, &split); >+ if (need_setlocale) >+ setlocale(LC_TIME, "C"); I don't see why the setlocale calls and logic should be in jsdate.c instead of at a much higher level. Probably I was confused in asking for setlocale save/restore code -- for some reason I had thought you could do that in the DOM callback, but it runs after the PRMJ_FormatTime call returns. So, sorry for the bum steer. I don't think this belongs in the bowels of the JS engine, thread safety issues aside. I'm coming to the conclusion that we should require all Gecko embedders to use the "" locale. Does this come up apart from JS Date issues? If Gecko depends on the locale for other kinds of conversions (, instead of . in numbers converted to strings?), then that supports the argument that embedders need to do one setlocale at startup. /be
Attached patch another update... (obsolete) (deleted) — Splinter Review
Move setlocale back to nsAppRunner, remove ##PLATFORM from mozilla.
Attachment #134489 - Attachment is obsolete: true
Cc'ing dbaron in case he knows of other Gecko locale dependencies that argue for a setlocale(LC_ALL, ""); in a common place in Gecko/XPCOM startup code. /be
Attached patch minor update (obsolete) (deleted) — Splinter Review
Fix existing bug in nsLocaleService::nsLocaleService that pkw noticed. If LANG was null the language was set to "eu-US", while I think it should be "en-US".
Attachment #134496 - Attachment is obsolete: true
chris may know more about this topic.
Attachment #134497 - Attachment is obsolete: true
Attached patch different approach (obsolete) (deleted) — Splinter Review
Don't reinvent the wheel and instead use NS_CopyNativeToUnicode. Not working yet on win32 for some reason - investigating.
Attached patch forgot to diff xpfe (obsolete) (deleted) — Splinter Review
Attachment #134525 - Attachment is obsolete: true
Comment on attachment 134528 [details] [diff] [review] forgot to diff xpfe Patch is shrinking, nice. NS_ERROR_FAILURE is better than NS_ERROR_UNEXPECTED, unless we can be sure it's really NS_ERROR_OUT_OF_MEMORY. But UNEXPECTED is for assertion failure defense, not for a condition we test explicitly and therefore "expect" (OOM, some kind of locale-specific CSE botch in the src string). I still think Gecko wants that setlocale(LC_ALL, "") in its startup code. It doesn't have to go in NS_InitXPCOM2. Dbaron, are there other needs in Gecko for this? What's a good early/common init hook? /be
Keywords: intl
i am not against putting it in XPCOM initalization. I just want to be sure I understand this change and what it means to embedders.
Attachment #134528 - Attachment is obsolete: true
Attached patch back to plan A (obsolete) (deleted) — Splinter Review
* Switch back to old method of converting string, as NS_NativeToUnicode is only appropriate for filenames (uses the OS native charset). * Move setlocale back to xpcom for the fun of it, only done if the locale wasn't previously set.
Blocks: dateandtime
Attachment #134574 - Attachment is obsolete: true
Attachment #143978 - Flags: superreview?(brendan)
Attachment #143978 - Flags: review?(jst)
Comment on attachment 143978 [details] [diff] [review] update last patch to trunk (offsets/fuzz) - In LocaleToUnicode(): + if (!str) { + nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED); + return JS_FALSE; There's a couple of cases in the code above where you really should throw NS_ERROR_OUT_OF_MEMORY (i.e. if malloc, realloc, or JS_NewUCString returns null). Check for those, return OOM in those cases in stead of NS_ERROR_UNEXPECTED. r=jst
Attachment #143978 - Flags: review?(jst) → review+
Comment on attachment 143978 [details] [diff] [review] update last patch to trunk (offsets/fuzz) >+ JSString *str = nsnull; >+ PRInt32 srcLength = PL_strlen(src); >+ >+ if (gDecoder) { >+ PRInt32 unicharLength = srcLength; >+ PRUnichar *unichars = (PRUnichar *)malloc((srcLength + 1) * sizeof(PRUnichar)); >+ if (unichars) { >+ rv = gDecoder->Convert(src, &srcLength, unichars, &unicharLength); >+ if (NS_SUCCEEDED(rv)) { >+ // terminate the returned string >+ unichars[unicharLength] = 0; >+ >+ // nsIUnicodeDecoder::Convert may use fewer than srcLength PRUnichars >+ if (unicharLength + 1 < srcLength) { Shouldn't this be <= because unichars points to (srcLength + 1) PRUnichars? Or < srcLength + 1, which may be clearer, but the compiler should optimize to <= srcLength. >+ PRUnichar *shrunkUnichars = >+ (PRUnichar *)realloc(unichars, (unicharLength + 1) * sizeof(PRUnichar)); >+ if (shrunkUnichars) >+ unichars = shrunkUnichars; >+ } >+ str = JS_NewUCString(cx, >+ NS_REINTERPRET_CAST(jschar*, unichars), >+ unicharLength); >+ } >+ if (!str) >+ free(unichars); >+ } >+ } >+ >+ if (!str) { >+ nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED); >+ return JS_FALSE; >+ } NS_ERROR_OUT_OF_MEMORY seems slightly better to me, or FAILURE. UNEXPECTED is more for internal inconsistencies, from what I've seen and hacked. May want others (jshin?) to r= separate modules. /be
Attachment #143978 - Flags: review+ → review?(jst)
Comment on attachment 143978 [details] [diff] [review] update last patch to trunk (offsets/fuzz) I'm afraid your changes in intl/locale can break other things. It's quite subtle and we shouldn't move in a hurry here.
This patch has been hanging around for about four months - I hardly consider that "moving in a hurry." If you have specific problems with the intl changes I'd like to hear them. As far as I could tell the ##PLATFORM stuff wasn't used in a meaningful way anymore.
Comment on attachment 143978 [details] [diff] [review] update last patch to trunk (offsets/fuzz) r=jst with the earlier review comments addressed.
Attachment #143978 - Flags: review?(jst) → review+
Comment on attachment 143978 [details] [diff] [review] update last patch to trunk (offsets/fuzz) sr=me with comment 65 and comment 66 addressed. /be
Attachment #143978 - Flags: superreview?(brendan) → superreview+
Sorry how long it has been around doesn't matter. I've made a similar change to intl/locale while trying to fix another bug and it regressed other things. (sorry I can't remember exactly which bug and which patch were. I'll get back later. It may take a few days, though). We shouldn't check in this patch until everything is clear.
Attached patch address brendan and jst comments (obsolete) (deleted) — Splinter Review
Attachment #145502 - Attachment is obsolete: true
jshin: it's been quite some time since you brought up your concerns. Have you found the bug in question?
Comment on attachment 146210 [details] [diff] [review] previous diff messed up - corrected version > +static JSBool JS_DLL_CALLBACK would JS_STATIC_DLL_CALLBACK(__x) be appropriate? >+ // get us out the "C" locale and into the system ... out _of_ the ... ? :)
Checked in with timeless' nits fixed - watching for regressions.
Status: REOPENED → RESOLVED
Closed: 21 years ago20 years ago
Resolution: --- → FIXED
Bug still there in Mozilla 1.7.2 and FireFox 0.9.3 They give "Saturday, August 21, 2004 12:23:55", while IE gives "21. avgust 2004 12:22:07" In Regional Settings I have : Long Date : 21. avgust 2004 My system is Windows 2003 server
Attached patch POSIXLOCALE is a service (deleted) — Splinter Review
So don't do_CreateInstance it.
Attachment #165412 - Flags: superreview?(jst)
Attachment #165412 - Flags: review?(jshin)
Comment on attachment 165412 [details] [diff] [review] POSIXLOCALE is a service r+sr=jst
Attachment #165412 - Flags: superreview?(jst)
Attachment #165412 - Flags: superreview+
Attachment #165412 - Flags: review?(jshin)
Attachment #165412 - Flags: review+
Bug still there in Mozilla 1.7.5 and FireFox 1.0 tested on Windows XP with Spanish(Mexico) in Regional Options. Mozilla shows "Monday, December 13, 2004 14:32:00", while IE gives "Lunes, 13 de Diciembre de 2004 02:32:00 p.m." Meed to reopen the bug..
Bug still there in Mozilla 1.7.5 and FireFox 1.0 tested on Windows XP with Spanish(Mexico) in Regional Options. Mozilla shows "Monday, December 13, 2004 14:32:00", while IE gives "Lunes, 13 de Diciembre de 2004 02:32:00 p.m." Need to reopen the bug..
Ray, this was fixed on the trunk, not on the 1.7.x branches. Verified with Firefox and Seamonkey trunk builds. This will be fixed in Firefox 1.1.
Status: RESOLVED → VERIFIED
*** Bug 290052 has been marked as a duplicate of this bug. ***
Flags: testcase?
*** Bug 307004 has been marked as a duplicate of this bug. ***
*** Bug 311588 has been marked as a duplicate of this bug. ***
Flags: testcase? → testcase-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: