Bug 71479 – [chromium] Beef up FakeCCThread for easier compositor testing

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Iain Merrick]

[chromium] Beef up FakeCCThread for easier compositor testing

[Iain Merrick]

Created attachment 113507 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113507 [details]
Patch

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

[Nat Duca]

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.

[Iain Merrick]

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.

[Iain Merrick]

Created attachment 113520 [details]
Patch

[Iain Merrick]

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.

[Nat Duca]

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?

[Iain Merrick]

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. :)

[Nat Duca]

(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.

[Iain Merrick]

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