Implement buffer{Sub}Data overloads with sub source.

Implement buffer{Sub}Data overloads with sub source.

BUG=639145
TEST=webgl2_conformance
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/2f904fc2724f49084be34f7b730353a66770d8d4
Cr-Commit-Position: refs/heads/master@{#413671}

[Zhenyao Mo, 2016-08-19 21:06:01]

Description was changed from

==========
Implement buffer{Sub}Data overloads with sub source.

BUG=639145
TEST=webgl2_conformance
R=kbr@chromium.org
==========

to

==========
Implement buffer{Sub}Data overloads with sub source.

BUG=639145
TEST=webgl2_conformance
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-08-19 21:06:12]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[Zhenyao Mo, 2016-08-19 21:06:37]

kbr: PTAL

Tests will follow shortly after in github.

[commit-bot: I haz the power, 2016-08-19 21:07:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:21:29]

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

[commit-bot: I haz the power, 2016-08-19 21:21:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Zhenyao Mo, 2016-08-19 21:25:17]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:26:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 23:00:19]

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

[commit-bot: I haz the power, 2016-08-19 23:00:20]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Zhenyao Mo, 2016-08-19 23:45:54]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 23:46:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 01:37:12]

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

[commit-bot: I haz the power, 2016-08-20 01:37:13]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Ken Russell (switch to Gerrit), 2016-08-20 03:05:35]

Very nice. Looks good overall. A few questions and comments.

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

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:46:
SubArrayBufferView(WTF::ArrayBufferView::ViewType viewType, void*
viewBaseAddress, long long viewByteLength, GLuint subOffset, GLuint subLength)
Is "long long" the best type to use for viewByteLength? Is there a better, more
explicit one, like int64_t?

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:51: ,
m_subLength(subLength)
Since initialize() returns void, what about moving the call to the constructor,
and passing viewType, viewBaseAddress, etc. as arguments to it? That would
eliminate them as members and would reduce confusion about the possible states
of this class.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:61:
initialize();
Per above, suggest moving the call to initialize() to the constructor.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:99: }
Should there be a separate check like:

  if (!byteLength && byteOffset >= m_viewByteLength)
    return?

The intent would be to handle the case of an attempt to upload 0 bytes from the
byte just beyond the end of the view. I might have gotten this proposed check
wrong.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:104: data
+= byteOffset;
Any possibility of overflow on the address? Should this addition be done using
CheckedInt too? Or at least a DCHECK that data + byteOffset >=
m_viewBaseAddress, in unsigned arithmetic?

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:140: bool
m_initialized;
I think it's possible and desirable to eliminate all of these data members; see
above.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:293:
DCHECK(srcData);
Maybe worth adding a comment that this is guaranteed non-null by the DOM
bindings.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:294:
SubArrayBufferView subView(srcData->type(), srcData->baseAddress(),
srcData->byteLength(), srcOffset, length);
Perhaps the SubArrayBufferView constructor should just take the ArrayBufferView*
as argument, and fetch all of these properties from it. It would simplify the
call sites.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:390:
synthesizeGLError(GL_INVALID_VALUE, funcName, "dstOffset + length too large");
Return too, correct?

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl
(right):

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl:292: void
bufferSubData(GLenum target, GLintptr dstByteOffset, ArrayBufferView srcData,
GLuint srcOffset, optional GLuint length = 0);
Ah, so you didn't find it necessary to replicate the WebGL 1.0 bindings here,
like in the spec's IDL?

[Zhenyao Mo, 2016-08-20 03:44:23]

Will revise accordingly. Thanks for the review.

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

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:46:
SubArrayBufferView(WTF::ArrayBufferView::ViewType viewType, void*
viewBaseAddress, long long viewByteLength, GLuint subOffset, GLuint subLength)
On 2016/08/20 03:05:35, Ken Russell wrote:
> Is "long long" the best type to use for viewByteLength? Is there a better,
more
> explicit one, like int64_t?

This is to match all the other byteLength types in this class - probably change
them all in a separate CL.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:51: ,
m_subLength(subLength)
On 2016/08/20 03:05:35, Ken Russell wrote:
> Since initialize() returns void, what about moving the call to the
constructor,
> and passing viewType, viewBaseAddress, etc. as arguments to it? That would
> eliminate them as members and would reduce confusion about the possible states
> of this class.

That was my original design, but I remmebered we are not supposed to put heavy
computation into class constructor.  If you feel it's OK, I can revised.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:99: }
On 2016/08/20 03:05:35, Ken Russell wrote:
> Should there be a separate check like:
> 
>   if (!byteLength && byteOffset >= m_viewByteLength)
>     return?
> 
> The intent would be to handle the case of an attempt to upload 0 bytes from
the
> byte just beyond the end of the view. I might have gotten this proposed check
> wrong.

Looking at bufferSubData, where the offset is beyond the buffer object, even
with size 0, we still generate an INVALID_VALUE.  Here we should be consistent
with that.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:104: data
+= byteOffset;
On 2016/08/20 03:05:35, Ken Russell wrote:
> Any possibility of overflow on the address? Should this addition be done using
> CheckedInt too? Or at least a DCHECK that data + byteOffset >=
> m_viewBaseAddress, in unsigned arithmetic?

We already checked byteOffset is within the ArrayBufferView bounds, so this will
not overflow.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:140: bool
m_initialized;
On 2016/08/20 03:05:35, Ken Russell wrote:
> I think it's possible and desirable to eliminate all of these data members;
see
> above.

Now I think again, we can actually just do this in a function, so no need for a
class.  Will revise.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:293:
DCHECK(srcData);
On 2016/08/20 03:05:35, Ken Russell wrote:
> Maybe worth adding a comment that this is guaranteed non-null by the DOM
> bindings.

Will do.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:294:
SubArrayBufferView subView(srcData->type(), srcData->baseAddress(),
srcData->byteLength(), srcOffset, length);
On 2016/08/20 03:05:35, Ken Russell wrote:
> Perhaps the SubArrayBufferView constructor should just take the
ArrayBufferView*
> as argument, and fetch all of these properties from it. It would simplify the
> call sites.

The reason I didn't pass DOMArrayBufferView is that it's a JS object, so if we
keep it as a class member, the class becomes complicated.  Since we can revise
it to a function, this is no longer an concern.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:390:
synthesizeGLError(GL_INVALID_VALUE, funcName, "dstOffset + length too large");
On 2016/08/20 03:05:35, Ken Russell wrote:
> Return too, correct?

Ah thanks for catching this.

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl
(right):

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl:292: void
bufferSubData(GLenum target, GLintptr dstByteOffset, ArrayBufferView srcData,
GLuint srcOffset, optional GLuint length = 0);
On 2016/08/20 03:05:35, Ken Russell wrote:
> Ah, so you didn't find it necessary to replicate the WebGL 1.0 bindings here,
> like in the spec's IDL?

Not really. I have to duplicate the overloaded function on the C++ side though.

[Ken Russell (switch to Gerrit), 2016-08-20 04:03:34]

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

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:51: ,
m_subLength(subLength)
On 2016/08/20 03:44:23, Zhenyao Mo wrote:
> On 2016/08/20 03:05:35, Ken Russell wrote:
> > Since initialize() returns void, what about moving the call to the
> constructor,
> > and passing viewType, viewBaseAddress, etc. as arguments to it? That would
> > eliminate them as members and would reduce confusion about the possible
states
> > of this class.
> 
> That was my original design, but I remmebered we are not supposed to put heavy
> computation into class constructor.  If you feel it's OK, I can revised.

I think it'd be OK, and quite a bit simpler.

[Ken Russell (switch to Gerrit), 2016-08-20 04:05:55]

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

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:99: }
On 2016/08/20 03:44:23, Zhenyao Mo wrote:
> On 2016/08/20 03:05:35, Ken Russell wrote:
> > Should there be a separate check like:
> > 
> >   if (!byteLength && byteOffset >= m_viewByteLength)
> >     return?
> > 
> > The intent would be to handle the case of an attempt to upload 0 bytes from
> the
> > byte just beyond the end of the view. I might have gotten this proposed
check
> > wrong.
> 
> Looking at bufferSubData, where the offset is beyond the buffer object, even
> with size 0, we still generate an INVALID_VALUE.  Here we should be consistent
> with that.

As long as the tests verify this, sounds good. It's not obvious to me that this
is what's enforced for zero-length uploads.

[Zhenyao Mo, 2016-08-23 00:22:42]

Revised, please review again.

This time, I didn't take out the getBufferSubData(ArrayBuffer) overload, so we
can land without marking failing tests.

When we fix the test to use new overload(with ArrayBufferView) and roll the fix
in, we can take the obsolete overload out.

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

https://codereview.chromium.org/2266563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:51: ,
m_subLength(subLength)
On 2016/08/20 04:03:34, Ken Russell wrote:
> On 2016/08/20 03:44:23, Zhenyao Mo wrote:
> > On 2016/08/20 03:05:35, Ken Russell wrote:
> > > Since initialize() returns void, what about moving the call to the
> > constructor,
> > > and passing viewType, viewBaseAddress, etc. as arguments to it? That would
> > > eliminate them as members and would reduce confusion about the possible
> states
> > > of this class.
> > 
> > That was my original design, but I remmebered we are not supposed to put
heavy
> > computation into class constructor.  If you feel it's OK, I can revised.
> 
> I think it'd be OK, and quite a bit simpler.

Done.

[Zhenyao Mo, 2016-08-23 00:22:53]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 00:24:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Russell (switch to Gerrit), 2016-08-23 00:28:28]

Very nice. Much simpler than the first iteration. LGTM

https://codereview.chromium.org/2266563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2266563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:43: //
FIXME: obsolete, remove after WebGL2 conformance tests are updated.
Change FIXME to TODO(zmo).

https://codereview.chromium.org/2266563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl
(right):

https://codereview.chromium.org/2266563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl:295: //
FIXME: obsolete, remove after WebGL2 conformance tests are updated.
FIXME -> TODO(zmo)

[Zhenyao Mo, 2016-08-23 00:32:41]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-08-23 00:32:41]

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/2266563002/#ps80001
(title: "fix comments")

[commit-bot: I haz the power, 2016-08-23 00:33:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 01:40:05]

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

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

Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Zhenyao Mo, 2016-08-23 02:05:43]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-08-23 02:05:43]

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/2266563002/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2016-08-23 02:06:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 05:21:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-23 05:25:44]

Description was changed from

==========
Implement buffer{Sub}Data overloads with sub source.

BUG=639145
TEST=webgl2_conformance
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Implement buffer{Sub}Data overloads with sub source.

BUG=639145
TEST=webgl2_conformance
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/2f904fc2724f49084be34f7b730353a66770d8d4
Cr-Commit-Position: refs/heads/master@{#413671}
==========

[commit-bot: I haz the power, 2016-08-23 05:25:45]

Patchset 6 (id:??) landed as
https://crrev.com/2f904fc2724f49084be34f7b730353a66770d8d4
Cr-Commit-Position: refs/heads/master@{#413671}