Summary: | [Qt] Enable GraphicsContext3D only when the window surface support OpenGL | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jarkko Sakkinen <jarkko.j.sakkinen> | ||||||||||||||||
Component: | WebGL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, hausmann, jarkko.j.sakkinen | ||||||||||||||||
Priority: | P5 | Keywords: | Qt, QtTriaged | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Jarkko Sakkinen
2011-03-17 02:56:20 PDT
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. |