| Summary: | implement Date.toLocaleString() using ICU | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eli Fidler <efidler> | ||||
| Component: | JavaScriptCore | Assignee: | Eli Fidler <efidler> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ap, darin, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Eli Fidler
2012-01-20 10:21:13 PST
Created attachment 123341 [details]
Patch
Comment on attachment 123341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123341&action=review > Source/JavaScriptCore/runtime/DatePrototype.cpp:210 > + UDateFormat* df = udat_open(timeStyle, dateStyle, 0, 0, -1, 0, 0, &status); Since the point of these functions is to format for the current locale, it seems critical to pass in a suitable locale string. See, for example, what is done for text break locales in the currentTextBreakLocaleID function. I’m surprised that is OK to pass 0 for the locale string. 0 is the "default locale". We rely on ICU for localization in many areas, so apps will have the default locale set appropriately to the desired locale. I guess that's not true for OSX or IOS, but there's a different path here for that. I could put the strftime path into HAVE(STRFTIME) or make a USE(ICU_DEFAULT_LOCALE) if you prefer. (In reply to comment #3) > 0 is the "default locale" Is that a general ICU feature, or one specific to RIM platforms? I ask because I don’t see it mentioned in ICU documentation. > We rely on ICU for localization in many areas Sure, OS X and iOS do too. My question is about how the ICU API works. > I could put the strftime path into HAVE(STRFTIME) or make a USE(ICU_DEFAULT_LOCALE) if you prefer. Since this code passes 0 for the locale it should be used only on platforms where that will do the right thing. To get the #if right we need to know how where that “0 means default locale” feature exists. This may not be a regression from strftime, but the behavior would not be correct when locale changes at runtime, especially with WebKit2. In Mac Safari, output of (new Date).toLocaleString() changes immediately after a user changes the format in system preferences. NULL for default locale is an ICU-standard feature: "Using the ICU C functions, NULL can be passed for a locale parameter to specify the default locale." from: http://userguide.icu-project.org/locale#TOC-Default-Locales For Alexey's point, Date.toLocaleString() will always use the current default locale. If that is changed using uloc_setDefault(), the new value should be immediately take effect. Comment on attachment 123341 [details] Patch Rejecting attachment 123341 [details] from commit-queue. New failing tests: media/audio-garbage-collect.html Full output: http://queues.webkit.org/results/11342538 flaky test looks unrelated, so let's try cq+ again Comment on attachment 123341 [details] Patch Clearing flags on attachment: 123341 Committed r105939: <http://trac.webkit.org/changeset/105939> All reviewed patches have been landed. Closing bug. This broke all of the Mac builds on build.webkit.org as unicode/udat.h is not available. I checked in a fix in r105943 to try and fix the Mac builds. This has reportedly caused a regression, bug 80262. |