[webkit-reviews] review granted: [Bug 109331] EXT_draw_buffers needs implementation : [Attachment 190555] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 109331: EXT_draw_buffers needs implementation
https://bugs.webkit.org/show_bug.cgi?id=109331

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

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


Looks good. r=me with a couple of minor modifications.

> Source/Platform/chromium/public/WebGraphicsContext3D.h:476
> +    virtual void drawBuffersChromium(WGC3Dsizei n, const WGC3Denum* bufs) {
}

Capitalization doesn't follow the naming convention elsewhere in this file.
Should either be drawBuffersCHROMIUM or, if semantics don't differ from
EXT_draw_buffers, drawBuffersEXT.

> Source/WebCore/bindings/v8/V8Binding.h:246
> +    // Convert a value to a 32-bit integer. The conversion fails if the

Note to other reviewers: these helpers had to be moved higher in the file to
allow "unsigned nativeValue(...)", below, to be defined. The JSC bindings
already exposed this conversion.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:53
> +    return adoptPtr(x);

This should just read "adoptPtr(new EXTDrawBuffers(context));". The point of
adoptPtr is to ensure there are no "naked news" in the code base.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:78
> +	   // Because the backbuffer is simulated we need to change BACK to
COLOR_ATTACHMENT0

It's probably worth saying: "Because the backbuffer is simulated on all current
WebKit ports, ...". Also, please end the comment with a period.

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:607
> +    for (size_t i = 0; i < m_drawBuffers.size(); i++) {

Please add a comment indicating that this filtering works around graphics
driver bugs on Mac OS X. I assume it is sufficient to do so? Also, it's
important that it be possible to disable this workaround at run time so that we
can file a Radar about it. Since it looks like that will involve adding a lot
of infrastructure, please file another WebKit bug for that and make it depend
on this one.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5396
> +    // COLOR_ATTACHMENT0_EXT is equal to COLOR_ATTACHMENT0

Please indent and add period.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5924
> +    if (!m_maxDrawBuffers && EXTDrawBuffers::supported(this))

Theoretically, this and getMaxColorAttachments should also be calling
m_context->getExtensions()->ensureEnabled("GL_EXT_draw_buffers"), but that
would really only be needed in the Chromium port and it will work without it,
so maybe best not to bother.

>> Source/WebCore/platform/graphics/Extensions3D.h:154
>> +	    UNPACK_UNPREMULTIPLY_ALPHA_CHROMIUM = 0x9242,
> 
> enum members should use InterCaps with an initial capital letter. 
[readability/enum_casing] [4]

Note to other reviewers: the enums in this file follow the OpenGL naming
convention, so I'm ignoring the reported style errors.