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

Fixes additional calls to rAF. #9401

Merged
merged 3 commits into from
Jan 28, 2016
Merged

Fixes additional calls to rAF. #9401

merged 3 commits into from
Jan 28, 2016

Conversation

glennw
Copy link
Member

@glennw glennw commented Jan 22, 2016

Often, a rAF callback will request another rAF from the callback itself.

Previously, the constellation would quickly receive two messages saying
that there were no animations, and then there are animations again in the
situation above. This would make the compositor tick the new animation straight
away, causing strange fluctuations and timings in rAF callbacks.

Instead, only send the NoAnimationCallbacks message if the animation
callback queue is still empty after invoking the callbacks.

This fixes rAF timing, which now runs at the correct (vsync) framerate.

Review on Reviewable

Often, a rAF callback will request another rAF from the callback itself.

Previously, the constellation would quickly receive two messages saying
that there were no animations, and then there are animations again in the
situation above. This would make the compositor tick the new animation straight
away, causing strange fluctuations and timings in rAF callbacks.

Instead, only send the NoAnimationCallbacks message if the animation
callback queue is still empty after invoking the callbacks.

This fixes rAF timing, which now runs at the correct (vsync) framerate.
@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 22, 2016
@glennw
Copy link
Member Author

glennw commented Jan 22, 2016

r? @jdm

I don't think there's any way to add a reliable test for this - can you think of a way?

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 22, 2016

Maybe add a comment about the interactions here? It's not quite obvious on first sight that the message needs to be sent after the callbacks.

@glennw
Copy link
Member Author

glennw commented Jan 26, 2016

Added comment to the code explaining the ordering.

r? @jdm

@jdm
Copy link
Member

jdm commented Jan 26, 2016

@bors-servo: r+
Lovely!

@bors-servo
Copy link
Contributor

📌 Commit de81d88 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit de81d88 with merge 58d60df...

bors-servo pushed a commit that referenced this pull request Jan 26, 2016
Fixes additional calls to rAF.

Often, a rAF callback will request another rAF from the callback itself.

Previously, the constellation would quickly receive two messages saying
that there were no animations, and then there are animations again in the
situation above. This would make the compositor tick the new animation straight
away, causing strange fluctuations and timings in rAF callbacks.

Instead, only send the NoAnimationCallbacks message if the animation
callback queue is still empty after invoking the callbacks.

This fixes rAF timing, which now runs at the correct (vsync) framerate.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9401)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 26, 2016
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 27, 2016
@jdm
Copy link
Member

jdm commented Jan 27, 2016

Tests with unexpected results:
  ▶ TIMEOUT [expected PASS] /_mozilla/css/transition_calc.html

The test uses rAF, so I'm suspicious.

…uired.

Fixes timeout in transition_calc.html.
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 28, 2016
@glennw
Copy link
Member Author

glennw commented Jan 28, 2016

@jdm The last commit fixes that test failure. It ensures that when compositing for a screenshot, animation ticks still occur once each frame is correctly painted.

@jdm
Copy link
Member

jdm commented Jan 28, 2016

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 6a85102 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 6a85102 with merge bc44ae6...

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jan 28, 2016
bors-servo pushed a commit that referenced this pull request Jan 28, 2016
Fixes additional calls to rAF.

Often, a rAF callback will request another rAF from the callback itself.

Previously, the constellation would quickly receive two messages saying
that there were no animations, and then there are animations again in the
situation above. This would make the compositor tick the new animation straight
away, causing strange fluctuations and timings in rAF callbacks.

Instead, only send the NoAnimationCallbacks message if the animation
callback queue is still empty after invoking the callbacks.

This fixes rAF timing, which now runs at the correct (vsync) framerate.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9401)
<!-- Reviewable:end -->
@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 28, 2016
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants