WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer

WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in WebGLVertexArrayObjectBase.

BUG=570453
Committed: https://crrev.com/ab18bc73dddce5b609a2d73239ab3874b2a5382d
Cr-Commit-Position: refs/heads/master@{#368015}

[yunchao, 2015-12-29 13:18:57]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer

BUG=
==========

to

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 13:18:57]

yunchao.he@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, qiankun.miao@intel.com,
zmo@chromium.org

[yunchao, 2015-12-29 13:21:31]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validation are remove when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 13:22:12]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validation are remove when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are remove when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 13:23:50]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are remove when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 13:24:54]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generated correct glErrors to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generates correct glError to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 13:26:52]

Description was changed from

==========
WebGL code refacotring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generates correct glError to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refactoring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generates correct glError to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

[yunchao, 2015-12-29 14:23:29]

Patchset #2 (id:20001) has been deleted

[yunchao, 2015-12-29 14:25:33]

PTAL. Thanks a lot!

[Zhenyao Mo, 2015-12-29 22:04:04]

https://codereview.chromium.org/1555523002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1555523002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8770: GL_INVALID_VALUE,
This should also be INVALID_OPERATION.  See
https://www.opengl.org/registry/specs/ARB/vertex_attrib_binding.txt

https://codereview.chromium.org/1555523002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8857: GL_INVALID_VALUE,
Same here, INVALID_OPERATION.

https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(left):

https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1526:
m_boundVertexArrayObject->setVertexAttribState(index, bytesPerElement, size,
type, false, stride, static_cast<GLintptr>(offset), m_boundArrayBuffer);
When you remove the above validation, then you ended up caching the wrong here
info if some args are invalid.

[yunchao, 2015-12-30 01:02:11]

Thanks for your review, Zhenyao. Could you see the comments inline? I think this
may be a common issue when we remove validations in Blink side.

https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(left):

https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1526:
m_boundVertexArrayObject->setVertexAttribState(index, bytesPerElement, size,
type, false, stride, static_cast<GLintptr>(offset), m_boundArrayBuffer);
On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> When you remove the above validation, then you ended up caching the wrong here
> info if some args are invalid.

Yeah, that's true. But Chromium will report error/failure soon in command
buffer. So the web developers will know that their app have some bugs. The wrong
data cached here will not be used then. I think this is OK. In my opinion, we
need to make sure that intentional or unintentional wrong parameter(s) will
trigger appropriate error. And it should not lead to some security problem(crash
the browser, memory leak, etc). WDYT?

I think we would have the similar cases when we do code-refactoring for other
APIs.

[yunchao, 2015-12-30 01:06:39]

Description was changed from

==========
WebGL code refactoring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

This change also generates correct glError to fix 2 small bugs in command
buffer. These bugs 
are exposed after the blink-side validations are removed when running webgl
conformance tests.

BUG=570453
TEST=gpu_unittests
==========

to

==========
WebGL code refactoring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

BUG=570453
==========

[Zhenyao Mo, 2015-12-30 01:10:36]

On 2015/12/30 01:02:11, yunchao wrote:
> Thanks for your review, Zhenyao. Could you see the comments inline? I think
this
> may be a common issue when we remove validations in Blink side.
> 
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (left):
> 
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1526:
> m_boundVertexArrayObject->setVertexAttribState(index, bytesPerElement, size,
> type, false, stride, static_cast<GLintptr>(offset), m_boundArrayBuffer);
> On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > When you remove the above validation, then you ended up caching the wrong
here
> > info if some args are invalid.
> 
> Yeah, that's true. But Chromium will report error/failure soon in command
> buffer. So the web developers will know that their app have some bugs. The
wrong
> data cached here will not be used then. I think this is OK. In my opinion, we
> need to make sure that intentional or unintentional wrong parameter(s) will
> trigger appropriate error. And it should not lead to some security
problem(crash
> the browser, memory leak, etc). WDYT?
> 
> I think we would have the similar cases when we do code-refactoring for other
> APIs.

I have a different opinion on this.  When we cache the wrong data, the data can
be used without the understanding that the data could be wrong.  I'd rather not
have to worry about this. Otherwise who knows what types of nasty bugs that
manifest in seemingly unrelated places?  It will be a maintenance cost.

[Ken Russell (switch to Gerrit), 2015-12-30 02:19:12]

On 2015/12/30 01:10:36, Zhenyao Mo wrote:
> On 2015/12/30 01:02:11, yunchao wrote:
> > Thanks for your review, Zhenyao. Could you see the comments inline? I think
> this
> > may be a common issue when we remove validations in Blink side.
> > 
> >
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> > (left):
> > 
> >
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1526:
> > m_boundVertexArrayObject->setVertexAttribState(index, bytesPerElement, size,
> > type, false, stride, static_cast<GLintptr>(offset), m_boundArrayBuffer);
> > On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > > When you remove the above validation, then you ended up caching the wrong
> here
> > > info if some args are invalid.
> > 
> > Yeah, that's true. But Chromium will report error/failure soon in command
> > buffer. So the web developers will know that their app have some bugs. The
> wrong
> > data cached here will not be used then. I think this is OK. In my opinion,
we
> > need to make sure that intentional or unintentional wrong parameter(s) will
> > trigger appropriate error. And it should not lead to some security
> problem(crash
> > the browser, memory leak, etc). WDYT?
> > 
> > I think we would have the similar cases when we do code-refactoring for
other
> > APIs.
> 
> I have a different opinion on this.  When we cache the wrong data, the data
can
> be used without the understanding that the data could be wrong.  I'd rather
not
> have to worry about this. Otherwise who knows what types of nasty bugs that
> manifest in seemingly unrelated places?  It will be a maintenance cost.

I agree 100% with Mo. If values are being cached at the Blink level then they
need to be validated. We should try to remove as much caching and validation
from the Blink side as possible.

[Ken Russell (switch to Gerrit), 2015-12-30 02:20:50]

On 2015/12/30 02:19:12, Ken Russell wrote:
> On 2015/12/30 01:10:36, Zhenyao Mo wrote:
> > On 2015/12/30 01:02:11, yunchao wrote:
> > > Thanks for your review, Zhenyao. Could you see the comments inline? I
think
> > this
> > > may be a common issue when we remove validations in Blink side.
> > > 
> > >
> >
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> > > File
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> > > (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1555523002/diff/40001/third_party/WebKit/Sour...
> > >
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1526:
> > > m_boundVertexArrayObject->setVertexAttribState(index, bytesPerElement,
size,
> > > type, false, stride, static_cast<GLintptr>(offset), m_boundArrayBuffer);
> > > On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > > > When you remove the above validation, then you ended up caching the
wrong
> > here
> > > > info if some args are invalid.
> > > 
> > > Yeah, that's true. But Chromium will report error/failure soon in command
> > > buffer. So the web developers will know that their app have some bugs. The
> > wrong
> > > data cached here will not be used then. I think this is OK. In my opinion,
> we
> > > need to make sure that intentional or unintentional wrong parameter(s)
will
> > > trigger appropriate error. And it should not lead to some security
> > problem(crash
> > > the browser, memory leak, etc). WDYT?
> > > 
> > > I think we would have the similar cases when we do code-refactoring for
> other
> > > APIs.
> > 
> > I have a different opinion on this.  When we cache the wrong data, the data
> can
> > be used without the understanding that the data could be wrong.  I'd rather
> not
> > have to worry about this. Otherwise who knows what types of nasty bugs that
> > manifest in seemingly unrelated places?  It will be a maintenance cost.
> 
> I agree 100% with Mo. If values are being cached at the Blink level then they
> need to be validated. We should try to remove as much caching and validation
> from the Blink side as possible.

Also: this CL is not a refactoring -- please see
https://en.wikipedia.org/wiki/Code_refactoring -- a refactoring restructures
existing code without changing its external behavior. This CL (probably) changes
the visible behavior in error states. not lgtm

[yunchao, 2015-12-31 08:36:17]

Patchset #4 (id:80001) has been deleted

[yunchao, 2015-12-31 08:46:45]

Description was changed from

==========
WebGL code refactoring: remove validation code for vertexAttribPointer and
vertexAttribIPointer.

BUG=570453
==========

to

==========
Blink WebGL: remove validation code for vertexAttribPointer
vertexAttribIPointer.

BUG=570453
==========

[yunchao, 2015-12-31 08:47:05]

Description was changed from

==========
Blink WebGL: remove validation code for vertexAttribPointer
vertexAttribIPointer.

BUG=570453
==========

to

==========
WebGL: remove validation code for vertexAttribPointer vertexAttribIPointer.

BUG=570453
==========

[yunchao, 2015-12-31 14:38:14]

> > > On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > > > When you remove the above validation, then you ended up caching the
wrong
> > here
> > > > info if some args are invalid.
> > > 
> > > Yeah, that's true. But Chromium will report error/failure soon in command
> > > buffer. So the web developers will know that their app have some bugs. The
> > wrong
> > > data cached here will not be used then. I think this is OK. In my opinion,
> we
> > > need to make sure that intentional or unintentional wrong parameter(s)
will
> > > trigger appropriate error. And it should not lead to some security
> > problem(crash
> > > the browser, memory leak, etc). WDYT?
> > > 
> > > I think we would have the similar cases when we do code-refactoring for
> other
> > > APIs.
> > 
> > I have a different opinion on this.  When we cache the wrong data, the data
> can
> > be used without the understanding that the data could be wrong.  I'd rather
> not
> > have to worry about this. Otherwise who knows what types of nasty bugs that
> > manifest in seemingly unrelated places?  It will be a maintenance cost.
> 
> I agree 100% with Mo. If values are being cached at the Blink level then they
> need to be validated. We should try to remove as much caching and validation
> from the Blink side as possible.

Hi, Ken and Zhenyao, I removed almost all cached data from VertexAttribState in
WebGLVertexArrayObjectBase. Now only ArrayBuffer and ElementArrayBuffer are
stored in Blink side VAO. Other items are removed. And these items have been
cached in Command buffer already. 

VertexAttribPointer only store ArrayBuffer and index into VertexAttribState,
both of them have been validated now.

Could you have a look?

[yunchao, 2015-12-31 14:40:06]

Description was changed from

==========
WebGL: remove validation code for vertexAttribPointer vertexAttribIPointer.

BUG=570453
==========

to

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed most of the items cached in VertexAttribState in 
WebGLVertexArrayObjectBase.

BUG=570453
==========

[Zhenyao Mo, 2016-01-04 22:17:22]

On 2015/12/31 14:38:14, yunchao wrote:
> > > > On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > > > > When you remove the above validation, then you ended up caching the
> wrong
> > > here
> > > > > info if some args are invalid.
> > > > 
> > > > Yeah, that's true. But Chromium will report error/failure soon in
command
> > > > buffer. So the web developers will know that their app have some bugs.
The
> > > wrong
> > > > data cached here will not be used then. I think this is OK. In my
opinion,
> > we
> > > > need to make sure that intentional or unintentional wrong parameter(s)
> will
> > > > trigger appropriate error. And it should not lead to some security
> > > problem(crash
> > > > the browser, memory leak, etc). WDYT?
> > > > 
> > > > I think we would have the similar cases when we do code-refactoring for
> > other
> > > > APIs.
> > > 
> > > I have a different opinion on this.  When we cache the wrong data, the
data
> > can
> > > be used without the understanding that the data could be wrong.  I'd
rather
> > not
> > > have to worry about this. Otherwise who knows what types of nasty bugs
that
> > > manifest in seemingly unrelated places?  It will be a maintenance cost.
> > 
> > I agree 100% with Mo. If values are being cached at the Blink level then
they
> > need to be validated. We should try to remove as much caching and validation
> > from the Blink side as possible.
> 
> Hi, Ken and Zhenyao, I removed almost all cached data from VertexAttribState
in
> WebGLVertexArrayObjectBase. Now only ArrayBuffer and ElementArrayBuffer are
> stored in Blink side VAO. Other items are removed. And these items have been
> cached in Command buffer already. 
> 
> VertexAttribPointer only store ArrayBuffer and index into VertexAttribState,
> both of them have been validated now.
> 
> Could you have a look?

Apparently something was removed incorrectly as the conformance tests start
failing.

[yunchao, 2016-01-05 13:58:42]

On 2016/01/04 22:17:22, Zhenyao Mo wrote:
> On 2015/12/31 14:38:14, yunchao wrote:
> > > > > On 2015/12/29 22:04:04, Zhenyao Mo wrote:
> > > > > > When you remove the above validation, then you ended up caching the
> > wrong
> > > > here
> > > > > > info if some args are invalid.
> > > > > 
> > > > > Yeah, that's true. But Chromium will report error/failure soon in
> command
> > > > > buffer. So the web developers will know that their app have some bugs.
> The
> > > > wrong
> > > > > data cached here will not be used then. I think this is OK. In my
> opinion,
> > > we
> > > > > need to make sure that intentional or unintentional wrong parameter(s)
> > will
> > > > > trigger appropriate error. And it should not lead to some security
> > > > problem(crash
> > > > > the browser, memory leak, etc). WDYT?
> > > > > 
> > > > > I think we would have the similar cases when we do code-refactoring
for
> > > other
> > > > > APIs.
> > > > 
> > > > I have a different opinion on this.  When we cache the wrong data, the
> data
> > > can
> > > > be used without the understanding that the data could be wrong.  I'd
> rather
> > > not
> > > > have to worry about this. Otherwise who knows what types of nasty bugs
> that
> > > > manifest in seemingly unrelated places?  It will be a maintenance cost.
> > > 
> > > I agree 100% with Mo. If values are being cached at the Blink level then
> they
> > > need to be validated. We should try to remove as much caching and
validation
> > > from the Blink side as possible.
> > 
> > Hi, Ken and Zhenyao, I removed almost all cached data from VertexAttribState
> in
> > WebGLVertexArrayObjectBase. Now only ArrayBuffer and ElementArrayBuffer are
> > stored in Blink side VAO. Other items are removed. And these items have been
> > cached in Command buffer already. 
> > 
> > VertexAttribPointer only store ArrayBuffer and index into VertexAttribState,
> > both of them have been validated now.
> > 
> > Could you have a look?
> 
> Apparently something was removed incorrectly as the conformance tests start
> failing.

The failure is fixed. The other failures seem to be irrelevant. Could you have a
look again? Zhenyao and Ken.

[Zhenyao Mo, 2016-01-05 21:13:21]

lgtm

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
(right):

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h:26: class
VertexAttribState final : public GarbageCollected<VertexAttribState> {
Do we still need this class?  Can't we just put bufferBinding as a direct member
of WebGLVertexArrayObjectBase?

[yunchao, 2016-01-06 00:19:24]

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

[commit-bot: I haz the power, 2016-01-06 00:20:52]

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

[commit-bot: I haz the power, 2016-01-06 00:20:53]

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

[commit-bot: I haz the power, 2016-01-06 00:20:54]

Dry run: A disapproval has been posted by following reviewers: kbr@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Ken Russell (switch to Gerrit), 2016-01-06 03:55:27]

Yunchao, thanks, this looks a lot better.

lgtm at this point with one cleanup request.

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
(right):

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h:26: class
VertexAttribState final : public GarbageCollected<VertexAttribState> {
On 2016/01/05 21:13:21, Zhenyao Mo wrote:
> Do we still need this class?  Can't we just put bufferBinding as a direct
member
> of WebGLVertexArrayObjectBase?

It looks to me like m_vertexAttribState could be changed to:

HeapVector&lt;Member&lt;WebGLBuffer&gt;&gt; m_boundArrayBuffers;

and yes, that this class could be removed.

[yunchao, 2016-01-06 07:02:07]

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

[commit-bot: I haz the power, 2016-01-06 07:02:53]

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

[commit-bot: I haz the power, 2016-01-06 08:21:51]

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

[commit-bot: I haz the power, 2016-01-06 08:21:51]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-01-06 16:45:30]

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

[commit-bot: I haz the power, 2016-01-06 16:45:56]

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

[yunchao, 2016-01-06 17:31:42]

Thanks for your review, Zhenyao and Ken. I have updated the code to remove
VertexAttribState from WebGLVertexArrayObject. 

PTAL again when you have time. Thanks a lot!

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
(right):

https://codereview.chromium.org/1555523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h:26: class
VertexAttribState final : public GarbageCollected<VertexAttribState> {
On 2016/01/06 03:55:27, Ken Russell wrote:
> On 2016/01/05 21:13:21, Zhenyao Mo wrote:
> > Do we still need this class?  Can't we just put bufferBinding as a direct
> member
> > of WebGLVertexArrayObjectBase?
> 
> It looks to me like m_vertexAttribState could be changed to:
> 
> HeapVector&lt;Member&lt;WebGLBuffer&gt;&gt; m_boundArrayBuffers;
> 
> and yes, that this class could be removed.

Thanks for your review, Zhenyao and Ken. I have tried to remove
VertexAttribState, but It need some change in getVertexAttrib at that time.
Please see the other comment.

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:3482:
return WebGLAny(scriptState, buffer.get());
The simplest way is to use "WebGLBuffer* buffer" to get ArrayBuffer for attrib.
However, this line would treat "buffer" as "boolean" when overloading, but it
treated m_boundArrayBuffer.get() as "WebGLBuffer *" when
overloading,(interesting). It is not correct to return a bool to JS. 

I tried many methods to fix this bug today, like 1) PassRefPtr<WebGLBuffer> (It
doesn't work), 
2)adding a function variable in WebGLRenderingContextBase.h, say
m_arrayBufferForAttrib, and trace it by the visitor(seems to be ugly). 

Now I found "WeakMember<WebGLBuffer>" . Seems that it is OK.

[Zhenyao Mo, 2016-01-06 18:04:08]

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:3482:
return WebGLAny(scriptState, buffer.get());
On 2016/01/06 17:31:42, yunchao wrote:
> The simplest way is to use "WebGLBuffer* buffer" to get ArrayBuffer for
attrib.
> However, this line would treat "buffer" as "boolean" when overloading, but it
> treated m_boundArrayBuffer.get() as "WebGLBuffer *" when
> overloading,(interesting). It is not correct to return a bool to JS. 
> 
> I tried many methods to fix this bug today, like 1) PassRefPtr<WebGLBuffer>
(It
> doesn't work), 
> 2)adding a function variable in WebGLRenderingContextBase.h, say
> m_arrayBufferForAttrib, and trace it by the visitor(seems to be ugly). 
> 
> Now I found "WeakMember<WebGLBuffer>" . Seems that it is OK. 

Can't you just return WebGLAny(scriptState,
m_boundVertexArrayObject->getArrayBufferForAttrb(index)?

in getParameter(), we do the same for GL_ELEMENT_ARRAY_BUFFER_BINDING.  I really
can't imagine why it works there but not here.

[commit-bot: I haz the power, 2016-01-06 18:40:40]

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

[commit-bot: I haz the power, 2016-01-06 18:40:41]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-01-06 20:53:54]

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:3482:
return WebGLAny(scriptState, buffer.get());
On 2016/01/06 18:04:08, Zhenyao Mo wrote:
> On 2016/01/06 17:31:42, yunchao wrote:
> > The simplest way is to use "WebGLBuffer* buffer" to get ArrayBuffer for
> attrib.
> > However, this line would treat "buffer" as "boolean" when overloading, but
it
> > treated m_boundArrayBuffer.get() as "WebGLBuffer *" when
> > overloading,(interesting). It is not correct to return a bool to JS. 
> > 
> > I tried many methods to fix this bug today, like 1) PassRefPtr<WebGLBuffer>
> (It
> > doesn't work), 
> > 2)adding a function variable in WebGLRenderingContextBase.h, say
> > m_arrayBufferForAttrib, and trace it by the visitor(seems to be ugly). 
> > 
> > Now I found "WeakMember<WebGLBuffer>" . Seems that it is OK. 
> 
> Can't you just return WebGLAny(scriptState,
> m_boundVertexArrayObject->getArrayBufferForAttrb(index)?
> 
> in getParameter(), we do the same for GL_ELEMENT_ARRAY_BUFFER_BINDING.  I
really
> can't imagine why it works there but not here.

It should definitely be possible to do this. There's no reason the code should
need to be different. The ScriptValue::createNull() code should be able to be
deleted now. Using a WeakMember here is incorrect.

[yunchao, 2016-01-07 00:04:29]

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

[commit-bot: I haz the power, 2016-01-07 00:05:18]

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

[yunchao, 2016-01-07 00:24:08]

Zhenyao and Ken, the code has been updated accordingly, please have a
double-check. Thanks.

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1555523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:3482:
return WebGLAny(scriptState, buffer.get());
On 2016/01/06 20:53:54, Ken Russell wrote:
> On 2016/01/06 18:04:08, Zhenyao Mo wrote:
> > On 2016/01/06 17:31:42, yunchao wrote:
> > > The simplest way is to use "WebGLBuffer* buffer" to get ArrayBuffer for
> > attrib.
> > > However, this line would treat "buffer" as "boolean" when overloading, but
> it
> > > treated m_boundArrayBuffer.get() as "WebGLBuffer *" when
> > > overloading,(interesting). It is not correct to return a bool to JS. 
> > > 
> > > I tried many methods to fix this bug today, like 1)
PassRefPtr<WebGLBuffer>
> > (It
> > > doesn't work), 
> > > 2)adding a function variable in WebGLRenderingContextBase.h, say
> > > m_arrayBufferForAttrib, and trace it by the visitor(seems to be ugly). 
> > > 
> > > Now I found "WeakMember<WebGLBuffer>" . Seems that it is OK. 
> > 
> > Can't you just return WebGLAny(scriptState,
> > m_boundVertexArrayObject->getArrayBufferForAttrb(index)?
> > 
> > in getParameter(), we do the same for GL_ELEMENT_ARRAY_BUFFER_BINDING.  I
> really
> > can't imagine why it works there but not here.
> 
> It should definitely be possible to do this. There's no reason the code should
> need to be different. The ScriptValue::createNull() code should be able to be
> deleted now. Using a WeakMember here is incorrect.


Ah... I have made a detour, and it turned out to be a wrong way.

[Zhenyao Mo, 2016-01-07 00:46:36]

lgtm

[commit-bot: I haz the power, 2016-01-07 01:23:18]

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

[commit-bot: I haz the power, 2016-01-07 01:23:19]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-01-07 02:35:20]

Description was changed from

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed most of the items cached in VertexAttribState in 
WebGLVertexArrayObjectBase.

BUG=570453
==========

to

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in
WebGLVertexArrayObjectBase.

BUG=570453
==========

[yunchao, 2016-01-07 05:00:32]

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

[yunchao, 2016-01-07 05:00:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1555523002/#ps180001
(title: "addressed feedbacks from Zhenyao and Ken")

[commit-bot: I haz the power, 2016-01-07 05:00:51]

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

[commit-bot: I haz the power, 2016-01-07 05:05:46]

Description was changed from

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in
WebGLVertexArrayObjectBase.

BUG=570453
==========

to

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in
WebGLVertexArrayObjectBase.

BUG=570453
==========

[commit-bot: I haz the power, 2016-01-07 05:05:47]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-01-07 05:06:44]

Description was changed from

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in
WebGLVertexArrayObjectBase.

BUG=570453
==========

to

==========
WebGL: remove validation code for vertexAttribPointer and vertexAttribIPointer.
This change also removed items cached in VertexAttribState in
WebGLVertexArrayObjectBase.

BUG=570453

Committed: https://crrev.com/ab18bc73dddce5b609a2d73239ab3874b2a5382d
Cr-Commit-Position: refs/heads/master@{#368015}
==========

[commit-bot: I haz the power, 2016-01-07 05:06:45]

Patchset 8 (id:??) landed as
https://crrev.com/ab18bc73dddce5b609a2d73239ab3874b2a5382d
Cr-Commit-Position: refs/heads/master@{#368015}