search

w3c / requestidlecallback

Cooperative Scheduling of Background Tasks
https://w3c.github.io/requestidlecallback/
Other
50 stars 19 forks source link

Address some TAG review feedback. #32

Closed rmcilroy closed 8 years ago

rmcilroy commented 8 years ago

Addresses some of #31

rmcilroy commented 8 years ago

@dbaron, @igrigorik, @toddreifsteck, please take a look and let me know if you have any comments, thanks!

dbaron commented 8 years ago

@rmcilroy Curious why you dropped the optional from the timeout in IdleRequestOptions.

Also, the "who's timeout occurred before" seems rather less precise than it ought to be; it should probably say explicitly what timeout (if present) is added to. (Also, "who's" -> "whose".)

rmcilroy commented 8 years ago

@rmcilroy Curious why you dropped the optional from the timeout in IdleRequestOptions.

Apparently 'optional' is not allowed in a dictionary in IDL (dictionary properties are optional by default), so this was fixing an IDL bug (see https://github.com/w3c/requestidlecallback/pull/30#issuecomment-148805790).

Also, the "who's timeout occurred before" seems rather less precise than it ought to be; it should probably say explicitly what timeout (if present) is added to. (Also, "who's" -> "whose".)

Done in the latest patch.

rmcilroy commented 8 years ago

Is this change good to land?

rmcilroy commented 8 years ago

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)

rmcilroy commented 8 years ago

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.

igrigorik commented 8 years ago

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.

rmcilroy commented 8 years ago

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.

igrigorik commented 8 years ago

sg. With that resolved, LGTM. @dbaron @toddreifsteck any other feedback on this?

igrigorik commented 8 years ago

@dbaron @toddreifsteck can you guys please review this?

toddreifsteck commented 8 years ago

LGTM