Bug 43754 - Use Chromium OpenGL bindings rather than GLEW in WebGraphicsContextDefaultImpl
Summary: Use Chromium OpenGL bindings rather than GLEW in WebGraphicsContextDefaultImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks: 43811
  Show dependency tree / graph
 
Reported: 2010-08-09 15:56 PDT by Kenneth Russell
Modified: 2010-08-10 14:43 PDT (History)
6 users (show)

See Also:

Attachments
Patch (26.53 KB, patch)
2010-08-10 11:14 PDT, Kenneth Russell 		dglazkov: review+
kbr: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-08-09 15:56:33 PDT 
In the Chromium WebGL port, the "default" implementation of WebGraphicsContext3D calls OpenGL directly, currently via GLEW. We need to switch this to use Chromium's OpenGL bindings rather than GLEW so that Chromium's test shell can use the OSMesa renderer. This will allow WebGL tests to be run on the build bots all of the time.
Comment 1 Kenneth Russell 2010-08-10 11:14:00 PDT 
Created attachment 64032 [details]
Patch

From the ChangeLog:

Deleted per-platform OpenGL context management code, now abstracted via GLContext. Built and tested in Chromium with --in-process-webgl flag.

Thanks to Al Patrick for the initial patch.
Comment 2 Kenneth Russell 2010-08-10 11:14:01 PDT 
Created attachment 64033


From the ChangeLog:

Deleted per-platform OpenGL context management code, now abstracted via GLContext. Built and tested in Chromium with --in-process-webgl flag.

Thanks to Al Patrick for the initial patch.
Comment 3 WebKit Review Bot 2010-08-10 11:18:39 PDT 
Attachment 64032 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:35:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:72:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:107:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dimitri Glazkov (Google) 2010-08-10 11:23:46 PDT 
Comment on attachment 64032 [details]
Patch

r=me, after style fixes:

WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:72
 +      , m_glContext(NULL)
(0)

WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:98
 +          delete m_glContext;
Maybe make OwnPtr?

WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:107
 +      m_glContext = gfx::GLContext::CreateOffscreenGLContext(NULL);
(0)

WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:427
 +      // TODO(apatrick): OpenGL ES 2 does not support GL_BGRA so this fails when
FIXME:
Comment 5 Kenneth Russell 2010-08-10 14:24:41 PDT 
(In reply to comment #4)
> (From update of attachment 64032 [details])
> r=me, after style fixes:
> 
> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:72
>  +      , m_glContext(NULL)
> (0)
> 
> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:98
>  +          delete m_glContext;
> Maybe make OwnPtr?
> 
> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:107
>  +      m_glContext = gfx::GLContext::CreateOffscreenGLContext(NULL);
> (0)
> 
> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:427
>  +      // TODO(apatrick): OpenGL ES 2 does not support GL_BGRA so this fails when
> FIXME:

Will fix these issues before commit.
Comment 6 Kenneth Russell 2010-08-10 14:27:44 PDT 
Committed r65093: <http://trac.webkit.org/changeset/65093>
Comment 7 WebKit Review Bot 2010-08-10 14:41:12 PDT 
http://trac.webkit.org/changeset/65093 might have broken Chromium Linux Release