Check to ensure MultisampleRenderbuffer creation succeeds
Created attachment 192092 [details] Patch
This will allow us to re-enable WebGL antialiasing on AMD Macs while we wait for a driver fix.
Comment on attachment 192092 [details] Patch Attachment 192092 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17070270
Comment on attachment 192092 [details] Patch Attachment 192092 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17024054
Created attachment 192095 [details] Patch
Comment on attachment 192095 [details] Patch Attachment 192095 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17075184
Comment on attachment 192095 [details] Patch Attachment 192095 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17038111
Comment on attachment 192095 [details] Patch 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.
Created attachment 192291 [details] Patch
Comment on attachment 192291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192291&action=review > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:193 > +// TODO(bajones): This can be removed once renderbufferStorageMultisample starts reporting GL_OUT_OF_MEMORY properly. We don't normally use names in comments, and WebKit tends to use "FIXME:" rather than TODO.
Created attachment 192292 [details] Patch
Comment on attachment 192292 [details] Patch Clearing flags on attachment: 192292 Committed r145280: <http://trac.webkit.org/changeset/145280>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 112217
Comment on attachment 192292 [details] Patch Attachment 192292 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17136144 New failing tests: xmlviewer/extensions-api.html
Created attachment 193187 [details] Patch
Issue breaking the tests was that glColorMask or GL_SCISSOR state could interfere with the validation check by preventing the correct color from being written. Updated patch ensures valid state before testing.
Comment on attachment 193187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193187&action=review Good catch. Couple of questions. > Source/WebCore/ChangeLog:-6566 > - Don't remove the old ChangeLog entry. > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202 > + m_context->colorMask(true, true, true, true); Don't you also need to restore the user's color mask afterward?
Comment on attachment 193187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193187&action=review >> Source/WebCore/ChangeLog:-6566 >> - > > Don't remove the old ChangeLog entry. Okay, thanks. Wasn't sure what the protocol was in this case. >> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202 >> + m_context->colorMask(true, true, true, true); > > Don't you also need to restore the user's color mask afterward? If so then we're already broken. reset() does the same thing (Line 342) without ever restoring it. Perhaps this is a bug?
Comment on attachment 193187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193187&action=review Looks fine modulo the ChangeLog entry. >>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:202 >>> + m_context->colorMask(true, true, true, true); >> >> Don't you also need to restore the user's color mask afterward? > > If so then we're already broken. reset() does the same thing (Line 342) without ever restoring it. Perhaps this is a bug? Looking again, WebGLRenderingContext::restoreStateAfterClear resets the color mask, so this is OK.
Created attachment 193196 [details] Patch
Comment on attachment 193196 [details] Patch Rejecting attachment 193196 [details] from commit-queue. bajones@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 193196 [details] Patch Clearing flags on attachment: 193196 Committed r145856: <http://trac.webkit.org/changeset/145856>