[webkit-reviews] review denied: [Bug 66145] [chromium] Assert that main thread and compositor thread are used safely : [Attachment 103838] Cleanup. Don't declare ScopedFakeCompositorThread if THREADED_COMPOSITOR is enabled.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Iain Merrick
<husky at google.com>'s request for review:
Bug 66145: [chromium] Assert that main thread and compositor thread are used
safely
https://bugs.webkit.org/show_bug.cgi?id=66145

Attachment 103838: Cleanup. Don't declare ScopedFakeCompositorThread if
THREADED_COMPOSITOR is enabled.
https://bugs.webkit.org/attachment.cgi?id=103838&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103838&action=review


Please fix the style issue the bot caught and comments.  I also think you
should make sure that this all compiles out in release mode, since the only
real side effect is ASSERT()s which are debug only.  You might have to be
creative to avoid adding too many #ifndef NDEBUG lines, but it should be
possible.  You don't need to guard ASSERT()s since they already compile out.

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:46
> +    ASSERT(ccThreadID == 0);

should be ASSERT(!ccThreadID)

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:92
> +

nit: extra newline

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:97
> +

nit: extra newline

> WebCore/platform/graphics/chromium/cc/CCThread.h:36
> +// True if we're on the compositor thread.
> +bool isCCThread();

I don't think it's a good idea to put this directly in the WebCore namespace. 
Can you make this a static function on CCThread, just so that it's namespaced
more tightly?