-
Notifications
You must be signed in to change notification settings - Fork 19
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
Address some TAG review feedback. #32
Conversation
Addresses some of w3c#31
@dbaron, @igrigorik, @toddreifsteck, please take a look and let me know if you have any comments, thanks! |
@rmcilroy Curious why you dropped the Also, the "who's timeout occurred before" seems rather less precise than it ought to be; it should probably say explicitly what |
Apparently 'optional' is not allowed in a dictionary in IDL (dictionary properties are optional by default), so this was fixing an IDL bug (see #30 (comment)).
Done in the latest patch. |
Is this change good to land? |
@@ -267,7 +267,7 @@ | |||
<li>If the <var>timeout</var> property is present in <var>options</var> and has a positive value: | |||
<ol type='a'> | |||
<li>Wait for <var>timeout</var> milliseconds.</li> |
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.
Hmm, should there be a hook in here somewhere to cancel the timer steps if and when the callback is executed before these timers fire?
Good point! Added a line in the latest patch to ensure we only call the timeout callback algorithm if the handle is still in the list of callbacks (it would get removed if it was run during an idle period, or explicitly cancelled) |
Actually, I just realized, the timeout callback algorithm already does this check, so the additional check at that line isn't really necessary. We could either keep both checks (for clarity) or remove on or the other, whichever you think makes things clearer. |
Ahh, I completely missed the point of the first step there.. I guess there is no reason to keep it in both places. Is there any merit in doing that check in the new location, instead of after the task is queued? I'll defer to your judgement here. |
I removed the change for the timeout check in the requestIdleCallback algorithm - I think it makes more sense to do it where it originally was in the timeout algorithm. |
sg. With that resolved, LGTM. @dbaron @toddreifsteck any other feedback on this? |
@dbaron @toddreifsteck can you guys please review this? |
LGTM |
Address some TAG review feedback.
Addresses some of #31