Bug 36194

Temporarily disabling these tests on the build bots under https://bugs.webkit.org/show_bug.cgi?id=36200 to get the commit queue going again.

Be sure to file a bug at bugreporter.apple.com on the underlying OpenGL issue. Why wasn't this seen before committing?

(In reply to comment #2) > Be sure to file a bug at bugreporter.apple.com on the underlying OpenGL issue. We will once we figure out what the root cause appears to be. Right now it points to EXT_framebuffer_multisample / EXT_framebuffer_blit. We also don't know the graphics hardware in the build bots. > Why wasn't this seen before committing? The tests pass on our local (NVIDIA) hardware and drivers. I sidestepped the commit queue due to other failing tests which were blocking it, which it looks like was a mistake.

Because the Snow Leopard bots were already failing tests, I neglected to notice that basically all of the WebGL layout tests are failing on that bot after this change. In particular, all of the following fail or crash, in addition to the other tests mentioned: fast/canvas/webgl/bug-32364.html fast/canvas/webgl/bug-32692.html fast/canvas/webgl/bug-32888.html fast/canvas/webgl/drawArraysOutOfBounds.html fast/canvas/webgl/drawElementssOutOfBounds.html fast/canvas/webgl/error-reporting.html fast/canvas/webgl/getActiveTest.html fast/canvas/webgl/gl-object-get-calls.html fast/canvas/webgl/incorrect-context-object-behaviour.html fast/canvas/webgl/index-validation-copies-indices.html fast/canvas/webgl/index-validation.html fast/canvas/webgl/invalidPassedParams.html fast/canvas/webgl/null-object-behaviour.html fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html fast/canvas/webgl/texImage2DImageDataTest.html fast/canvas/webgl/uniform-location.html Eric Seidel pointed out that the Snow Leopard bot doesn't block the commit queue, so I'll not disable these for the moment, but we need to make sure that the fix (which will likely be to disable multisampling in some situations) addresses these failures.

Created attachment 50870 [details] patch

Attachment 50870 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:209: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:210: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/chromium/src/GraphicsContext3D.cpp:636: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/chromium/src/GraphicsContext3D.cpp:637: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 50871 [details] fixed style issue

Comment on attachment 50871 [details] fixed style issue > Index: WebCore/platform/graphics/GraphicsContext3D.h > =================================================================== > --- WebCore/platform/graphics/GraphicsContext3D.h (revision 56097) > +++ WebCore/platform/graphics/GraphicsContext3D.h (working copy) > @@ -713,6 +713,10 @@ namespace WebCore { > AlphaOp* neededAlphaOp, > unsigned int* format); > > +#if PLATFORM(MAC) > + void validateAttributes(); > +#endif Would this be better as a virtual method with an empty implementation here, so that you can use it to fix up idiosyncrasies on other platforms too? > +void GraphicsContext3D::validateAttributes() > +{ > + // Take into account the user's requested context creation attributes, in > + // particular stencil and antialias, and determine which could and could > + // not be honored based on the capabilities of the OpenGL implementation. > + > + const char* vendor = reinterpret_cast<const char*>(::glGetString(GL_VENDOR)); > + const char* extensions = reinterpret_cast<const char*>(::glGetString(GL_EXTENSIONS)); You only use the vendor string if m_attrs.antialias is true, so get it inside that clause. I'll let someone more familiar with WebGL do the final review.

Created attachment 50924 [details] revised patch : responding to Simon Fraser's review About the GraphicsContext3D::validateAttributes(), my feeling is that we should leave PLATFORM(MAC) there. The reason is that for PLATFORM(CHROMIUM/QT), this functionality is implemented in GraphicsContex3DInternal::validateAttributes(). Either use GraphicsContext3D or GraphicsContext3DInternal implementation, but not both. The same situation is with all the FBO variables: they are both in GraphicsContext3D PLATFORM(MAC) and GraphicsContext3DInternal PLATFORM(CHROMIUM/QT). Adding the PLATFORM(MAC) is a reminder that those functions and variables are duplicated and could be combined into one single implementation. The vendor quest is moved into the if block as suggested.

After more thought and discussion with Mo, we are going to roll back the original patch for https://bugs.webkit.org/show_bug.cgi?id=33416 and reapply it later via the commit queue. A large refactoring of the affected code is imminent, and it will be much easier to iterate on any heuristics needed to work around driver bugs if all of the changes are made after the refactoring has taken place. Cancelling the review request. We will probably close this bug after the rollback.

Mo discovered that fast/canvas/webgl/index-validation.html is flaky after 33416 as well. Intermittent failures of this test on some of the bots (Leopard Intel Release (Tests) for example) confirm this. This must also be triggered by a driver bug since the failure is in the initial FBO setup and this code path is taken by all of the other tests.

This bug is caused by a patch for https://bugs.webkit.org/show_bug.cgi?id=33416. We canceled the original patch and dealt with the issue in revised patch in #33416. Close this one.