Skip to content

Sync RequestAnimationFrameCallback with WebIDL interface #46786

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 27, 2025
Merged

Sync RequestAnimationFrameCallback with WebIDL interface #46786

merged 1 commit into from
Jun 27, 2025
+6 −1

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Jun 15, 2025 •
edited by webkit-early-warning-system
Loading

cad7439

Sync `RequestAnimationFrameCallback` with WebIDL interface
https://bugs.webkit.org/show_bug.cgi?id=283862

Reviewed by Chris Dumez.

This patch aligns WebKit with Web Specification [1]:

[1] https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames

It fixes where 'highResTime' was 'unrestricted double', which is not same
as 'double', since earlier can allow values like non-finite, and special
"not a number" values (NaNs). This matches what specification says and
also other browser implementation as well.

* Source/WebCore/dom/RequestAnimationFrameCallback.idl:

Canonical link: https://commits.webkit.org/296745@main

5f4dad7

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 requested review from rniwa and cdumez as code owners June 15, 2025 22:29
@Ahmad-S792 Ahmad-S792 self-assigned this Jun 15, 2025
@Ahmad-S792 Ahmad-S792 added the Animations Bugs related to CSS + SVG animations and transitions label Jun 15, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 15, 2025 •
edited
Loading

EWS run on current version of this PR (hash 5f4dad7)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 requested a review from graouts June 16, 2025 01:21
@graouts graouts requested review from smfr, shallawa and annevk June 18, 2025 05:55
@annevk
Copy link
Contributor

annevk commented Jun 18, 2025

Your commit message makes it sound like this change is observable, but is it? I suspect it's not as the engine is responsible for invoking this callback we probably never invoke it with a value it's not supposed to have.

It then raises the question as to whether this incurs some kind of additional branching cost at the binding layer. And as I have a hard time reading the binding code I'm not sure I can answer that.

@annevk annevk requested review from youennf and removed request for annevk June 18, 2025 06:47
@Ahmad-S792
Copy link
Contributor Author

Your commit message makes it sound like this change is observable, but is it? I suspect it's not as the engine is responsible for invoking this callback we probably never invoke it with a value it's not supposed to have.

It then raises the question as to whether this incurs some kind of additional branching cost at the binding layer. And as I have a hard time reading the binding code I'm not sure I can answer that.

This is not observable, I created test to see if it is working pre and post to simulate NaN, +Inf, -Inf and it seems to work fine in current implementation as well. It is now just to align 'idl' part with specification.

@Ahmad-S792 Ahmad-S792 added the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
@Ahmad-S792
Sync RequestAnimationFrameCallback with WebIDL interface
cad7439 
https://bugs.webkit.org/show_bug.cgi?id=283862

Reviewed by Chris Dumez.

This patch aligns WebKit with Web Specification [1]:

[1] https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames

It fixes where 'highResTime' was 'unrestricted double', which is not same
as 'double', since earlier can allow values like non-finite, and special
"not a number" values (NaNs). This matches what specification says and
also other browser implementation as well.

* Source/WebCore/dom/RequestAnimationFrameCallback.idl:

Canonical link: https://commits.webkit.org/296745@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/RequestAnimationFrameCallback-should-not-be-unrestricted-double branch from 5f4dad7 to cad7439 Compare June 27, 2025 19:40
@webkit-commit-queue
Copy link
Collaborator

Committed 296745@main (cad7439): https://commits.webkit.org/296745@main

Reviewed commits have been landed. Closing PR #46786 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
@webkit-commit-queue webkit-commit-queue merged commit cad7439 into WebKit:main Jun 27, 2025
@Ahmad-S792 Ahmad-S792 deleted the eng/RequestAnimationFrameCallback-should-not-be-unrestricted-double branch June 27, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@graouts graouts Awaiting requested review from graouts

@smfr smfr Awaiting requested review from smfr

@shallawa shallawa Awaiting requested review from shallawa

@youennf youennf Awaiting requested review from youennf

Assignees

@Ahmad-S792 Ahmad-S792

Labels
Animations Bugs related to CSS + SVG animations and transitions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ahmad-S792 @webkit-early-warning-system @annevk @webkit-commit-queue @cdumez