WebGL 2: add types into glReadPixels

WebGL 2: add types into glReadPixels

UNSIGNED_SHORT and SHORT can be supported in glReadPixels in WebGL 2, see Table 4.5 
in GLES 3.0 spec: https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.3.pdf

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

[yunchao, 2015-08-13 07:46:24]

yunchao.he@intel.com changed reviewers:
+ zmo@chromium.org

[yunchao, 2015-08-13 09:40:22]

A small change. PTAL. Thanks!

[Zhenyao Mo, 2015-08-13 15:22:01]

https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
Source/modules/webgl/WebGLRenderingContextBase.cpp:3741: case GL_UNSIGNED_SHORT:
This is incorrect.  For OpenGL ES 2 / WebGL 1, it is clearly defined in Table
3.2 (page 62) what types are allowed.

[yunchao, 2015-08-14 00:07:11]

Thanks for your reviewing, zmo@. Please see the comments.

https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
Source/modules/webgl/WebGLRenderingContextBase.cpp:3741: case GL_UNSIGNED_SHORT:
On 2015/08/13 15:22:01, Zhenyao Mo wrote:
> This is incorrect.  For OpenGL ES 2 / WebGL 1, it is clearly defined in Table
> 3.2 (page 62) what types are allowed.

Yeah. I know that WebGL 1.0 should not support UNSIGNED_SHORT / SHORT. But for
these two types (and UNSIGNED_INT, UNSIGNED_INT_*, INT), the validation function
(validateReadPixelsFormatAndType) will report INVLID_ENUM and return. So the
code snippet is not wrong. 

Well, we  can override this code snippet respectively for WebGL 1 and WebGL 2 to
get 'expectedViewType', which should be more clearer. WDYT?

[yunchao, 2015-08-14 02:50:17]

On 2015/08/14 00:07:11, yunchao wrote:
> Thanks for your reviewing, zmo@. Please see the comments.
> 
>
https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
> File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/1288303002/diff/1/Source/modules/webgl/WebGLR...
> Source/modules/webgl/WebGLRenderingContextBase.cpp:3741: case
GL_UNSIGNED_SHORT:
> On 2015/08/13 15:22:01, Zhenyao Mo wrote:
> > This is incorrect.  For OpenGL ES 2 / WebGL 1, it is clearly defined in
Table
> > 3.2 (page 62) what types are allowed.
> 
> Yeah. I know that WebGL 1.0 should not support UNSIGNED_SHORT / SHORT. But for
> these two types (and UNSIGNED_INT, UNSIGNED_INT_*, INT), the validation
function
> (validateReadPixelsFormatAndType) will report INVLID_ENUM and return. So the
> code snippet is not wrong. 
> 
> Well, we  can override this code snippet respectively for WebGL 1 and WebGL 2
to
> get 'expectedViewType', which should be more clearer. WDYT?

I have updated the code... zmo@, could you take a look when you have time?

[Zhenyao Mo, 2015-08-14 16:07:42]

LGTM with nits

https://codereview.chromium.org/1288303002/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right):

https://codereview.chromium.org/1288303002/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGL2RenderingContextBase.cpp:2139:
DOMArrayBufferView::ViewType expectedViewType;
nit: you don't need this expectedViewType.  You can just return in each case,
saving one extra copying (although that's trivia).

https://codereview.chromium.org/1288303002/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1288303002/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:3714:
DOMArrayBufferView::ViewType expectedViewType;
nit: you don't need this expectedViewType.  You can just return in each case,
saving one extra copying (although that's trivia).

[Zhenyao Mo, 2015-08-14 16:08:01]

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

[yunchao, 2015-08-15 02:01:19]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-15 02:01:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288303002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288303002/40001

[commit-bot: I haz the power, 2015-08-15 03:10:23]

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

[commit-bot: I haz the power, 2015-08-15 03:10:24]

Dry run: This issue passed the CQ dry run.

[yunchao, 2015-08-15 05:24:51]

On 2015/08/15 03:10:24, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

This is a small and simple change, with some code refactoring. I will merge it.

[yunchao, 2015-08-15 05:25:15]

The CQ bit was checked by yunchao.he@intel.com

[yunchao, 2015-08-15 05:25:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1288303002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2015-08-15 05:25:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288303002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288303002/40001

[commit-bot: I haz the power, 2015-08-15 05:29:38]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200599