[webkit-reviews] review denied: [Bug 233781] Change IDL `Date` to be backed by `WallTime` to avoid confusion when converting to native dates : [Attachment 445875] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 233781: Change IDL `Date` to be backed by `WallTime` to avoid confusion
when converting to native dates
https://bugs.webkit.org/show_bug.cgi?id=233781

Attachment 445875: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 445875
  --> https://bugs.webkit.org/attachment.cgi?id=445875
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.