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

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

This hooks up V8<->Var conversion in the pepper message channel so that Array/Dictionaries can be transmitted to and from the plugin using Post/HandleMessage.

This CL also adds integration tests for transmitting Arrays/Dictionaries.

BUG=236958
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200880

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

https://codereview.chromium.org/14636009/diff/2001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/14636009/diff/2001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:560: // Arrays/Dictionaries aren't
supported with PostMessage in-process.
nit: please list them as "DISABLED_". I think it's more self-documenting, and I
eventually want to put checking in that all tests are listed.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:79:
(*visited_ids)[expected.pp_var().value.as_id] = actual.pp_var().value.as_id;
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.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:102: pp::Var key =
expected_dict.GetKeys().Get(i);
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 }

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:438: ss << array.GetLength();
How about:
std::string length_as_string(ss.str());
just for readability's sake

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:458: ASSERT_TRUE(ClearListeners());
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

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:550: // Purposely leave index 2 empty.
This doesn't actually leave index 2 empty in our implementation; it sticks an
Undefined in, right? Granted, it's a subtle difference.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:560: dictionary.Set(pp::Var("dictionary2"),
dictionary2);
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

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:103: // happens already when the Var is
serialized.
Oops, this TODO has been done, so you can remove it.

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:186: PP_Var argument;
nit: PP_Var argument = PP_MakeUndefined();

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

https://codereview.chromium.org/14636009/diff/2001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/14636009/diff/2001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:560: // Arrays/Dictionaries aren't
supported with PostMessage in-process.
On 2013/05/09 20:21:12, dmichael wrote:
> nit: please list them as "DISABLED_". I think it's more self-documenting, and
I
> eventually want to put checking in that all tests are listed.

Done.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:79:
(*visited_ids)[expected.pp_var().value.as_id] = actual.pp_var().value.as_id;
Cool thanks - yeah I didn't know (or I knew and forgot). I used this in the
other CL already to add a DCHECK :)

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:102: pp::Var key =
expected_dict.GetKeys().Get(i);
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).

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:438: ss << array.GetLength();
On 2013/05/09 20:21:12, dmichael wrote:
> How about:
> std::string length_as_string(ss.str());
> just for readability's sake

Done.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:458: ASSERT_TRUE(ClearListeners());
On 2013/05/09 20:21:12, dmichael wrote:
> 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

Done.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:550: // Purposely leave index 2 empty.
Right - I updated the comment to clarify.

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:560: dictionary.Set(pp::Var("dictionary2"),
dictionary2);
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.

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:103: // happens already when the Var is
serialized.
On 2013/05/09 20:21:12, dmichael wrote:
> Oops, this TODO has been done, so you can remove it.

Done.

https://codereview.chromium.org/14636009/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:186: PP_Var argument;
On 2013/05/09 20:21:12, dmichael wrote:
> nit: PP_Var argument = PP_MakeUndefined();

Done.

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

lgtm

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/14636009/diff/2001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:102: pp::Var key =
expected_dict.GetKeys().Get(i);
On 2013/05/13 17:09:43, raymes wrote:
> 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).
Sorry, I was misreading, as if you were using the i-th key in both cases.

https://codereview.chromium.org/14636009/diff/9001/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/14636009/diff/9001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:500: length_as_string);
nit: indentation is off

[raymes, 2013-05-17 17:39:12]

https://codereview.chromium.org/14636009/diff/9001/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/14636009/diff/9001/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:500: length_as_string);
On 2013/05/13 18:02:26, dmichael wrote:
> nit: indentation is off

Done.

[commit-bot: I haz the power, 2013-05-17 17:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/14636009/17001

[commit-bot: I haz the power, 2013-05-17 20:54:01]

Change committed as 200880