Closed
Bug 153586
Opened 22 years ago
Closed 20 years ago
Date.prototype.toLocaleString ( ) not implemented correctly
Categories
(Core :: JavaScript Engine, defect)
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+
jst
:
superreview+
|
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
Comment 1•22 years ago
|
||
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
Comment 2•22 years ago
|
||
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...)
Reporter | ||
Comment 3•22 years ago
|
||
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.
Comment 4•22 years ago
|
||
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
Comment 5•22 years ago
|
||
Comment 6•22 years ago
|
||
I'm getting the correct danish string when I do this...
Comment 7•22 years ago
|
||
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...
Comment 8•22 years ago
|
||
*** Bug 156143 has been marked as a duplicate of this bug. ***
Comment 9•22 years ago
|
||
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.
Comment 10•22 years ago
|
||
*** Bug 176785 has been marked as a duplicate of this bug. ***
Comment 11•22 years ago
|
||
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.
Comment 12•22 years ago
|
||
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 -
Comment 13•22 years ago
|
||
I have filed bug 188211:
"(Mac-only): Date.prototype.toLocale___String() error on future dates"
Comment 14•22 years ago
|
||
*** Bug 87263 has been marked as a duplicate of this bug. ***
Comment 15•22 years ago
|
||
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.]
Assignee | ||
Comment 16•21 years ago
|
||
Comment 17•21 years ago
|
||
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.
Comment 19•21 years ago
|
||
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 20•21 years ago
|
||
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-
Assignee | ||
Comment 21•21 years ago
|
||
Attachment #132417 -
Attachment is obsolete: true
Assignee | ||
Comment 22•21 years ago
|
||
Attachment #133520 -
Attachment is obsolete: true
Comment 23•21 years ago
|
||
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+
Assignee | ||
Comment 24•21 years ago
|
||
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 25•21 years ago
|
||
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
Assignee | ||
Comment 26•21 years ago
|
||
Address brendan's comments.
Attachment #133528 -
Attachment is obsolete: true
Comment 27•21 years ago
|
||
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
Comment 28•21 years ago
|
||
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
Comment 29•21 years ago
|
||
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
Comment 30•21 years ago
|
||
> 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
Assignee | ||
Comment 31•21 years ago
|
||
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 32•21 years ago
|
||
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+
Comment 33•21 years ago
|
||
(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 34•21 years ago
|
||
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+
Assignee | ||
Comment 35•21 years ago
|
||
The "if (!str) free(...)" is inside "if (unichars)", so the situation you
mentioned doesn't happen.
Comment 36•21 years ago
|
||
Oh, duh. My eyes must not have waken up yet, or something.
Assignee | ||
Comment 37•21 years ago
|
||
Thanks for the feedback, everyone - checked in with the last nits fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 38•21 years ago
|
||
Patch breaks building on GCC 3.3.1, AFAIK.
http://forums.mozillazine.org/viewtopic.php?t=30746
Assignee | ||
Comment 39•21 years ago
|
||
Build bustage fix checked in (r+sr=jst, a=tor).
Comment 40•21 years ago
|
||
*** Bug 83092 has been marked as a duplicate of this bug. ***
Comment 41•21 years ago
|
||
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+
Assignee | ||
Comment 42•21 years ago
|
||
Backed out due to this breaking toLocaleString completely on
non-unix platforms (Win32, OS/2).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 43•21 years ago
|
||
Assignee | ||
Comment 44•21 years ago
|
||
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 45•21 years ago
|
||
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
Assignee | ||
Comment 46•21 years ago
|
||
Attachment #134276 -
Attachment is obsolete: true
Comment 47•21 years ago
|
||
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.
Assignee | ||
Comment 48•21 years ago
|
||
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.
Comment 49•21 years ago
|
||
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?
Assignee | ||
Comment 50•21 years ago
|
||
Possibly not, but on the other hand do we want to trust embedders/users
of gecko to set the locale properly?
Comment 51•21 years ago
|
||
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
Assignee | ||
Comment 52•21 years ago
|
||
Sure, that would work. I doubt anyone would be using locale specific
date output in a performance critical fashion.
Assignee | ||
Comment 53•21 years ago
|
||
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 54•21 years ago
|
||
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
Assignee | ||
Comment 55•21 years ago
|
||
Move setlocale back to nsAppRunner, remove ##PLATFORM from mozilla.
Attachment #134489 -
Attachment is obsolete: true
Comment 56•21 years ago
|
||
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
Assignee | ||
Comment 57•21 years ago
|
||
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
Comment 58•21 years ago
|
||
chris may know more about this topic.
Attachment #134497 -
Attachment is obsolete: true
Assignee | ||
Comment 59•21 years ago
|
||
Don't reinvent the wheel and instead use NS_CopyNativeToUnicode.
Not working yet on win32 for some reason - investigating.
Assignee | ||
Comment 60•21 years ago
|
||
Attachment #134525 -
Attachment is obsolete: true
Comment 61•21 years ago
|
||
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
Comment 62•21 years ago
|
||
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
Assignee | ||
Comment 63•21 years ago
|
||
* 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
Assignee | ||
Comment 64•21 years ago
|
||
Attachment #134574 -
Attachment is obsolete: true
Attachment #143978 -
Flags: superreview?(brendan)
Attachment #143978 -
Flags: review?(jst)
Comment 65•21 years ago
|
||
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 66•21 years ago
|
||
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 67•21 years ago
|
||
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.
Assignee | ||
Comment 68•21 years ago
|
||
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 69•21 years ago
|
||
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 70•21 years ago
|
||
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+
Comment 71•21 years ago
|
||
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.
Assignee | ||
Comment 72•21 years ago
|
||
Assignee | ||
Comment 73•21 years ago
|
||
Attachment #145502 -
Attachment is obsolete: true
Assignee | ||
Comment 74•21 years ago
|
||
jshin: it's been quite some time since you brought up your concerns. Have
you found the bug in question?
Comment 75•21 years ago
|
||
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 ... ? :)
Assignee | ||
Comment 76•20 years ago
|
||
Checked in with timeless' nits fixed - watching for regressions.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 20 years ago
Resolution: --- → FIXED
Comment 77•20 years ago
|
||
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
Comment 78•20 years ago
|
||
So don't do_CreateInstance it.
Updated•20 years ago
|
Attachment #165412 -
Flags: superreview?(jst)
Attachment #165412 -
Flags: review?(jshin)
Comment 79•20 years ago
|
||
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+
Comment 80•20 years ago
|
||
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..
Comment 81•20 years ago
|
||
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..
Comment 82•20 years ago
|
||
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
Comment 83•20 years ago
|
||
*** Bug 290052 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Flags: testcase?
Comment 84•19 years ago
|
||
*** Bug 307004 has been marked as a duplicate of this bug. ***
Comment 85•19 years ago
|
||
*** Bug 311588 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Flags: testcase? → testcase-
You need to log in
before you can comment on or make changes to this bug.
Description
•