[webkit-reviews] review denied: [Bug 56431] Add support for preserveDrawingBuffer context creation attribute : [Attachment 86130] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied John Bauman <jbauman at chromium.org>'s
request for review:
Bug 56431: Add support for preserveDrawingBuffer context creation attribute
https://bugs.webkit.org/show_bug.cgi?id=56431

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86130&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:293
> +	   WebGLRenderingContext* ctx =
static_cast<WebGLRenderingContext*>(m_context.get());
> +
> +	   ctx->markLayerComposited();

Odd to not just do this cast inline.  Then you don't need the extra lines or {
}

> Source/WebCore/html/HTMLCanvasElement.cpp:315
> +	   // The buffer contains the last presented data, so save a
> +	   // copy of it.

We don't wrap to 80c in webkit.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:505
> +    if (m_context->layerComposited() && !m_layerCleared
> +	   && !getContextAttributes()->preserveDrawingBuffer()) {

Seems we could early-return here instead, no?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:512
> +	       m_context->clearColor(m_colorMask[0] ? m_currentClearColor[0] :
0.0f,

Isn't there a clearer way to write this?  Using a Color object perhaps?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:517
> +	       m_context->clearColor(0.0f, 0.0f, 0.0f, 0.0f);

Do these really need to be 0.0f instead of 0?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:531
> +	   m_context->clearColor(m_currentClearColor[0],
m_currentClearColor[1],
> +				 m_currentClearColor[2],
m_currentClearColor[3]);

Again, seems long-winded.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:555
> +    clearIfComposited(0);

What's the 0 mean here?  should it be a default parameter?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:979
> +    m_currentClearColor[0] = r;
> +    m_currentClearColor[1] = g;
> +    m_currentClearColor[2] = b;
> +    m_currentClearColor[3] = a;

I see, these are GC3DFloat values.  Seems we need a GC3DRGBA class, no?

> Source/WebCore/html/canvas/WebGLRenderingContext.h:450
> +    bool m_layerCleared;
> +    GC3Dfloat m_currentClearColor[4];
> +    bool m_scissorEnabled;

You might want to keep the bools together for possible future bitfield usage. 
I'm not sure it maters, and I am not sure we'll ever have very many of these
object either.	Just noting.