Skip to content
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

Add the HostSystemUTCEpochNanoseconds abstract operation #9038

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

Ms2ger
Copy link
Member

@Ms2ger Ms2ger commented Mar 16, 2023

See tc39/proposal-temporal#1654

(See WHATWG Working Mode: Changes for more details.)


/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was not entirely obvious in the other PR, but this needs to directly invoke https://w3c.github.io/hr-time/#dfn-current-high-resolution-time. The primitives defined in that specification are what all web platform specifications hook into.

I also wonder if we can get more context on nsMinInstant and nsMaxInstant as performance.now() and other APIs don't have that.

(I do think it makes sense for HTML to define the implementation of the hook as HTML generally manages the hooks.)

@annevk annevk added the integration Better coordination across standards needed label Mar 16, 2023
@Ms2ger
Copy link
Member Author

Ms2ger commented Mar 17, 2023

I guess this was not entirely obvious in the other PR, but this needs to directly invoke https://w3c.github.io/hr-time/#dfn-current-high-resolution-time. The primitives defined in that specification are what all web platform specifications hook into.

That returns a relative time stamp like performance.now(), not a time relative to the unix epoch. The "current wall time" seems to make more sense, especially since HRT already says this is the clock ES uses.

I also wonder if we can get more context on nsMinInstant and nsMaxInstant as performance.now() and other APIs don't have that.

Temporal APIs "only" support the time range from JS Date (100 million days around the Unix epoch). See https://tc39.es/proposal-temporal/#sec-temporal-instant-range

@annevk
Copy link
Member

annevk commented Mar 17, 2023

Oh right, we should use https://w3c.github.io/hr-time/#dfn-epoch-relative-timestamp coupled with https://w3c.github.io/hr-time/#dfn-coarsen-time then. @noamr do you know why the former isn't prefixed with unsafe?

@Ms2ger
Copy link
Member Author

Ms2ger commented Mar 20, 2023

I think "epoch-relative timestamp" already returns a value at whole-millisecond precision, so no point in coarsening it by a few microseconds. Incidentally, this also makes it unsuitable for Temporal.

@noamr
Copy link
Contributor

noamr commented Mar 20, 2023

I think "epoch-relative timestamp" already returns a value at whole-millisecond precision, so no point in coarsening it by a few microseconds. Incidentally, this also makes it unsuitable for Temporal.

It returns a millisecond value at double precision. Still, probably not accurate enough for Temporal.

@annevk
Copy link
Member

annevk commented Mar 20, 2023

I see, the division of labor between hr-time and HTML doesn't strike me as ideal, but I also don't have a concrete suggestion. @jyasskin do you have any thoughts about this? I see that "epoch-relative timestamp" wasn't refactored to return a duration.

@Ms2ger
Copy link
Member Author

Ms2ger commented Mar 20, 2023

I think "epoch-relative timestamp" already returns a value at whole-millisecond precision, so no point in coarsening it by a few microseconds. Incidentally, this also makes it unsuitable for Temporal.

It returns a millisecond value at double precision. Still, probably not accurate enough for Temporal.

Are you reading "the number of milliseconds" as returning a fractional value? That doesn't seem like the obvious interpretation to me.

@noamr
Copy link
Contributor

noamr commented Mar 20, 2023

I think "epoch-relative timestamp" already returns a value at whole-millisecond precision, so no point in coarsening it by a few microseconds. Incidentally, this also makes it unsuitable for Temporal.

Apologies, you are correct.

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left "epoch-relative timestamp" as the interface to existing callers, so that I could land the refactoring without updating all callers. My recommendations for new users are in https://w3c.github.io/hr-time/#sec-tools, and I think this patch follows them.

In https://w3c.github.io/hr-time/#dfn-epoch-relative-timestamp, I think "the number of milliseconds" is ambiguous about whether it rounds to the nearest integer (safe) or returns a full-precision double (unsafe), and I should send a patch to say that it rounds.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@jyasskin
Copy link
Member

It looks like "epoch-relative timestamp" was only used in 1 spec, so instead of clarifying it, w3c/hr-time#153 suggests to delete it.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with nits. I tried fixing the nits myself and pushing, but I guess "allow edits from maintainers" was not checked or something; it gave me an error.

Indeed, I guess if Temporal is not really a ship-worthy proposal, we should not merge this yet. We generally include features in HTML that browsers are willing to ship, and somehow for this TC39 stage 3 feature I guess that is not the case?

So, I guess we'll mark this as "do not merge yet" until we get the PR template filled out, and the relevant marker removed from the Temporal spec. But if it helps unblock the TC39 side of things, please don't feel blocked on HTML editor review anymore. From our perspective it looks good, modulo the editorial nits.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Apr 12, 2023
@Ms2ger
Copy link
Member Author

Ms2ger commented Apr 12, 2023

Thanks! I'll ask @ptomato to clarify the shippability here.

I guess https://github.com/orgs/community/discussions/5634 is why you couldn't edit directly - I keep forgetting about that.

@ptomato
Copy link

ptomato commented Apr 12, 2023

Follow tc39/proposal-temporal#1450 for the status of the blocker on shippability. The current agreement is that Temporal remains behind a flag until this proposed standard for the calendar and time zone annotations used in serialization of Temporal objects is ratified by the IETF.

@nicolo-ribaudo
Copy link
Contributor

Note: tc39/proposal-temporal#1450, which was marked as a blocked for this PR, has been solved.

@annevk
Copy link
Member

annevk commented Jun 7, 2024

Indeed, as per #9038 (review) filling out the PR template appears to be what remains here.

@annevk
Copy link
Member

annevk commented Jun 10, 2024

Thanks for updating @Ms2ger!

I'm comfortable with landing this given the state of things explained in tc39/proposal-temporal#2628. @domenic?

@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 12, 2024
@annevk
Copy link
Member

annevk commented Jun 12, 2024

I've put this in my calendar to merge on June 19. Happy to do it sooner if @domenic or @syg can confirm things are okay from their side.

@annevk annevk merged commit c56e559 into whatwg:main Jun 19, 2024
2 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the temporal branch June 19, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Better coordination across standards needed
Development

Successfully merging this pull request may close these issues.

7 participants