Bug 239865 – Format time zone name using ICU instead of platform calls

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yury Semikhatsky]

Format time zone name using ICU instead of platform calls

[Yury Semikhatsky]

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.

[Darin Adler]

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.

[Yury Semikhatsky]

Created attachment 458543 [details]
Patch

[Yury Semikhatsky]

(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)"

[Yusuke Suzuki]

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.

[Yury Semikhatsky]

Created attachment 458556 [details]
Patch

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 458557 [details]
Patch

[Yusuke Suzuki]

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 :)

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 458564 [details]
Patch

[Yusuke Suzuki]

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)
...
```

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 458566 [details]
Patch

[Yusuke Suzuki]

Looks like imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/input-valueasdate.html 's test result needs to be updated too.

[Yury Semikhatsky]

Created attachment 458589 [details]
Patch

[Yury Semikhatsky]

(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.

[Darin Adler]

> 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?

[Yury Semikhatsky]

(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?

[Darin Adler]

(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.

[Yusuke Suzuki]

Comment on attachment 458589 [details]
Patch

r=me

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/92536587>