-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Handle deferred processing when document
is hidden
#402
Conversation
9f078b6
to
7dcd334
Compare
document
is hidden
16c2f2f
to
a02d471
Compare
ba4d372
to
ee26730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
d226865
to
e764dfa
Compare
What are the nuances of this? If a window is visible, and I Also: Do we know what was the motivation for choosing |
I think the primary original motivation wasn't about consideration of system resources so much as it was about responsiveness. Consider this example somebody created that is very illustrative: https://codepen.io/YOONBYEONGIN/pen/RwxrEPN It seems pretty unlikely that you could hide the window in the time between when you invoke |
@afshin It might be unlikely, but if it happens once a month for a heavy user, and the consequence is catastrophic, then it still matters. So the question the becomes, what will be the failure mode if one callback gets stuck waiting on an animation frame while other callbacks keep firing with timeouts? Do we end up processing messages out of order? I don't think "probably nothing bad will happen" is an ok answer in this case. Waiting a bit for something after resuming a browser tab is in general a more ok user experience than undefined behavior (you can fit a lot into "undefined behavior"). |
I should have stated that more strongly: you have <1ms to switch tabs from the time you have invoked an animation frame to the time it fires. The only time I'll give it some more thought. Does anyone else who has considered this part of the code base have any thoughts? |
Hm. You're right, @vidartf. This is possible. I put the PR back in draft mode. Perhaps client code needs to specify whether it wants to run when the document is hidden (slower because it is |
12dd394
to
6a11f9f
Compare
4ed8402
to
b096c8e
Compare
b096c8e
to
a43acf4
Compare
After a lot of back and forth, this is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @afshin
I have two comments.
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@afshin should we move forward with this one? |
rejected = true; | ||
}; | ||
} | ||
)(Promise.resolve()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in last weeks lab dev meeting, using a Promise or async will schedule the task as a microtask, compared to requestAnimationFrame
/ setTimeout
etc that will schedule it as a macro task. This difference is likely to be significant (@afshin has the details).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is safe in this circumstance because message propagation is both lightweight and synchronous. This is the reason why the same technique is unavailable to us in Poll
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does the following.
when-hidden
standby to tick once after being hidden (the defaultoptions.linger
value of1
time can be overridden at instantiation time)MessageLoop
to use a new promise-based implementation ofschedule
for immediate deferral to support always running in background tabsPoll
to export a protectedhidden
attribute to indicate to sub-classes when the document is hidden.We can backport this to 1.x as well.
CC: @thetorpedodog – I kept your suggested
setTimeout
forPoll
instances because almost all polls have a nonzerointerval
anyway and it simplifies that code. I updatedMessageLoop
to work in the background without having to resort tosetTimeout
.Fixes #400