Incorporated initial WebGL implementation for Chrome, currently...

Incorporated initial WebGL implementation for Chrome, currently
disabled in the build. Change ENABLE_3D_CANVAS to 1 in
src/third_party/WebKit/WebKit/chromium/features.gypi and regenerate
project files to compile. Current code is Windows-specific and
requires the sandbox to be disabled. Follow-on work will add ports to
other platforms and eventually work with the sandbox enabled.

This CL follows https://bugs.webkit.org/show_bug.cgi?id=29664 .

BUG=http://code.google.com/p/chromium/issues/detail?id=21852
TEST=none (runs preexisting WebGL layout tests; more coming)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27404

[darin (slow to review), 2009-09-23 06:01:05]

I know I advised you to put this in webkit/glue earlier, but I think it would be
better in webkit/api/src.  A couple reasons:

1) If you want it in webkit/glue then it needs to use Google style.

2) In the future, we'll want the implementation of GraphicsContext3D to be in
webkit/api/src, so we might as well start with it there.  (In the future, it'll
be implemented in terms of vtable calls out of WebKit API to the embedder.)

[Ken Russell (Google), 2009-09-23 20:37:12]

On 2009/09/23 06:01:05, darin wrote:
> I know I advised you to put this in webkit/glue earlier, but I think it would
be
> better in webkit/api/src.  A couple reasons:
> 
> 1) If you want it in webkit/glue then it needs to use Google style.
> 
> 2) In the future, we'll want the implementation of GraphicsContext3D to be in
> webkit/api/src, so we might as well start with it there.  (In the future,
it'll
> be implemented in terms of vtable calls out of WebKit API to the embedder.)

File moved and new patch set uploaded.

[darin (slow to review), 2009-09-23 21:05:24]

looks good.  just some minor style nits.

also,

The glew directory should contain a README.chromium explaining how to reproduce
the contents of that directory.  Take a look at some of the other
README.chromium files for examples.

Usually, directories should be named using lower case letters, so "gl" instead
of "GL".  however, is it a convention that the directory continaing glew headers
is named "GL"?  If so, then we should probably stick to the convention.

http://codereview.chromium.org/219001/diff/8004/7007
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/219001/diff/8004/7007#newcode673
Line 673: case GL_BLEND:
nit: webkit style is to not indent the 'case' labels.

http://webkit.org/coding/coding-style.html

http://codereview.chromium.org/219001/diff/8004/7007#newcode687
Line 687: m_vertexAttribPointerState[0].enabled = true;
nit: webkit style is to exclude the {}'s if the statement is only one line.

[Ken Russell (Google), 2009-09-24 05:49:22]

Added README.chromium. The capital "GL" is the standard directory name
containing the OpenGL and related headers, so we should preserve it.

http://codereview.chromium.org/219001/diff/8004/7007
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/219001/diff/8004/7007#newcode673
Line 673: case GL_BLEND:
On 2009/09/23 21:05:24, darin wrote:
> nit: webkit style is to not indent the 'case' labels.
> 
> http://webkit.org/coding/coding-style.html

Fixed here and throughout file.

http://codereview.chromium.org/219001/diff/8004/7007#newcode687
Line 687: m_vertexAttribPointerState[0].enabled = true;
On 2009/09/23 21:05:24, darin wrote:
> nit: webkit style is to exclude the {}'s if the statement is only one line.

Fixed here and throughout file.

[Ken Russell (Google), 2009-09-25 22:58:58]

New patch uploaded synchronizing with recent upstream changes.

[darin (slow to review), 2009-09-25 23:32:49]

I assume the printf logging is all temporary.  We usually avoid using it.

http://codereview.chromium.org/219001/diff/11001/11002
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/219001/diff/11001/11002#newcode277
Line 277: setSwapInterval(1);
nit: no {}'s around this statement.  best to review the rest of the file to be
sure you got the rest.

http://codereview.chromium.org/219001/diff/11001/11002#newcode293
Line 293: GraphicsContext3DInternal::~GraphicsContext3DInternal() {
nit: webkit style has the "{" on the next line for function definitions.  other
functions in this file have this issue to.

http://codereview.chromium.org/219001/diff/11001/11002#newcode327
Line 327: if (wglMakeCurrent(m_canvasDC, m_contextObj)) {
fix

http://codereview.chromium.org/219001/diff/11001/11002#newcode517
Line 517: if (state.enabled) {
fix

http://codereview.chromium.org/219001/diff/11001/11002#newcode525
Line 525: if (m_blendEnabled) {
fix

http://codereview.chromium.org/219001/diff/11001/11002#newcode603
Line 603: texture->setCubeMapRWrapModeInitialized(false);
fix

http://codereview.chromium.org/219001/diff/11001/11002#newcode804
Line 804: void GRAPHICS_CONTEXT_NAME::##name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5)
{        \
fix: "{" placement

[Ken Russell (Google), 2009-09-26 02:12:48]

On 2009/09/25 23:32:49, darin wrote:
> I assume the printf logging is all temporary.  We usually avoid using it.

Yes, it's temporary. The error handling in this code will be improved in the
near future.

> http://codereview.chromium.org/219001/diff/11001/11002
> File webkit/api/src/GraphicsContext3D.cpp (right):
> 
> http://codereview.chromium.org/219001/diff/11001/11002#newcode277
> Line 277: setSwapInterval(1);
> nit: no {}'s around this statement.  best to review the rest of the file to be
> sure you got the rest.
> 
> http://codereview.chromium.org/219001/diff/11001/11002#newcode293
> Line 293: GraphicsContext3DInternal::~GraphicsContext3DInternal() {
> nit: webkit style has the "{" on the next line for function definitions. 
other
> functions in this file have this issue to.

Took care of these, the others you mentioned and any others I could find.

[darin (slow to review), 2009-09-26 03:30:44]

LGTM