Bug 213884 – [WK2] Add API to allow embedder to set a timezone override

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Philippe Normand]

There are ways to configure the timezone with the TZ environment variable, but having a generic API for this in the WebsiteDataStoreConfiguration would be nice to have as well. It would also allow fine-grained control over multiple pages, for instance it's not possible currently to have two pages in different timezones. For automation and testing purposes this is a limitation currently.

[Philippe Normand]

Created attachment 403368 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Created attachment 403451 [details]
Patch

[Alex Christensen]

Comment on attachment 403451 [details]
Patch

This isn't really website data or associated with a network session.  I think this should go on WKPageConfigurationRef/WKWebViewConfiguration.

[Philippe Normand]

Created attachment 403787 [details]
Patch

[Philippe Normand]

Created attachment 403789 [details]
Patch

[Philippe Normand]

Comment on attachment 403789 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:486
> +        WTF::setTimeZoneOverride(*parameters.timeZoneOverride);

There's no error handling here. If this returns false it might make sense to send an error message back to the UIProcess...

[Philippe Normand]

Comment on attachment 403789 [details]
Patch

V3 coming ... soon.

[Philippe Normand]

Created attachment 404553 [details]
Patch

[Philippe Normand]

Ping?

[Philippe Normand]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 403451 [details]
> Patch
> 
> This isn't really website data or associated with a network session.  I
> think this should go on WKPageConfigurationRef/WKWebViewConfiguration.

This is the case now. A new review would be most welcome ;)

[Alex Christensen]

I'm curious what the motivation for the addition of this API is.  Who needs this ability?

[Philippe Normand]

It's often needed for testing purposes. See these posts for instance:

https://stackoverflow.com/questions/16448754/how-to-use-a-custom-time-in-browser-to-test-for-client-vs-server-time-difference
https://stackoverflow.com/questions/12220717/how-to-mock-the-browsers-timezone/31379288#comment60641443_31379288
https://stackoverflow.com/questions/11453740/fake-time-zone-for-web-app-testing

One way to simulate different timezone is to set the TZ env variable but it doesn't work the same way on all platforms, and it affects all webviews of the browser context. With this API you can simulate timezones on a per-webview basis.

[Alex Christensen]

Comment on attachment 404553 [details]
Patch

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

> Source/WTF/wtf/DateMath.cpp:339
> +    if (tz.cal) {

I believe this uses uninitialized memory.  Add a { nullptr } to its declaration.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:7
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.

Is there a reason this is not using the 2-clause BSD we usually use for new files?  I think I'd prefer that unless there's a good reason to do this.

[Carlos Garcia Campos]

Comment on attachment 404553 [details]
Patch

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

> Source/WTF/wtf/DateMath.cpp:1096
> +    // Timezone is ascii.
> +    Vector<UChar> buffer(timeZone.length());
> +    UChar* bufferStart = buffer.data();
> +    CString ctz = timeZone.utf8();
> +    if (!Unicode::convertUTF8ToUTF16(ctz.data(), ctz.data() + ctz.length(), &bufferStart, bufferStart + timeZone.length()))
> +        return false;
> +
> +    Vector<UChar, 32> canonicalBuffer(32);
> +    UErrorCode status = U_ZERO_ERROR;
> +    auto canonicalLength = ucal_getCanonicalTimeZoneID(buffer.data(), buffer.size(), canonicalBuffer.data(), canonicalBuffer.size(), nullptr, &status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +        status = U_ZERO_ERROR;
> +        canonicalBuffer.grow(canonicalLength);
> +        ucal_getCanonicalTimeZoneID(buffer.data(), buffer.size(), canonicalBuffer.data(), canonicalLength, nullptr, &status);
> +    }
> +    if (!U_SUCCESS(status))
> +        return false;

This is duplicated in isTimeZoneValid(). Could we make isTimeZoneValid() return an Optional<Vector<UChar, 32>>, for example?

> Source/WTF/wtf/DateMath.cpp:1119
> +String timeZoneOverride()

String& ? or do we really need to always copy?

> Source/WTF/wtf/DateMath.cpp:1124
> +String timeZoneDisplayNameOverride()

Ditto.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:165
> +    encoder << timeZoneOverride;

The time zone is applied globally to the web process, not specific to a page, so I think it would be better in WebProcessCreationParameters

> Source/WebKit/UIProcess/API/APIPageConfiguration.h:167
> +    const WTF::String timeZoneOverride() const { return m_timeZoneOverride; }

String&

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> +- (void)_setTimeZoneOverride:(NSString *)timeZone

I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:354
> +        g_value_set_string(value, context->priv->timeZoneOverride.data());

I would add a public getter for the property.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:387
> +        context->priv->timeZoneOverride = g_value_get_string(value);

I think we should validate the given timezone here, before setting it.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:580
> +     * The timezone override for this web context.

We should explain here what the timezone override is for and the expected format.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1939
> +    pageConfiguration->setTimeZoneOverride(context->priv->timeZoneOverride.data());

String::fromUTF8()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)

I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:475
> +    // By default the test harness uses the Pacific/Los_Angeles timezone which is +7 hours (420
> +    // minutes) compared to GMT.

This could be one line.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:476
> +    g_assert_cmpint(WebViewTest::javascriptResultToNumber(javascriptResult), ==, 420);

Doesn't this depend on the current locale? or do we change it always for tests?

> Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.cpp:32
> +const char* Test::timeZoneOverride = "";

Better leave it uninitialized an nullptr will be passed when not set.

> Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.h:144
> +            "time-zone-override", timeZoneOverride,

We don't need this is you simply create a custom web context in the test.

> Tools/flatpak/flatpakutils.py:720
> -                "TZ": "PST8PDT",
> +                "TZ": "America/Los_Angeles",

Ah! here it's. So, the test will always fail when run manually or using jhbuild. We shouldn't make the tests depend on this. Can't we change the environment variable in TestMain.cpp?

[Philippe Normand]

Comment on attachment 404553 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
>> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> 
> I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.

Alex, what do you think about this?

[Philippe Normand]

Comment on attachment 404553 [details]
Patch

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

>> Source/WTF/wtf/DateMath.cpp:1096
>> +        return false;
> 
> This is duplicated in isTimeZoneValid(). Could we make isTimeZoneValid() return an Optional<Vector<UChar, 32>>, for example?

The canonicalLength would need to be returned as well. We could return an Optional<Vector<...>, int32_t>... I suppose. I'm not sure it would pay off though.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
>> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)
> 
> I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.

But then I can't reuse the API of WebViewTest to run JS code and wait for the result.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:476
>> +    g_assert_cmpint(WebViewTest::javascriptResultToNumber(javascriptResult), ==, 420);
> 
> Doesn't this depend on the current locale? or do we change it always for tests?

I can sprinkle a setenv call before this code and hope for the best!

[Philippe Normand]

Comment on attachment 404553 [details]
Patch

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

>> Source/WebKit/Shared/WebPageCreationParameters.cpp:165
>> +    encoder << timeZoneOverride;
> 
> The time zone is applied globally to the web process, not specific to a page, so I think it would be better in WebProcessCreationParameters

This contradicts comment 4 then? Before moving this flag from APIPageConfiguration to APIProcessPoolConfiguration I would like to reach an agreement between the reviewers involved here.

[Carlos Garcia Campos]

Comment on attachment 404553 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
>>> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)
>> 
>> I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.
> 
> But then I can't reuse the API of WebViewTest to run JS code and wait for the result.

Yes, it's just a function call, and you can use a lambda for the callback. I think it's weird to have two test cases for the same thing.

[Alex Christensen]

(In reply to Philippe Normand from comment #16)
> Comment on attachment 404553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404553&action=review
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> >> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> > 
> > I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.
> 
> Alex, what do you think about this?

Not quite, but close.  This property should definitely be on the WKWebViewConfiguration instead of WebsiteDataStoreConfiguration.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #20)
> (In reply to Philippe Normand from comment #16)
> > Comment on attachment 404553 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404553&action=review
> > 
> > >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> > >> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> > > 
> > > I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.
> > 
> > Alex, what do you think about this?
> 
> Not quite, but close.  This property should definitely be on the
> WKWebViewConfiguration instead of WebsiteDataStoreConfiguration.

Not website data sore, but process pool, since it's global to the web process, it's not specific to the web view.

[Philippe Normand]

Hi Alex, back to this patch after a week off. So, would you agree to have this API in _WKProcessPoolConfiguration as Carlos is suggesting?

[Alex Christensen]

The implementation currently uses a process-global override in the web process.  There is no reason it must be process-global, right?  It seems like an implementation shortcut.

[Philippe Normand]

(In reply to Alex Christensen from comment #23)
> The implementation currently uses a process-global override in the web
> process.  There is no reason it must be process-global, right?  It seems
> like an implementation shortcut.

It's a process-global implementation because the DateTime functions like formatDateTime() are not context-aware, AFAICT. The timezone information is retrieved from ICU or from the override static struct.

[Alex Christensen]

formatDateTime might not be context aware now, but could it be made context aware?

[Philippe Normand]

(In reply to Alex Christensen from comment #25)
> formatDateTime might not be context aware now, but could it be made context
> aware?

Maybe yes, the TimeZoneOverride struct could move to the JSC::VM, for instance. But then another issue is with calculateLocalTimeOffset() which is called from various places in WebCore, that does look like a non-trivial amount of work.

[Alex Christensen]

I agree it's not a trivial amount of work, but I think it's better than cementing us into an API shape that is based on how convenient it is to implement a certain way.

[Carlos Garcia Campos]

I think it makes sense to override the timezone globally, since we are emulating the system has a different timezone. This is just for testing and should be documented as such, so I don't think it's worth the effort to make the timezone per web view just for this.

[Philippe Normand]

Created attachment 409021 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Created attachment 409036 [details]
Patch

[Darin Adler]

Comment on attachment 409036 [details]
Patch

Test is failing on macOS. Need a revised version that resolves that.

[Philippe Normand]

Created attachment 411545 [details]
Patch

[Philippe Normand]

Comment on attachment 411545 [details]
Patch

Failed
    TestWebKitAPI.TimeZoneOverrideTest.TimeZoneOverride
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:66
        Expected equality of these values:
          [result stringValue]
            Which is: "420"
          "-120"

[Yury Semikhatsky]

Created attachment 457711 [details]
Patch

[Yury Semikhatsky]

(In reply to Darin Adler from comment #32)
> Comment on attachment 409036 [details]
> Patch
> 
> Test is failing on macOS. Need a revised version that resolves that.

Resurrecting this patch. I've updated the implementation to make it work on all platforms, the macOS test is no passing.

Darin, Alex, can you do a review?

[Darin Adler]

Comment on attachment 457711 [details]
Patch

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

Looks fine assuming all tests pass, but I think they’re not all passing yet

> Source/JavaScriptCore/ChangeLog:3
> +        [WK2] Add API to allow embedder to set a timezone override

I’m not sure I love the use of "override" here as a noun meaning "an explicitly set value that overrides the normal automatic time zone determination". I wish there was a less peculiar term of art. But for now, I just went with it.

> Source/JavaScriptCore/runtime/DateConversion.cpp:101
> +            String timeZoneOverride = WTF::timeZoneDisplayNameOverride();

This is not how we currently handle namespace in WTF. There should be a "using" in the header and no WTF prefix here.

> Source/JavaScriptCore/runtime/DateConversion.cpp:102
> +            if (!timeZoneOverride.isEmpty())

Nice style to scope the value by putting it inside the if, also allows using a one word name:

    if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
        timeZoneName = override;

> Source/JavaScriptCore/runtime/DateConversion.cpp:109
> +                const WCHAR* winTimeZoneName = t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName;
> +                timeZoneName = String(winTimeZoneName);

Can we merge these into one line?

    timeZoneName = String { t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName };

> Source/JavaScriptCore/runtime/DateConversion.cpp:114
> +                struct tm gtm = t;
> +                char tzName[70];
> +                strftime(tzName, sizeof(tzName), "%Z", &gtm);
> +                timeZoneName = String::fromUTF8(tzName);

This code is, and always has been, incorrect for cases where the time zone doesn’t fit into the buffer. In that case, strftime returns 0, and the contents of the buffer are *indeterminate*. We should fix this small mistake in how we are calling strftime by not looking at tzName if strftime returns 0.

We also could give the local variables better names, but that’s not required.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
> +    String tz = WTF::timeZoneOverride();
> +    if (!tz.isEmpty())
> +        return tz;

In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:

    if (auto& override = timeZoneOverride; !override.isEmpty())
        return override;

> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));

Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.

I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.

> Source/WTF/wtf/DateMath.cpp:343
> +        int32_t offset = ucal_get(tz.cal, UCAL_ZONE_OFFSET, &status);
> +        int32_t dstOffset = ucal_get(tz.cal, UCAL_DST_OFFSET, &status);

May need some error checking here. We are just ignoring status. That’s not necessarily safe.

> Source/WTF/wtf/DateMath.cpp:1046
> +    // Timezone is ascii.

Not clear on the purpose of this comment.

> Source/WTF/wtf/DateMath.cpp:1051
> +    UChar* bufferStart = buffer.data();
> +    CString ctz = timeZone.utf8();
> +    if (!Unicode::convertUTF8ToUTF16(ctz.data(), ctz.data() + ctz.length(), &bufferStart, bufferStart + timeZone.length()))
> +        return std::nullopt;

This is a very strange way to convert a WTF::String to UTF-16. There is no reason to first convert the UTF-8 and then back to UTF-16. A better way is:

  StringView { timeZone }.getCharactersWithUpconvert(buffer.data());

We can possibly optimize even further. Changing the argument to this function to already be StringView will optimize still further.

> Source/WTF/wtf/DateMath.h:396
> +WTF_EXPORT_PRIVATE std::optional<Vector<UChar, 32>> isTimeZoneValid(const String&);

I suggest changing the argument type here to StringView, since this function takes no advantage of the fact that it’s already in a String.

This is not the correct name for a function that parses a time zone and returns the canonical form. I would call this parseTimeZone or computeCanonicalTimeZone, or something like that.

If the external caller only wants to check *if* the time zone is valid, I suggest publishing only the version that returns a bool rather than returning the parsed time zone to that caller. That can be a cover that just converts the optional to a boolean on the way out.

> Source/WTF/wtf/DateMath.h:397
> +WTF_EXPORT_PRIVATE bool setTimeZoneOverride(const String&);

Callers are making WTF::String just to pass to this function, but that is unnecessary. Given how it’s used, I suggest we instead have this function take a StringView, or even a const char*. It would be easy for the callers to deal with either of those. If we must use UTF-16, then I suggest StringView rather than const String&, but I think const char* would work well.

> Source/WTF/wtf/DateMath.h:399
> +WTF_EXPORT_PRIVATE String& timeZoneOverride();
> +WTF_EXPORT_PRIVATE String& timeZoneDisplayNameOverride();

These should return const String&, unlesss we intend callers to use the returned references to modify the values.

> Source/WebKit/Shared/WebProcessCreationParameters.h:259
> +    std::optional<String> timeZoneOverride;

I suggest we use String here, since String has a null value. Using std::optional<String> is unnecessary. When the code is not generic across multiple types, I strongly suggest not ever using optional<String>.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400
> +    case PROP_TIME_ZONE_OVERRIDE: {
> +        const char* timeZoneOverride = g_value_get_string(value);
> +        if (timeZoneOverride)
> +            webkit_web_context_set_time_zone_override(context, timeZoneOverride);
> +        break;
> +    }

No braces needed:

    if (auto* override = g_value_get_string(value))
        webkit_web_context_set_time_zone_override(context, override);
    break;

> Source/WebKit/WebProcess/WebProcess.cpp:505
> +    if (parameters.timeZoneOverride)
> +        WTF::setTimeZoneOverride(*parameters.timeZoneOverride);
> +    else
> +        WTF::setTimeZoneOverride({ });

Should not have WTF:: prefixes here. See my comments above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:28
> +#if PLATFORM(MAC)

This should be COCOA instead of MAC. Unless there’s some reason this doesn’t work on iOS?

[Darin Adler]

Comment on attachment 457711 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
>> +        return tz;
> 
> In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> 
>     if (auto& override = timeZoneOverride; !override.isEmpty())
>         return override;

Or if you prefer the word "zone" to "override", or even "timeZone".

[Yury Semikhatsky]

Comment on attachment 457711 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        [WK2] Add API to allow embedder to set a timezone override
> 
> I’m not sure I love the use of "override" here as a noun meaning "an explicitly set value that overrides the normal automatic time zone determination". I wish there was a less peculiar term of art. But for now, I just went with it.

Some of the methods in the code use *ForAutomation suffix, we could employ it here as well to make its intent clear (e.g. timeZoneForAutomation/setTimeZoneForAutomation), it makes less sense in the public WebKit API though.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>> +            String timeZoneOverride = WTF::timeZoneDisplayNameOverride();
> 
> This is not how we currently handle namespace in WTF. There should be a "using" in the header and no WTF prefix here.

Fixed.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:102
>> +            if (!timeZoneOverride.isEmpty())
> 
> Nice style to scope the value by putting it inside the if, also allows using a one word name:
> 
>     if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
>         timeZoneName = override;

Done.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:109
>> +                timeZoneName = String(winTimeZoneName);
> 
> Can we merge these into one line?
> 
>     timeZoneName = String { t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName };

Done.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:114
>> +                timeZoneName = String::fromUTF8(tzName);
> 
> This code is, and always has been, incorrect for cases where the time zone doesn’t fit into the buffer. In that case, strftime returns 0, and the contents of the buffer are *indeterminate*. We should fix this small mistake in how we are calling strftime by not looking at tzName if strftime returns 0.
> 
> We also could give the local variables better names, but that’s not required.

Fixed.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
>> +        return tz;
> 
> In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> 
>     if (auto& override = timeZoneOverride; !override.isEmpty())
>         return override;

Done.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
> 
> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
> 
> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.

Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).

As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.

>> Source/WTF/wtf/DateMath.cpp:343
>> +        int32_t dstOffset = ucal_get(tz.cal, UCAL_DST_OFFSET, &status);
> 
> May need some error checking here. We are just ignoring status. That’s not necessarily safe.

Added checks that return 0 offset in case of error.

>> Source/WTF/wtf/DateMath.cpp:1046
>> +    // Timezone is ascii.
> 
> Not clear on the purpose of this comment.

Removed.

>> Source/WTF/wtf/DateMath.cpp:1051
>> +        return std::nullopt;
> 
> This is a very strange way to convert a WTF::String to UTF-16. There is no reason to first convert the UTF-8 and then back to UTF-16. A better way is:
> 
>   StringView { timeZone }.getCharactersWithUpconvert(buffer.data());
> 
> We can possibly optimize even further. Changing the argument to this function to already be StringView will optimize still further.

Done. Updated parameter type to StringView.

>> Source/WTF/wtf/DateMath.h:396
>> +WTF_EXPORT_PRIVATE std::optional<Vector<UChar, 32>> isTimeZoneValid(const String&);
> 
> I suggest changing the argument type here to StringView, since this function takes no advantage of the fact that it’s already in a String.
> 
> This is not the correct name for a function that parses a time zone and returns the canonical form. I would call this parseTimeZone or computeCanonicalTimeZone, or something like that.
> 
> If the external caller only wants to check *if* the time zone is valid, I suggest publishing only the version that returns a bool rather than returning the parsed time zone to that caller. That can be a cover that just converts the optional to a boolean on the way out.

Changed argument to StringView, renamed the method to validateTimeZone and made it private to DateMath.cpp. Only isTimeZoneValid exposed in the header now.

>> Source/WTF/wtf/DateMath.h:397
>> +WTF_EXPORT_PRIVATE bool setTimeZoneOverride(const String&);
> 
> Callers are making WTF::String just to pass to this function, but that is unnecessary. Given how it’s used, I suggest we instead have this function take a StringView, or even a const char*. It would be easy for the callers to deal with either of those. If we must use UTF-16, then I suggest StringView rather than const String&, but I think const char* would work well.

Changed the argument type to StringView for consistency, but this method is only invoked during WebProcess initialization with the value that comes from the creating parameters, i.e. always a String.

>> Source/WTF/wtf/DateMath.h:399
>> +WTF_EXPORT_PRIVATE String& timeZoneDisplayNameOverride();
> 
> These should return const String&, unlesss we intend callers to use the returned references to modify the values.

Done.

>> Source/WebKit/Shared/WebProcessCreationParameters.h:259
>> +    std::optional<String> timeZoneOverride;
> 
> I suggest we use String here, since String has a null value. Using std::optional<String> is unnecessary. When the code is not generic across multiple types, I strongly suggest not ever using optional<String>.

Good point. Changed to String.

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400
>> +    }
> 
> No braces needed:
> 
>     if (auto* override = g_value_get_string(value))
>         webkit_web_context_set_time_zone_override(context, override);
>     break;

Done.

>> Source/WebKit/WebProcess/WebProcess.cpp:505
>> +        WTF::setTimeZoneOverride({ });
> 
> Should not have WTF:: prefixes here. See my comments above.

Done.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:28
>> +#if PLATFORM(MAC)
> 
> This should be COCOA instead of MAC. Unless there’s some reason this doesn’t work on iOS?

Changed to COCOA.

[Yury Semikhatsky]

Created attachment 457739 [details]
Patch

[Yury Semikhatsky]

(In reply to Darin Adler from comment #38)
> Comment on attachment 457711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457711&action=review
> 
> >> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
> >> +        return tz;
> > 
> > In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> > 
> >     if (auto& override = timeZoneOverride; !override.isEmpty())
> >         return override;
> 
> Or if you prefer the word "zone" to "override", or even "timeZone".

"override" compiled well, leaving it.

[Darin Adler]

Comment on attachment 457711 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
>> 
>> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
>> 
>> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.
> 
> Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).
> 
> As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.

The change I was suggesting, sorry I was not more explicit, for bitwise_cast is to *encapsulate* the technique in a single pair of inline functions to convert between icu::TimeZone* and OpaqueICUTimeZone*; all the call sites would use this in a type-safe way and wouldn’t literally have individual calls to bitwise_cast.

[Yusuke Suzuki]

Comment on attachment 457739 [details]
Patch

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

Commented.

> Source/JavaScriptCore/runtime/DateConversion.cpp:104
> +            String timeZoneName;
> +            String timeZoneOverride = timeZoneDisplayNameOverride();
> +            if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
> +                timeZoneName = override;
> +            else {

Can we avoid String ref-churn here? (And we need to do that since String is not thread safe).

> Source/WTF/wtf/DateMath.cpp:101
> +    UCalendar* cal { nullptr };

Use std::unique_ptr<UCalendar, ICUDeleter<ucal_close>> instead.

> Source/WTF/wtf/DateMath.cpp:103
> +    String id;
> +    String displayName;

This does not work since String is not thread safe.
If it is used by multiple threads, we need another way to keep String.
Maybe Vector<UChar, ...> instead?

> Source/WTF/wtf/DateMath.cpp:108
> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;

static in WebKit C++ is not thread safe. Let's use

LazyNeverDestroyed<TimeZoneOverride>

and std::call_once to initialize it once.

> Source/WTF/wtf/DateMath.cpp:1080
> +    if (innerTimeZoneOverride().cal) {
> +        ucal_close(innerTimeZoneOverride().cal);
> +        innerTimeZoneOverride().cal = nullptr;
> +    }

Let's not do that, and instead, use std::unique_ptr.

> Source/WTF/wtf/DateMath.cpp:1093
> +    UCalendar* cal = ucal_open(canonicalBuffer->data(), canonicalLength, nullptr, UCAL_TRADITIONAL, &status);

For ICU data structures, we use

std::unique_ptr<UCalendar, ICUDeleter<ucal_close>>

etc. to avoid memory leak. See Intl code in JSC.

> Source/WTF/wtf/DateMath.cpp:1105
> +    Vector<UChar, 32> displayNameBuffer(32);
> +    auto displayNameLength = ucal_getTimeZoneDisplayName(cal, UCAL_STANDARD, defaultLanguage().utf8().data(), displayNameBuffer.data(), displayNameBuffer.size(), &status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +        status = U_ZERO_ERROR;
> +        displayNameBuffer.grow(displayNameLength);
> +        ucal_getTimeZoneDisplayName(cal, UCAL_STANDARD, defaultLanguage().utf8().data(), displayNameBuffer.data(), displayNameLength, &status);
> +    }
> +    if (!U_SUCCESS(status))
> +        return false;

We should use callBufferProducingFunction.

[Yusuke Suzuki]

Comment on attachment 457739 [details]
Patch

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

> Source/WTF/wtf/DateMath.cpp:1075
> +bool setTimeZoneOverride(const StringView& timeZone)
> +{

This function is modifying global state. Do we need to have Lock to ensure this function can be a thread safe?
If so, then, it is also true that accessors (timeZoneOverride etc.) also need to take a lock.

[Yury Semikhatsky]

Comment on attachment 457739 [details]
Patch

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

>> Source/WTF/wtf/DateMath.cpp:108
>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
> 
> static in WebKit C++ is not thread safe. Let's use
> 
> LazyNeverDestroyed<TimeZoneOverride>
> 
> and std::call_once to initialize it once.

I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."

>> Source/WTF/wtf/DateMath.cpp:1075
>> +{
> 
> This function is modifying global state. Do we need to have Lock to ensure this function can be a thread safe?
> If so, then, it is also true that accessors (timeZoneOverride etc.) also need to take a lock.

This function itself is only called once when WebProcess start, so perhaps we can rename it to initializeTimeZoneOverride and make sure it's called only once. If I understand correctly, web workers will have JSDateMath instances that are used on different threads and they will access timeZoneOverride() concurrently. Even though the access to timeZoneOverride() is always on a slow path it's a bit annoying that we need to introduce locks to access it. I thought maybe there is a trick/technique that could be used here to avoid locking and e.g. just get the override in the DateCache/VM constructor at some point where another (global?) lock is already held? If not I will just introduce locks.

[Darin Adler]

Comment on attachment 457739 [details]
Patch

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

>>> Source/WTF/wtf/DateMath.cpp:108
>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>> 
>> static in WebKit C++ is not thread safe. Let's use
>> 
>> LazyNeverDestroyed<TimeZoneOverride>
>> 
>> and std::call_once to initialize it once.
> 
> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."

It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.

The option is -fno-threadsafe-statics

[Yury Semikhatsky]

Comment on attachment 457711 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>>>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
>>> 
>>> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
>>> 
>>> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.
>> 
>> Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).
>> 
>> As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.
> 
> The change I was suggesting, sorry I was not more explicit, for bitwise_cast is to *encapsulate* the technique in a single pair of inline functions to convert between icu::TimeZone* and OpaqueICUTimeZone*; all the call sites would use this in a type-safe way and wouldn’t literally have individual calls to bitwise_cast.

Moved bitwise_cast into 2 functions as you suggested.

[Yury Semikhatsky]

Created attachment 457847 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 457739 [details]
Patch

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

>> Source/JavaScriptCore/runtime/DateConversion.cpp:104
>> +            else {
> 
> Can we avoid String ref-churn here? (And we need to do that since String is not thread safe).

Changed timeZoneDisplayNameOverride to return a new instance of the string.

>> Source/WTF/wtf/DateMath.cpp:101
>> +    UCalendar* cal { nullptr };
> 
> Use std::unique_ptr<UCalendar, ICUDeleter<ucal_close>> instead.

Done.

>> Source/WTF/wtf/DateMath.cpp:103
>> +    String displayName;
> 
> This does not work since String is not thread safe.
> If it is used by multiple threads, we need another way to keep String.
> Maybe Vector<UChar, ...> instead?

Done.

>>>> Source/WTF/wtf/DateMath.cpp:108
>>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>>> 
>>> static in WebKit C++ is not thread safe. Let's use
>>> 
>>> LazyNeverDestroyed<TimeZoneOverride>
>>> 
>>> and std::call_once to initialize it once.
>> 
>> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."
> 
> It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.
> 
> The option is -fno-threadsafe-statics

Got it, updated the code to use a lock while accessing innerTimeZoneOverride.

>> Source/WTF/wtf/DateMath.cpp:1080
>> +    }
> 
> Let's not do that, and instead, use std::unique_ptr.

Done.

>> Source/WTF/wtf/DateMath.cpp:1093
>> +    UCalendar* cal = ucal_open(canonicalBuffer->data(), canonicalLength, nullptr, UCAL_TRADITIONAL, &status);
> 
> For ICU data structures, we use
> 
> std::unique_ptr<UCalendar, ICUDeleter<ucal_close>>
> 
> etc. to avoid memory leak. See Intl code in JSC.

Done.

>> Source/WTF/wtf/DateMath.cpp:1105
>> +        return false;
> 
> We should use callBufferProducingFunction.

Done. Thanks for the pointers!

[Yury Semikhatsky]

Comment on attachment 457739 [details]
Patch

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

>>>>> Source/WTF/wtf/DateMath.cpp:108
>>>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>>>> 
>>>> static in WebKit C++ is not thread safe. Let's use
>>>> 
>>>> LazyNeverDestroyed<TimeZoneOverride>
>>>> 
>>>> and std::call_once to initialize it once.
>>> 
>>> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."
>> 
>> It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.
>> 
>> The option is -fno-threadsafe-statics
> 
> Got it, updated the code to use a lock while accessing innerTimeZoneOverride.

I also added a test that runs the code in workers, it should provide some coverage for the multi-threaded case.

[Yury Semikhatsky]

Darin, Yusuke, I've addressed your comments, can you have another look at the patch?

[Darin Adler]

Comment on attachment 457847 [details]
Patch

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

I want to be sure that we have been careful enough about performance implications here. How are we measuring the performance impact? Typically when touching code like this I want to improve performance and to me it seems here there is a risk of slowing things down

Typically getting thread safety right while preserving high performance is a challenge; the thread safety is difficult to test and may have to be assessed by code inspection, but the performance typically can be tested

I’m mostly concerned about performance when there is no override, not performance when there is an override, but that is a secondary concern

> Source/JavaScriptCore/runtime/DateConversion.cpp:101
> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())

So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?

> Source/JavaScriptCore/runtime/DateConversion.cpp:102
> +                timeZoneName = override;

Quite unfortunate to convert this to a temporary WTF::String every time; that’s not needed to append a time zone name to a StringBuilder. I’m really unclear on how performance critical this code is. The way it’s currently working is overusing String. There’s no need to create and destroy a String to append some character to a StringBuilder. This applies to the strftime code below just as much to this override code.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:345
> +    if (auto override = timeZoneOverride(); !override.isEmpty())
> +        return String::adopt(WTFMove(override));

Seems we would want this to be fast when called multiple times, and this object is called a "cache". Can we find a way to not lock/unlock every time this is called just to check if an override is in effect?

> Source/JavaScriptCore/runtime/JSDateMath.cpp:399
> +    Vector<UChar> override = timeZoneOverride();

Seems unfortunate we make no attempt to optimize this to not use the heap. These are short strings and always doing malloc/free is costly.

> Source/WTF/wtf/DateMath.cpp:1055
> +static std::optional<Vector<UChar>> validateTimeZone(const StringView& timeZone)

Type should be StringView, not const StringView&, in all these cases. In production builds a StringView fits into registers, and passing const StringView& instead typically just hurts performance and has no benefit.

> Source/WTF/wtf/DateMath.cpp:1057
> +    Vector<UChar> buffer(timeZone.length());

In a case like this we almost always use inline capacity so there’s no unnecessary heap allocation. This allocates/deallocates every time, which is unnecessary.

> Source/WTF/wtf/DateMath.cpp:1061
> +    canonicalBuffer.reserveCapacity(buffer.size());

Not sure this is a good optimization. It could cause a problem if the canonical ID is often slightly larger than the non-canonical.

> Source/WTF/wtf/DateMath.cpp:1065
> +    return canonicalBuffer;

I believe we may need WTFMove here to avoid allocating a new heap buffer, since the return type is not Vector<UChar> but std::optional<Vector<UChar>>.

> Source/WTF/wtf/DateMath.cpp:1068
> +bool isTimeZoneValid(const StringView& timeZone)

StringView

> Source/WTF/wtf/DateMath.cpp:1073
> +bool setTimeZoneOverride(const StringView& timeZone)

StringView

> Source/WTF/wtf/DateMath.cpp:1116
> +    auto& displayName = innerTimeZoneOverride().displayName;
> +    return String(displayName);

This should be a single line.

> Source/WTF/wtf/DateMath.h:432
> +using WTF::isTimeZoneValid;
> +using WTF::setTimeZoneOverride;
> +using WTF::timeZoneOverride;
> +using WTF::timeZoneDisplayNameOverride;
>  using WTF::calculateLocalTimeOffset;
>  using WTF::timeClip;
>  using WTF::jsCurrentTime;

We sort these alphabetically. The new ones should be sorted in with the existing ones.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:2
> + * Copyright (C) 2020 Igalia S.L.

2022?

[Yury Semikhatsky]

Comment on attachment 457847 [details]
Patch

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

>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())
> 
> So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?

Looking at this code I see that GregorianDateTime already contains timezone information (in utcOffsetInMinute()) and we should not need to pass the name separately as it should have been picked at the construction time. However, `strftime` below seems to ignore timezone information in tm.tm_gmtoff and always returns 'system' value of the timezone. The code that overrides time zone can (and I think should) override that value as well. But maybe the general code can be switched to use ucal_getTimeZoneDisplayName or something similar from ucal instead of strftime? Do you know what's the reason strftime is called here to format the timezone name while rest of the code uses ucal to manipulate date/time?

[Yury Semikhatsky]

Comment on attachment 457847 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>>> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())
>> 
>> So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?
> 
> Looking at this code I see that GregorianDateTime already contains timezone information (in utcOffsetInMinute()) and we should not need to pass the name separately as it should have been picked at the construction time. However, `strftime` below seems to ignore timezone information in tm.tm_gmtoff and always returns 'system' value of the timezone. The code that overrides time zone can (and I think should) override that value as well. But maybe the general code can be switched to use ucal_getTimeZoneDisplayName or something similar from ucal instead of strftime? Do you know what's the reason strftime is called here to format the timezone name while rest of the code uses ucal to manipulate date/time?

Removed this call altogether in favor of setting "TZ" environment variable, it will change result of strftime accordingly. On Windows though explicit override for the timezone name is used but it now uses override value stored in DateCache once.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:102
>> +                timeZoneName = override;
> 
> Quite unfortunate to convert this to a temporary WTF::String every time; that’s not needed to append a time zone name to a StringBuilder. I’m really unclear on how performance critical this code is. The way it’s currently working is overusing String. There’s no need to create and destroy a String to append some character to a StringBuilder. This applies to the strftime code below just as much to this override code.

Switched the code to StringView to avoid extra copies.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:345
>> +        return String::adopt(WTFMove(override));
> 
> Seems we would want this to be fast when called multiple times, and this object is called a "cache". Can we find a way to not lock/unlock every time this is called just to check if an override is in effect?

Removed this call altogether. The override is now only accessed in DateCache::timeZoneCacheSlow which should be called only once per DateCache instance unless system timezone changes.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:399
>> +    Vector<UChar> override = timeZoneOverride();
> 
> Seems unfortunate we make no attempt to optimize this to not use the heap. These are short strings and always doing malloc/free is costly.

Changed to use Vector<UChar, 32> and pass it as param to timeZoneOverride().

>> Source/WTF/wtf/DateMath.cpp:1055
>> +static std::optional<Vector<UChar>> validateTimeZone(const StringView& timeZone)
> 
> Type should be StringView, not const StringView&, in all these cases. In production builds a StringView fits into registers, and passing const StringView& instead typically just hurts performance and has no benefit.

Done.

>> Source/WTF/wtf/DateMath.cpp:1057
>> +    Vector<UChar> buffer(timeZone.length());
> 
> In a case like this we almost always use inline capacity so there’s no unnecessary heap allocation. This allocates/deallocates every time, which is unnecessary.

changed type to Vector<UChar, 32>

>> Source/WTF/wtf/DateMath.cpp:1061
>> +    canonicalBuffer.reserveCapacity(buffer.size());
> 
> Not sure this is a good optimization. It could cause a problem if the canonical ID is often slightly larger than the non-canonical.

Removed reserveCapacity call, instead changed type to Vector<UChar, 32>.

>> Source/WTF/wtf/DateMath.cpp:1065
>> +    return canonicalBuffer;
> 
> I believe we may need WTFMove here to avoid allocating a new heap buffer, since the return type is not Vector<UChar> but std::optional<Vector<UChar>>.

Good catch! Done.

>> Source/WTF/wtf/DateMath.cpp:1068
>> +bool isTimeZoneValid(const StringView& timeZone)
> 
> StringView

Done.

>> Source/WTF/wtf/DateMath.cpp:1073
>> +bool setTimeZoneOverride(const StringView& timeZone)
> 
> StringView

Done.

>> Source/WTF/wtf/DateMath.cpp:1116
>> +    return String(displayName);
> 
> This should be a single line.

Merged this method with timeZoneOverride.

>> Source/WTF/wtf/DateMath.h:432
>>  using WTF::jsCurrentTime;
> 
> We sort these alphabetically. The new ones should be sorted in with the existing ones.

Done. Restored alphabetic order.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:2
>> + * Copyright (C) 2020 Igalia S.L.
> 
> 2022?

Done.

[Yury Semikhatsky]

Created attachment 458474 [details]
Patch

[Yury Semikhatsky]

Darin, PTAL.

I've update the code to make sure the reading of the timezone override that requires a lock is only performed from DateCache::timeZoneCacheSlow(). In other places cached information is used and doesn't require locking.

On a side note, I don't know the history behind formatDateTime and why it uses platform calls (GetTimeZoneInformation on Windows and strftime on other systems) to format the time zone name but having it use ICU similar to the rest of the code base would allow to simplify the overrides logic. Switching that to ICU would affect the timezone formatting in the Web API though (make it similar to what Firefox and Chromium produce but still), so probably not worth it.

[Darin Adler]

(In reply to Yury Semikhatsky from comment #56)
> On a side note, I don't know the history behind formatDateTime and why it
> uses platform calls (GetTimeZoneInformation on Windows and strftime on other
> systems) to format the time zone name but having it use ICU similar to the
> rest of the code base would allow to simplify the overrides logic.

Not sure the history matters much at this point. I suspect it had to do with getting good formatting that adapts to the user’s locale and preferences, and long predates significant support for this in ICU.

> Switching
> that to ICU would affect the timezone formatting in the Web API though

This is the thing that matters now. If the result is better with ICU, we can make the change, and the consistency with other browsers would be great! But if the result debatably not as good at respecting user’s preferences and locale with ICU, then we would have to think more before making a change.

[Yury Semikhatsky]

Created attachment 458478 [details]
Patch

[Darin Adler]

Comment on attachment 458478 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)

I think TimezZone is a typo here?

[Yury Semikhatsky]

Comment on attachment 458478 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
>> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)
> 
> I think TimezZone is a typo here?

Fixed.

[Yury Semikhatsky]

Created attachment 458485 [details]
Patch

[Yury Semikhatsky]

(In reply to Yury Semikhatsky from comment #60)
> Comment on attachment 458478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458478&action=review
> 
> >> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
> >> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)
> > 
> > I think TimezZone is a typo here?
> 
> Fixed.

Is there anything else you want me to improve in this patch?


(In reply to Darin Adler from comment #57)
> (In reply to Yury Semikhatsky from comment #56)
> > Switching
> > that to ICU would affect the timezone formatting in the Web API though
> 
> This is the thing that matters now. If the result is better with ICU, we can
> make the change, and the consistency with other browsers would be great! But
> if the result debatably not as good at respecting user’s preferences and
> locale with ICU, then we would have to think more before making a change.

I can give it a try in a separate patch but I can't assess how many changes it entails and if the result will be acceptable, so I'd rather not commit to it. Given that this patch preserves existing behavior can we proceed with it?

[Darin Adler]

(In reply to Yury Semikhatsky from comment #62)
> Is there anything else you want me to improve in this patch?

Sorry, haven’t yet had time to carefully read it through again. I plan to when I have an opportunity.

[Yury Semikhatsky]

Created attachment 458624 [details]
Patch

[Yury Semikhatsky]

(In reply to Yury Semikhatsky from comment #64)
> Created attachment 458624 [details]
> Patch

Rebased on top of r293625

[Yury Semikhatsky]

Created attachment 458694 [details]
Patch

[Yury Semikhatsky]

(In reply to Yury Semikhatsky from comment #66)
> Created attachment 458694 [details]
> Patch

Updated tests to not depend on current time as it would produce different result during winter time.

[Yusuke Suzuki]

Comment on attachment 458694 [details]
Patch

r=me

[EWS]

Committed r293729 (250217@main): <https://commits.webkit.org/250217@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458694 [details].

[Radar WebKit Bug Importer]

<rdar://problem/92676360>

[Michael Catanzaro]

Comment on attachment 458694 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1903
> +    return context->priv->timeZoneOverride.utf8().data();

Hi, this is a use-after-free vulnerability. The CString object is already destroyed when the function returns, so the caller receives a pointer to an invalid buffer. To fix this, please store a CString in the priv struct, not a String.

[Yury Semikhatsky]

Comment on attachment 458694 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1903
>> +    return context->priv->timeZoneOverride.utf8().data();
> 
> Hi, this is a use-after-free vulnerability. The CString object is already destroyed when the function returns, so the caller receives a pointer to an invalid buffer. To fix this, please store a CString in the priv struct, not a String.

Sent a fix to https://bugs.webkit.org/show_bug.cgi?id=240327