Bug 229651 – [JSC] Implement Temporal.Calendar

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 436730 [details]
Patch

[Yusuke Suzuki]

Created attachment 436735 [details]
Patch

[Yusuke Suzuki]

Created attachment 436757 [details]
Patch

[Yusuke Suzuki]

Created attachment 436761 [details]
Patch

[Yusuke Suzuki]

Created attachment 436799 [details]
Patch

[Philip Chimento]

Comment on attachment 436799 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:1594
> +            if (string.is8Bit())

I believe all the BCP47 identifiers are ASCII-only, by definition?

> Source/JavaScriptCore/runtime/TemporalCalendarPrototype.cpp:98
> +        return throwVMTypeError(globalObject, scope, "Temporal.Calendar.prototype.fields called on value that's not an object initialized as a Calendar"_s);

As in the Duration patch I think the brand check exception message would be clearer to developers without "an object initialized as"

[Yusuke Suzuki]

Comment on attachment 436799 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlObject.cpp:1594
>> +            if (string.is8Bit())
> 
> I believe all the BCP47 identifiers are ASCII-only, by definition?

This is8Bit is WTF::String implementation detail, and it is possible that ASCII-only string can get 16bit strings.
And it would be possible to make String 16bit internally in some cases (e.g. string is determined to be shared with the existing 16bit string). So putting both cases is better for safety and correctness :)

[Ross Kirsling]

Comment on attachment 436799 [details]
Patch

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

Looks good, given Philip's comments and the resolution of the Slack thread about implementation-definedness.

> Source/WTF/ChangeLog:11
> +        * wtf/text/StringImpl.cpp:
> +        (WTF::StringImpl::createStaticStringImpl):
> +        * wtf/text/StringImpl.h:
> +        (WTF::StringImpl::createStaticStringImpl):

I think this deserves a brief explanation. :)

> JSTests/stress/temporal-calendar.js:80
> +        password: 'hunter2', // Note: Don't really store passwords like that

🤣

[Yusuke Suzuki]

Comment on attachment 436799 [details]
Patch

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

>> Source/WTF/ChangeLog:11
>> +        (WTF::StringImpl::createStaticStringImpl):
> 
> I think this deserves a brief explanation. :)

Added :)

>> Source/JavaScriptCore/runtime/TemporalCalendarPrototype.cpp:98
>> +        return throwVMTypeError(globalObject, scope, "Temporal.Calendar.prototype.fields called on value that's not an object initialized as a Calendar"_s);
> 
> As in the Duration patch I think the brand check exception message would be clearer to developers without "an object initialized as"

Changed to drop `an object initialized as...` part :)

[Yusuke Suzuki]

Committed r281788 (241125@main): <https://commits.webkit.org/241125@main>

[Radar WebKit Bug Importer]

<rdar://problem/82559111>