[webkit-reviews] review denied: [Bug 56131] [chromium] Compositor thread infrastructure : [Attachment 88149] Thread infrastructure only.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 4 16:37:17 PDT 2011


James Robinson <jamesr 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 88149: Thread infrastructure only.
https://bugs.webkit.org/attachment.cgi?id=88149&action=review

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

R- because the waitForThreadCompletion() param scares me, but everything else
looks pretty good.

The nonthreadsafe types aren't used yet so it's hard to say how generally
useful they are.  They probably will make more sense in the context of stuff
that uses them.

> Source/WebCore/platform/graphics/chromium/cc/CCTask.h:76
> +template <typename C, typename M, typename ARG0, typename ARG1>

in chrome we use T for the type of object, Method instead of M, and A/B/C/D/...
instead of ARG0/ARG1/ARG2/...  which I find a bit more readable.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:41
> +#define COMMIT_SLOWLY 0

seems unnecessary

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:46
> +PassRefPtr<CCThread> CCThread::getInstance()
> +{
> +    if (!ccThread)
> +	   return adoptRef(new CCThread());
> +    return PassRefPtr<CCThread>(ccThread);

this should ASSERT() that it's called from the main thread

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:60
> +    void* unused;
> +    waitForThreadCompletion(m_threadId, &unused);

just pass 0 as the second param to waitForThreadCompletion(). this might
actually attempt to write to some random location pointed to by whatever's on
the stack here.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:85
> +    // If the thread is dead, then this task is stale, e.g. posted
> +    // during or slightly before compositor shutdown.
> +    if (!ccThread)
> +	   return;

This check seems a bit strange - if the compositor thread is going down it
seems that we still may want to run any already-enqueued tasks on the main
thread.  It's a little hard to reason about this in the abstract.


More information about the webkit-reviews mailing list