Summary: | [chromium] Implement CCThread in terms of WebThread | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nat Duca <nduca> | ||||||||||
Component: | New Bugs | Assignee: | 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
Nat Duca
2011-08-19 16:42:23 PDT
Created attachment 104594 [details]
Patch
Comment on attachment 104594 [details] Patch Attachment 104594 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9432522 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? 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. postDelayedTask for implementing dead reckoning, i.e. I just drew, now wait 16-drawtime ms before drawing again. :) 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. (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. Created attachment 104685 [details]
Patch
Comment on attachment 104685 [details] Patch Attachment 104685 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9464800 Created attachment 104858 [details]
Patch
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 Created attachment 104940 [details]
Patch for landing
Comment on attachment 104940 [details] Patch for landing Clearing flags on attachment: 104940 Committed r93680: <http://trac.webkit.org/changeset/93680> All reviewed patches have been landed. Closing bug. |