Skip to content

DOMTimeStamp confusion #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
domenic opened this issue Nov 26, 2013 · 22 comments · Fixed by #1021
Closed

DOMTimeStamp confusion #2

domenic opened this issue Nov 26, 2013 · 22 comments · Fixed by #1021
Labels
☕☕ difficulty:medium Hard to fix ⌛⌛⌛ duration:long There goes your week-end

Comments

@domenic
Copy link
Member

domenic commented Nov 26, 2013

DOMTimeStamp has a few questionable things I was hoping to get cleared up:

  • Because it's unsigned long long, it cannot represent dates before 1970. Is that a desired property for a timestamp?
  • It says that it can be either absolute or relative. Do any specs use it for relative timestamps? I have not found any in a quick Google search.
  • Why does it exist? This is probably part of a larger conversation regarding the utility of WebIDL's various number types, but, it was pretty confusing to find DOMTimeStamp when I could have just found unsigned long long (or ideally number, which would fix the 1970 restriction).

I'd love to discuss the general problem of number types in WebIDL after I've had time to put together a more concrete proposal, but I am wondering if there's anything we can do to clean up the DOMTimeStamp corner of this mess.

@heycam
Copy link
Collaborator

heycam commented Nov 27, 2013

DOMTimeStamp exists because it was in DOM 2 Core and was used as the type of Event.timeStamp. Is it being used elsewhere?

We could redefine it to being a double and thus a JS Number value, and give it the same semantics as a JS Date's value, but relative to the current timezone.

@heycam
Copy link
Collaborator

heycam commented Nov 27, 2013

What people want from a timestamp type is a good question to ask, though. :)

@travisleithead
Copy link
Member

EME is now also trying to use DOMTimeStamp (for absolute time comparisons, I believe). w3c/encrypted-media#59

@travisleithead
Copy link
Member

Can we recommend to the EME folks to just use unsigned long long for now?

@domenic
Copy link
Member Author

domenic commented Nov 17, 2015

No, we should recommend using the correct types and then we can fix up the type definitionsin one place to benefit everyone.

@annevk
Copy link
Member

annevk commented Aug 2, 2021

DOM no longer uses this. Geolocation API does though.

@marcoscaceres
Copy link
Member

We could transition Geolocation to using a relative timestamp (or just inline "unsigned long long" over on Geo to get rid of the typedef). I'm pretty open to whatever folks think is best. My worry from the geo side is that devs will be relying on the current timestamp to make decisions about asking for a position refresh.

@whatwg whatwg deleted a comment Aug 3, 2021
@whatwg whatwg deleted a comment Aug 3, 2021
@annevk
Copy link
Member

annevk commented Aug 3, 2021

I think it's fine to rely on this, though it should maybe move out of IDL. Perhaps we should put all web platform time things in hr-time to consolidate on that and also have a single place to make recommendations with regards which abstraction to pick.

cc @yoavweiss

@yoavweiss
Copy link

Makes sense to me to have all time related definitions under a single spec. At the same time, it may expand the scope of the spec and I need to make sure WG folks are ok with that. I can bring it up in the next call.

@yoavweiss
Copy link

yoavweiss commented Aug 26, 2021

The WebPerfWG discussed this last week. We'd be fine with moving the definition over if we can do that without renaming HR-time nor changing its scope in the charter. Would that work?

@annevk
Copy link
Member

annevk commented Aug 30, 2021

That seems fine. It's not clear to me it can be changed to a double though. Note that the other thing here would be to take some ownership of the questions raised in #2 (comment).

@yoavweiss
Copy link

Looking at users of this, I found:

Some of those seem pretty malleable still, but others are sufficiently baked in. At the same time, I'm not sure what breakage would look like if we switched them over to DOMHighResTimestamp, which is a double.

@mkruisselbrink
Copy link

Did somebody file an issue/PR against the tag design principles as well? https://w3ctag.github.io/design-principles/#times-and-dates says after all "When representing date-times on the platform, use the DOMTimeStamp type."

wacky6 added a commit to wacky6/handwriting-recognition that referenced this issue Sep 2, 2021
wacky6 added a commit to WICG/handwriting-recognition that referenced this issue Sep 2, 2021
@marcoscaceres
Copy link
Member

I'd like to consolidate the suggestions above and formalize this as:

  • If possible, WICG specs -> DOMHighResTimestamp (move on this quickly - @yoavweiss already filed the bugs, so we are good).
  • Move DOMTimestamp -> HR Time - and freeze the definition as "the number of milliseconds since 1970-01-01T00:00:00Z." (or similar, whichever spec defines it best).
  • Rename it to LegacyDOMTimestamp.
  • Update Geo, Push, Notifications, and WebRTC.
  • Update TAG Guidance.

Anything else?

@annevk
Copy link
Member

annevk commented Sep 7, 2021

If we are going to rename it, let's remove the DOM prefix. That went out of fashion quite a while ago. So just LegacyTimestamp.

@marcoscaceres
Copy link
Member

I put up a draft at w3c/hr-time#124 for review, plus and sent #1021.

I'll send some PRs for Geo and Push.

@yoavweiss
Copy link

whatwg/notifications#171 is at least one case where compat would block us from moving all cases to DOMHighResTimestamp.

@marcoscaceres
Copy link
Member

We've put up a proposal for "EpochTimeStamp" at w3c/hr-time#124 ... would be great to get some feedback from folks here.

@annevk annevk removed the enhancement label Oct 5, 2021
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 5, 2021

Would changing DOMTimeStamp from uint64/unsigned long long to int64/long long be an acceptable breaking change?

@annevk
Copy link
Member

annevk commented Oct 5, 2021

@ExE-Boss I don't think so. Why would we want to do that?

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Oct 11, 2021
https://bugs.webkit.org/show_bug.cgi?id=231496

Reviewed by Sam Weinig.

Source/WebCore:

DOMTimeStamp was renamed EpochTimeStamp. There is no observable behavioral change - it's just a name change.

Relevant WebIDL discussions/issue:
- whatwg/webidl#2

Which lead to:
- w3c/hr-time#124

* Headers.cmake:
* Modules/geolocation/Geolocation.cpp:
(WebCore::createGeolocationPosition):
(WebCore::Geolocation::haveSuitableCachedPosition):
* Modules/geolocation/GeolocationPosition.h:
(WebCore::GeolocationPosition::create):
(WebCore::GeolocationPosition::timestamp const):
(WebCore::GeolocationPosition::GeolocationPosition):
* Modules/geolocation/GeolocationPosition.idl:
* Modules/notifications/Notification.idl:
* Modules/push-api/PushSubscription.cpp:
(WebCore::PushSubscription::PushSubscription):
(WebCore::PushSubscription::expirationTime const):
* Modules/push-api/PushSubscription.h:
* Modules/push-api/PushSubscription.idl:
* Modules/push-api/PushSubscriptionJSON.h:
* Modules/push-api/PushSubscriptionJSON.idl:
* Modules/push-api/PushSubscriptionOptions.h:
* WebCore.xcodeproj/project.pbxproj:
* bindings/scripts/IDLParser.pm:
(addBuiltinTypedefs):
* bindings/scripts/test/TestTypedefs.idl:
* dom/EpochTimeStamp.h: Renamed from Source/WebCore/dom/DOMTimeStamp.h.
(WebCore::convertSecondsToEpochTimeStamp):
(WebCore::convertEpochTimeStampToSeconds):
* testing/Internals.cpp:
(WebCore::Internals::createPushSubscription):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKitLegacy/mac:

* DOM/DOMEvent.h:
* DOM/DOMEvent.mm:
(-[DOMEvent timeStamp]):
* DOM/DOMObject.h:

Source/WebKitLegacy/win:

* DOMEventsClasses.cpp:
(DOMEvent::timeStamp):
* DOMEventsClasses.h:
(DOMUIEvent::timeStamp):
(DOMKeyboardEvent::timeStamp):
(DOMMouseEvent::timeStamp):
(DOMMutationEvent::timeStamp):
(DOMOverflowEvent::timeStamp):
(DOMWheelEvent::timeStamp):
* Interfaces/DOMEvents.idl:


Canonical link: https://commits.webkit.org/242783@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283912 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@saschanaz
Copy link
Member

What's the remaining work item here to keep this open as w3c/hr-time#124 is merged?

@annevk
Copy link
Member

annevk commented Oct 13, 2021

#1021.

domenic pushed a commit that referenced this issue Oct 19, 2021
Closes #2. It has been renamed to EpochTimeStamp and moved to https://w3c.github.io/hr-time/#the-epochtimestamp-typedef.
bertogg pushed a commit to Igalia/webkit that referenced this issue Oct 19, 2021
https://bugs.webkit.org/show_bug.cgi?id=231496

Reviewed by Sam Weinig.

Source/WebCore:

DOMTimeStamp was renamed EpochTimeStamp. There is no observable behavioral change - it's just a name change.

Relevant WebIDL discussions/issue:
- whatwg/webidl#2

Which lead to:
- w3c/hr-time#124

* Headers.cmake:
* Modules/geolocation/Geolocation.cpp:
(WebCore::createGeolocationPosition):
(WebCore::Geolocation::haveSuitableCachedPosition):
* Modules/geolocation/GeolocationPosition.h:
(WebCore::GeolocationPosition::create):
(WebCore::GeolocationPosition::timestamp const):
(WebCore::GeolocationPosition::GeolocationPosition):
* Modules/geolocation/GeolocationPosition.idl:
* Modules/notifications/Notification.idl:
* Modules/push-api/PushSubscription.cpp:
(WebCore::PushSubscription::PushSubscription):
(WebCore::PushSubscription::expirationTime const):
* Modules/push-api/PushSubscription.h:
* Modules/push-api/PushSubscription.idl:
* Modules/push-api/PushSubscriptionJSON.h:
* Modules/push-api/PushSubscriptionJSON.idl:
* Modules/push-api/PushSubscriptionOptions.h:
* WebCore.xcodeproj/project.pbxproj:
* bindings/scripts/IDLParser.pm:
(addBuiltinTypedefs):
* bindings/scripts/test/TestTypedefs.idl:
* dom/EpochTimeStamp.h: Renamed from Source/WebCore/dom/DOMTimeStamp.h.
(WebCore::convertSecondsToEpochTimeStamp):
(WebCore::convertEpochTimeStampToSeconds):
* testing/Internals.cpp:
(WebCore::Internals::createPushSubscription):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKitLegacy/mac:

* DOM/DOMEvent.h:
* DOM/DOMEvent.mm:
(-[DOMEvent timeStamp]):
* DOM/DOMObject.h:

Source/WebKitLegacy/win:

* DOMEventsClasses.cpp:
(DOMEvent::timeStamp):
* DOMEventsClasses.h:
(DOMUIEvent::timeStamp):
(DOMKeyboardEvent::timeStamp):
(DOMMouseEvent::timeStamp):
(DOMMutationEvent::timeStamp):
(DOMOverflowEvent::timeStamp):
(DOMWheelEvent::timeStamp):
* Interfaces/DOMEvents.idl:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283912 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☕☕ difficulty:medium Hard to fix ⌛⌛⌛ duration:long There goes your week-end
Development

Successfully merging a pull request may close this issue.

10 participants