[webkit-reviews] review denied: [Bug 82275] WebGL content swapped at wrong time in threaded compositing mode : [Attachment 135924] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied James Robinson
<jamesr at chromium.org>'s request for review:
Bug 82275: WebGL content swapped at wrong time in threaded compositing mode
https://bugs.webkit.org/show_bug.cgi?id=82275

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=135924&action=review


Overall this looks very good. Nice work. There's only one small issue, but I
feel pretty strongly that it should be taken care of (and re-testing done),
thus the r-.

> Source/WebCore/ChangeLog:29
> +	   since we're shadowing the relevant GL state on the
WebGLRenderingContext anyway.

This one change concerns me. Noting that FECustomFilter.cpp is using
DrawingBuffer, it is not a good idea to assume that the WebGLRenderingContext
is the caller of DrawingBuffer::clearFramebuffer(). Minimally, this change
should be split off from the rest of this patch, but I also think it should
just not be done.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:145
> +    m_drawingBuffer = DrawingBuffer::create(m_context.get(), contextSize,
DrawingBuffer::Discard, DrawingBuffer::Alpha);

Because this class uses DrawingBuffer I think it is not a good idea to assume
that the WebGLRenderingContext will preserve all of the needed state during
calls to DrawingBuffer::clearFramebuffer().

> LayoutTests/fast/canvas/webgl/webgl-composite-modes-repaint.html:4
> +This test is only useful as a pixel test. You should see two rows with 4
canvases in each. The top row of canvases should have a black background, the
bottom row should have a pink background.

Is the comment about the pink background accurate in this test and the other
one?