Bug 233781 - Change IDL `Date` to be backed by `WallTime` to avoid confusion when converting to native dates
Summary: Change IDL `Date` to be backed by `WallTime` to avoid confusion when converti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 236360
  Show dependency treegraph
 
Reported: 2021-12-02 12:45 PST by Devin Rousso
Modified: 2022-02-14 13:32 PST (History)
34 users (show)

See Also:


Attachments
Patch (46.93 KB, patch)
2021-12-02 12:54 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.41 KB, patch)
2021-12-02 18:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (45.40 KB, patch)
2021-12-03 11:04 PST, Devin Rousso
darin: review-
Details | Formatted Diff | Diff
Patch (45.41 KB, patch)
2021-12-03 12:03 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.40 KB, patch)
2021-12-03 12:39 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.05 KB, patch)
2021-12-03 14:00 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.26 KB, patch)
2021-12-03 14:35 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.77 KB, patch)
2021-12-03 14:45 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (25.22 KB, patch)
2021-12-03 14:58 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-12-02 12:45:44 PST
JS `Date` is milliseconds-based, but some native dates (e.g. `NSDate`) are seconds-based.  `WallTime` will avoid any confusion by having a more defined type instead of something very generic like `double`.
Comment 1 Devin Rousso 2021-12-02 12:54:35 PST
Created attachment 445766 [details]
Patch
Comment 2 Sihui Liu 2021-12-02 17:04:26 PST
Comment on attachment 445766 [details]
Patch

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

Overall the IDB part looks good to me; the test failure looks legit. It seems IDBKeyData::IDBKeyData(const IDBKey* key) can set m_value to double for Date type.

> Source/WebCore/bindings/js/JSDOMConvertDate.cpp:50
> +    return WallTime::fromSeconds(Seconds::fromMilliseconds(milliseconds));

Could milliseconds be NaN at this point?
Comment 3 Devin Rousso 2021-12-02 17:07:28 PST
Comment on attachment 445766 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMConvertDate.cpp:50
>> +    return WallTime::fromSeconds(Seconds::fromMilliseconds(milliseconds));
> 
> Could milliseconds be NaN at this point?

Yes it's possible for `milliseconds` to be NaN, but that's fine because both `Seconds` and `WallTime` support NaN :)
Comment 4 Devin Rousso 2021-12-02 18:38:55 PST
Created attachment 445801 [details]
Patch

rebased and fixed typo in `WebCore::IDBKeyData::hash`
Comment 5 Devin Rousso 2021-12-03 11:04:47 PST
Created attachment 445875 [details]
Patch
Comment 6 Darin Adler 2021-12-03 11:50:28 PST
Comment on attachment 445875 [details]
Patch

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

Great change!

review- because of the "-1.0" mistake

> Source/WebCore/Modules/applepay/cocoa/PaymentSummaryItemsCocoa.mm:66
> -static NSDate *toDate(double date)
> +static NSDate *toDate(WallTime date)
>  {
> -    return [NSDate dateWithTimeIntervalSince1970:(date / 1000)];
> +    return [NSDate dateWithTimeIntervalSince1970:date.secondsSinceEpoch().value()];
>  }

More follow-up: seems to me this function should be somewhere reusable.

Other files that either have a copy or could use it: SearchPopupMenuCocoa.mm, WebFrameLoaderClient.cpp, _WKResourceLoadInfo.mm, WebProcessCocoa.mm, NetworkProcessCocoa.mm, and NetworkSessionCocoa.mm.

I would call it createNSDate and have it return a RetainPtr<NSDate>. Just have to figure out which header to put it in. If no existing header, then <wtf/cocoa/NSDateExtras.h>. But could even put it in Seconds.h with the implementation in an .mm file if we follow the pattern that String, StringView, URL, and more have established.

> Source/WebCore/Modules/indexeddb/IDBKey.h:65
> +        return adoptRef(*new IDBKey(date));

Could keep the patch and this class simpler by only making a change here and in the date() function. Convert to a double and back. Other changes not needed. Not sure it’s all that valuable to actually keep the WallTime intact inside the IDBKey class as long as we change these two public functions.

> Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:420
> +void IDBKeyData::setDateValue(WallTime value)

Same suggestion here. Just convert here and many other changes won’t be needed.

> Source/WebCore/Modules/indexeddb/IDBKeyData.h:167
> -        return std::get<double>(m_value);
> +        return std::get<WallTime>(m_value);

Same suggestion here. Just convert right here.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1610
> +            write(-1);

This is wrong! Needs to be:

    write(-1.0);

Otherwise it will write an integer, not a double. May need to find out if we have test cases covering this.
Comment 7 Devin Rousso 2021-12-03 11:58:41 PST
Comment on attachment 445875 [details]
Patch

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

>> Source/WebCore/Modules/applepay/cocoa/PaymentSummaryItemsCocoa.mm:66
>>  }
> 
> More follow-up: seems to me this function should be somewhere reusable.
> 
> Other files that either have a copy or could use it: SearchPopupMenuCocoa.mm, WebFrameLoaderClient.cpp, _WKResourceLoadInfo.mm, WebProcessCocoa.mm, NetworkProcessCocoa.mm, and NetworkSessionCocoa.mm.
> 
> I would call it createNSDate and have it return a RetainPtr<NSDate>. Just have to figure out which header to put it in. If no existing header, then <wtf/cocoa/NSDateExtras.h>. But could even put it in Seconds.h with the implementation in an .mm file if we follow the pattern that String, StringView, URL, and more have established.

Ooo absolutely!  I actually wanted to create implicit conversions between `WTF::Seconds` and `NSTimeInterval` (just like `WTF::String` and `NSString`) so I'll do all those as a followup too :)

>> Source/WebCore/Modules/indexeddb/IDBKey.h:65
>> +        return adoptRef(*new IDBKey(date));
> 
> Could keep the patch and this class simpler by only making a change here and in the date() function. Convert to a double and back. Other changes not needed. Not sure it’s all that valuable to actually keep the WallTime intact inside the IDBKey class as long as we change these two public functions.

I personally think it's nicer/cleaner to encode `WallTime` inside the `std::variant` because then there's a 1-1 mapping between `IndexedDB::KeyType` and `std::variant` subtype (i.e. if you pull out a `WallTime` from the `std::variant` then you must also be a `IndexedDB::KeyType::Date`), but I don't feel super strongly so if you'd prefer I can change it as you described.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1610
>> +            write(-1);
> 
> This is wrong! Needs to be:
> 
>     write(-1.0);
> 
> Otherwise it will write an integer, not a double. May need to find out if we have test cases covering this.

Oh wow good catch.  Will change :)
Comment 8 Devin Rousso 2021-12-03 11:58:45 PST
Comment on attachment 445875 [details]
Patch

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

>> Source/WebCore/Modules/applepay/cocoa/PaymentSummaryItemsCocoa.mm:66
>>  }
> 
> More follow-up: seems to me this function should be somewhere reusable.
> 
> Other files that either have a copy or could use it: SearchPopupMenuCocoa.mm, WebFrameLoaderClient.cpp, _WKResourceLoadInfo.mm, WebProcessCocoa.mm, NetworkProcessCocoa.mm, and NetworkSessionCocoa.mm.
> 
> I would call it createNSDate and have it return a RetainPtr<NSDate>. Just have to figure out which header to put it in. If no existing header, then <wtf/cocoa/NSDateExtras.h>. But could even put it in Seconds.h with the implementation in an .mm file if we follow the pattern that String, StringView, URL, and more have established.

Ooo absolutely!  I actually wanted to create implicit conversions between `WTF::Seconds` and `NSTimeInterval` (just like `WTF::String` and `NSString`) so I'll do all those as a followup too :)

>> Source/WebCore/Modules/indexeddb/IDBKey.h:65
>> +        return adoptRef(*new IDBKey(date));
> 
> Could keep the patch and this class simpler by only making a change here and in the date() function. Convert to a double and back. Other changes not needed. Not sure it’s all that valuable to actually keep the WallTime intact inside the IDBKey class as long as we change these two public functions.

I personally think it's nicer/cleaner to encode `WallTime` inside the `std::variant` because then there's a 1-1 mapping between `IndexedDB::KeyType` and `std::variant` subtype (i.e. if you pull out a `WallTime` from the `std::variant` then you must also be a `IndexedDB::KeyType::Date`), but I don't feel super strongly so if you'd prefer I can change it as you described.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1610
>> +            write(-1);
> 
> This is wrong! Needs to be:
> 
>     write(-1.0);
> 
> Otherwise it will write an integer, not a double. May need to find out if we have test cases covering this.

Oh wow good catch.  Will change :)
Comment 9 Devin Rousso 2021-12-03 12:03:16 PST
Created attachment 445887 [details]
Patch
Comment 10 Devin Rousso 2021-12-03 12:39:40 PST
Created attachment 445889 [details]
Patch
Comment 11 Darin Adler 2021-12-03 12:59:25 PST
(In reply to Devin Rousso from comment #8)
> I don't feel super strongly so if you'd
> prefer I can change it as you described.

I do feel pretty strongly about this. We just don’t need the change and extra code. Consider how much smaller the patch and the code will be: that’s not nothing.
Comment 12 Darin Adler 2021-12-03 13:01:18 PST
Comment on attachment 445875 [details]
Patch

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

>>>> Source/WebCore/Modules/applepay/cocoa/PaymentSummaryItemsCocoa.mm:66
>>>>  }
>>> 
>>> More follow-up: seems to me this function should be somewhere reusable.
>>> 
>>> Other files that either have a copy or could use it: SearchPopupMenuCocoa.mm, WebFrameLoaderClient.cpp, _WKResourceLoadInfo.mm, WebProcessCocoa.mm, NetworkProcessCocoa.mm, and NetworkSessionCocoa.mm.
>>> 
>>> I would call it createNSDate and have it return a RetainPtr<NSDate>. Just have to figure out which header to put it in. If no existing header, then <wtf/cocoa/NSDateExtras.h>. But could even put it in Seconds.h with the implementation in an .mm file if we follow the pattern that String, StringView, URL, and more have established.
>> 
>> Ooo absolutely!  I actually wanted to create implicit conversions between `WTF::Seconds` and `NSTimeInterval` (just like `WTF::String` and `NSString`) so I'll do all those as a followup too :)
> 
> Ooo absolutely!  I actually wanted to create implicit conversions between `WTF::Seconds` and `NSTimeInterval` (just like `WTF::String` and `NSString`) so I'll do all those as a followup too :)

An implicit conversion to and from NSTimeInterval is *not* a good idea, because NSTimeInterval is just a typedef for double. Like CFTypeRef, it seems to be a type but is really just syntactic sugar and so doesn’t provide the safety and opportunity for overloading and conversion that it would if it was a separate type.
Comment 13 Devin Rousso 2021-12-03 14:00:52 PST
Created attachment 445895 [details]
Patch

removed unnecessary changes
Comment 14 Devin Rousso 2021-12-03 14:35:21 PST
Created attachment 445900 [details]
Patch
Comment 15 Devin Rousso 2021-12-03 14:45:32 PST
Created attachment 445904 [details]
Patch
Comment 16 Devin Rousso 2021-12-03 14:58:10 PST
Created attachment 445906 [details]
Patch
Comment 17 EWS 2021-12-06 12:04:31 PST
Committed r286560 (244889@main): <https://commits.webkit.org/244889@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445906 [details].
Comment 18 Radar WebKit Bug Importer 2021-12-06 12:05:25 PST
<rdar://problem/86116947>