Bug 230033

Summary: [JSC] Implement Temporal.PlainDate
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, dino, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, philip.chimento, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 223166    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch ysuzuki: review+, ews-feeder: commit-queue-

Description Yusuke Suzuki 2021-09-07 19:29:11 PDT 
...
Comment 1 Radar WebKit Bug Importer 2021-09-14 19:30:16 PDT 
<rdar://problem/83127747>
Comment 2 Yusuke Suzuki 2021-10-10 17:39:58 PDT 
Created attachment 440747 [details]
Patch

WIP
Comment 3 Dean Jackson 2022-02-13 11:09:12 PST 
Created attachment 451827 [details]
Patch
Comment 4 Dean Jackson 2022-02-13 12:28:25 PST 
It seems the non-Apple bots are not generating the lut.h file, but I don't understand why not. DerivedSources.make has been updated.
Comment 5 Dean Jackson 2022-02-13 12:30:40 PST 
Created attachment 451837 [details]
Patch
Comment 6 Dean Jackson 2022-02-13 13:13:09 PST 
This is not intended to implement all of the API, but provide enough to easily add the rest. Yusuke's original patch is a more complete implementation - so I intend to merge bits from that as I go.
Comment 7 Dean Jackson 2022-02-15 09:44:20 PST 
Created attachment 452041 [details]
Patch
Comment 8 Yusuke Suzuki 2022-02-18 16:03:34 PST 
Comment on attachment 452041 [details]
Patch

r=me
Comment 9 EWS 2022-02-18 20:17:30 PST 
Tools/Scripts/svn-apply failed to apply attachment 452041 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 10 Dean Jackson 2022-02-19 09:43:51 PST 
Committed r290209 (247537@trunk): <https://commits.webkit.org/247537@trunk>
Comment 11 Yusuke Suzuki 2022-02-19 11:03:07 PST 
I'll extract my old patch's parsing code and upload it separately.
Comment 12 Philip Chimento 2022-02-21 12:16:40 PST 
Comment on attachment 452041 [details]
Patch

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

Thanks! Although I'm not a WebKit reviewer I've taken a quick look at this from the Temporal perspective and noted some things that could be covered in one of your follow-ups :-)

What would be a good way to keep track of what parts of PlainDate are done and what still needs doing?

> Source/JavaScriptCore/runtime/ISO8601.cpp:992
> +    return makeString(pad('0', 4, plainDate.year()), '-', pad('0', 2, plainDate.month()), '-', pad('0', 2, plainDate.day()));

Just a note for future improvements: this should be able to output 6-digit extended years if the year is outside the range 1000-9999.

> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:87
> +static ISO8601::PlainDate toPlainDate(JSGlobalObject* globalObject, ISO8601::Duration&& duration)

Nothing wrong with this usage of ISO8601::Duration as a "bag of ISO units" but maybe we should rename it to something like ISO8601::Units, or create ISO8601::Units as a separate type without the other Duration-like methods from Duration?

> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:120
> +

There's a step missing here (a call to ISODateTimeWithinLimits, https://tc39.es/proposal-temporal/#_ref_222) although I don't know if it belongs in this method or in toPlainDate() or create().
Comment 13 Yusuke Suzuki 2022-02-21 12:32:09 PST 
Comment on attachment 452041 [details]
Patch

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

>> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:120
>> +
> 
> There's a step missing here (a call to ISODateTimeWithinLimits, https://tc39.es/proposal-temporal/#_ref_222) although I don't know if it belongs in this method or in toPlainDate() or create().

This is done in https://bugs.webkit.org/show_bug.cgi?id=236936