[webkit-reviews] review denied: [Bug 72738] [chromium] Accelerated canvas broken in threaded compositing mode : [Attachment 118588] Added a unit test

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 118588: Added a unit test
https://bugs.webkit.org/attachment.cgi?id=118588&action=review

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


I doubt this works when the canvas is resized after an initial composite, have
you tested that case?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:77
> +    if (m_rendererTextureId && !m_compositorTextureId) {

what if the compositor texture exists but no longer matches the canvas' size?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:100
> +PassRefPtr<CCLayerImpl> Canvas2DLayerChromium::createCCLayerImpl()
> +{
> +    return CCCanvasLayerImpl::create(m_layerId);
> +}

this is redundant with CanvasLayerChromium's createCCLayerImpl

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:148
> +void Canvas2DLayerChromium::pushPropertiesTo(CCLayerImpl* layer)
>  {
> -    if (layerTreeHost())
> -	   layerTreeHost()->startRateLimiter(m_context);
> +    LayerChromium::pushPropertiesTo(layer);
> +
> +    CCCanvasLayerImpl* canvasLayer = static_cast<CCCanvasLayerImpl*>(layer);

> +    canvasLayer->setTextureId(m_compositorTextureId);

this is pretty much the same as CanvasLayerChromium's pushPropertiesTo

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:44
> +class Canvas2DLayerChromium : public LayerChromium {

why are you changing the class hierarchy? it seems like you're just creating
extra work for yourself by duplicating pushPropertiesTo()

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:46
> +    static PassRefPtr<Canvas2DLayerChromium> create(GraphicsContext3D*,
const IntSize&);

how are you handling canvas resizes?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:49
> +    // Called by ImageBufferSkia.

we typically don't try to document the caller at the callsite. The caller can
change at any point and the caller is not part of this interface. if you want
to add function-level documentation talk about what the function does, don't
talk about what other classes do

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:52
> +    // Called by CanvasRenderingContext2D.

don't say _who_ calls this function, that could change at any point, say _what_
this function does