Bug 71479

Summary: [chromium] Beef up FakeCCThread for easier compositor testing
Product: WebKit Reporter: Iain Merrick <husky>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dglazkov, husky, jamesr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70955, 71511    
Attachments:
Description Flags
Patch
none
Patch none

Description Iain Merrick 2011-11-03 09:30:54 PDT
[chromium] Beef up FakeCCThread for easier compositor testing
Comment 1 Iain Merrick 2011-11-03 09:40:05 PDT
Created attachment 113507 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-03 09:46:32 PDT
Comment on attachment 113507 [details]
Patch

Attachment 113507 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10290013
Comment 3 Nat Duca 2011-11-03 10:38:51 PDT
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.
Comment 4 Iain Merrick 2011-11-03 10:44:37 PDT
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.
Comment 5 Iain Merrick 2011-11-03 10:49:15 PDT
Created attachment 113520 [details]
Patch
Comment 6 Iain Merrick 2011-11-03 11:51:42 PDT
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 7 Nat Duca 2011-11-03 12:26:41 PDT
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 8 Iain Merrick 2011-11-04 04:10:46 PDT
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. :)
Comment 9 Nat Duca 2011-11-04 10:35:28 PDT
(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.
Comment 10 Iain Merrick 2011-11-04 11:08:32 PDT
Hmm, sure, I can add pendingDelay() back in.