Bug 71479

Created attachment 113507 [details] Patch

Comment on attachment 113507 [details] Patch Attachment 113507 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10290013

Comment on attachment 113507 [details] Patch Can we keep time off of the thread for now? This has proved a bit controversial here --- there's a good win here just for the fake thread components, and we can pull the time discussion out to another cleanup. Afict, there is very very little code that actually needs time anyway.

Er, would help if I actually added all the files... Yeah, I'm leaving monotonicallyIncreasingTime() unchanged, except for piping it through to FakeCCThread::getTime() in the existing code. FakeCCThread needs to own a time so it knows when to run tasks.

Created attachment 113520 [details] Patch

The current patch is intended to "keep time off the thread" as much as possible, but let me know if you think further changes are needed. I don't see any way to remove FakeCCThread::getTime and FakeCCThread::setTime, but they're only in the subclass, not CCThread itself.

Comment on attachment 113520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113520&action=review > Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:41 > +TEST(CCDelayBasedTimeSourceTest, TaskPostedAndtickCalled) uppercase t > Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:-92 > - EXPECT_FALSE(thread.hasPendingTask()); why remove this? > Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:-107 > - for (int i = 0; i < numIterations; ++i) { > - long long delay = thread.pendingDelay(); I'm really confused here, setTime causes the message loop to tick? I don't like that at all, it might make simpler code but its very hard to reason about what's going on -- much less set a breakpoint. Manipulating the fake thread's state should not run tasks. > Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:122 > + thread.setTime(3000); why set the time here?

Comment on attachment 113520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113520&action=review >> Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:-107 >> - long long delay = thread.pendingDelay(); > > I'm really confused here, setTime causes the message loop to tick? I don't like that at all, it might make simpler code but its very hard to reason about what's going on -- much less set a breakpoint. Manipulating the fake thread's state should not run tasks. I'm convinced that this approach is a win for more complex tests, but setTime() is definitely a misleading name. How about if it were called runTasksUntilTime()? >> Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:122 >> + thread.setTime(3000); > > why set the time here? Just threw it in because it was easy, but I guess it doesn't give any extra coverage. :)

(In reply to comment #8) > I'm convinced that this approach is a win for more complex tests, but setTime() is definitely a misleading name. > > How about if it were called runTasksUntilTime()? That's fine, I buy that for other use cases its useful. Is there a way to support the use case I prefer, too? I'd like to keep the way delaybasedtimesource interval test as written before, because I was meaning to write a different test that adds noise to the time.

Hmm, sure, I can add pendingDelay() back in.