implement getBufferSubDataAsync prototype

third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
index 3d352b67a928bbbc0d3d3414aab1d80bae729430..4888c55b55adc0d6c929660115e83945e9cee893 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
@@ -5,11 +5,13 @@
 #include "modules/webgl/WebGL2RenderingContextBase.h"
 
 #include "bindings/modules/v8/WebGLAny.h"
+#include "core/dom/DOMException.h"
 #include "core/frame/ImageBitmap.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLImageElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/ImageData.h"
+#include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "modules/webgl/WebGLActiveInfo.h"
 #include "modules/webgl/WebGLBuffer.h"
@@ -187,6 +189,15 @@ WebGL2RenderingContextBase::~WebGL2RenderingContextBase() {
   m_currentTransformFeedbackPrimitivesWrittenQuery = nullptr;
 }
 
+void WebGL2RenderingContextBase::destroyContext() {
+  for (auto& callback : m_getBufferSubDataAsyncCallbacks) {
+    callback->destroy();
+  }
+  m_getBufferSubDataAsyncCallbacks.clear();
+
+  WebGLRenderingContextBase::destroyContext();
+}
+
 void WebGL2RenderingContextBase::initializeNewContext() {
   ASSERT(!isContextLost());
   ASSERT(drawingBuffer());
@@ -389,6 +400,161 @@ void WebGL2RenderingContextBase::getBufferSubData(GLenum target,
   contextGL()->UnmapBuffer(target);
 }
 
+WebGL2RenderingContextBase::GetBufferSubDataAsyncCallback::
+    GetBufferSubDataAsyncCallback(WebGL2RenderingContextBase* context,
+                                  DOMArrayBufferView* dstData,
+                                  ScriptPromiseResolver* resolver,
+                                  void* subBaseAddress,
+                                  void* mappedData,

[esprehn, 2016/10/08 03:02:48]

What's the purpose and relationship of these two void* ptrs? That's pretty
unusual in blink, are there things you can CHECK below about them?

[Kai Ninomiya, 2016/10/11 01:34:13]

I've clarified this a lot and added some DCHECKs.

+                                  long long subByteLength)
+    : context(context),
+      dstData(dstData),
+      resolver(resolver),
+      subBaseAddress(subBaseAddress),
+      mappedData(mappedData),
+      subByteLength(subByteLength) {}
+
+void WebGL2RenderingContextBase::GetBufferSubDataAsyncCallback::destroy() {
+  context->contextGL()->FreeSharedMemory(mappedData);

[esprehn, 2016/10/08 03:02:48]

do these fields need to be public?

[Kai Ninomiya, 2016/10/11 01:34:13]

Not sure what you mean. The fields of (WebGL)GetBufferSubDataAsyncCallback don't
need to be (and are not) public.

+  mappedData = nullptr;

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

Upon entry please DCHECK(mappedData != NULL). We should not attempt to destroy
these callbacks twice.

[Kai Ninomiya, 2016/10/11 01:34:14]

Done.

+  DOMException* exception =
+      DOMException::create(InvalidStateError, "context lost or destroyed");

[esprehn, 2016/10/08 03:02:48]

Capitalize error messages. "Context ..."

[Kai Ninomiya, 2016/10/11 01:34:13]

Done.

+  this->resolver->reject(exception);
+}
+
+void WebGL2RenderingContextBase::GetBufferSubDataAsyncCallback::resolve(
+    GetBufferSubDataAsyncCallback* self) {
+  if (!self->context || !self->mappedData) {

[esprehn, 2016/10/08 03:02:48]

lets move this into a method instead, you can proxy back if you need a static,
then you can avoid all of this self->

it's just self->doSomething()

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

+1. That would make this code more comprehensible.

[Kai Ninomiya, 2016/10/11 01:34:14]

Done.

+    DOMException* exception =
+        DOMException::create(InvalidStateError, "context lost or destroyed");

[esprehn, 2016/10/08 03:02:48]

ditto

[Kai Ninomiya, 2016/10/11 01:34:14]

Done.

+    self->resolver->reject(exception);
+    return;
+  }
+  if (self->dstData->buffer()->isNeutered()) {
+    DOMException* exception = DOMException::create(
+        InvalidStateError, "ArrayBufferView became invalid asynchronously");
+    self->resolver->reject(exception);
+    return;
+  }
+  memcpy(self->subBaseAddress, self->mappedData, self->subByteLength);
+  // XXX: What would happen if the DOM was suspended when the promise

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

It's OK to leave this question in place but please make it TODO(kainino) per the
style guide: https://google.github.io/styleguide/cppguide.html#TODO_Comments

[Kai Ninomiya, 2016/10/11 01:34:14]

Still working on this issue, will address this comment before committing.

+  // became resolved? Could another JS task happen between the memcpy
+  // and the promise resolution task?
+  self->resolver->resolve(self->dstData);
+
+  self->destroy();
+  self->context->unregisterGetBufferSubDataAsyncCallback(self);
+}
+
+ScriptPromise WebGL2RenderingContextBase::getBufferSubDataAsync(

[esprehn, 2016/10/08 03:02:49]

This function is massive, can we make it into smaller steps?

[Kai Ninomiya, 2016/10/11 01:34:14]

Great point, thanks. I will work on this tomorrow.

[Kai Ninomiya, 2016/10/11 21:54:43]

I factored out a lot of common validation between getBufferSubData and
getBufferSubDataAsync, which makes it much shorter.

+    ScriptState* scriptState,
+    GLenum target,
+    GLintptr srcByteOffset,
+    DOMArrayBufferView* dstData,
+    GLuint dstOffset,
+    GLuint length) {
+  ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+  ScriptPromise promise = resolver->promise();
+
+  const char* funcName = "getBufferSubDataAsync";

[esprehn, 2016/10/08 03:02:48]

static ?, probably kFunctionName.

Don't abbreviate in blink, actually isn't there a C++ thing for like this
__FUNCTION__?

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

Thanks Elliott for pointing that out. Kai, it does look like other code in
Chromium relies on the __FUNCTION__ macro working, so let's use that here.

[Kai Ninomiya, 2016/10/11 01:34:14]

Done. There is future work to do if we want to remove the "funcName" pattern in
other parts of this file.

+  if (isContextLost()) {
+    DOMException* exception =
+        DOMException::create(InvalidStateError, "context lost");
+    resolver->reject(exception);
+    return promise;
+  }
+  if (!validateValueFitNonNegInt32(funcName, "srcByteOffset", srcByteOffset)) {
+    DOMException* exception =
+        DOMException::create(InvalidStateError, "invalid value: srcByteOffset");
+    resolver->reject(exception);
+    return promise;
+  }
+  if (target == GL_TRANSFORM_FEEDBACK_BUFFER && m_transformFeedbackBinding) {
+    // XXX: is this restriction enforced in the original getBufferSubData? Does

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

It looks like it isn't enforced and it should be. Could you file a follow-on bug
about doing that? Then this comment (which should be a TODO) can be removed. It
looks like that needs to be tested in conformance2/buffers/getBufferSubData.html
, too.

[Kai Ninomiya, 2016/10/11 01:34:13]

Filed and assigned to myself: http://crbug.com/654609

+    // it need to be added there, or can it be removed here?
+    synthesizeGLError(GL_INVALID_OPERATION, funcName,
+                      "targeted transform feedback buffer is bound");
+    DOMException* exception = DOMException::create(
+        InvalidStateError,
+        "invalid operation: targeted transform feedback buffer is bound");
+    resolver->reject(exception);
+    return promise;
+  }
+  WebGLBuffer* buffer = validateBufferDataTarget(funcName, target);
+  if (!buffer) {
+    DOMException* exception = DOMException::create(
+        InvalidStateError, "invalid operation: no buffer bound to target");
+    resolver->reject(exception);
+    return promise;
+  }
+
+  void* subBaseAddress = nullptr;
+  long long subByteLength = 0;

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

These "sub" names are confusing, especially when used within the callback. Could
you change the naming conventions to be more clear? Perhaps:

  - dstData -> destinationView
  - dstOffset -> destinationOffset
  - subBaseAddress -> destinationAddress
  - subByteLength -> destinationByteLength

?

If the naming convention for the output variables from
validateSubSourceAndGetData is well established in this file, though, don't
worry about this.

[Kai Ninomiya, 2016/10/11 01:34:14]

For dstData and dstOffset, I don't want to change these to mismatch the spec,
since the code for error messages will be very confusing.

For subBaseADdress and subByteLength, the names are well established. But I did
rename all of the fields in WebGLGetBufferSubDataAsyncCallback.

+  if (!validateSubSourceAndGetData(dstData, dstOffset, length, &subBaseAddress,
+                                   &subByteLength)) {
+    synthesizeGLError(GL_INVALID_VALUE, funcName,
+                      "dstOffset and length out of bounds of dstData");
+    DOMException* exception =
+        DOMException::create(InvalidStateError,
+                             "invalid value: dstOffset and length "
+                             "out of bounds of dstData");
+    resolver->reject(exception);
+    return promise;
+  }
+  if (subByteLength == 0) {

[esprehn, 2016/10/08 03:02:48]

if (!subByteLength)

[Kai Ninomiya, 2016/10/11 01:34:14]

Done.

+    resolver->resolve(dstData);
+    return promise;
+  }
+
+  CheckedNumeric<long long> dstEnd = subByteLength;

[esprehn, 2016/10/08 03:02:48]

everything down here seems like a separate function at least

[Kai Ninomiya, 2016/10/11 21:54:43]

Done.

+  dstEnd += srcByteOffset;
+  if (!dstEnd.IsValid() || dstEnd.ValueOrDie() > buffer->getSize()) {
+    synthesizeGLError(
+        GL_INVALID_VALUE, funcName,
+        "offset + numBytes would extend beyond the end of buffer");
+    DOMException* exception =
+        DOMException::create(InvalidStateError,
+                             "invalid value: offset + numBytes would extend "
+                             "beyond the end of buffer");

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

end of -> end of the?

[Kai Ninomiya, 2016/10/11 01:34:14]

Done.

+    resolver->reject(exception);
+    return promise;
+  }
+
+  GLuint queryID;
+  contextGL()->GenQueriesEXT(1, &queryID);
+  contextGL()->BeginQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM, queryID);
+  void* mappedData = contextGL()->GetBufferSubDataAsyncCHROMIUM(
+      target, srcByteOffset, subByteLength);
+  if (!mappedData) {
+    DOMException* exception =
+        DOMException::create(InvalidStateError, "out of memory");
+    resolver->reject(exception);
+    return promise;
+  }
+  contextGL()->EndQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM);
+
+  auto callbackObject = adoptRef(new GetBufferSubDataAsyncCallback(
+      this, dstData, resolver, subBaseAddress, mappedData, subByteLength));
+  registerGetBufferSubDataAsyncCallback(callbackObject.get());
+  auto callback =
+      WTF::bind(&GetBufferSubDataAsyncCallback::resolve, callbackObject);
+  drawingBuffer()->contextProvider()->signalQuery(
+      queryID, convertToBaseCallback(std::move(callback)));
+  contextGL()->DeleteQueriesEXT(1, &queryID);

[Ken Russell (switch to Gerrit), 2016/10/10 22:21:30]

It seems incorrect to delete the query here. It seems to me it should be deleted
in the completion callback. See this code for reference:
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?q=SignalQuery&s...

Maybe the SignalQuery call implicitly references the query object until the
callback is called, though, making the DeleteQueriesEXT call a no-op. Did you
investigate this?

[Kai Ninomiya, 2016/10/11 01:34:14]

I didn't investigate this previously, but I wrote it assuming that the deletion
wouldn't be processed by the command buffer until after the query was signalled
(since both are ordered in the command stream). I'm not sure whether that is
true.

The query is removed from the client side query tracker immediately, but pretty
much everything else is on the service side, which I haven't looked into. The
round trip makes it difficult to figure out. Of course, it works currently, but
I will move it since I think it makes more sense.

+
+  return promise;
+}
+
+void WebGL2RenderingContextBase::registerGetBufferSubDataAsyncCallback(
+    GetBufferSubDataAsyncCallback* callback) {
+  m_getBufferSubDataAsyncCallbacks.insert(callback);
+}
+
+void WebGL2RenderingContextBase::unregisterGetBufferSubDataAsyncCallback(
+    GetBufferSubDataAsyncCallback* callback) {
+  m_getBufferSubDataAsyncCallbacks.erase(callback);
+}
+
 void WebGL2RenderingContextBase::blitFramebuffer(GLint srcX0,
                                                  GLint srcY0,
                                                  GLint srcX1,