Hook up V8<->Var conversion in the pepper message channel

webkit/plugins/ppapi/message_channel.cc

Index: webkit/plugins/ppapi/message_channel.cc
diff --git a/webkit/plugins/ppapi/message_channel.cc b/webkit/plugins/ppapi/message_channel.cc
index 4a9fdd09dc7b14f108200c6d12ef8921c37455aa..e5be512a59658afa6ccd83e26774247c0e6db092 100644
--- a/webkit/plugins/ppapi/message_channel.cc
+++ b/webkit/plugins/ppapi/message_channel.cc
@@ -26,6 +26,7 @@
 #include "webkit/plugins/ppapi/npapi_glue.h"
 #include "webkit/plugins/ppapi/plugin_module.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
+#include "webkit/plugins/ppapi/v8_var_converter.h"
 
 using ppapi::ArrayBufferVar;
 using ppapi::PpapiGlobals;
@@ -62,96 +63,83 @@ bool IdentifierIsPostMessage(NPIdentifier identifier) {
   return WebBindings::getStringIdentifier(kPostMessage) == identifier;
 }
 
-// Converts the given PP_Var to a v8::Value, returning true on success.
-// False means that the given variant is invalid. In this case, |result| will
-// be set to an empty handle.
-bool PPVarToV8Value(PP_Var var, v8::Handle<v8::Value>* result) {
+bool NPVariantToPPVar(const NPVariant* variant, PP_Var* result) {
+  switch (variant->type) {
+    case NPVariantType_Void:
+      *result = PP_MakeUndefined();
+      return true;
+    case NPVariantType_Null:
+      *result = PP_MakeNull();
+      return true;
+    case NPVariantType_Bool:
+      *result = PP_MakeBool(PP_FromBool(NPVARIANT_TO_BOOLEAN(*variant)));
+      return true;
+    case NPVariantType_Int32:
+      *result = PP_MakeInt32(NPVARIANT_TO_INT32(*variant));
+      return true;
+    case NPVariantType_Double:
+      *result = PP_MakeDouble(NPVARIANT_TO_DOUBLE(*variant));
+      return true;
+    case NPVariantType_String:
+      *result = StringVar::StringToPPVar(
+          NPVARIANT_TO_STRING(*variant).UTF8Characters,
+          NPVARIANT_TO_STRING(*variant).UTF8Length);
+      return true;
+    case NPVariantType_Object:
+      V8VarConverter converter;
+      // Calling WebBindings::toV8Value creates a wrapper around NPVariant so it
+      // shouldn't result in a deep copy.
+      return converter.FromV8Value(WebBindings::toV8Value(variant),
+                                   v8::Context::GetCurrent(), result);
+  }
+  return false;
+}
+
+// Copy a PP_Var in to a PP_Var that is appropriate for sending via postMessage.
+// This currently just copies the value.  For a string Var, the result is a
+// PP_Var with the a copy of |var|'s string contents and a reference count of 1.
+//
+// TODO(dmichael): Bypass this step for out-of-process plugins, since a copy
+// happens already when the Var is serialized.

[dmichael (off chromium), 2013/05/09 20:21:12]

Oops, this TODO has been done, so you can remove it.

[raymes, 2013/05/13 17:09:43]

Done.

+PP_Var CopyPPVar(const PP_Var& var) {
   switch (var.type) {
     case PP_VARTYPE_UNDEFINED:
-      *result = v8::Undefined();
-      break;
     case PP_VARTYPE_NULL:
-      *result = v8::Null();
-      break;
     case PP_VARTYPE_BOOL:
-      *result = (var.value.as_bool == PP_TRUE) ? v8::True() : v8::False();
-      break;
     case PP_VARTYPE_INT32:
-      *result = v8::Integer::New(var.value.as_int);
-      break;
     case PP_VARTYPE_DOUBLE:
-      *result = v8::Number::New(var.value.as_double);
-      break;
+      return var;
     case PP_VARTYPE_STRING: {
       StringVar* string = StringVar::FromPPVar(var);
-      if (!string) {
-        result->Clear();
-        return false;
-      }
-      const std::string& value = string->value();
-      // TODO(dmichael): We should consider caching the V8 string in the host-
-      // side StringVar, so that we only have to convert/copy once if a
-      // string is sent more than once.
-      *result = v8::String::New(value.c_str(), value.size());
-      break;
+      if (!string)
+        return PP_MakeUndefined();
+      return StringVar::StringToPPVar(string->value());
     }
     case PP_VARTYPE_ARRAY_BUFFER: {
       ArrayBufferVar* buffer = ArrayBufferVar::FromPPVar(var);
-      if (!buffer) {
-        result->Clear();
-        return false;
-      }
-      HostArrayBufferVar* host_buffer =
-          static_cast<HostArrayBufferVar*>(buffer);
-      *result =
-          v8::Local<v8::Value>::New(host_buffer->webkit_buffer().toV8Value());
-      break;
+      if (!buffer)
+        return PP_MakeUndefined();
+      PP_Var new_buffer_var = PpapiGlobals::Get()->GetVarTracker()->
+          MakeArrayBufferPPVar(buffer->ByteLength());
+      DCHECK(new_buffer_var.type == PP_VARTYPE_ARRAY_BUFFER);
+      if (new_buffer_var.type != PP_VARTYPE_ARRAY_BUFFER)
+        return PP_MakeUndefined();
+      ArrayBufferVar* new_buffer = ArrayBufferVar::FromPPVar(new_buffer_var);
+      DCHECK(new_buffer);
+      if (!new_buffer)
+        return PP_MakeUndefined();
+      memcpy(new_buffer->Map(), buffer->Map(), buffer->ByteLength());
+      return new_buffer_var;
     }
     case PP_VARTYPE_OBJECT:
     case PP_VARTYPE_ARRAY:
     case PP_VARTYPE_DICTIONARY:
-      // These are not currently supported.
-      NOTIMPLEMENTED();
-      result->Clear();
-      return false;
-  }
-  return true;
-}
-
-// Copy a PP_Var in to a PP_Var that is appropriate for sending via postMessage.
-// This currently just copies the value.  For a string Var, the result is a
-// PP_Var with the a copy of |var|'s string contents and a reference count of 1.
-//
-// TODO(dmichael): Bypass this step for out-of-process plugins, since a copy
-// happens already when the Var is serialized.
-PP_Var CopyPPVar(const PP_Var& var) {
-  if (var.type == PP_VARTYPE_OBJECT) {
-    // Objects are not currently supported.
-    NOTIMPLEMENTED();
-    return PP_MakeUndefined();
-  } else if (var.type == PP_VARTYPE_STRING) {
-    StringVar* string = StringVar::FromPPVar(var);
-    if (!string)
-      return PP_MakeUndefined();
-    return StringVar::StringToPPVar(string->value());
-  } else if (var.type == PP_VARTYPE_ARRAY_BUFFER) {
-    ArrayBufferVar* buffer = ArrayBufferVar::FromPPVar(var);
-    if (!buffer)
-      return PP_MakeUndefined();
-    PP_Var new_buffer_var = PpapiGlobals::Get()->GetVarTracker()->
-        MakeArrayBufferPPVar(buffer->ByteLength());
-    DCHECK(new_buffer_var.type == PP_VARTYPE_ARRAY_BUFFER);
-    if (new_buffer_var.type != PP_VARTYPE_ARRAY_BUFFER)
+      // Objects/Arrays/Dictionaries not supported by PostMessage in-process.
+      NOTREACHED();
       return PP_MakeUndefined();
-    ArrayBufferVar* new_buffer = ArrayBufferVar::FromPPVar(new_buffer_var);
-    DCHECK(new_buffer);
-    if (!new_buffer)
-      return PP_MakeUndefined();
-    memcpy(new_buffer->Map(), buffer->Map(), buffer->ByteLength());
-    return new_buffer_var;
-  } else {
-    return var;
   }
+  NOTREACHED();
+  return PP_MakeUndefined();
 }
 
 //------------------------------------------------------------------------------
@@ -195,7 +183,11 @@ bool MessageChannelInvoke(NPObject* np_obj, NPIdentifier name,
   if (IdentifierIsPostMessage(name) && (arg_count == 1)) {
     MessageChannel* message_channel = ToMessageChannel(np_obj);
     if (message_channel) {
-      PP_Var argument(NPVariantToPPVar(message_channel->instance(), &args[0]));
+      PP_Var argument;

[dmichael (off chromium), 2013/05/09 20:21:12]

nit: PP_Var argument = PP_MakeUndefined();

[raymes, 2013/05/13 17:09:43]

Done.

+      if (!NPVariantToPPVar(&args[0], &argument)) {
+        NOTREACHED();
+        return false;
+      }
       message_channel->PostMessageToNative(argument);
       PpapiGlobals::Get()->GetVarTracker()->ReleaseVar(argument);
       return true;
@@ -357,11 +349,23 @@ void MessageChannel::PostMessageToJavaScript(PP_Var message_data) {
   v8::Context::Scope context_scope(context);
 
   v8::Local<v8::Value> v8_val;
-  if (!PPVarToV8Value(message_data, &v8_val)) {
+  V8VarConverter converter;
+  if (!converter.ToV8Value(message_data, context, &v8_val)) {
     NOTREACHED();
     return;
   }
 
+  // This is for backward compatibility. It usually makes sense for us to return
+  // a string object rather than a string primitive because it allows multiple
+  // references to the same string (as with PP_Var strings). However, prior to
+  // implementing dictionary and array, vars we would return a string primitive
+  // here. Changing it to an object now will break existing code that uses
+  // strict comparisons for strings returned from PostMessage. e.g. x === "123"
+  // will no longer return true. So if the only value to return is a string
+  // object, just return the string primitive.
+  if (v8_val->IsStringObject())
+    v8_val = v8_val->ToString();
+
   WebSerializedScriptValue serialized_val =
       WebSerializedScriptValue::serialize(v8_val);