Bug 60167 - [Qt] [WebGL] implement Qt WebGL extensions
Summary: [Qt] [WebGL] implement Qt WebGL extensions
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-04 07:01 PDT by Igor Trindade Oliveira
Modified: 2014-01-09 20:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (32.69 KB, patch)
2011-05-04 07:02 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (deleted)
2011-05-04 10:38 PDT, Igor Trindade Oliveira
menard: review-
menard: commit-queue-
Details | Formatted Diff | Diff
Patch (72.47 KB, patch)
2011-05-05 05:55 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
patch (72.46 KB, patch)
2011-05-11 08:36 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (72.87 KB, patch)
2011-06-02 06:18 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Igor Trindade Oliveira 2011-05-04 07:02:11 PDT
Created attachment 92234 [details]
Patch

Working in progress patch
Comment 2 Igor Trindade Oliveira 2011-05-04 10:38:46 PDT
Created attachment 92276 [details]
Patch

Proposed patch
Comment 3 Alexis Menard (darktears) 2011-05-05 05:40:42 PDT
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.
Comment 4 Igor Trindade Oliveira 2011-05-05 05:55:09 PDT
Created attachment 92406 [details]
Patch

Proposed Patch. Fix copyright issues.
Comment 5 Igor Trindade Oliveira 2011-05-11 08:36:11 PDT
Created attachment 93121 [details]
patch

Proposed Patch. Remove odd characters from html files.
Comment 6 Noam Rosenthal 2011-05-11 13:05:30 PDT
It still has some Google headers, is that correct? Did you write those tests, or did Google?
Comment 7 Igor Trindade Oliveira 2011-05-11 13:08:04 PDT
(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.
Comment 8 Noam Rosenthal 2011-05-11 13:15:04 PDT
(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.
Comment 9 Benjamin Poulain 2011-05-22 08:23:48 PDT
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.
Comment 10 Igor Trindade Oliveira 2011-06-02 06:18:13 PDT
Created attachment 95756 [details]
Patch

Updated patch.
Comment 11 Benjamin Poulain 2011-06-05 08:05:16 PDT
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?
Comment 12 Eric Seidel (no email) 2012-01-30 15:27:00 PST
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. :(
Comment 13 Igor Trindade Oliveira 2012-01-30 15:57:34 PST
Comment on attachment 95756 [details]
Patch

Remove flags, the patch is out dated.
Comment 14 Brent Fulgham 2014-01-09 20:52:46 PST
The QT port is no longer part of WebKit.