[Webkit-unassigned] [Bug 66145] [chromium] Assert that main thread and compositor thread are used safely

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66145


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104202|review?                     |review-
               Flag|                            |




--- Comment #16 from James Robinson <jamesr at chromium.org>  2011-08-17 14:12:32 PST ---
(From update of attachment 104202)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.