Bug 223783 – [JSC] Use new Apple ICU APIs to avoid C++ ICU API usage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use new Apple ICU APIs to avoid C++ ICU API usage

[Yusuke Suzuki]

Created attachment 424320 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/75060240>

[Yusuke Suzuki]

Created attachment 424321 [details]
Patch

[Mark Lam]

Comment on attachment 424321 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSDateMath.cpp:107
> +#endif

Why not just have an #else
    using OpaqueICUTimeZone = icu::Timezone;
here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below?

[Yusuke Suzuki]

Comment on attachment 424321 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:107
>> +#endif
> 
> Why not just have an #else
>     using OpaqueICUTimeZone = icu::Timezone;
> here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below?

That sounds nice. Changed.

[Yusuke Suzuki]

Created attachment 424337 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 424321 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSDateMath.cpp:107
>>> +#endif
>> 
>> Why not just have an #else
>>     using OpaqueICUTimeZone = icu::Timezone;
>> here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below?
> 
> That sounds nice. Changed.

Ah, this does not work. using (typedef) and class forward declaration are different.

[Yusuke Suzuki]

Created attachment 424339 [details]
Patch

[Mark Lam]

Comment on attachment 424339 [details]
Patch

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

r=me.  Please make sure that the mac-AS-debug-wk2 bot failure is not real.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:133
> +    // The above can fail if input date is invalid: NaN etc.
> +    // We can return any values in this case since later we fail when computing non timezone offset part anyway.
> +    constexpr LocalTimeOffset failed { false, 0 };

"The above" sounds wrong since there's no code that has any effect above.  How about changing it to "This function"?

This failed return value also seem to contradict the comment above the function which states that "If this function is called with NaN it returns NaN."  Maybe remove the comment above the function?

[Yusuke Suzuki]

Comment on attachment 424339 [details]
Patch

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

Thanks. And AS fail is media/media-fragments/TC0051.html, which is known flaky these days.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:133
>> +    constexpr LocalTimeOffset failed { false, 0 };
> 
> "The above" sounds wrong since there's no code that has any effect above.  How about changing it to "This function"?
> 
> This failed return value also seem to contradict the comment above the function which states that "If this function is called with NaN it returns NaN."  Maybe remove the comment above the function?

Yes, I've changed them.

[Yusuke Suzuki]

Committed r275109 (235817@main): <https://commits.webkit.org/235817@main>

[Yusuke Suzuki]

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