[webkit-reviews] review denied: [Bug 66145] [chromium] Assert that main thread and compositor thread are used safely : [Attachment 104202] Moved isCCThread to CCLayerTreeHostImplProxy::isImplThread

[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 104202: Moved isCCThread to CCLayerTreeHostImplProxy::isImplThread
https://bugs.webkit.org/attachment.cgi?id=104202&action=review

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


The logic seems a little broken for the !USE(THREADED_COMPOSITOR) case. I also
don't think that having a helper class and whatnot is necessary.  Do the simple
thing.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:55
> +    static bool isMainThread();
> +    static bool isImplThread();

#ifndef NDEBUG around these?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:63
> +#if !USE(THREADED_COMPOSITING) && !defined(NDEBUG)
> +    // Pretend that we're on the impl thread for the lifetime of this
object.
> +    // Use this to make ASSERT(isImplThread()) valid when we're not
threaded.
> +    // TODO: CCLayerTreeHostImplProxy is going to be split into two classes,
one
> +    // threaded and one unthreaded. Move this feature to the unthreaded
class.
> +    class ScopedImplThread {
> +	  WTF_MAKE_NONCOPYABLE(ScopedImplThread);

Do we really need this?  Why not just a setter?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:141
> +bool CCLayerTreeHostImplProxy::isMainThread()
>  {
>      return ::isMainThread();
>  }
>  
> -bool CCLayerTreeHostImplProxy::isCCThread() const
> +#if USE(THREADED_COMPOSITING)
> +bool CCLayerTreeHostImplProxy::isImplThread()
>  {
>      return currentThread() == ccThread->threadID();
>  }
> +#elif defined(NDEBUG)
> +bool CCLayerTreeHostImplProxy::isImplThread()
> +{
> +    return true;
> +}

It seems like in !USE(THREADED_COMPOSITOR) isMainThread() and isImplThread()
will always return true.  That's not very useful - I think you want to check
the fakeImplThread in the !USE(THREADED_COMPOSITOR) case.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:155
> +CCLayerTreeHostImplProxy::ScopedImplThread::ScopedImplThread() :
m_previous(fakeImplThread)
> +{
> +    fakeImplThread = true;
> +}

Do we really need to support arbitrary nesting here?  This seems overdesigned. 
Just have setter for the bool.