Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upTimers clean up #8603
Timers clean up #8603
Conversation
highfive
commented
Nov 19, 2015
|
|
|
|
|
Dreadfully sorry for the long delay. I started this review several times and it always turned out to be more complex than I expected, and then there was a week long conference and seasonal holidays... Reviewed 5 of 5 files at r1. components/script/dom/window.rs, line 425 [r1] (raw file): components/script/dom/window.rs, line 434 [r1] (raw file): components/script/dom/window.rs, line 448 [r1] (raw file): components/script/dom/window.rs, line 457 [r1] (raw file): components/script/timers.rs, line 144 [r1] (raw file): {
let mut timers = self.timers.borrow_mut();
let insertion_index = timers.binary_search(&timer).err().unwrap();
timers.insert(insertion_index, timer);
}
if self.is_next_timer(OneshotTimerHandler(new_handle)) {components/script/timers.rs, line 338 [r1] (raw file): components/script/timers.rs, line 366 [r1] (raw file): components/script/timers.rs, line 475 [r1] (raw file): Comments from the review on Reviewable.io |
|
Again, no need to apologize. ;) Branch was rebased. Review status: 0 of 5 files reviewed at latest revision, all discussions resolved. Comments from the review on Reviewable.io |
|
@bors-servo: r+ Reviewed 5 of 5 files at r2. Comments from the review on Reviewable.io |
|
|
Timers clean up This PR splits the `ActiveTimers` abstraction into - `OneshotTimers` for scheduling "arbitrary" oneshot timers, such as XHR timeouts, and - `JsTimers`, based on `OneshotTimers`, for scheduling JS timers (`setTimeout`/`setInterval`). The result is mich cleaner and the timer initialization steps now closely resemble the specification. **Notes** - The second and third commit are strictly renames and code rearrangements. - I'm not particularily happy with the `OneshotTimerCallback` enum and its circular dependency with `XHRTimeoutCallback`, but I couldn't come up with anything better. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8603) <!-- Reviewable:end -->
|
|
...curious |
|
|
|
Ah, the object graph contains the following reference cycle.
"Obviously" the derived I'll think about it some more. |
|
Well, I got rid of the cycle by turning the |
|
Eeep. I'll need to think about this problem when I don't have a headache. |
|
I thoughty about this again today. Originally I introduced the cycle because I didn't want to give So, how about I simply transfer ownership of |
|
Yes, I am in favour of removing the cycle and not having to think about this. Let me know if you'd like me to do the rebase and make the change! |
|
I will get to it tomorrow. Thanks for offering though. =) |
|
Alright, there was another cycle required to let repeating timers to update To facilitate reviewing the change I haven't squashed yet. Feel free to squash yourself or let me know and I'll do it. |
|
|
|
Looks good, although I might be tempted to expose set_js_timer_or_interval and clear_js_timer_or_interval instead of get_js_timers. While rebasing, could you make sure that the indentation is correct for method calls with arguments on multiple lines? |
The code was split into the following two abstractions. - OneshotTimers can be used to schedule arbitrary oneshot timers, such as XHR-Timeouts. - JsTimers (`setTimeout` and `setInterval`) which use OneshotTimers to schedule individual callbacks. With this change the implementation (of JsTimers in particular) is in much closer alignment with the specification.
Done and done. |
|
@bors-servo: r+ |
|
|
Timers clean up This PR splits the `ActiveTimers` abstraction into - `OneshotTimers` for scheduling "arbitrary" oneshot timers, such as XHR timeouts, and - `JsTimers`, based on `OneshotTimers`, for scheduling JS timers (`setTimeout`/`setInterval`). The result is mich cleaner and the timer initialization steps now closely resemble the specification. **Notes** - The second and third commit are strictly renames and code rearrangements. - I'm not particularily happy with the `OneshotTimerCallback` enum and its circular dependency with `XHRTimeoutCallback`, but I couldn't come up with anything better. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8603) <!-- Reviewable:end -->
|
|
|
Thanks @benschulz! |
benschulz commentedNov 19, 2015
This PR splits the
ActiveTimersabstraction intoOneshotTimersfor scheduling "arbitrary" oneshot timers, such as XHR timeouts, andJsTimers, based onOneshotTimers, for scheduling JS timers (setTimeout/setInterval).The result is mich cleaner and the timer initialization steps now closely resemble the specification.
Notes
OneshotTimerCallbackenum and its circular dependency withXHRTimeoutCallback, but I couldn't come up with anything better.