Adding Blink bindings for WebGL 2

Adding Blink bindings for WebGL 2

Some entry points are still unimplemented. Must be enabled with --enable-unsafe-es3-apis

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

[bajones, 2015-02-02 19:10:53]

bajones@chromium.org changed reviewers:
+ kbr@chromium.org

[bajones, 2015-02-02 19:10:54]

Here's the latest version of the WebGL2 bindings in all it's incomplete glory.
Not concerned about the sprinkling of "notImplemented()" calls, but if you see
other issues let me know! I'll pull in the other appropriate owners for review
once I've got your sign off, Ken.

[Ken Russell (switch to Gerrit), 2015-02-03 01:23:25]

This looks good overall. A few relatively minor issues. Please feel free to get
others to review in parallel. Will sign off once these are fixed.

https://codereview.chromium.org/894143002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8HTMLCanvasElementCustom.cpp (right):

https://codereview.chromium.org/894143002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8HTMLCanvasElementCustom.cpp:39: #include
"core/inspector/InspectorCanvasInstrumentation.h"
Thanks for cleaning up these #includes. This one can definitely be removed;
maybe others too.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:247:
blink::Platform::current()->histogramEnumeration("Canvas.ContextType",
contextType, ContextTypeCount);
Does any histogram need to be updated on the Chromium side in response to this
change?

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContext.cpp (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContext.cpp:51:
shouldFailContextCreationForTesting = false;
Please refactor this boilerplate error handling code to share it between the
WebGL 1 and 2 contexts.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContext.idl (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContext.idl:12: WebGL2RenderingContext
implements WebGL2RenderingContextBase;
Strictly speaking, WebGL2RenderingContextBase implements
WebGLRenderingContextBase.

If Blink's IDL compiler doesn't support that, can we make
WebGL2RenderingContextBase extend WebGLRenderingContextBase?

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.h:33: 
Please add TODO about adding getBufferSubData.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContextBase.idl (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:63: const GLenum
BUFFER_MAP_POINTER                            = 0x88BD;
BUFFER_MAPPED and BUFFER_MAPPED_POINTER are removed from the spec.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:210: const GLenum
MAP_UNSYNCHRONIZED_BIT                        = 0x0020;
MAP_READ_BIT through MAP_UNSYNCHRONIZED_BIT were removed.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:316: const GLuint
TIMEOUT_IGNORED                               = 0xFFFFFFFF;
Add a TODO about the difference from the spec here.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:320: 
Please add TODO about adding getBufferSubData.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:328: void
renderbufferStorageMultisample(GLenum target, GLsizei samples, GLenum
internalformat, GLsizei width, GLsizei height);
renderbufferStorageMultisample is in its own section /* Renderbuffer objects */
in the spec.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:346: void
uniform1ui(WebGLUniformLocation? location, GLuint v0);
Add comment /* Uniforms and attributes */ from spec.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:377: 
Let's add drawRangeElements since it's still in the spec -- or at least a TODO.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:404: void
samplerParameteriv(WebGLSampler? sampler, GLenum pname, sequence<GLint> param);
The samplerParameteriv variants were removed from the spec because they were
useless.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:407: void
samplerParameterfv(WebGLSampler? sampler, GLenum pname, sequence<GLfloat>
param);
Same for samplerParameterfv.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:427: void
transformFeedbackVaryings(WebGLProgram? program, GLsizei count,
sequence<DOMString> varyings, GLenum bufferMode);
The "count" parameter was removed because it's redundant.

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:444:
WebGLVertexArrayObjectOES createVertexArray();
Should we add a TODO about cleaning up the visible type name here?

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/894143002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:2469: if
(extensionEnabled(OESStandardDerivativesName) || version() > 1)
Could you add a private isVersion2OrHigher() helper just to make searching for
all these instances easier? I have a feeling we may need to revisit the call
sites at least for those extensions which affect the behavior of the shader
compiler.

[bajones, 2015-02-03 21:46:57]

bajones@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[bajones, 2015-02-03 21:46:57]

jochen@ or mkwst@: Could you review the minor changes to:

bindings/
platform/
WebRuntimeFeatures.h/cpp

@kbr: Thanks for pointing out all the places the spec has changed since my
initial (many months old) implementation pass. Addressed most everything you
mentioned. A couple of notes.

 - V8 currently doesn't allow 64 bit const values. Attributes or parameters seem
OK.
 - I've added WebGL2RenderingContextBase implements WebGLRenderingContextBase to
WebGL2RenderingContextBase.idl but it doesn't propegate, so it's duplicated into
WebGL2RenderingContext.idl still.

[Ken Russell (switch to Gerrit), 2015-02-03 21:58:23]

Great. Nice cleanup of the WebGraphicsContext3D creation. LGTM

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContextBase.idl (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContextBase.idl:312: // TODO: This
currently differs from the WebGL2 spec.
Add owner for TODO.

[haraken, 2015-02-04 04:24:17]

On 2015/02/03 21:58:23, Ken Russell wrote:
> Great. Nice cleanup of the WebGraphicsContext3D creation. LGTM
> 
>
https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
> File Source/core/html/canvas/WebGL2RenderingContextBase.idl (right):
> 
>
https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
> Source/core/html/canvas/WebGL2RenderingContextBase.idl:312: // TODO: This
> currently differs from the WebGL2 spec.
> Add owner for TODO.

TODO(xxx) => FIXME

bindings/ LGTM.

[jochen (gone - plz use gerrit), 2015-02-04 12:54:08]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2015-02-04 12:54:09]

https://codereview.chromium.org/894143002/diff/40001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/894143002/diff/40001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:146: UnsafeES3APIs status=experimental
is this a going to be shipped really soon now? Otherwise, adding without a
status would be probably preferable

[dshwang, 2015-02-04 14:05:21]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[dshwang, 2015-02-04 14:05:22]

lgtm. looks great! with small nits.

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLFenceSync.h (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLFenceSync.h:17: virtual ~WebGLFenceSync();
after c++11, the code style is changed to "~WebGLFenceSync() override;" am I
right?

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLQuery.cpp (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLQuery.cpp:26: //
setObject(ctx->graphicsContext3D()->createQuery());
could you add comment?

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLQuery.h (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLQuery.h:23:
WebGLQuery(WebGL2RenderingContextBase*);
explicit

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:579: static bool
shouldFailContextCreationForTesting = false;
how about defining it in anonymous namespace at the begin of this file?

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLSampler.h (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLSampler.h:23:
WebGLSampler(WebGL2RenderingContextBase*);
explicit

[bajones, 2015-02-04 18:36:06]

On 2015/02/04 at 12:54:09, jochen wrote:
>
https://codereview.chromium.org/894143002/diff/40001/Source/platform/RuntimeE...
> File Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/894143002/diff/40001/Source/platform/RuntimeE...
> Source/platform/RuntimeEnabledFeatures.in:146: UnsafeES3APIs
status=experimental
> is this a going to be shipped really soon now? Otherwise, adding without a
status would be probably preferable

I noticed a thread about this just today, could you clarify the behavior? What
exactly does status=experimental provide here? (And no, this feature won't be
shipping short term.)

[bajones, 2015-02-04 18:58:36]

New patchsets have been uploaded after l-g-t-m from
kbr@chromium.org,haraken@chromium.org,dongseong.hwang@intel.com

[bajones, 2015-02-04 20:05:00]

CL updated to account for the feedback thus far. Thanks!

[Ken Russell (switch to Gerrit), 2015-02-04 22:36:52]

Still LGTM

[bajones, 2015-02-05 18:46:13]

bajones@chromium.org changed reviewers:
+ dglazkov@chromium.org

[bajones, 2015-02-05 18:46:14]

Only missing a review on public/web/WebRuntimeFeatures.h at this point. Any
owners willing to give it their stamp of approval?

[bajones, 2015-02-05 18:51:55]

New patchsets have been uploaded after l-g-t-m from kbr@chromium.org

[dglazkov, 2015-02-05 22:01:15]

Do we need Intent to implement?

[Ken Russell (switch to Gerrit), 2015-02-05 22:12:40]

On 2015/02/05 22:01:15, dglazkov wrote:
> Do we need Intent to implement?

This is an evolution of an existing spec -- is it necessary?

[dglazkov, 2015-02-05 22:52:03]

On 2015/02/05 at 22:12:40, kbr wrote:
> On 2015/02/05 22:01:15, dglazkov wrote:
> > Do we need Intent to implement?
> 
> This is an evolution of an existing spec -- is it necessary?

Yup.

[bajones, 2015-02-06 00:20:46]

On 2015/02/05 at 22:52:03, dglazkov wrote:
> On 2015/02/05 at 22:12:40, kbr wrote:
> > On 2015/02/05 22:01:15, dglazkov wrote:
> > > Do we need Intent to implement?
> > 
> > This is an evolution of an existing spec -- is it necessary?
> 
> Yup.

Done.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t7J4Zft8dHk

[dglazkov, 2015-02-06 02:04:32]

lgtm

[bajones, 2015-02-06 20:46:23]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-02-06 20:47:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894143002/80001

[commit-bot: I haz the power, 2015-02-06 20:59:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-06 20:59:41]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[Ken Russell (switch to Gerrit), 2015-02-06 21:06:39]

On 2015/02/06 20:59:41, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

Conflicts between desktop GL and GLES headers. Let's discuss offline how to work
around this.

[bajones, 2015-02-06 23:39:34]

New patchsets have been uploaded after l-g-t-m from dglazkov@chromium.org

[yunchao, 2015-02-09 08:19:44]

yunchao.he@intel.com changed reviewers:
+ yunchao.he@intel.com

[yunchao, 2015-02-09 08:19:46]

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGL2RenderingContext.idl (right):

https://codereview.chromium.org/894143002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGL2RenderingContext.idl:11: WebGL2RenderingContext
implements WebGLRenderingContextBase;
WebGL2RenderingContext implements/inherts WebGL2RenderingContextBase, and
WebGL2RenderingContextBase implements/inherits WebGLRenderingContextBase. So
this line is unnecessary. 

Unfortunately, I came across the same problem that WebGL2RenderingContext can
not binding APIs those defined in WebGLRenderingContextBase. Personally, I think
there should be a TODO to revise Blink binding code to binding those APIs
inherit from its grandparents or older ancestors.

[jochen (gone - plz use gerrit), 2015-02-09 14:22:04]

lgtm

[bajones, 2015-02-09 19:05:57]

New patchsets have been uploaded after l-g-t-m from jochen@chromium.org

[bajones, 2015-02-09 19:06:29]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-02-09 19:06:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894143002/120001

[commit-bot: I haz the power, 2015-02-09 19:10:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 19:10:19]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[bajones, 2015-02-09 21:18:33]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-02-09 21:18:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894143002/130033

[commit-bot: I haz the power, 2015-02-09 21:28:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 21:28:39]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[bajones, 2015-02-10 03:58:04]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-02-10 03:59:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894143002/190001

[commit-bot: I haz the power, 2015-02-10 05:18:20]

Committed patchset #11 (id:190001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189868

[sof, 2015-02-10 06:26:32]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-02-10 06:26:33]

Would have been good to have pinged oilpan-reviews about this one; not entirely
ready for Oilpan.

[haraken, 2015-02-10 06:32:08]

On 2015/02/10 06:26:33, sof wrote:
> Would have been good to have pinged oilpan-reviews about this one; not
entirely
> ready for Oilpan.

Yeah, it looks like this broke oilpan builds in a lot of places. I'm not happy
about reverting a CL for oilpan builds, but would it be ok to revert the CL?

[sof, 2015-02-10 07:51:33]

On 2015/02/10 06:32:08, haraken wrote:
> On 2015/02/10 06:26:33, sof wrote:
> > Would have been good to have pinged oilpan-reviews about this one; not
> entirely
> > ready for Oilpan.
> 
> Yeah, it looks like this broke oilpan builds in a lot of places. I'm not happy
> about reverting a CL for oilpan builds, but would it be ok to revert the CL?

Will fix.

( Might https://code.google.com/p/chromium/issues/detail?id=387516 be worthy of
some attention by now? )

[bajones, 2015-02-10 16:02:28]

On 2015/02/10 at 06:32:08, haraken wrote:
> On 2015/02/10 06:26:33, sof wrote:
> > Would have been good to have pinged oilpan-reviews about this one; not
entirely
> > ready for Oilpan.
> 
> Yeah, it looks like this broke oilpan builds in a lot of places. I'm not happy
about reverting a CL for oilpan builds, but would it be ok to revert the CL?

Thanks to sigbjornf@ for posting a fix, but please don't hesitate to revert if
my patches break the builds in any way, no matter how obscure! No offense will
be taken. :) That said it's all too easy to forget about potential Oilpan issues
when in the middle of a larger code review. http://crbug.com/387516 would have
been a nice safety to have in place.

I apologize for neglecting to ask for an oilpan review initially. Thanks for the
reminder!