WebSocket Pepper API: WebArrayBuffer support

webkit/plugins/ppapi/ppb_websocket_impl.cc

Index: webkit/plugins/ppapi/ppb_websocket_impl.cc
diff --git a/webkit/plugins/ppapi/ppb_websocket_impl.cc b/webkit/plugins/ppapi/ppb_websocket_impl.cc
index 00dd7f504eb525db2661e39a06f57a1955ea32e4..8bdcf57540b0b572ddb3c2fbe4292a74258b1935 100644
--- a/webkit/plugins/ppapi/ppb_websocket_impl.cc
+++ b/webkit/plugins/ppapi/ppb_websocket_impl.cc
@@ -18,13 +18,14 @@
 #include "ppapi/c/ppb_var.h"
 #include "ppapi/shared_impl/var.h"
 #include "ppapi/shared_impl/var_tracker.h"
-#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebData.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebArrayBuffer.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebElement.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebPluginContainer.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSocket.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURL.h"
+#include "webkit/plugins/ppapi/host_array_buffer_var.h"
 #include "webkit/plugins/ppapi/host_globals.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource_helper.h"
@@ -35,7 +36,7 @@ using ppapi::StringVar;
 using ppapi::thunk::PPB_WebSocket_API;
 using ppapi::TrackedCallback;
 using ppapi::VarTracker;
-using WebKit::WebData;
+using WebKit::WebArrayBuffer;
 using WebKit::WebDocument;
 using WebKit::WebString;
 using WebKit::WebSocket;
@@ -94,14 +95,6 @@ PPB_WebSocket_Impl::PPB_WebSocket_Impl(PP_Instance instance)
 PPB_WebSocket_Impl::~PPB_WebSocket_Impl() {
   if (websocket_.get())
     websocket_->disconnect();
-
-  // Clean up received and unread messages
-  VarTracker* var_tracker = PpapiGlobals::Get()->GetVarTracker();
-  while (!received_messages_.empty()) {
-    PP_Var var = received_messages_.front();
-    received_messages_.pop();
-    var_tracker->ReleaseVar(var);
-  }
 }
 
 // static
@@ -203,6 +196,9 @@ int32_t PPB_WebSocket_Impl::Connect(PP_Var url,
   if (!websocket_.get())
     return PP_ERROR_NOTSUPPORTED;
 
+  // Set receiving binary object type.
+  websocket_->setBinaryType(static_cast<WebSocket::BinaryType>(binary_type_));

[dmichael (off chromium), 2012/01/13 17:36:14]

It seems like it would be a good idea to either convert explicitly with a switch
statement or to have assertions somewhere (maybe even statically) so you know
the values match up.

[Takashi Toyoshima, 2012/01/17 10:14:59]

OK, I hold this value as WebSocket::BinaryType, then explicit conversion is done
in Set/GetBinaryType().

+
   websocket_->connect(web_url, web_protocols);
   state_ = PP_WEBSOCKETREADYSTATE_CONNECTING_DEV;
 
@@ -357,17 +353,13 @@ int32_t PPB_WebSocket_Impl::SendMessage(PP_Var message) {
     if (!websocket_->sendText(web_message))
       return PP_ERROR_BADARGUMENT;
   } else if (message.type == PP_VARTYPE_ARRAY_BUFFER) {
-    // Convert message to WebData.
-    // TODO(toyoshim): Must add a WebKit interface which handles WebArrayBuffer
-    // directly.
-    scoped_refptr<ArrayBufferVar> message_array_buffer =
-        ArrayBufferVar::FromPPVar(message);
-    if (!message_array_buffer)
+    // Convert message to WebArrayBuffer.
+    scoped_refptr<HostArrayBufferVar> host_message =
+        static_cast<HostArrayBufferVar*>(ArrayBufferVar::FromPPVar(message));
+    if (!host_message)
       return PP_ERROR_BADARGUMENT;
-    WebData web_message = WebData(
-        static_cast<const char*>(message_array_buffer->Map()),
-        static_cast<size_t>(message_array_buffer->ByteLength()));
-    if (!websocket_->sendBinary(web_message))
+    WebArrayBuffer& web_message = host_message->webkit_buffer();
+    if (!websocket_->sendArrayBuffer(web_message))
       return PP_ERROR_BADARGUMENT;
   } else {
     // TODO(toyoshim): Support Blob.
@@ -424,8 +416,11 @@ PP_Var PPB_WebSocket_Impl::GetURL() {
 
 PP_Bool PPB_WebSocket_Impl::SetBinaryType(
     PP_WebSocketBinaryType_Dev binary_type) {
-  // TODO(toyoshim): Use WebKit new API to set the receiving binary type.
-  return PP_FALSE;
+  binary_type_ = binary_type;
+  if (websocket_.get())
+    websocket_->setBinaryType(
+        static_cast<WebSocket::BinaryType>(binary_type_));

[dmichael (off chromium), 2012/01/13 17:36:14]

Ditto, some better checking would be good here. Recall that SetBinaryType is
called by the plugin, and is free to send us bad values. It's very easy to send
us an enum to a value that's not actually valid.

[Takashi Toyoshima, 2012/01/17 10:14:59]

Done.

+  return PP_TRUE;
 }
 
 PP_WebSocketBinaryType_Dev PPB_WebSocket_Impl::GetBinaryType() {
@@ -445,8 +440,7 @@ void PPB_WebSocket_Impl::didReceiveMessage(const WebString& message) {
 
   // Append received data to queue.
   std::string string = message.utf8();
-  PP_Var var = StringVar::StringToPPVar(string);
-  received_messages_.push(var);
+  received_messages_.push(new StringVar(string));

[dmichael (off chromium), 2012/01/13 17:36:14]

So now the vars in your queue have no ID and no ref-count. That might be okay,
but you have to be careful about your use of the Vars and their ref-counts.

I think you probably need to at least use scoped_refptr to give it an internal
reference. I think as-written that you're leaking vars, since the queue has raw
pointers and you never increment the StringVar's ref count.

[Takashi Toyoshima, 2012/01/17 10:14:59]

Oh, sorry. I confused on managing ref counted objects in quieue.
I hold them as scoped_refptr in order to decrement reference at destruction of
queue or pop.

 
   if (!wait_for_receive_)
     return;
@@ -454,17 +448,14 @@ void PPB_WebSocket_Impl::didReceiveMessage(const WebString& message) {
   TrackedCallback::ClearAndRun(&receive_callback_, DoReceive());
 }
 
-void PPB_WebSocket_Impl::didReceiveBinaryData(const WebData& binaryData) {
+void PPB_WebSocket_Impl::didReceiveArrayBuffer(
+    const WebArrayBuffer& binaryData) {
   // Dispose packets after receiving an error or in invalid state.
   if (error_was_received_ || !InValidStateToReceive(state_))
     return;
 
   // Append received data to queue.
-  PP_Var var = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
-      binaryData.size());
-  scoped_refptr<ArrayBufferVar> arraybuffer = ArrayBufferVar::FromPPVar(var);
-  void* data = arraybuffer->Map();
-  memcpy(data, binaryData.data(), binaryData.size());
+  scoped_refptr<ArrayBufferVar> var = new HostArrayBufferVar(binaryData);
   received_messages_.push(var);

[dmichael (off chromium), 2012/01/13 17:36:14]

Does this compile, to pass a scoped_refptr when a raw pointer is expected? If
so, that seems like a big hole in scoped_refptr... I think in this case, the var
gets deleted when you exit scope, so you probably have use-after-free problems.

[Takashi Toyoshima, 2012/01/17 10:14:59]

Fixed.

 
   if (!wait_for_receive_)
@@ -548,7 +539,7 @@ int32_t PPB_WebSocket_Impl::DoReceive() {
   if (!receive_callback_var_)
     return PP_OK;
 
-  *receive_callback_var_ = received_messages_.front();
+  *receive_callback_var_ = received_messages_.front()->GetPPVar();
   received_messages_.pop();
   receive_callback_var_ = NULL;
   wait_for_receive_ = false;