Bug 233781 – Change IDL `Date` to be backed by `WallTime` to avoid confusion when converting to native dates

• Component: New Bugs
• Status: RESOLVED FIXED

[Devin Rousso]

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`.

[Devin Rousso]

Created attachment 445766 [details]
Patch

[Sihui Liu]

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?

[Devin Rousso]

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 :)

[Devin Rousso]

Created attachment 445801 [details]
Patch

rebased and fixed typo in `WebCore::IDBKeyData::hash`

[Devin Rousso]

Created attachment 445875 [details]
Patch

[Darin Adler]

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.

[Devin Rousso]

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 :)

[Devin Rousso]

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 :)

[Devin Rousso]

Created attachment 445887 [details]
Patch

[Devin Rousso]

Created attachment 445889 [details]
Patch

[Darin Adler]

(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.

[Darin Adler]

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.

[Devin Rousso]

Created attachment 445895 [details]
Patch

removed unnecessary changes

[Devin Rousso]

Created attachment 445900 [details]
Patch

[Devin Rousso]

Created attachment 445904 [details]
Patch

[Devin Rousso]

Created attachment 445906 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86116947>