Bug 239865 - Format time zone name using ICU instead of platform calls
Summary: Format time zone name using ICU instead of platform calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords: InRadar
Depends on:
Blocks: 239981
  Show dependency treegraph
 
Reported: 2022-04-28 12:51 PDT by Yury Semikhatsky
Modified: 2022-05-02 13:39 PDT (History)
14 users (show)

See Also:


Attachments
Patch (23.46 KB, patch)
2022-04-28 13:04 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (27.54 KB, patch)
2022-04-28 16:47 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (28.29 KB, patch)
2022-04-28 16:55 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (30.14 KB, patch)
2022-04-28 20:13 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (30.96 KB, patch)
2022-04-28 20:32 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (35.37 KB, patch)
2022-04-29 08:35 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2022-04-28 12:51:55 PDT
Format time zone name using ICU instead of platform calls
Comment 1 Yury Semikhatsky 2022-04-28 12:54:40 PDT
This came up in the code review https://bugs.webkit.org/show_bug.cgi?id=213884#c57

formatDateTime function in DateConversion.cpp uses platform-specific calls to format time zone name and may benefit from switching to ICU library.
Comment 2 Darin Adler 2022-04-28 12:59:55 PDT
It’s a nice idea to do this if it’s an improvement. I like the idea of consistency with the other web browsers for interoperability reasons. We also have to check that there’s no localization benefit or benefit of adhering to the user’s preferences (e.g. 24-hour time vs. not, Language & Region advanced settings on Mac) to the platform-specific calls on the Apple platforms before landing this.
Comment 3 Yury Semikhatsky 2022-04-28 13:04:24 PDT
Created attachment 458543 [details]
Patch
Comment 4 Yury Semikhatsky 2022-04-28 13:09:45 PDT
(In reply to Darin Adler from comment #2)
> It’s a nice idea to do this if it’s an improvement. I like the idea of
> consistency with the other web browsers for interoperability reasons. We
> also have to check that there’s no localization benefit or benefit of
> adhering to the user’s preferences (e.g. 24-hour time vs. not, Language &
> Region advanced settings on Mac) to the platform-specific calls on the Apple
> platforms before landing this.

Any advice which test suite would be the best for it? Can I rely on the try bots output?

I tried running JSTests, tests262 and it was hard to tell if there are new regressions. I looked at the summary and it didn't seem to change. This maybe at least partially explained by the fact that in many tests there is already logic like this: https://github.com/WebKit/WebKit/blob/6712f9f2c510731fde20aff12f723017b3fb9cc4/JSTests/ChakraCore/test/Date/formatting.js#L6-L9 which converts "(PST)" => "(Pacific Standard Time)"
Comment 5 Yusuke Suzuki 2022-04-28 13:25:57 PDT
Comment on attachment 458543 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458543&action=review

Looks good to me.
Can you add a JSTests/stress/xxx.js test which exercise this change in JS?

> Source/JavaScriptCore/runtime/JSDateMath.cpp:376
> +            auto status = callBufferProducingFunction(ucal_getTimeZoneDisplayName, timeZoneCache.m_calendar.get(), UCAL_STANDARD, language.utf8().data(), standardDisplayNameBuffer);

Let's generate language.utf8() once, and use for both. We do not need to allocate it twice.
Comment 6 Yury Semikhatsky 2022-04-28 16:47:04 PDT
Created attachment 458556 [details]
Patch
Comment 7 Yury Semikhatsky 2022-04-28 16:52:49 PDT
(In reply to Darin Adler from comment #2)
> We also have to check that there’s no localization benefit or benefit of
> adhering to the user’s preferences (e.g. 24-hour time vs. not, Language &
> Region advanced settings on Mac) to the platform-specific calls on the Apple
> platforms before landing this.

Manually checked that the timezone is now formatted using preferred system language on macOS (previously the timezone was always in English). The new test sets language override to de-DE to demonstrate that. This change does not touch the code that does time formatting, only the timezone.

(In reply to Yusuke Suzuki from comment #5)
> Comment on attachment 458543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458543&action=review
> 
> Looks good to me.
> Can you add a JSTests/stress/xxx.js test which exercise this change in JS?
> 

Added, but it's somewhat tricky as it now assumes system timezone to be 'America/Los_Angeles', I didn't find a way to test it without relying on the host system configuration and would appreciate any pointers.


> > Source/JavaScriptCore/runtime/JSDateMath.cpp:376
> > +            auto status = callBufferProducingFunction(ucal_getTimeZoneDisplayName, timeZoneCache.m_calendar.get(), UCAL_STANDARD, language.utf8().data(), standardDisplayNameBuffer);
> 
> Let's generate language.utf8() once, and use for both. We do not need to
> allocate it twice.

Done.
Comment 8 Yury Semikhatsky 2022-04-28 16:55:01 PDT
Created attachment 458557 [details]
Patch
Comment 9 Yusuke Suzuki 2022-04-28 17:51:40 PDT
Comment on attachment 458557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458557&action=review

r=me with nit.

> JSTests/stress/date-format-timezone.js:12
> +if (Intl.DateTimeFormat().resolvedOptions().timeZone === 'America/Los_Angeles') {
> +  // 'Thu Apr 28 2022 14:42:34 GMT-0700 (Pacific Daylight Time)'
> +  const date = new Date(1651182154409);
> +  $vm.setUserPreferredLanguages(['de-DE']);
> +  shouldBe(date.toString(), 'Thu Apr 28 2022 14:42:34 GMT-0700 (Nordamerikanische Westküsten-Sommerzeit)');
> +  shouldBe(date.toTimeString(), '14:42:34 GMT-0700 (Nordamerikanische Westküsten-Sommerzeit)');
> +}

You can use JSTests/complex.yaml and JSTests/complex directory to control TZ configuration. There are some tests setting TZ to "America/Los_Angeles", so you can just copy the approach :)
Comment 10 Yury Semikhatsky 2022-04-28 20:13:30 PDT
Comment on attachment 458557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458557&action=review

>> JSTests/stress/date-format-timezone.js:12
>> +}
> 
> You can use JSTests/complex.yaml and JSTests/complex directory to control TZ configuration. There are some tests setting TZ to "America/Los_Angeles", so you can just copy the approach :)

Done. Thank you.
Comment 11 Yury Semikhatsky 2022-04-28 20:13:56 PDT
Created attachment 458564 [details]
Patch
Comment 12 Yusuke Suzuki 2022-04-28 20:19:17 PDT
Ah, let's update testapi. It is failing because of this update.


```
...
assertEqualsAsUTF8String failed at character 51:  (32) != s(115)
value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
assertEqualsAsUTF8String failed at character 52: T(84) != s(115)
value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
assertEqualsAsUTF8String failed at character 53: i(105) != a(97)
value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
assertEqualsAsUTF8String failed at character 54: m(109) != g(103)
value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
...
```
Comment 13 Yury Semikhatsky 2022-04-28 20:32:02 PDT
(In reply to Yusuke Suzuki from comment #12)
> Ah, let's update testapi. It is failing because of this update.
> 
> 
> ```
> ...
> assertEqualsAsUTF8String failed at character 51:  (32) != s(115)
> value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
> expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
> assertEqualsAsUTF8String failed at character 52: T(84) != s(115)
> value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
> expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
> assertEqualsAsUTF8String failed at character 53: i(105) != a(97)
> value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
> expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
> assertEqualsAsUTF8String failed at character 54: m(109) != g(103)
> value: Wed Dec 31 1969 16:00:00 GMT-0800 (Pacific Standard Time)
> expectedValue: Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
> ...
> ```

Done.
Comment 14 Yury Semikhatsky 2022-04-28 20:32:19 PDT
Created attachment 458566 [details]
Patch
Comment 15 Yusuke Suzuki 2022-04-29 03:20:19 PDT
Looks like imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/input-valueasdate.html 's test result needs to be updated too.
Comment 16 Yury Semikhatsky 2022-04-29 08:35:05 PDT
Created attachment 458589 [details]
Patch
Comment 17 Yury Semikhatsky 2022-04-29 08:35:17 PDT
(In reply to Yusuke Suzuki from comment #15)
> Looks like
> imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/input-
> valueasdate.html 's test result needs to be updated too.

Updated.
Comment 18 Darin Adler 2022-04-29 09:56:14 PDT
> We also have to check that there’s no localization benefit or benefit of adhering to
> the user’s preferences (e.g. 24-hour time vs. not, Language & Region advanced
> settings on Mac) to the platform-specific calls on the Apple platforms before
> landing this.

Who’s doing this check?
Comment 19 Yury Semikhatsky 2022-04-29 10:36:39 PDT
(In reply to Darin Adler from comment #18)
> > We also have to check that there’s no localization benefit or benefit of adhering to
> > the user’s preferences (e.g. 24-hour time vs. not, Language & Region advanced
> > settings on Mac) to the platform-specific calls on the Apple platforms before
> > landing this.
> 
> Who’s doing this check?

I mentioned above that I did a manual check, more details on that (tried on macOS 12):

* before the change only timezone setting affected the timezone formatting and it was always a short code: PST/PDT etc
* after the change timezone and system preferred language affect timezone formatting, it is now long and localized similar to other browsers: (Central European Summer Time) etc.

* all other settings (Region, Calendar and 24-hour format) do not affect the output of Date(), neither before nor after.

To illustrate that, here is the output of 
new Date(1651252882890).toString()

Before
"Fri Apr 29 2022 19:20:22 GMT+0200 (CEST)" - en-US / CEST
"Fri Apr 29 2022 10:21:22 GMT-0700 (PDT)" - en-US / PDT
"Fri Apr 29 2022 10:26:05 GMT-0700 (PDT)" - ru-RU / PDT

After
"Fri Apr 29 2022 19:21:22 GMT+0200 (Central European Summer Time)" - en-US / CEST
"Fri Apr 29 2022 10:21:22 GMT-0700 (Pacific Daylight Time)" - en-US / PDT
"Fri Apr 29 2022 10:21:22 GMT-0700 (Тихоокеанское летнее время)" - ru-RU / PDT

New tests under JSTests/complex cover this too.

Do you want me to check something else or it gives enough confidence?
Comment 20 Darin Adler 2022-04-29 10:38:00 PDT
(In reply to Yury Semikhatsky from comment #19)
> Do you want me to check something else or it gives enough confidence?

No, that’s good. I didn’t fully understand the remarks above, but you reiterating them makes them clearer.
Comment 21 Yusuke Suzuki 2022-04-29 10:40:12 PDT
Comment on attachment 458589 [details]
Patch

r=me
Comment 22 EWS 2022-04-29 11:56:35 PDT
Committed r293625 (250131@main): <https://commits.webkit.org/250131@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458589 [details].
Comment 23 Radar WebKit Bug Importer 2022-04-29 11:57:13 PDT
<rdar://problem/92536587>