Bug 66610 – [chromium] Implement CCThread in terms of WebThread

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Implement CCThread in terms of WebThread

[Nat Duca]

Created attachment 104594 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104594 [details]
Patch

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

[James Robinson]

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?

[James Robinson]

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.

[Nat Duca]

postDelayedTask for implementing dead reckoning, i.e. I just drew, now wait 16-drawtime ms before drawing again. :)

[Darin Fisher (:fishd, Google)]

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.

[Nat Duca]

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

[Nat Duca]

Created attachment 104685 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104685 [details]
Patch

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

[Nat Duca]

Created attachment 104858 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Nat Duca]

Created attachment 104940 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 104940 [details]
Patch for landing

Clearing flags on attachment: 104940

Committed r93680: <http://trac.webkit.org/changeset/93680>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.