[webkit-reviews] review granted: [Bug 72738] [chromium] Accelerated canvas broken in threaded compositing mode : [Attachment 118777] Inheriting from CanvasLayerChromium again

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Iain Merrick
<husky at google.com>'s request for review:
Bug 72738: [chromium] Accelerated canvas broken in threaded compositing mode
https://bugs.webkit.org/show_bug.cgi?id=72738

Attachment 118777: Inheriting from CanvasLayerChromium again
https://bugs.webkit.org/attachment.cgi?id=118777&action=review

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


R=me but add the lost context check back and address other nits before landing.
 We don't attempt to handle lost contexts at all, but if we do get one somehow
we should just stop displaying the canvas and not attempt to issue any GL
commands on the old context.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:107
> +    return m_rendererTextureId && compositedTextureId() &&
!m_size.isEmpty();

why did you remove the m_context and getGraphicsResetStatusARB() check from
here? they seem to still be useful

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:52
> +    unsigned compositedTextureId() const { return m_compositedTextureId; }

I don't think a protected getter is buying you all that much vs just accessing
m_compositedTextureId, since we never override this or do anything else fancy,
but that's up to you

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:66
> +    const WebGLId rendererTexId = 1;
> +    const WebGLId compositorTexId = 2;

we normally wouldn't abbreviate texture to 'tex' in a variable name