Make PostMessageToJavaScript use new WebKit API instead of script.

Make PostMessageToJavaScript use new WebKit API instead of script.

I added some JavaScript code to test_case.html that previously would have made PostMessage not function (which I verified).

This depends on the following WebKit issue:
https://bugs.webkit.org/show_bug.cgi?id=71478

BUG=82604
TEST=N/A

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110513

[viettrungluu, 2011-11-04 20:29:05]

It looks okay to me, though I'm not qualified to approve this change. You should
probably get someone else to look at this (Darin? -- I added him).

http://codereview.chromium.org/8437093/diff/2001/webkit/plugins/ppapi/message...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/2001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:314:
WebSerializedScriptValue::serialize(v8_val);
I don't know much about this, but having to make a v8::Value seems a bit
roundabout (but maybe it's the best thing to do).

Is message_data pretty arbitrary, and not necessarily just a string? (If it is a
string, then perhaps WebSerializedScriptValue should have an alternate
serialize() method that takes a string.)

[dmichael (off chromium), 2011-11-04 20:50:32]

Don't worry, I'm probably not qualified to write this, either ;-)

http://codereview.chromium.org/8437093/diff/2001/webkit/plugins/ppapi/message...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/2001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:314:
WebSerializedScriptValue::serialize(v8_val);
On 2011/11/04 20:29:05, viettrungluu wrote:
> I don't know much about this, but having to make a v8::Value seems a bit
> roundabout (but maybe it's the best thing to do).
> 
> Is message_data pretty arbitrary, and not necessarily just a string? (If it is
a
> string, then perhaps WebSerializedScriptValue should have an alternate
> serialize() method that takes a string.)
It's only kind of arbitrary. Currently it can be any basic JS type (number,
null, undefined, bool) or string...  eventually I want to add support for arrays
and objects (explicitly disallowed in PPVarToNPVariantNoCopy right now).

I must confess I didn't look carefully at what toV8Value does. Now that I peeked
at it, it looks like sometimes it can just return an NPObject, but occasionally
it takes a little more work to wrap it.

Maybe it would be better for me to convert the PP_Var to a v8::Value directly
myself. At this point I have no use for the NPVariant in the native-to-JS
direction; it's just helping me go from PP_Var to WebSerializedScriptValue.

[dmichael (off chromium), 2011-11-16 21:57:50]

+piman, since viettrungluu wasn't comfortable reviewing.

I now directly convert from PP_Var to v8::Value (there was no need to use an
NPVariant anymore).

Note that while this CL fixes the referenced bug, it is also a step towards
making it possible to send more interesting types over PostMessage.

[darin (slow to review), 2011-11-16 22:14:11]

http://codereview.chromium.org/8437093/diff/12001/webkit/plugins/ppapi/messag...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/12001/webkit/plugins/ppapi/messag...
webkit/plugins/ppapi/message_channel.cc:324: WebKit::WebDOMEvent event =
element.document().createEvent("MessageEvent");
nit: add a 'using WebKit::WebDOMEvent' to the top of this file so you can nix
this WebKit:: bit?  for consistency :)

[piman, 2011-11-16 22:17:10]

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:322: const WebKit::WebElement& element =
instance_->container()->element();
container() can be NULL after the plugin has been removed from the DOM (but the
instance is not destroyed yet). I could see getting into that case if one sends
PostMessage from DidDestroyInstance. Well, to be safe, I think we should
(silently?) fail if container is NULL.

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:324: WebKit::WebDOMEvent event =
element.document().createEvent("MessageEvent");
For consistency, mind 'using' this and the other WebKit types?

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:325: WebDOMMessageEvent&
msg_event(static_cast<WebDOMMessageEvent&>(event));
WebDOMMessageEvent msg_event = event.to<WebDOMMessageEvent>()
At the C++ level, event is not a WebDOMMessageEvent, so that cast is technically
invalid (even though it works, it is fragile to changes to WebDOMMessageEvent).

[dmichael (off chromium), 2011-11-16 23:17:31]

Thanks! Fixed based on comments.

Note: It works locally, but I need to wait to land until the associated WebKit
roll happens and I get some good try runs.

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:322: const WebKit::WebElement& element =
instance_->container()->element();
On 2011/11/16 22:17:10, piman wrote:
> container() can be NULL after the plugin has been removed from the DOM (but
the
> instance is not destroyed yet). I could see getting into that case if one
sends
> PostMessage from DidDestroyInstance. Well, to be safe, I think we should
> (silently?) fail if container is NULL.
Done. Agree it should be silent if it's a real possibility.

It's a little scary to me if we can lose the container and still have the
PluginInstance; this is bound to bite us in other places. Here, I was careful to
ensure that if the PluginInstance goes away, its MessageChannel goes away, and
all tasks will not fire (due to using a WeakPtr in MessageChannel). I thought
that was 'safe enough'. :-(

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:324: WebKit::WebDOMEvent event =
element.document().createEvent("MessageEvent");
On 2011/11/16 22:17:10, piman wrote:
> For consistency, mind 'using' this and the other WebKit types?
Done.

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:325: WebDOMMessageEvent&
msg_event(static_cast<WebDOMMessageEvent&>(event));
On 2011/11/16 22:17:10, piman wrote:
> WebDOMMessageEvent msg_event = event.to<WebDOMMessageEvent>()
> At the C++ level, event is not a WebDOMMessageEvent, so that cast is
technically
> invalid (even though it works, it is fragile to changes to
WebDOMMessageEvent).
Thanks for catching that. I felt a little dirty when I wrote that line, but I
knew it would work and oddly didn't notice the to<> function.

Done.

http://codereview.chromium.org/8437093/diff/12001/webkit/plugins/ppapi/messag...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/12001/webkit/plugins/ppapi/messag...
webkit/plugins/ppapi/message_channel.cc:324: WebKit::WebDOMEvent event =
element.document().createEvent("MessageEvent");
On 2011/11/16 22:14:11, darin wrote:
> nit: add a 'using WebKit::WebDOMEvent' to the top of this file so you can nix
> this WebKit:: bit?  for consistency :)
Done.

[piman, 2011-11-16 23:37:26]

lgtm

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
File webkit/plugins/ppapi/message_channel.cc (right):

http://codereview.chromium.org/8437093/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:322: const WebKit::WebElement& element =
instance_->container()->element();
On 2011/11/16 23:17:31, dmichael wrote:
> On 2011/11/16 22:17:10, piman wrote:
> > container() can be NULL after the plugin has been removed from the DOM (but
> the
> > instance is not destroyed yet). I could see getting into that case if one
> sends
> > PostMessage from DidDestroyInstance. Well, to be safe, I think we should
> > (silently?) fail if container is NULL.
> Done. Agree it should be silent if it's a real possibility.
> 
> It's a little scary to me if we can lose the container and still have the
> PluginInstance;

It is expected because of re-entrancy cases. I guess they may only happen with
synchronous scriting, but the case is real: browser calls plugin which calls
back browser to remove self from the DOM.
The instance can't go away because it's on the stack. But the container has to
go away because the plugin isn't in the DOM any more.
PluginInstance is ref-counted, so you can't have any control over its lifetime,
however the WebPluginContainer is not (and is reset in PluginInstance::Delete),
so unless you control all references to PluginInstance, you can't guarantee that
it won't outlive the container.

> this is bound to bite us in other places. Here, I was careful to
> ensure that if the PluginInstance goes away, its MessageChannel goes away, and
> all tasks will not fire (due to using a WeakPtr in MessageChannel). I thought
> that was 'safe enough'. :-(