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

ppapi/tests/test_post_message.cc

Index: ppapi/tests/test_post_message.cc
diff --git a/ppapi/tests/test_post_message.cc b/ppapi/tests/test_post_message.cc
index 39102e1f6ebb64cf964c1d490d399158f4c0f2b7..ded9edc59c59f2c1e8f4c10a2d276b85aa839059 100644
--- a/ppapi/tests/test_post_message.cc
+++ b/ppapi/tests/test_post_message.cc
@@ -5,6 +5,7 @@
 #include "ppapi/tests/test_post_message.h"
 
 #include <algorithm>
+#include <map>
 #include <sstream>
 
 #include "ppapi/c/dev/ppb_testing_dev.h"
@@ -58,6 +59,62 @@ void InvokePostMessageThreadFunc(void* user_data) {
   delete arg;
 }
 
+// TODO(raymes): Consider putting something like this into pp::Var.
+bool VarsEqual(const pp::Var& expected,
+               const pp::Var& actual,
+               std::map<int64_t, int64_t>* visited_ids) {
+  if (expected.pp_var().type != actual.pp_var().type) {
+    if (!expected.is_number() && !actual.is_number())
+      return false;
+  }
+  // TODO(raymes): Implement a pp::Var::IsRefCounted() function.
+  if (expected.pp_var().type > PP_VARTYPE_DOUBLE) {
+    std::map<int64_t, int64_t>::const_iterator it =
+        visited_ids->find(expected.pp_var().value.as_id);
+    if (it != visited_ids->end()) {
+      if (it->second == actual.pp_var().value.as_id)
+        return true;
+      return false;
+    }
+    (*visited_ids)[expected.pp_var().value.as_id] = actual.pp_var().value.as_id;

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

random trivia: You can do this with one lookup using map::insert. If there is no
matching item in the map, it will not insert. The return is a pair with an
iterator and a bool...  the bool tells you whether the insert happened, the
iterator is always where the item with the given key is in the map.

Nice when performance is more important than readability. Usually what you have
is more readable, IMHO, so don't change unless you want to.

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

Cool thanks - yeah I didn't know (or I knew and forgot). I used this in the
other CL already to add a DCHECK :)

+  }
+
+  if (expected.is_number()) {
+    return fabs(expected.AsDouble() - actual.AsDouble()) < 1.0e-4;
+  } else if (expected.is_array()) {
+    pp::VarArray_Dev expected_array(expected);
+    pp::VarArray_Dev actual_array(actual);
+    if (expected_array.GetLength() != actual_array.GetLength())
+      return false;
+    for (uint32_t i = 0; i < expected_array.GetLength(); ++i) {
+      if (!VarsEqual(expected_array.Get(i), actual_array.Get(i), visited_ids))
+        return false;
+    }
+    return true;
+  } else if (expected.is_dictionary()) {
+    pp::VarDictionary_Dev expected_dict(expected);
+    pp::VarDictionary_Dev actual_dict(actual);
+    if (expected_dict.GetKeys().GetLength() !=
+        actual_dict.GetKeys().GetLength()) {
+      return false;
+    }
+    for (uint32_t i = 0; i < expected_dict.GetKeys().GetLength(); ++i) {
+      pp::Var key = expected_dict.GetKeys().Get(i);

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

Don't you also need to compare the value of the key? Otherwise, you might say
that the following are equivalent:
{ "a": 1, "b": 2 }
{ "c": 1, "d": 2 }

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

Well it's not iterating through the values here, but the keys. But I should
explicitly check that the other dictionary contains that key (otherwise Get()
will just return PP_VARTYPE_UNDEFINED which may cause a false positive here if
unlucky).

[dmichael (off chromium), 2013/05/13 18:02:26]

Sorry, I was misreading, as if you were using the i-th key in both cases.

+      if (!VarsEqual(expected_dict.Get(key), actual_dict.Get(key), visited_ids))
+        return false;
+    }
+    return true;
+  } else {
+    return expected == actual;
+  }
+}
+
+bool VarsEqual(const pp::Var& expected,
+               const pp::Var& actual) {
+  std::map<int64_t, int64_t> visited_ids;
+  return VarsEqual(expected, actual, &visited_ids);
+}
+
 class ScopedArrayBufferSizeSetter {
  public:
   ScopedArrayBufferSizeSetter(const PPB_Testing_Dev* interface,
@@ -135,6 +192,9 @@ void TestPostMessage::RunTests(const std::string& filter) {
   RUN_TEST(SendInInit, filter);
   RUN_TEST(SendingData, filter);
   RUN_TEST(SendingArrayBuffer, filter);
+  RUN_TEST(SendingArray, filter);
+  RUN_TEST(SendingDictionary, filter);
+  RUN_TEST(SendingComplexVar, filter);
   RUN_TEST(MessageEvent, filter);
   RUN_TEST(NoHandler, filter);
   RUN_TEST(ExtraParam, filter);
@@ -360,6 +420,169 @@ std::string TestPostMessage::TestSendingArrayBuffer() {
   PASS();
 }
 
+std::string TestPostMessage::TestSendingArray() {
+  // Clean up after previous tests. This also swallows the message sent by Init
+  // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
+  // should start with these.
+  WaitForMessages();
+  ASSERT_TRUE(ClearListeners());
+
+  pp::VarArray_Dev array;
+  array.Set(0, pp::Var(kTestBool));
+  array.Set(1, pp::Var(kTestString));
+  // Purposely leave index 2 empty.
+  array.Set(3, pp::Var(kTestInt));
+  array.Set(4, pp::Var(kTestDouble));
+
+  std::stringstream ss;
+  ss << array.GetLength();

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

How about:
std::string length_as_string(ss.str());
just for readability's sake

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

Done.

+
+  // Have the listener test some properties of the Array.
+  std::vector<std::string> properties_to_check;
+  properties_to_check.push_back(
+      "message_event.data.constructor.name === 'Array'");
+  properties_to_check.push_back(
+      std::string("message_event.data.length === ") + ss.str());
+  for (std::vector<std::string>::iterator iter = properties_to_check.begin();
+       iter != properties_to_check.end();
+       ++iter) {
+    ASSERT_TRUE(AddEchoingListener(*iter));
+    message_data_.clear();
+    instance_->PostMessage(array);
+    ASSERT_EQ(message_data_.size(), 0);
+    ASSERT_EQ(WaitForMessages(), 1);
+    ASSERT_TRUE(message_data_.back().is_bool());
+    if (!message_data_.back().AsBool())
+      return std::string("Failed: ") + *iter + ", size: " + ss.str();
+    ASSERT_TRUE(message_data_.back().AsBool());
+    ASSERT_TRUE(ClearListeners());

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

I wonder how hard it would be to wrap this property-checking stuff in a
function, since we're using it in 3 places now? Then replace this here with...

ASSERT_SUBTEST_SUCCESS(
    CheckMessageProperties(properties_to_check));

Or something

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

Done.

+  }
+
+  // Set up the JavaScript message event listener to echo the data part of the
+  // message event back to us.
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  message_data_.clear();
+  instance_->PostMessage(array);
+  // PostMessage is asynchronous, so we should not receive a response yet.
+  ASSERT_EQ(message_data_.size(), 0);
+  ASSERT_EQ(WaitForMessages(), 1);
+  ASSERT_TRUE(message_data_.back().is_array());
+  ASSERT_TRUE(VarsEqual(array, message_data_.back()));
+
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  PASS();
+}
+
+std::string TestPostMessage::TestSendingDictionary() {
+  // Clean up after previous tests. This also swallows the message sent by Init
+  // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
+  // should start with these.
+  WaitForMessages();
+  ASSERT_TRUE(ClearListeners());
+
+  pp::VarDictionary_Dev dictionary;
+  dictionary.Set(pp::Var("foo"), pp::Var(kTestBool));
+  dictionary.Set(pp::Var("bar"), pp::Var(kTestString));
+  dictionary.Set(pp::Var("abc"), pp::Var(kTestInt));
+  dictionary.Set(pp::Var("def"), pp::Var());
+
+  std::stringstream ss;
+  ss << dictionary.GetKeys().GetLength();
+
+  // Have the listener test some properties of the Dictionary.
+  std::vector<std::string> properties_to_check;
+  properties_to_check.push_back(
+      "message_event.data.constructor.name === 'Object'");
+  properties_to_check.push_back(
+      std::string("Object.keys(message_event.data).length === ") + ss.str());
+  for (std::vector<std::string>::iterator iter = properties_to_check.begin();
+       iter != properties_to_check.end();
+       ++iter) {
+    ASSERT_TRUE(AddEchoingListener(*iter));
+    message_data_.clear();
+    instance_->PostMessage(dictionary);
+    ASSERT_EQ(message_data_.size(), 0);
+    ASSERT_EQ(WaitForMessages(), 1);
+    ASSERT_TRUE(message_data_.back().is_bool());
+    if (!message_data_.back().AsBool())
+      return std::string("Failed: ") + *iter + ", size: " + ss.str();
+    ASSERT_TRUE(message_data_.back().AsBool());
+    ASSERT_TRUE(ClearListeners());
+  }
+
+  // Set up the JavaScript message event listener to echo the data part of the
+  // message event back to us.
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  message_data_.clear();
+  instance_->PostMessage(dictionary);
+  // PostMessage is asynchronous, so we should not receive a response yet.
+  ASSERT_EQ(message_data_.size(), 0);
+  ASSERT_EQ(WaitForMessages(), 1);
+  ASSERT_TRUE(message_data_.back().is_dictionary());
+  ASSERT_TRUE(VarsEqual(dictionary, message_data_.back()));
+
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  PASS();
+}
+
+std::string TestPostMessage::TestSendingComplexVar() {
+  // Clean up after previous tests. This also swallows the message sent by Init
+  // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
+  // should start with these.
+  WaitForMessages();
+  ASSERT_TRUE(ClearListeners());
+
+  pp::VarDictionary_Dev dictionary;
+  dictionary.Set(pp::Var("foo"), pp::Var(kTestBool));
+  dictionary.Set(pp::Var("bar"), pp::Var(kTestString));
+  dictionary.Set(pp::Var("abc"), pp::Var(kTestInt));
+  dictionary.Set(pp::Var("def"), pp::Var());
+  dictionary.Set(pp::Var("dictionary"), dictionary);  // Self-reference.
+
+  // Reference to array.
+  pp::VarArray_Dev array;
+  array.Set(0, pp::Var(kTestBool));
+  array.Set(1, pp::Var(kTestString));
+  // Purposely leave index 2 empty.

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

This doesn't actually leave index 2 empty in our implementation; it sticks an
Undefined in, right? Granted, it's a subtle difference.

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

Right - I updated the comment to clarify.

+  array.Set(3, pp::Var(kTestInt));
+  array.Set(4, pp::Var(kTestDouble));
+
+  dictionary.Set(pp::Var("array1"), array);
+  dictionary.Set(pp::Var("array2"), array);
+
+  // Set up a (dictionary -> dictionary2 -> dictionary) cycle.
+  pp::VarDictionary_Dev dictionary2;
+  dictionary2.Set(pp::Var("dictionary"), dictionary);
+  dictionary.Set(pp::Var("dictionary2"), dictionary2);

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

Can we involve an array in a cycle too? And maybe have a situation where one
child refers to another child of the same parent?
  A
 / \
v   v
B-->C

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

Done - I modified it to add these 2 cases. I didn't think it was necessary to be
too thorough here because the unittests should cover the important corner cases.

+
+  // Set up the JavaScript message event listener to echo the data part of the
+  // message event back to us.
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  message_data_.clear();
+  instance_->PostMessage(dictionary);
+  // PostMessage is asynchronous, so we should not receive a response yet.
+  ASSERT_EQ(message_data_.size(), 0);
+  ASSERT_EQ(WaitForMessages(), 1);
+  ASSERT_TRUE(message_data_.back().is_dictionary());
+  pp::VarDictionary_Dev result(message_data_.back());
+  ASSERT_TRUE(VarsEqual(dictionary, message_data_.back()));
+
+  // Break the cycles.
+  dictionary.Delete(pp::Var("dictionary"));
+  dictionary.Delete(pp::Var("dictionary2"));
+  result.Delete(pp::Var("dictionary"));
+  result.Delete(pp::Var("dictionary2"));
+
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  PASS();
+}
+
 std::string TestPostMessage::TestMessageEvent() {
   // Set up the JavaScript message event listener to pass us some values from
   // the MessageEvent and make sure they match our expectations.