Bug 76714 - implement Date.toLocaleString() using ICU
Summary: implement Date.toLocaleString() using ICU
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eli Fidler
Depends on:
Reported: 2012-01-20 10:21 PST by Eli Fidler
Modified: 2012-03-05 07:55 PST (History)
3 users (show)

See Also:

Patch (2.39 KB, patch)
2012-01-20 10:29 PST, Eli Fidler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eli Fidler 2012-01-20 10:21:13 PST
Some platforms don't localize strftime well, so use ICU to implement Date.toLocaleString()
Comment 1 Eli Fidler 2012-01-20 10:29:09 PST
Created attachment 123341 [details]
Comment 2 Darin Adler 2012-01-20 11:29:13 PST
Comment on attachment 123341 [details]

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.
Comment 3 Eli Fidler 2012-01-20 13:46:04 PST
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.
Comment 4 Darin Adler 2012-01-21 19:02:02 PST
(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.
Comment 5 Alexey Proskuryakov 2012-01-22 16:51:10 PST
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.
Comment 6 Eli Fidler 2012-01-23 09:45:21 PST
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 7 WebKit Review Bot 2012-01-25 08:09:01 PST
Comment on attachment 123341 [details]

Rejecting attachment 123341 [details] from commit-queue.

New failing tests:
Full output: http://queues.webkit.org/results/11342538
Comment 8 Eli Fidler 2012-01-25 15:54:07 PST
flaky test looks unrelated, so let's try cq+ again
Comment 9 WebKit Review Bot 2012-01-25 16:11:50 PST
Comment on attachment 123341 [details]

Clearing flags on attachment: 123341

Committed r105939: <http://trac.webkit.org/changeset/105939>
Comment 10 WebKit Review Bot 2012-01-25 16:11:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Mark Rowe (bdash) 2012-01-25 16:41:40 PST
This broke all of the Mac builds on build.webkit.org as unicode/udat.h is not available.
Comment 12 Mark Rowe (bdash) 2012-01-25 16:48:19 PST
I checked in a fix in r105943 to try and fix the Mac builds.
Comment 13 Alexey Proskuryakov 2012-03-05 07:55:29 PST
This has reportedly caused a regression, bug 80262.