Bug 229703 – [JSC] Implement Temporal.TimeZone

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 437111 [details]
Patch

[Yusuke Suzuki]

Created attachment 437118 [details]
Patch

[Yusuke Suzuki]

Created attachment 437122 [details]
Patch

[Ross Kirsling]

Comment on attachment 437122 [details]
Patch

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

> Source/JavaScriptCore/runtime/ISO8601.cpp:42
> +int32_t parseDecimalInt32(const CharType* characters, unsigned length)

Nice! You should be able to remove the parseInt.h include too.

> Source/JavaScriptCore/runtime/ISO8601.cpp:255
> +    //  Maximum and minimum values are +-23:59:59.999999999 = +-86399999999999ns, which can be represented by int64_t / double's integer part.

nit: Maybe use ± so that it doesn't look like "+-" can occur?

> Source/JavaScriptCore/runtime/ISO8601.cpp:256
> +    return readCharactersForParsing(string, [](auto buffer) -> std::optional<int64_t> {

I feel like it's useful to keep this part in a separate function to avoid having a huge indented lambda, but what do you think?

> Source/JavaScriptCore/runtime/ISO8601.cpp:288
> +        ASSERT(buffer.hasCharactersRemaining());

This one seems a little excessive, right after checking atEnd().

> Source/JavaScriptCore/runtime/ISO8601.cpp:289
> +        bool splittedByColon = false;

nit: Technically the participle is still "split", not "splitted". English is weird...

> Source/JavaScriptCore/runtime/ISO8601.cpp:313
> +        ASSERT(buffer.hasCharactersRemaining());

Ditto (to L288).

> Source/JavaScriptCore/runtime/ISO8601.cpp:339
> +        ASSERT(buffer.hasCharactersRemaining());

Ditto (to L288).

> Source/JavaScriptCore/runtime/ISO8601.cpp:380
> +        auto fraction = WTF::numberToStringUnsigned<Vector<LChar, 9>>(nanoseconds);

Clever!

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:210
> +#include "TemporalTimeZoneConstructor.h"

I don't think you need to include the Constructor header.

> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:65
> +std::optional<TimeZoneID> TemporalTimeZone::isValidTimeZoneName(StringView string)

Hmm, this name doesn't make sense given its type. How about something more like "idForTimeZoneName"?

> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:98
> +            return jsCast<JSObject*>(timeZoneLike);

May as well use timeZoneLikeObject, since you've got it. :)

> JSTests/stress/temporal-timezone.js:21
> +let failures = [

"2059:00" might be a good one to add.

[Yusuke Suzuki]

Comment on attachment 437122 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ISO8601.cpp:42
>> +int32_t parseDecimalInt32(const CharType* characters, unsigned length)
> 
> Nice! You should be able to remove the parseInt.h include too.

Ah, still parseDuration etc. is using that.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:255
>> +    //  Maximum and minimum values are +-23:59:59.999999999 = +-86399999999999ns, which can be represented by int64_t / double's integer part.
> 
> nit: Maybe use ± so that it doesn't look like "+-" can occur?

Changed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:256
>> +    return readCharactersForParsing(string, [](auto buffer) -> std::optional<int64_t> {
> 
> I feel like it's useful to keep this part in a separate function to avoid having a huge indented lambda, but what do you think?

Sounds good! Changed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:288
>> +        ASSERT(buffer.hasCharactersRemaining());
> 
> This one seems a little excessive, right after checking atEnd().

Removed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:289
>> +        bool splittedByColon = false;
> 
> nit: Technically the participle is still "split", not "splitted". English is weird...

Ah! Fixed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:313
>> +        ASSERT(buffer.hasCharactersRemaining());
> 
> Ditto (to L288).

Removed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:339
>> +        ASSERT(buffer.hasCharactersRemaining());
> 
> Ditto (to L288).

Removed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:210
>> +#include "TemporalTimeZoneConstructor.h"
> 
> I don't think you need to include the Constructor header.

Right! Removed.

>> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:65
>> +std::optional<TimeZoneID> TemporalTimeZone::isValidTimeZoneName(StringView string)
> 
> Hmm, this name doesn't make sense given its type. How about something more like "idForTimeZoneName"?

Changed

>> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:98
>> +            return jsCast<JSObject*>(timeZoneLike);
> 
> May as well use timeZoneLikeObject, since you've got it. :)

Right :) Changed.

>> JSTests/stress/temporal-timezone.js:21
>> +let failures = [
> 
> "2059:00" might be a good one to add.

Added.

[Yusuke Suzuki]

Created attachment 437253 [details]
Patch

[Ross Kirsling]

Comment on attachment 437253 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:36
> +static constexpr bool verbose = false;

Seems like you're not actually using this at present.

> Source/JavaScriptCore/runtime/TemporalTimeZonePrototype.cpp:31
> +#include "IntlObject.h"

Looks like you're already getting this from TemporalTimeZone.h.

[Yusuke Suzuki]

Comment on attachment 437253 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/TemporalTimeZone.cpp:36
>> +static constexpr bool verbose = false;
> 
> Seems like you're not actually using this at present.

Removed

>> Source/JavaScriptCore/runtime/TemporalTimeZonePrototype.cpp:31
>> +#include "IntlObject.h"
> 
> Looks like you're already getting this from TemporalTimeZone.h.

Removed

[Yusuke Suzuki]

Committed r282018 (241323@main): <https://commits.webkit.org/241323@main>

[Radar WebKit Bug Importer]

<rdar://problem/82736513>