[webkit-reviews] review denied: [Bug 80540] [Chromium] Single buffered canvas layers with the threaded compositor : [Attachment 138168] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Justin Novosad
<junov at chromium.org>'s request for review:
Bug 80540: [Chromium] Single buffered canvas layers with the threaded
compositor
https://bugs.webkit.org/show_bug.cgi?id=80540

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

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


Seems pretty good.  Before landing you need to update the unit test (and
corresponding ChangeLog line) and fix up a few things around
m_useDoubleBuffering

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

this won't blend, and I don't think it is entirely accurate.

you need to update Canvas2DLayerChromiumTests.cpp to cover these new cases, but
I suspect you already knew that

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:64
> +    setUseDoubleBuffering(false);

did you mean to set useDoubleBuffering to true in some cases (like maybe
implThread = true and non-deferred mode)? it looks like it's just dead code
here currently.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> +void Canvas2DLayerChromium::setUseDoubleBuffering(bool useDoubleBuffering)

i don't understand why this is a public setter. it appears to only be called
from the constructor, if so then it should be folded in to the c'tor

trying to change this at runtime seems difficult

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:90
> +	   return;

nit: newline after this return

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:91
> +    if (m_backTextureId && !m_useDoubleBuffering && CCProxy::hasImplThread()
&& layerTreeHost()) {

is the hasImplThread() check necessary here? when would m_useDoubleBuffering be
true in the single-threaded case?

do we have to worry about lost context here?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:160
> +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() &&
layerTreeHost()) {

again, when would m_useDoubleBuffering be true and CCProxy::hasImplThread() be
false?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:55
> +#include "cc/CCLayerTreeHost.h"

what is this #include for? please remove if it's not being used