Update WebGLVertexArrayObjectOES to WebGLVertexArrayObject for WebGL2

Update WebGLVertexArrayObjectOES to WebGLVertexArrayObject for WebGL2

Keep using WebGLVertexArrayObjectOES in WebGL1 for compatibility.

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

[legend, 2015-05-26 06:02:24]

Patchset #1 (id:1) has been deleted

[legend, 2015-05-26 06:06:16]

guanxian.li@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[legend, 2015-05-26 06:08:07]

PTAL if you have time. Thanks!

[bajones, 2015-05-26 16:45:50]

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

https://codereview.chromium.org/1157983002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGL2RenderingContextBase.cpp:66: // Release
WebGLVertexArrayObjectOES which is initialized at WebGLRenderingContextBase.
Rather than allocate/release/allocate again could you move this allocation above
WebGLRenderingContextBase::initializeNewContext() and in that function check for
if (!m_defaultVertexArrayObject) before allocating? It's a minor change but it
feels cleaner.

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

https://codereview.chromium.org/1157983002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectImpl.h:15: class
WebGLVertexArrayObjectImpl : public WebGLContextObject {
WebGLVertexArrayObjectImpl seems like the wrong name pattern here. Typically we
use that for an implementation of a abstract interface. Maybe
WebGLVertexArrayObjectBase instead?

[legend, 2015-05-27 02:05:47]

Thanks for bajones@ comments. Please review again.

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

https://codereview.chromium.org/1157983002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGL2RenderingContextBase.cpp:66: // Release
WebGLVertexArrayObjectOES which is initialized at WebGLRenderingContextBase.
On 2015/05/26 16:45:50, bajones wrote:
> Rather than allocate/release/allocate again could you move this allocation
above
> WebGLRenderingContextBase::initializeNewContext() and in that function check
for
> if (!m_defaultVertexArrayObject) before allocating? It's a minor change but it
> feels cleaner.

Done. Since m_defaultVertexArrayObject constructor depends on
m_maxVertexAttribs, the initialization of m_maxVertexAttrib needs to added
before.

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

https://codereview.chromium.org/1157983002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectImpl.h:15: class
WebGLVertexArrayObjectImpl : public WebGLContextObject {
On 2015/05/26 16:45:50, bajones wrote:
> WebGLVertexArrayObjectImpl seems like the wrong name pattern here. Typically
we
> use that for an implementation of a abstract interface. Maybe
> WebGLVertexArrayObjectBase instead?

Done. Agree. My carelessness.

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

https://codereview.chromium.org/1157983002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:726: m_maxVertexAttribs =
numVertexAttribs;
The initialization of m_maxVertexAttribs is moved to here.

[legend, 2015-05-27 09:20:05]

PTAL.
Add one patch set to fix crash in
conformance/context/context-lost-restored.html.

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

https://codereview.chromium.org/1157983002/diff/60001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:728: if
(isWebGL2OrHigher()) {
@bajones, since `if (!m_defaultVertexArrayObject)` would lead to crash when
testing conformance/context/context-lost-restored.html, it changes to initialize
in this way in order to fix crash and make clearer. What do you think about
this?

[bajones, 2015-05-27 18:26:19]

On 2015/05/27 09:20:05, legend wrote:
> @bajones, since `if (!m_defaultVertexArrayObject)` would lead to crash when
> testing conformance/context/context-lost-restored.html, it changes to
initialize
> in this way in order to fix crash and make clearer. What do you think about
> this?

Seems reasonable to me. Thanks for updating!

Non-owners LGTM.

[Ken Russell (switch to Gerrit), 2015-05-27 23:43:58]

Thanks, good refactoring. It's a pity that two classes are needed just because
WebGLVertexArrayObjectOES is [NoInterfaceObject] but WebGLVertexArrayObject
isn't, but so be it. LGTM

[legend, 2015-05-28 01:24:18]

The CQ bit was checked by guanxian.li@intel.com

[commit-bot: I haz the power, 2015-05-28 01:24:50]

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

[commit-bot: I haz the power, 2015-05-28 04:22:14]

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