[webkit-reviews] review granted: [Bug 78139] [chromium] Resolve GraphicsContext3D use and re-enable filters in the threaded compositor : [Attachment 133881] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 78139: [chromium] Resolve GraphicsContext3D use and re-enable filters in
the threaded compositor
https://bugs.webkit.org/show_bug.cgi?id=78139

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

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


The context management logic looks OK. I'm less sure about FilterOperations -
can we look at that separately?

Left comments, mostly about naming/etc

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:321
> +    CCLayerTreeHost::setNeedsFilterContext(true);

this could be parameterless, since it's only used for ->true transitions

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:430
> +    if (CCLayerTreeHost::needsFilterContext() &&
!SharedGraphicsContext3D::getForImplThread())

I think here an explicit "haveForImplThread()" might make this interface easier
to understand / safer. with that, you could ASSERT() that the thread is correct
in the getForImplThread() getter

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:431
> +	   SharedGraphicsContext3D::createForImplThread();

if this fails for some reason (say out of some resource on the GPU process
side) we'll silently stop rendering filters and keep attempting to create this
context on every frame. is that what you want? I'm not sure if there is a valid
alternative.

> Source/WebCore/platform/graphics/filters/FilterOperation.h:47
> +class FilterOperation : public ThreadSafeRefCounted<FilterOperation> {

This I feel less comfortable with - this might be OK for the FilterOps we
support today, but it's not very future-proof.	For example, type CUSTOM is
definitely not safe to destroy from a non-main thread and while we don't set
ENABLE(CSS_SHADERS) today and may not for the immediate future, it's a subtle
landmine to leave. It also looks like the CA Code copies data out of the filter
ops, so if someone makes a change that makes RefCounting across threads break
it'll break in chromium but not mac.

I'd prefer to change this in a different patch and/or take a more serious look
at serializing the filter ops in our own code before crossing any thread
boundaries.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:78
> +static PassRefPtr<GraphicsContext3D> getContextForImplThread(bool create)

even though it's just a static i think a two-state enum would be better than a
bool

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:86
> +    return getContextForImplThread(false);

if you had an explicit have..() call, you could add an ASSERT() here that this
is only called on the impl thread

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:91
> +    return getContextForImplThread(true);

can you add an ASSERT() that this is only called on the main thread?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
> +    static PassRefPtr<GraphicsContext3D> createForImplThread();

I think this would be better if it returned a bool indicating whether the
creation is successful or not, since that's all the caller needs (if they want
to use the context they can get() it).