[webkit-reviews] review denied: [Bug 70838] [chromium] Enable threaded compositing via CCThreadProxy::hasThread only : [Attachment 115900] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 70838: [chromium] Enable threaded compositing via CCThreadProxy::hasThread
only
https://bugs.webkit.org/show_bug.cgi?id=70838

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

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


Mostly looks good but the shutdown() semantics seem a little scary. I think
that needs to be resolved and documented clearly in WebCompositor.h

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:199
> +    if (CCProxy::implThread())

a lot of code in this patch null-checks CCProxy::implThread() to tell if we're
in threaded mode or not. should we provide a bool getter with a friendlier
name?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:70
> +    return !mainThreadIsImplThread && currentThread() ==
s_mainThread->threadID();

this looks weird but maybe it's just because of the variable name.
"mainThreadIsImplThread" makes me think that when that flag is set we should
only return true if the current thread is the impl thread. This seems to be
doing something more permissive.

maybe the bool should be "currentThreadIsImplThread" ?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:76
> +    return mainThreadIsImplThread || currentThread() == implThreadID;

same here - from the variable name, i'd think that if the bool is true this
should check that the current thread == the main thread

> Source/WebKit/chromium/public/WebCompositor.h:45
> +    // compositing, pass a WebThread. To use single-threaded compositing,
pass
> +    // 0.

line wrapping a bit awkward - can you move the '0' up to the previous line at
least?

> Source/WebKit/chromium/public/WebCompositor.h:48
> +    WEBKIT_EXPORT static void shutdown();

the requirements and conditions on this function should be documented here -
i.e. it looks like you can't call this with active compositor instances and
it's not valid to call any functions on this interface after shutdown()
depending on what the initialize/shutdown requirements are

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:31
> +#include "WebKit.h"

this #include doesn't appear to be used

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:42
> +bool webCompositorInitialized = false;

can we make this a static member s_initialized? we're doing that for
WebComopsitorImpl::s_nextAvailableIndentifier and
WebCompositorImpl::s_compositors so it seems nice to be consistent

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:59
> +    if (webCompositorInitialized)
> +	   ASSERT_NOT_REACHED();

ASSERT(!webCompositorInitialized) ?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:62
> +   
CCProxy::setMainThread(CCThreadImpl::create(webKitPlatformSupport()->currentThr
ead()).leakPtr());

seems weird to .leakPtr() this here, and then make an explicit delete call in
shutdown(). Can we hang on to this pointer explicitly in a static with some
comments on the lifetime?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:64
> +	   CCProxy::setImplThread(CCThreadImpl::create(implThread).leakPtr());

we've been leaking this object since it's pretty small and only exists once per
process, but if we're going to delete the mainthread CCThreadImpl it seems to
make sense to treat this one the same way.

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:69
> +void WebCompositor::shutdown()

could you add an ASSERT() that the s_compositors map is empty (i.e. enforce
that you can't call this with active compositor instances) ?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:74
> +    webCompositorInitialized = false;

does this mean we now support shutting down and reinitializing in the same
address space? WebKit in general does not support this operation, and I'm not
sure that our compositor does either

Unless this is really necessary I'd prefer to not support this and leave the
initialized flag alone so another call to initialize() will ASSERT().
Alternately if we do need to support this we need some tests to make sure it
works and doesn't regress.

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:33
> +#include "cc/CCThreadProxy.h"

this looks unused

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:96
> +    GraphicsContext3DPrivate::ThreadUsage usage =
CCThreadProxy::implThread() ? GraphicsContext3DPrivate::ForUseOnAnotherThread :
GraphicsContext3DPrivate::ForUseOnThisThread;

shouldn't this be CCProxy::implThread() ?

> Source/WebKit/chromium/src/WebViewImpl.cpp:137
> +#include "cc/CCThreadProxy.h"

I think this should be CCProxy.h, since it's pulling in a header to check
CCProxy::implThread()