Bug 218348 – REGRESSION (r254038): Simple.com money transfer UI is very laggy (multiple seconds per keypress)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tim Horton]

Created attachment 412666 [details]
repro the bug

Steps to Repro:

simple.com -> Move Money -> “Transfer to or from a bank”

The animation when it transitions in is very slow, and every keypress is very slow.



The attached reduction shows the problem: the Date constructor is - in some cases - 20-30x slower now than it used to be, and Simple is calling it a /lot/.

ggaren says that we're always missing the Date cache, and that adding a Z to the end of the string makes you go fast again (I'll attach a "fixed" version of the repro case too).

[Tim Horton]

Created attachment 412667 [details]
fast version with Zs

[Tim Horton]

rdar://problem/70782204

[Tim Horton]

Geoff bisected (I think) this to https://trac.webkit.org/changeset/254038/webkit

[Geoffrey Garen]

Yes, that's right.

[Geoffrey Garen]

I wonder: If you change the test case's strings to specify local time explicitly (rather than relying on there parser's implicit default when not specified), was that always slow before r254038?

[Darin Adler]

(In reply to Tim Horton from comment #0)
> ggaren says that we're always missing the Date cache

Yes, the date cache is not expected to work here. It only works for one string. We could add a different date cache, but I don’t think the date cache had anything to do with why this was fast before.

[Darin Adler]

Before r254038, a time without a time zone would be parsed as UTC, not as local time.

My guess is that *localTimeOffset* is simply slow, and this code would never have called that before r254038 because it wasn’t .

To optimize this, one relevant question is why the caching in *localTimeOffset* is not effective.

[Darin Adler]

(In reply to Darin Adler from comment #7)
> My guess is that *localTimeOffset* is simply slow, and this code would never
> have called that before r254038 because it wasn’t .

because it wasn't treating it as a local time.

[Darin Adler]

I think the issue is that finding time offsets has always been slow, and involves calling underlying platform APIs. And our caching is inadequate to mitigate that. And it also seems that websites that don’t need this expensive operation end up asking for it accidentally.

It would be easy to fix this slowdown by restoring the old behavior, and much harder to fix it by optimizing local time computations.

[Darin Adler]

If this is much faster in other browser engines, I would love to know how they manage to quickly compute the correct answer.

[Geoffrey Garen]

With only 20 date strings in the whole test, I would expect localTimeOffset() for those strings to be fast, with a nearly 100% cache hit rate. That's the part that surprises me.

[Darin Adler]

(In reply to Geoffrey Garen from comment #11)
> With only 20 date strings in the whole test, I would expect
> localTimeOffset() for those strings to be fast, with a nearly 100% cache hit
> rate.

What’s the key for the cache? I am not surprised that there’s a 0% cache hit rate, but maybe I should be.

[Geoffrey Garen]

The key is { milliseconds, WTF::TimeType }.

Oh, wait a second. The cache only caches values within the mostly recently accessed month. So, I guess that must be why it's slow: All these time values are in different months.

[Geoffrey Garen]

I think we probably either need to expand the date string cache or expand the locatTimeOffset cache or both.

[Yusuke Suzuki]

(In reply to Geoffrey Garen from comment #14)
> I think we probably either need to expand the date string cache or expand
> the locatTimeOffset cache or both.

I've checked and our range caching mechanism fail to find the results.
To solve this problem, we need to have much larger cache because this micro benchmark is doing offset lookup for various years, and each year can have slightly different offset information due to DST.

Rather than that, we should use ICU's timezone cache directly instead. And that's what V8 and SpiderMonkey are doing (while they have range-cache for non-ICU environment, which is similar to us).

One sad thing is ICU's TimeZone is only available in C++... So we need to use C++ ICU APIs for this particular place unfortunately.

[Geoffrey Garen]

> Rather than that, we should use ICU's timezone cache directly instead. And
> that's what V8 and SpiderMonkey are doing (while they have range-cache for
> non-ICU environment, which is similar to us).

Also: v8 seems to use a 32 element cache (holds 32 distinct month ranges) when doing caching explicitly.

But yeah, just using ICU's cache would be great!

[Darin Adler]

It’s nice that switching to ICU will solve this problem, but I assume that making that switch will also change other things. Are all those changes improvements? Does ICU respond the way we would like to time zone changes while the process is running, clearing caches as appropriate? Does it use historically accurate DST information, or not? Does it matter that ICU’s time zone database is separate from the Apple platform ones and will be updated at a different time?

It would also be straightforward to fix our cache, so we should not treat ICU as our only option. But if it’s a superior option, I am open to it.

I’d like to know more about the costs of using ICU’s C++ API. Will there be practical problems with that? Or were we carefully sticking to the C API without a compelling reason?

[Yusuke Suzuki]

(In reply to Darin Adler from comment #17)
> It’s nice that switching to ICU will solve this problem, but I assume that
> making that switch will also change other things. Are all those changes
> improvements?
> Does ICU respond the way we would like to time zone changes
> while the process is running, clearing caches as appropriate?

ICU does not respond to that, but ICU offers the way to clear the cache.
V8 and SpiderMonkey do different things: SpiderMonkey clears the cache when new Realm is created IIRC. V8 offers the way to clear, but it doesn't clear ICU implicitly. Chrome renderer process clears it by calling it when timezone is changed.
Current way of handling timezone cache in JSC is a bit silly: every time we first enter the JSC VM scope.

> Does it use
> historically accurate DST information, or not? Does it matter that ICU’s
> time zone database is separate from the Apple platform ones and will be
> updated at a different time?

I think rest of Apple platforms uses ICU since CoreFoundation etc. uses it and we need to handle i18n in platform.
Possibly, switching to ICU can be improvement in terms of consistency with the platform.

> 
> It would also be straightforward to fix our cache, so we should not treat
> ICU as our only option. But if it’s a superior option, I am open to it.

I think ICU cache is better in terms of efficiency compared to the blind cache over the localtime_r.
ICU can have all timezone data and rules including DST.
So ICU can cache timezone data efficiently even if the Date is not within the same year.
ICU should know that the DST rule of the timezone in detail, and based on that, it can decide offset efficiently: e.g. ICU can know whether this month is never in DST etc.

> 
> I’d like to know more about the costs of using ICU’s C++ API. Will there be
> practical problems with that? Or were we carefully sticking to the C API
> without a compelling reason?

[Yusuke Suzuki]

Created attachment 412708 [details]
Patch

[Tim Horton]

Comment on attachment 412708 [details]
Patch

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

Neat! Very red, and I'll let other folks do the real reviewing, but thank you for fixing!

> Source/JavaScriptCore/Sources.txt:867
> +// Confine U_SHOW_CPLUSPLUS_API's effect in this file.

I think "to" instead of "in".

> JSTests/microbenchmarks/local-date-constructor.js:1
> +function doTheThing() {

Heh, now I wish I had named this something better.

[Yusuke Suzuki]

OK, we need to update our test results. Our results tested in our test suites were wrong.
Our test is using `new Date(1111, 1)` etc. And it assumes that the timezone is the same America/Los_Angeles.
But this is wrong strictly speaking. America/Los_Angeles timezone is effective only after 1991.
Before that, we must handle it UTC-0752, which is based on Los_Angeles' longitude (118.2347W).

[Yusuke Suzuki]

Comment on attachment 412708 [details]
Patch

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

>> Source/JavaScriptCore/Sources.txt:867
>> +// Confine U_SHOW_CPLUSPLUS_API's effect in this file.
> 
> I think "to" instead of "in".

Fixed.

>> JSTests/microbenchmarks/local-date-constructor.js:1
>> +function doTheThing() {
> 
> Heh, now I wish I had named this something better.

Renamed it to "test" :)

[Yusuke Suzuki]

Created attachment 412726 [details]
Patch

[Yusuke Suzuki]

Created attachment 412883 [details]
Patch

[Yusuke Suzuki]

Created attachment 412884 [details]
Patch

[Yusuke Suzuki]

Created attachment 412886 [details]
Patch

[Yusuke Suzuki]

Created attachment 412889 [details]
Patch

[Yusuke Suzuki]

Created attachment 412949 [details]
Patch

[Darin Adler]

Comment on attachment 412949 [details]
Patch

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

Seems like the refactoring to make everything instance functions could be done separately/first if we want to keep the big behavior changing patch smalle.

> Source/JavaScriptCore/Sources.txt:868
> +// Confine U_SHOW_CPLUSPLUS_API's effect to this file.
> +runtime/JSDateMath.cpp @no-unify

A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it.

But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change?

> Source/JavaScriptCore/runtime/JSDateMath.cpp:108
> +        constexpr bool isLocalTime = false;

Not personally a huge fan of this way of documenting the meaning of an argument. Normally I prefer our enum technique instead.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:189
> +Ref<DateInstanceData> DateCache::getOrCreateCachedDateInstanceData(double milli)

Not so fond of "miill" as the name for "date/time in milliseconds, relative to the 1970 epoch".

> Source/JavaScriptCore/runtime/JSDateMath.h:58
> +// We do not expose icu::TimeZone in this header file. And we cannot use icu::TimeZone forward declaration
> +// because icu namespace can be an alias to icu$verNum namespace.
> +class OpaqueICUTimeZone;
> +struct OpaqueICUTimeZoneDeleter {
> +    JS_EXPORT_PRIVATE void operator()(OpaqueICUTimeZone*);
> +};

I don’t think the forward declaration of OpaqueICUTimeZone needs this comment. The comment is for the deleter. Lets put the forward declaration above init the normal forward declaration paragraph. And put the deleter in its own paragraph. Doesn’t need to be grouped with the forward declaration.

> Source/JavaScriptCore/runtime/JSDateMath.h:69
> +    OpaqueICUTimeZone* ensureTimeZoneCache()

Not sure why we need verbs like "ensure". This function returns the cache. And yes, allocates it if needed. Such a function could just be named "timeZoneCache", I think.

> Source/JavaScriptCore/runtime/JSDateMath.h:76
> +    Ref<DateInstanceData> getOrCreateCachedDateInstanceData(double);

Not sure why we need verbs like "get or create" here. This function returns cached instance data. And yes, allocates it if needed. Such a function could just be named "cachedDateInstanceData", I think.

I don’t think the meaning of the argument is obvious. The fact that the double is the date, and what kind of double it is (in milliseconds, in seconds, what base?). The other functions have "milliseconds" in their names. Kind of wish we could use our classes that disambiguate what the double is used for.

> Source/JavaScriptCore/runtime/JSDateMath.h:78
> +    void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&);

Some day we could use a return value here instead of an out argument. And give this a better name perhaps?

> Source/JavaScriptCore/runtime/JSDateMath.h:79
> +    double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType);

Really unclear what the double argument is.

> Source/JavaScriptCore/runtime/JSDateMath.h:80
> +    double parseDate(JSGlobalObject*, VM&, const WTF::String&);

Can this take a StringView instead of a const String& some day?

[Saam Barati]

Comment on attachment 412949 [details]
Patch

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

> LayoutTests/js/script-tests/date-constructor.js:10
> +object.valueOf = function() { return 1995; }

why the change of this test?

[Yusuke Suzuki]

Comment on attachment 412949 [details]
Patch

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

Unfortunately, I think including DateCache refactoring is a bit important to include everything into JSDateMath.cpp and not leaking icu C++ API usage outside of JSDateMath.cpp. For example, currently, we are having VM::resetDateCache and it is clearing things in VM.cpp. We do not want to touch DateCache things in non JSDateMath.cpp and we would like to confine all operations into JSDateMath.cpp. That's why this patch involves these changes.

>> Source/JavaScriptCore/Sources.txt:868
>> +runtime/JSDateMath.cpp @no-unify
> 
> A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it.
> 
> But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change?

I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file.
I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:108
>> +        constexpr bool isLocalTime = false;
> 
> Not personally a huge fan of this way of documenting the meaning of an argument. Normally I prefer our enum technique instead.

This icu::TimeZone::getOffset is ICU API :)

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:189
>> +Ref<DateInstanceData> DateCache::getOrCreateCachedDateInstanceData(double milli)
> 
> Not so fond of "miill" as the name for "date/time in milliseconds, relative to the 1970 epoch".

I'll change it to milliSecondsFromEpoch. This is because of existing functions in DateInstanceCache etc.

>> Source/JavaScriptCore/runtime/JSDateMath.h:58
>> +};
> 
> I don’t think the forward declaration of OpaqueICUTimeZone needs this comment. The comment is for the deleter. Lets put the forward declaration above init the normal forward declaration paragraph. And put the deleter in its own paragraph. Doesn’t need to be grouped with the forward declaration.

OK, changed.

>> Source/JavaScriptCore/runtime/JSDateMath.h:69
>> +    OpaqueICUTimeZone* ensureTimeZoneCache()
> 
> Not sure why we need verbs like "ensure". This function returns the cache. And yes, allocates it if needed. Such a function could just be named "timeZoneCache", I think.

OK, changed :)

>> Source/JavaScriptCore/runtime/JSDateMath.h:76
>> +    Ref<DateInstanceData> getOrCreateCachedDateInstanceData(double);
> 
> Not sure why we need verbs like "get or create" here. This function returns cached instance data. And yes, allocates it if needed. Such a function could just be named "cachedDateInstanceData", I think.
> 
> I don’t think the meaning of the argument is obvious. The fact that the double is the date, and what kind of double it is (in milliseconds, in seconds, what base?). The other functions have "milliseconds" in their names. Kind of wish we could use our classes that disambiguate what the double is used for.

OK, changed this name to cachedDateInstanceData and put millisecondsFromEpoch variable.

>> Source/JavaScriptCore/runtime/JSDateMath.h:78
>> +    void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&);
> 
> Some day we could use a return value here instead of an out argument. And give this a better name perhaps?

Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double.

>> Source/JavaScriptCore/runtime/JSDateMath.h:79
>> +    double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType);
> 
> Really unclear what the double argument is.

This code is existing one. But I'll add `milliSecondsFromEpoch` to this variable.

>> Source/JavaScriptCore/runtime/JSDateMath.h:80
>> +    double parseDate(JSGlobalObject*, VM&, const WTF::String&);
> 
> Can this take a StringView instead of a const String& some day?

Interestingly, using WTF::String& is more efficient than StringView here. The reason is that we could cache this input string in m_cachedDateString. And this is String.
If we get this as `const WTF::String&`, then copying this to m_cachedDateString is ref(). But if we take this as StringView, we need to copy the content to m_cachedDateString.

[Yusuke Suzuki]

Created attachment 412972 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 412949 [details]
Patch

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

>> LayoutTests/js/script-tests/date-constructor.js:10
>> +object.valueOf = function() { return 1995; }
> 
> why the change of this test?

The reason is described in ChangeLog.

"The time before America/Los_Angeles timezone is effective should be handled as UTC-0752. Use 1995 to test the intent correctly."

Between 1111 and 1995, timezones are different. One is UTC-0752, and one is America/Los_Angeles.
This difference is tested in different tests. And in this test, we would like to avoid this difference because it is out of intent of this test.

[Darin Adler]

Comment on attachment 412949 [details]
Patch

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

>>> Source/JavaScriptCore/Sources.txt:868
>>> +runtime/JSDateMath.cpp @no-unify
>> 
>> A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it.
>> 
>> But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change?
> 
> I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file.
> I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly.

OK. What about my first comment, that it should be an "end of line" comment, not a "line before" comment?

>>> Source/JavaScriptCore/runtime/JSDateMath.h:78
>>> +    void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&);
>> 
>> Some day we could use a return value here instead of an out argument. And give this a better name perhaps?
> 
> Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double.

Great. (Please don’t capitalize the "S" in "milliseconds".)

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:46
> +        // FIXME: We should clear it only when we know that timezone has been changed.

"that timezone" -> "the timezone".

[Yusuke Suzuki]

Comment on attachment 412949 [details]
Patch

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

>>>> Source/JavaScriptCore/Sources.txt:868
>>>> +runtime/JSDateMath.cpp @no-unify
>>> 
>>> A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it.
>>> 
>>> But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change?
>> 
>> I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file.
>> I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly.
> 
> OK. What about my first comment, that it should be an "end of line" comment, not a "line before" comment?

Ah, yeah, that sounds good. I've changed.

[Yusuke Suzuki]

Comment on attachment 412949 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSDateMath.h:78
>>>> +    void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&);
>>> 
>>> Some day we could use a return value here instead of an out argument. And give this a better name perhaps?
>> 
>> Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double.
> 
> Great. (Please don’t capitalize the "S" in "milliseconds".)

Fixed.

>>> Source/JavaScriptCore/runtime/JSDateMath.h:79
>>> +    double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType);
>> 
>> Really unclear what the double argument is.
> 
> This code is existing one. But I'll add `milliSecondsFromEpoch` to this variable.

Interestingly, this double is not millisecondsFromEpoch. It is milliseconds unit of GregorianDateTime. So I named it as `milliseconds` while msToGregorianDateTime's double is `millisecondsFromEpoch`.

>> Source/JavaScriptCore/runtime/VMEntryScope.cpp:46
>> +        // FIXME: We should clear it only when we know that timezone has been changed.
> 
> "that timezone" -> "the timezone".

Fixed.

[Yusuke Suzuki]

Created attachment 412976 [details]
Patch

[Darin Adler]

Comment on attachment 412976 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSDateMath.cpp:119
> +        return LocalTimeOffset { false, 0 };
> +    return LocalTimeOffset { !!dstOffset, rawOffset + dstOffset };

We don’t actually have to write LocalTimeOffset here, do we?

> JSTests/ChangeLog:20
> +        * ChakraCore.yaml:
> +        * ChakraCore/test/Date/DateCtr.baseline-jsc: Added. The time before America/Los_Angeles timezone is effective should be handled as UTC-0752.
> +        * complex.yaml:
> +        * complex/timezone-offset-before-america-los-angeles-is-defined.js: Added for UTC-0752.
> +        (shouldBe):
> +        * microbenchmarks/local-date-constructor.js: Added.
> +        (test):
> +        * mozilla/ecma/Date/15.9.5.16.js:
> +        * mozilla/ecma/Date/15.9.5.18.js:
> +        * mozilla/ecma/Date/15.9.5.22-1.js:
> +        * mozilla/ecma/Date/15.9.5.22-2.js:
> +        * mozilla/ecma/Date/15.9.5.35-1.js:
> +        (getTestCases):

I wish this explained the test changes better. Could get rid of the silly quoting of the function names, and add comments explaining the reasons these are good changes to make.

> JSTests/mozilla/ecma/Date/15.9.5.16.js:-58
> -    addTestCase( TIME_YEAR_0 );

The comments don’t explain this. I think I understand it, but it would have been good to explain it in the change log.

> JSTests/mozilla/ecma/Date/15.9.5.18.js:-59
> -    addTestCase( TIME_YEAR_0 );

Ditto.

> JSTests/mozilla/ecma/Date/15.9.5.22-1.js:-61
> -    addTestCase( TIME_YEAR_0 );

Ditto.

> JSTests/mozilla/ecma/Date/15.9.5.22-2.js:60
> -    addTestCase( TIME_YEAR_0 );
>  /*
> +    addTestCase( TIME_YEAR_0 );

Ditto.

[Yusuke Suzuki]

Comment on attachment 412976 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:119
>> +    return LocalTimeOffset { !!dstOffset, rawOffset + dstOffset };
> 
> We don’t actually have to write LocalTimeOffset here, do we?

Yes, we do not need to have LocalTimeOffset here. Removed.

>> JSTests/ChangeLog:20
>> +        (getTestCases):
> 
> I wish this explained the test changes better. Could get rid of the silly quoting of the function names, and add comments explaining the reasons these are good changes to make.

Added explanation here.

>> JSTests/mozilla/ecma/Date/15.9.5.16.js:-58
>> -    addTestCase( TIME_YEAR_0 );
> 
> The comments don’t explain this. I think I understand it, but it would have been good to explain it in the change log.

Added explanation in ChangeLog.

>> JSTests/mozilla/ecma/Date/15.9.5.18.js:-59
>> -    addTestCase( TIME_YEAR_0 );
> 
> Ditto.

Added explanation in ChangeLog.

>> JSTests/mozilla/ecma/Date/15.9.5.22-1.js:-61
>> -    addTestCase( TIME_YEAR_0 );
> 
> Ditto.

Added explanation in ChangeLog.

>> JSTests/mozilla/ecma/Date/15.9.5.22-2.js:60
>> +    addTestCase( TIME_YEAR_0 );
> 
> Ditto.

Added explanation in ChangeLog.

[Yusuke Suzuki]

Committed r269320: <https://trac.webkit.org/changeset/269320>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 219915

[Yusuke Suzuki]

The reverted patch is once integrated since we workaround ICU missing API issue differently.

[Darin Adler]

(In reply to Yusuke Suzuki from comment #42)
> The reverted patch is once integrated since we workaround ICU missing API
> issue differently.

I’d like to know more. Where should I look to find out?

[Yusuke Suzuki]

(In reply to Darin Adler from comment #43)
> (In reply to Yusuke Suzuki from comment #42)
> > The reverted patch is once integrated since we workaround ICU missing API
> > issue differently.
> 
> I’d like to know more. Where should I look to find out?

Internal discussion, I'll send you a link :)

[Yusuke Suzuki]

*** Bug 196764 has been marked as a duplicate of this bug. ***