[webkit-reviews] review denied: [Bug 56131] [chromium] Compositor thread infrastructure : [Attachment 88594] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 56131: [chromium] Compositor thread infrastructure
https://bugs.webkit.org/show_bug.cgi?id=56131

Attachment 88594: Patch
https://bugs.webkit.org/attachment.cgi?id=88594&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88594&action=review

Just a few things to address. Thanks for making it fit in better. (I will be
starting my effort to clean up threading soon, and having this fit the patterns
should make it easier to change more quickly and get rid of boilerplate code --
it may take a while to complete this clean up.)

> Source/WebCore/platform/graphics/chromium/cc/CCMainThread.h:32
> +// CC doesn't have access to ScriptExecutionContxt so we have to duplicate
this

If you want to say this, the best place is the ChangeLog. typo:
ScriptExecutionContxt

> Source/WebCore/platform/graphics/chromium/cc/CCMainThread.h:33
> +// functionality. These tasks route to the webkit main loop

The last sentence is incomplete.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:56
> +CCThread::~CCThread()

I feel like the ownership model for CCThread isn't as clear as it should be.

For example, suppose that the ref counts when away completely on the main
thread and the only ref count was on the thread, then this destructor would be
called on the compositor thread, which is clearly wrong.

This implies that really CCThread should be owned on the main thread and I
suspect there is one place. Looking at your overall patch, I suspect that
CCViewManager is the real owner of CCThread.

So I would change getInstance to return a CCThread* and make CCViewManager own
it.  May there be multiple instances of CCViewManager? (If so, perhaps that is
what you are trying to handle, but in that case it still feels like giving out
PassRefPtr from getInstance feels like overkill.)

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:65
> +    // Slear singleton.

typo: Slear

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:92
> +	   MutexLocker lock(m_threadCreationMutex);

Nice. I was about to flag this issue above.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:46
> +    // These functions are thread safe and can be called from anywhere.

This comment appears misplaced.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:74
> +class CCCompletionEvent {

Seems deserving of its own header.

> Source/WebKit/chromium/tests/CCThreadTest.cpp:27
> +#include "cc/CCThread.h"

This cc/ shouldn't be needed. If it is, then the build should be changed so it
isn't. As far as I know, directory structure is never used in includes for
WebKit header files.

> Source/WebKit/chromium/tests/CCThreadTest.cpp:53
> +    WTF::ThreadIdentifier hitThreadID;

You shouldn't need WTF:: here.