Ported Chrome's WebGL implementation to Linux. Required changes to...

Ported Chrome's WebGL implementation to Linux. Required changes to
GLEW's bootstrapping sequence on both X11 and Windows platforms to
avoid linking against libGL.so.1 directly due to
http://crbug.com/16800 . Made GLEW's inclusion of glu.h optional.
Tested on Linux, Mac and Windows.

BUG=http://crbug.com/21852
TEST=none (runs existing WebGL tests; more coming)


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

[Ken Russell (switch to Gerrit), 2009-10-05 02:53:15]

Please review. Thanks.

[darin (slow to review), 2009-10-05 17:21:31]

LG.  just style nits:

http://codereview.chromium.org/258024/diff/2001/3002
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/258024/diff/2001/3002#newcode131
Line 131: static bool s_initializedGLEW;
nit: i don't think it is webkit style to use "s_" prefix for static member vars.
 i think in this case, it would be more common in webkit style to use a
function-level static (named using normal variable namingStyle).

http://codereview.chromium.org/258024/diff/2001/3002#newcode189
Line 189: typedef Bool ( * PFNGLXMAKECURRENTPROC) (Display *dpy, GLXDrawable
drawable, GLXContext ctx);
nit: webkit style puts the "*" next to the type, so "Display* dpy" instead. 
also, i think it should be "(* PFNGLXMAKECURRENTPROC)"

http://codereview.chromium.org/258024/diff/2001/3002#newcode191
Line 191: typedef void ( * PFNGLXDESTROYCONTEXTPROC) (Display *dpy, GLXContext
ctx);
ditto

http://codereview.chromium.org/258024/diff/2001/3002#newcode241
Line 241: , m_display(NULL)
i should have mentioned this before, but webkit style does not use NULL. 
instead, it uses 0 for null pointers.

http://codereview.chromium.org/258024/diff/2001/3002#newcode367
Line 367: #elif PLATFORM(LINUX)
nit: it seems like it would be nice to break this constructor up into some
helper functions.  maybe one per port?

http://codereview.chromium.org/258024/diff/2001/3002#newcode379
Line 379: for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++) {
nit: no need for "/ sizeof(const char*)" which is guaranteed to be 1

http://codereview.chromium.org/258024/diff/2001/3002#newcode387
Line 387: for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++) {
ditto

[Ken Russell (switch to Gerrit), 2009-10-05 18:05:09]

http://codereview.chromium.org/258024/diff/2001/3002
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/258024/diff/2001/3002#newcode131
Line 131: static bool s_initializedGLEW;
On 2009/10/05 17:21:31, darin wrote:
> nit: i don't think it is webkit style to use "s_" prefix for static member
vars.
>  i think in this case, it would be more common in webkit style to use a
> function-level static (named using normal variable namingStyle).

Done.

http://codereview.chromium.org/258024/diff/2001/3002#newcode189
Line 189: typedef Bool ( * PFNGLXMAKECURRENTPROC) (Display *dpy, GLXDrawable
drawable, GLXContext ctx);
On 2009/10/05 17:21:31, darin wrote:
> nit: webkit style puts the "*" next to the type, so "Display* dpy" instead. 
> also, i think it should be "(* PFNGLXMAKECURRENTPROC)"

Done.

http://codereview.chromium.org/258024/diff/2001/3002#newcode191
Line 191: typedef void ( * PFNGLXDESTROYCONTEXTPROC) (Display *dpy, GLXContext
ctx);
On 2009/10/05 17:21:31, darin wrote:
> ditto

Done.

http://codereview.chromium.org/258024/diff/2001/3002#newcode241
Line 241: , m_display(NULL)
On 2009/10/05 17:21:31, darin wrote:
> i should have mentioned this before, but webkit style does not use NULL. 
> instead, it uses 0 for null pointers.

I know. IMO this is a horrible convention. For now this code is self-consistent,
so I'd like to leave it. In the near future this will all be rewritten anyway to
use command buffers, at which point I'll change it to use 0 instead of NULL.

http://codereview.chromium.org/258024/diff/2001/3002#newcode367
Line 367: #elif PLATFORM(LINUX)
On 2009/10/05 17:21:31, darin wrote:
> nit: it seems like it would be nice to break this constructor up into some
> helper functions.  maybe one per port?

Agreed, this should be split up. All of this code is going to be rewritten soon
anyway to use command buffers instead of direct GL and window system calls, so
for the moment I'd like to leave it as is.

http://codereview.chromium.org/258024/diff/2001/3002#newcode379
Line 379: for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++) {
On 2009/10/05 17:21:31, darin wrote:
> nit: no need for "/ sizeof(const char*)" which is guaranteed to be 1

sizeof(const char*) == sizeof pointer, not 1. The division is definitely
required here.

http://codereview.chromium.org/258024/diff/2001/3002#newcode387
Line 387: for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++) {
On 2009/10/05 17:21:31, darin wrote:
> ditto

ditto.

[darin (slow to review), 2009-10-05 18:10:28]

http://codereview.chromium.org/258024/diff/2001/3002
File webkit/api/src/GraphicsContext3D.cpp (right):

http://codereview.chromium.org/258024/diff/2001/3002#newcode241
Line 241: , m_display(NULL)
On 2009/10/05 18:05:09, kbr wrote:
> On 2009/10/05 17:21:31, darin wrote:
> > i should have mentioned this before, but webkit style does not use NULL. 
> > instead, it uses 0 for null pointers.
> 
> I know. IMO this is a horrible convention. For now this code is
self-consistent,
> so I'd like to leave it. In the near future this will all be rewritten anyway
to
> use command buffers, at which point I'll change it to use 0 instead of NULL.
> 

ok..  it only matters once we go to upstream the webkit api, which won't be
tomorrow :)

http://codereview.chromium.org/258024/diff/2001/3002#newcode367
Line 367: #elif PLATFORM(LINUX)
On 2009/10/05 18:05:09, kbr wrote:
> On 2009/10/05 17:21:31, darin wrote:
> > nit: it seems like it would be nice to break this constructor up into some
> > helper functions.  maybe one per port?
> 
> Agreed, this should be split up. All of this code is going to be rewritten
soon
> anyway to use command buffers instead of direct GL and window system calls, so
> for the moment I'd like to leave it as is.
> 

ok, sounds good

http://codereview.chromium.org/258024/diff/2001/3002#newcode379
Line 379: for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++) {
On 2009/10/05 18:05:09, kbr wrote:
> On 2009/10/05 17:21:31, darin wrote:
> > nit: no need for "/ sizeof(const char*)" which is guaranteed to be 1
> 
> sizeof(const char*) == sizeof pointer, not 1. The division is definitely
> required here.
> 

so true!  /me takes another sip of coffee :-/

[darin (slow to review), 2009-10-05 18:11:18]

LGTM