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

Use current wall time instead of the one on environment settings object #858

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

linnan-github
Copy link
Collaborator

@linnan-github linnan-github commented Jun 22, 2023

The implementation uses base::Time::Now() which doesn't consider cross origin isolated contexts as well.


Preview | Diff

@apasel422
Copy link
Collaborator

Please add a reviewer who is familiar with the High Resolution Time spec so we can get some feedback on what the correct use is.

@yoavweiss
Copy link
Contributor

The implementation uses base::Time::Now()

It seems fine to move to more coarsened times by default, especially given that it seems the requirement time granularity you require here is very coarse. It's worthwhile to verify the implementation is similarly coarsening the timestamps correctly.

Can you think of ways to write a WPT that tests that? (not a blocker, but would be great to have)

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@linnan-github
Copy link
Collaborator Author

linnan-github commented Jun 22, 2023

The implementation uses base::Time::Now()

It seems fine to move to more coarsened times by default, especially given that it seems the requirement time granularity you require here is very coarse. It's worthwhile to verify the implementation is similarly coarsening the timestamps correctly.

Can you think of ways to write a WPT that tests that? (not a blocker, but would be great to have)

Thanks, we will look into the implementation to verify whether the time is similarly coarsened. Currently WPT may not support tests like this yet, but could be possible with the web driver support.

@linnan-github linnan-github merged commit 8b0014b into WICG:main Jun 22, 2023
@linnan-github linnan-github deleted the currentWallTime branch June 22, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants