Bug 66610

Summary: [chromium] Implement CCThread in terms of WebThread
Product: WebKit Reporter: Nat Duca <nduca>
Component: New BugsAssignee: Nat Duca <nduca>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, jamesr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Nat Duca 2011-08-19 16:42:23 PDT
[chromium] Implement CCThread in terms of WebThread
Comment 1 Nat Duca 2011-08-19 16:42:45 PDT
Created attachment 104594 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-19 17:10:12 PDT
Comment on attachment 104594 [details]
Patch

Attachment 104594 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9432522
Comment 3 James Robinson 2011-08-19 17:26:28 PDT
Comment on attachment 104594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104594&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:50
> +    virtual void postTask(PassOwnPtr<Task>) = 0; // Executes the task on context's thread asynchronously.

s/context/compositor/, perhaps? not sure what 'context' would mean here

> Source/WebKit/chromium/public/WebThread.h:45
> +    virtual void postDelayedTask(Task*, int64_t delayMs) = 0;

out of curiosity, why do we need this?
Comment 4 James Robinson 2011-08-19 17:27:43 PDT
I think you need to edit Source/WebKit/chromium/WebKit.gypi to make this compile.  Otherwise looks reasonable, although I haven't followed WebThread too closely so Darin should probably take a look as well.
Comment 5 Nat Duca 2011-08-19 19:02:35 PDT
postDelayedTask for implementing dead reckoning, i.e. I just drew, now wait 16-drawtime ms before drawing again. :)
Comment 6 Darin Fisher (:fishd, Google) 2011-08-19 21:20:16 PDT
Comment on attachment 104594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104594&action=review

>> Source/WebKit/chromium/public/WebThread.h:45
>> +    virtual void postDelayedTask(Task*, int64_t delayMs) = 0;
> 
> out of curiosity, why do we need this?

int64 comes from base/basictypes.h, which webkit should not be depending on.
int64_t comes from stdint.h, which webkit can depend on :)

but the webkit API should use 'long long' instead as we don't have any
machinery in the webkit API headers to simulate int64_t on windows.

> Source/WebKit/chromium/src/CCThreadImpl.cpp:46
> +    GetThreadIDTask(WTF::ThreadIdentifier* result, WebCore::CCCompletionEvent* completion)

nit: no need for the WebCore:: prefix, and i suspect the WTF:: prefixes can be dropped too.

> Source/WebKit/chromium/src/CCThreadImpl.cpp:50
> +    virtual ~GetThreadIDTask() { }

do you want to worry about what happens if the destructor is called before run?
i believe it is possible for the chromium MessageLoop to dispose of an unrun
Task if for some reason it cannot run it.

> Source/WebKit/chromium/src/CCThreadImpl.cpp:66
> +    CCThreadTaskAdapter(PassOwnPtr<CCThread::Task> task) : m_task(task) { }

add whitespace between these functions as you did for GetThreadIDTask?

> Source/WebKit/chromium/src/CCThreadImpl.cpp:103
> +    completion.wait();

are you sure it is safe to destroy a CCCompletionEvent before .signal() returns?
i think the answer is yes, but i don't see this documented anywhere.
Comment 7 Nat Duca 2011-08-22 09:49:44 PDT
(In reply to comment #6)
> (From update of attachment 104594 [details])
> > +    virtual ~GetThreadIDTask() { }
> 
> do you want to worry about what happens if the destructor is called before run?

The pointers this class owns are stack allocated, so not much "bad" will happen in terms of leaks. This task only runs when we create the thread, and if this task doesn't run, the completion event won't signal and the main loop will hang. Should I be designing for the possibility that the thread can't initialize at all?


> > Source/WebKit/chromium/src/CCThreadImpl.cpp:103
> > +    completion.wait();
> 
> are you sure it is safe to destroy a CCCompletionEvent before .signal() returns?

Great point. The thing that keeps this safe is the mutex unlock on ~CCCompletionEvent, which coordinates with the MutexLocker on the singal function. I added documentation to this effect on the CCCompletionEvent so this behavior is clearer.
Comment 8 Nat Duca 2011-08-22 09:55:49 PDT
Created attachment 104685 [details]
Patch
Comment 9 WebKit Review Bot 2011-08-22 10:01:37 PDT
Comment on attachment 104685 [details]
Patch

Attachment 104685 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9464800
Comment 10 Nat Duca 2011-08-23 09:29:10 PDT
Created attachment 104858 [details]
Patch
Comment 11 Darin Fisher (:fishd, Google) 2011-08-23 10:14:01 PDT
Comment on attachment 104858 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104858&action=review

> Source/WebKit/chromium/src/CCThreadImpl.cpp:59
> +    WebCore::CCCompletionEvent* m_completion;

nit: can drop the WebCore:: prefix
Comment 12 Nat Duca 2011-08-23 17:20:00 PDT
Created attachment 104940 [details]
Patch for landing
Comment 13 WebKit Review Bot 2011-08-23 18:22:36 PDT
Comment on attachment 104940 [details]
Patch for landing

Clearing flags on attachment: 104940

Committed r93680: <http://trac.webkit.org/changeset/93680>
Comment 14 WebKit Review Bot 2011-08-23 18:22:42 PDT
All reviewed patches have been landed.  Closing bug.