[webkit-reviews] review denied: [Bug 111780] Check to ensure MultisampleRenderbuffer creation succeeds : [Attachment 192095] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Brandon Jones
<bajones at chromium.org>'s request for review:
Bug 111780: Check to ensure MultisampleRenderbuffer creation succeeds
https://bugs.webkit.org/show_bug.cgi?id=111780

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

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


Looks pretty good overall but a few changes are needed. Also, please ensure
you've tested this with multisampling re-enabled on all GPU types on OS X.

> Source/WebCore/ChangeLog:8
> +	   No new tests.

Why not? Please describe the issue and workaround, and why an automated test
can't be written if necessary. Please also indicate what testing was done for
this patch.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:193
> +// TODO(bajones): This can be removed once renderbufferStorageMultisample
starts reporting GL_OUT_OF_MEMORY properly

Nit: please use periods after sentences, here and throughout the patch.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> +    uint32 pixel = 0;

Please use an unsigned char[4] instead of a uint32 to avoid byte ordering
problems.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:310
> +#if PLATFORM(MAC)

This won't take effect for the Chromium port. For that port you need to use
PLATFORM(CHROMIUM) and OS(DARWIN). Just using OS(DARWIN) may be the best way to
ensure this runs on all WebKit ports on the Mac. It would be better if you
could figure out a way to turn this into a run-time check that runs only on the
appropriate GPUs.