Bug 60167 – [Qt] [WebGL] implement Qt WebGL extensions

• Component: WebKit Qt
• Status: RESOLVED WONTFIX

[Igor Trindade Oliveira]

Qt implementation of Extensions3D is stub. Doing the following tests do not pass.

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-standard-derivatives.html

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-texture-float.html

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-vertex-array-object.html

[Igor Trindade Oliveira]

Created attachment 92234 [details]
Patch

Working in progress patch

[Igor Trindade Oliveira]

Created attachment 92276 [details]
Patch

Proposed patch

[Alexis Menard (darktears)]

Comment on attachment 92276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92276&action=review

> Source/WebCore/platform/graphics/qt/Extensions3DQt.h:28
>  

Copyright is Google?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt_p.h:1
> +#ifndef GraphicsContext3DQt_p_h

No copyright.

[Igor Trindade Oliveira]

Created attachment 92406 [details]
Patch

Proposed Patch. Fix copyright issues.

[Igor Trindade Oliveira]

Created attachment 93121 [details]
patch

Proposed Patch. Remove odd characters from html files.

[Noam Rosenthal]

It still has some Google headers, is that correct? Did you write those tests, or did Google?

[Igor Trindade Oliveira]

(In reply to comment #6)
> It still has some Google headers, is that correct? Did you write those tests, or did Google?

Was Google. these tests are from webgl conformance tests. In fact all the webgl tests in LayoutTests are from google webgl conformance tests.

[Noam Rosenthal]

(In reply to comment #5)
> Created an attachment (id=93121) [details]
> patch
> 
> Proposed Patch. Remove odd characters from html files.
This looks good to me.
However, I'm not a reviewer.

[Benjamin Poulain]

Comment on attachment 93121 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93121&action=review

> Source/WebCore/platform/graphics/qt/Extensions3DQt.cpp:43
> +    const QString extensions = QLatin1String(reinterpret_cast<const char *>(glGetString(GL_EXTENSIONS)));

glGetString() return 0 in case of error. I would add a test for that. Maybe just an assert if the error condition should never happen.

> Source/WebCore/platform/graphics/qt/Extensions3DQt.cpp:44
> +    m_supportedExtensions = extensions.split(QLatin1String(" "));

QLatin1String->QLatin1Char(' ').

I guess you could set SplitBehavior to be QString::SkipEmptyParts just in case.

[Igor Trindade Oliveira]

Created attachment 95756 [details]
Patch

Updated patch.

[Benjamin Poulain]

Comment on attachment 95756 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95756&action=review

Small comments:

> Source/WebCore/platform/graphics/qt/Extensions3DQt.cpp:43
> +    const QString extensions = QLatin1String(reinterpret_cast<const char *>(glGetString(GL_EXTENSIONS)));

Shouldn't you use a static_cast here? glGetString return a GLByte* so it should cast fine to char*.

> Source/WebCore/platform/graphics/qt/Extensions3DQt.cpp:44
> +    ASSERT(extensions.isNull());

I don't get this assertion. I guess you haven't tried the code in debug? :)

> Source/WebCore/platform/graphics/qt/Extensions3DQt.cpp:46
> +    m_supportedExtensions = extensions.split(QLatin1String(" "), QString::SkipEmptyParts);

QLatin1Char(' ')?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-308
> -#if defined (QT_OPENGL_ES_2)
>      return true;
> -#else
> -    return false;
> -#endif

This change looks strange.
What about stuff like precisions directives in the shader? That does not make GL 2 not GLES compliant?

[Eric Seidel (no email)]

Comment on attachment 95756 [details]
Patch

This seems like a trivial patch for someone familiar with Qt/WebGL to review.  Please r- or r+ so we can answer this 8-month-old review request. :(

[Igor Trindade Oliveira]

Comment on attachment 95756 [details]
Patch

Remove flags, the patch is out dated.

[Brent Fulgham]

The QT port is no longer part of WebKit.