Bug 56555

Created attachment 86043 [details] Bug fix. See the ChangeLog entry for more verbose description.

Adding Simon and Benjamin to CC list because this in a way related to: https://bugs.webkit.org/show_bug.cgi?id=40884 After some thinking I completely support your choice of not supporting any kind of software fallback at this point.

What about printing? Or rendering a particular frame to QImage? Wouldn't that disable the GraphicsContext3D for the following painting call?

You're right. In order to support those cases software fallback *is* needed. *Also*, there should be a check before allowing texture mapping in GraphicsContext3DImpl::paint() that painter->paintDevice() == m_viewportGLWidget. If not use software fallback. I think, because of cases that you mentioned, you should reconsider reopening 40884 (QImage and printing). I'll revise this patch.

Comment on attachment 86043 [details] Bug fix. See the ChangeLog entry for more verbose description. I do not mind updating 40884. Rejecting this for now.

Created attachment 86048 [details] Revised fix. Allows software fallback.

Comment on attachment 86048 [details] Revised fix. Allows software fallback. Quick comment from a quick look: we are not limiting lines to 80 characters in WebKit, you don't have to split everything like this: PageClientQWidget* webPageClient = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient());

Created attachment 86062 [details] Fixed indentation

Fixed indentation. This fixes now two issues: - Crash does not occur when viewport is switched. - Does only texture mapping if painter is associated to the viewport QGLWidget. These are anyway valid fixes. Let's scope software fallback discussion out of this bug :)

Comment on attachment 86062 [details] Fixed indentation View in context: https://bugs.webkit.org/attachment.cgi?id=86062&action=review > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:474 > + PageClientQWidget* webPageClient > + = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient()); This should be on one line. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:488 > + PageClientQWidget* webPageClient > + = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient()); This should be on one line. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:504 > + QGLWidget* viewportGLWidget = getViewportGLWidget(); > + if (testAcceleratedCompositing() > + && viewportGLWidget > + && viewportGLWidget == m_viewportGLWidget > + && viewportGLWidget == painter->device()) { I would put those test as the first statements of the functions, and return if not true. That way it is clear, if the initial condition are not correct, just return.

I r- just for style issues, the patch looks good to me otherwise.

Comment on attachment 86062 [details] Fixed indentation Thinking abou it... Aren't the static_cast<PageClientQWidget*> wrong? You should use QWebPageClient. The return value of m_hostWindow->platformPageClient() is gonna be PageClientQGraphicsWidget for QGraphicsWebView.

Created attachment 86066 [details] Fixed rest of the indentations. After that last if-statement that you commented comes the software fallback. Therefore, it is handled that way.

Created attachment 86138 [details] Check AC on creation of WebGLRenderingContext See the ChangeLog entry for description.

Comment on attachment 86138 [details] Check AC on creation of WebGLRenderingContext View in context: https://bugs.webkit.org/attachment.cgi?id=86138&action=review > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:469 > + QWebPageClient* webPageClient = m_hostWindow->platformPageClient(); > + > + QAbstractScrollArea* scrollArea = qobject_cast<QAbstractScrollArea*>(webPageClient->ownerWidget()); I would remove that unneeded newline > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:472 > + if (scrollArea) > + return qobject_cast<QGLWidget*>(scrollArea->viewport()); Doesn't casting 0 return a 0? Just wondered if you could do "return qobject..."

The pointer scrollArea might be NULL. If it were, then scrollArea->viewport() would cause SEGFAULT.

Created attachment 86149 [details] Removed unnecessary new line character

Created attachment 86150 [details] Mistake in taking diff in last submit

Comment on attachment 86150 [details] Mistake in taking diff in last submit Clearing flags on attachment: 86150 Committed r81468: <http://trac.webkit.org/changeset/81468>

All reviewed patches have been landed. Closing bug.