Moved GL enums from GraphicsContext3D to a more generic location.

Moved GL enums from GraphicsContext3D to a more generic location.

Updated all callers to use the new enums. This is the first step in eventually
removing our dependence on GraphicsContext3D's thin WebGraphicsContext3D wrapper.

BUG=326382
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163725

[bajones, 2013-12-06 18:09:25]

If you feel strongly about the enum naming convention, declaration style, or
anything else please speak up now! Even though this touches a lot of files it's
still relatively easy to change.

[jamesr, 2013-12-06 19:51:00]

Do we need another copy of these enums?  Why not use the khronos headers?  Back
in the day, when WebKit would bind to different GL implementations, it was
useful to have an isolated copy.  Now that Blink can only bind to the GL
implementation provided by the chromium command buffer, I don't think this extra
abstraction is very useful.

[Ken Russell (switch to Gerrit), 2013-12-06 20:19:55]

On 2013/12/06 19:51:00, jamesr wrote:
> Do we need another copy of these enums?  Why not use the khronos headers? 
Back
> in the day, when WebKit would bind to different GL implementations, it was
> useful to have an isolated copy.  Now that Blink can only bind to the GL
> implementation provided by the chromium command buffer, I don't think this
extra
> abstraction is very useful.

That would be cool.

It would still be necessary to have a header which defines the WebGL-specific
enums -- those don't exist in any of the Chromium GLES2 headers. That's a small
price to pay though. Could GraphicsTypes3D.h could still be that header for the
time being, and include the Khronos headers itself?

[bajones, 2013-12-06 20:28:01]

On 2013/12/06 19:51:00, jamesr wrote:
> Do we need another copy of these enums?  Why not use the khronos headers? 
Back
> in the day, when WebKit would bind to different GL implementations, it was
> useful to have an isolated copy.  Now that Blink can only bind to the GL
> implementation provided by the chromium command buffer, I don't think this
extra
> abstraction is very useful.

I would love that! Where can I find out more about the conventions for including
third party sources in Blink?

[Ken Russell (switch to Gerrit), 2013-12-06 20:29:31]

On 2013/12/06 20:28:01, bajones wrote:
> On 2013/12/06 19:51:00, jamesr wrote:
> > Do we need another copy of these enums?  Why not use the khronos headers? 
> Back
> > in the day, when WebKit would bind to different GL implementations, it was
> > useful to have an isolated copy.  Now that Blink can only bind to the GL
> > implementation provided by the chromium command buffer, I don't think this
> extra
> > abstraction is very useful.
> 
> I would love that! Where can I find out more about the conventions for
including
> third party sources in Blink?

Source/core/platform/DEPS seems to allow all third_party headers to be included,
so theoretically, GraphicsTypes3D.h should be able to just include
third_party/khronos/GLES2/gl2.h.

There are some implicit dependencies up to gpu/GLES2/ however so you may run
into some trouble.

[Stephen Chennney, 2013-12-06 22:07:15]

On 2013/12/06 20:29:31, Ken Russell wrote:
> On 2013/12/06 20:28:01, bajones wrote:
> > On 2013/12/06 19:51:00, jamesr wrote:
> > > Do we need another copy of these enums?  Why not use the khronos headers? 
> > Back
> > > in the day, when WebKit would bind to different GL implementations, it was
> > > useful to have an isolated copy.  Now that Blink can only bind to the GL
> > > implementation provided by the chromium command buffer, I don't think this
> > extra
> > > abstraction is very useful.
> > 
> > I would love that! Where can I find out more about the conventions for
> including
> > third party sources in Blink?
> 
> Source/core/platform/DEPS seems to allow all third_party headers to be
included,
> so theoretically, GraphicsTypes3D.h should be able to just include
> third_party/khronos/GLES2/gl2.h.
> 
> There are some implicit dependencies up to gpu/GLES2/ however so you may run
> into some trouble.

Just to bring you all up to date, most of this now lives in
Source/platform/graphics, not core.

And it's fine to include 3rd party software in Blink. We do it with all the
image libraries, icu, skia, ...

[jamesr, 2013-12-06 22:14:55]

I think the third_party/khronos headers are checked directly into the chromium
tree and not pulled via DEPS but the principle is the same.  If we can use those
I think we should.  If we can't, then maybe something like this makes sense as
an intermediate step but IMO the fewer copies of GL we have laying around the
better.

[bajones, 2013-12-06 22:29:45]

On 2013/12/06 22:14:55, jamesr wrote:
> I think the third_party/khronos headers are checked directly into the chromium
> tree and not pulled via DEPS but the principle is the same.  If we can use
those
> I think we should.  If we can't, then maybe something like this makes sense as
> an intermediate step but IMO the fewer copies of GL we have laying around the
> better.

CL has been updated.

I'm not sure if we need to update any DEPS or similar to account for this, but
as Ken suggested simply including "third_party/khronos/GLES2/gl2.h" (and
gl2ext.h) works on a "does it compile" level. 

I think that with this switch we should follow up this patch with another that
switches over to use the GLtypes as well (GLint, GLenum, etc.) I would have
included that in this CL, but it's going to be a large change and would make and
already messy CL harder to read.

[jamesr, 2013-12-06 22:41:00]

I think this looks great.  I have to admit that I don't fully understand how the
third_party/khronos header dependencies work - I think Ken is the expert there.

https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsContext3D.h (right):

https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsContext3D.h:35: #include
"third_party/khronos/GLES2/gl2ext.h"
are these used by this header, or just by stuff including this header and
relying on that defining the enums/etc? might be better to try to push #includes
to the files that actually use them - IWYU and all that

https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/custom/CustomFilterProgram.cpp
(right):

https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/CustomFilterProgram.cpp:35:
#include "third_party/khronos/GLES2/gl2.h"
is this include used?

[bajones, 2013-12-06 23:10:06]

On 2013/12/06 22:41:00, jamesr wrote:
>
https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
> Source/core/platform/graphics/GraphicsContext3D.h:35: #include
> "third_party/khronos/GLES2/gl2ext.h"
> are these used by this header, or just by stuff including this header and
> relying on that defining the enums/etc? might be better to try to push
#includes
> to the files that actually use them - IWYU and all that

Not in this patch, but I've been working on step 2 (replacing the GC3D types
with their standard GL counterparts) and that patch will require it here.

Given that, does the style guide dictate that we still copy it into all the
files which use it?

>
https://codereview.chromium.org/93873009/diff/20001/Source/core/platform/grap...
> Source/core/platform/graphics/filters/custom/CustomFilterProgram.cpp:35:
> #include "third_party/khronos/GLES2/gl2.h"
> is this include used?

Apparently not. Thanks for catching that!

[Ken Russell (switch to Gerrit), 2013-12-06 23:45:19]

Thanks for proposing and doing this cleanup.

I'd like to ask you to please hold off landing this until I can finish landing
https://codereview.chromium.org/105123003 . That is a serious bug requiring
merge back to M32, and this patch may introduce conflicts which will make
merging harder.

As far as I can tell, all of the direct #includes of gl2.h below are unnecessary
because the associated files pull in GraphicsContext3D.h.

https://codereview.chromium.org/93873009/diff/40001/Source/core/html/canvas/W...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/html/canvas/W...
Source/core/html/canvas/WebGLFramebuffer.cpp:132: if (attachment ==
GC3D_DEPTH_STENCIL_ATTACHMENT && m_renderbuffer->emulatedStencilBuffer()) {
Eventually we'll probably want to rename this synthetic attachment point to
something WebGL-specific, unless it's used elsewhere in Blink.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
(right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp:32:
#include "third_party/khronos/GLES2/gl2.h"
This #include should be unnecessary. GraphicsContext3D.h and consequently gl2.h
come in through CustomFilterCompiledProgram.h.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/custom/CustomFilterGlobalContext.cpp
(right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/CustomFilterGlobalContext.cpp:36:
#include "third_party/khronos/GLES2/gl2.h"
Should be unnecessary.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/custom/CustomFilterMesh.cpp (right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/CustomFilterMesh.cpp:35: #include
"third_party/khronos/GLES2/gl2.h"
Should be unnecessary.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/custom/CustomFilterRenderer.cpp
(right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/CustomFilterRenderer.cpp:43:
#include "third_party/khronos/GLES2/gl2.h"
Should be unnecessary.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/custom/FECustomFilter.cpp (right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/filters/custom/FECustomFilter.cpp:41: #include
"third_party/khronos/GLES2/gl2.h"
Should be unnecessary.

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/93873009/diff/40001/Source/core/platform/grap...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:44: #include
"third_party/khronos/GLES2/gl2.h"
Should be unnecessary.

[bajones, 2013-12-07 00:01:56]

On 2013/12/06 23:45:19, Ken Russell wrote:
> I'd like to ask you to please hold off landing this until I can finish landing
> https://codereview.chromium.org/105123003 . That is a serious bug requiring
> merge back to M32, and this patch may introduce conflicts which will make
> merging harder.

No problem! (Still need an LGTM anyway.)

[bajones, 2013-12-10 18:24:40]

Rebased again now that https://codereview.chromium.org/105123003 has landed.

Any further concerns regarding this change?

[Ken Russell (switch to Gerrit), 2013-12-10 19:21:05]

Thanks for your patience. Looks great. LGTM

[bajones, 2013-12-10 19:23:49]

On 2013/12/10 19:21:05, Ken Russell wrote:
> Thanks for your patience. Looks great. LGTM

Thank you!

@jamesr (or any other owner), can I get a LGTM on Source/platform and
Source/web?

[jamesr, 2013-12-10 23:43:26]

lgtm

https://codereview.chromium.org/93873009/diff/120001/Source/web/tests/Drawing...
File Source/web/tests/DrawingBufferTest.cpp (right):

https://codereview.chromium.org/93873009/diff/120001/Source/web/tests/Drawing...
Source/web/tests/DrawingBufferTest.cpp:87: ASSERT_EQ(target,
(WGC3Denum)GL_TEXTURE_2D);
static_cast<>, please. we don't use c-style casts in C++ code in blink or
chromium

[commit-bot: I haz the power, 2013-12-11 00:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/93873009/140001

[commit-bot: I haz the power, 2013-12-11 00:21:36]

Failed to apply patch for
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index:
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
  |diff --git
a/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
b/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
  |index
99077fa2e57740a5dc098d8e6b6372217bf6d19f..15a71a0296b3585c20fe6d101d8733f8d2fcb957
100644
  |---
a/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
  |+++
b/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:      
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
Index:
Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
diff --git
a/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
b/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
index
99077fa2e57740a5dc098d8e6b6372217bf6d19f..15a71a0296b3585c20fe6d101d8733f8d2fcb957
100644
---
a/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
+++
b/Source/core/platform/graphics/filters/custom/CustomFilterCompiledProgram.cpp
@@ -50,11 +50,11 @@
CustomFilterCompiledProgram::CustomFilterCompiledProgram(PassRefPtr<GraphicsCont
 {
     m_context->makeContextCurrent();
 
-    Platform3DObject vertexShader =
compileShader(GraphicsContext3D::VERTEX_SHADER, validatedVertexShader);
+    Platform3DObject vertexShader = compileShader(GL_VERTEX_SHADER,
validatedVertexShader);
     if (!vertexShader)
         return;
 
-    Platform3DObject fragmentShader =
compileShader(GraphicsContext3D::FRAGMENT_SHADER, validatedFragmentShader);
+    Platform3DObject fragmentShader = compileShader(GL_FRAGMENT_SHADER,
validatedFragmentShader);
     if (!fragmentShader) {
         m_context->deleteShader(vertexShader);
         return;
@@ -82,7 +82,7 @@ Platform3DObject
CustomFilterCompiledProgram::compileShader(GC3Denum shaderType,
     m_context->compileShader(shader);
 
     int compiled = 0;
-    m_context->getShaderiv(shader, GraphicsContext3D::COMPILE_STATUS,
&compiled);
+    m_context->getShaderiv(shader, GL_COMPILE_STATUS, &compiled);
     if (!compiled) {
         // FIXME: This is an invalid shader. Throw some errors.
         // https://bugs.webkit.org/show_bug.cgi?id=74416
@@ -101,7 +101,7 @@ Platform3DObject
CustomFilterCompiledProgram::linkProgram(Platform3DObject verte
     m_context->linkProgram(program);
 
     int linked = 0;
-    m_context->getProgramiv(program, GraphicsContext3D::LINK_STATUS, &linked);
+    m_context->getProgramiv(program, GL_LINK_STATUS, &linked);
     if (!linked) {
         // FIXME: Invalid vertex/fragment shader combination. Throw some errors
here.
         // https://bugs.webkit.org/show_bug.cgi?id=74416

[commit-bot: I haz the power, 2013-12-11 00:41:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/93873009/160001

[commit-bot: I haz the power, 2013-12-11 00:59:15]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Ken Russell (switch to Gerrit), 2013-12-11 03:05:16]

On 2013/12/11 00:59:15, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

Looks like you'll need to add "+third_party/khronos" to Source/platform/DEPS.

[commit-bot: I haz the power, 2013-12-11 18:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/93873009/180001

[commit-bot: I haz the power, 2013-12-11 18:56:06]

Retried try job too often on linux_blink_rel for step(s)
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-11 18:56:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/93873009/200001

[commit-bot: I haz the power, 2013-12-11 20:01:38]

Change committed as 163725

[adamk, 2013-12-11 20:34:24]

This broke the Android build, I've uploaded a revert in
https://codereview.chromium.org/113153002/

[adamk, 2013-12-11 20:44:03]

On 2013/12/11 20:34:24, adamk wrote:
> This broke the Android build, I've uploaded a revert in
> https://codereview.chromium.org/113153002/

And reverted in
https://src.chromium.org/viewvc/blink?revision=163726&view=revision