PPAPI: Refactor MessageChannel to prep for sync postMessae

PPAPI: Refactor MessageChannel to prep for sync postMessage

1) Remove unused CopyPPVar function from MessageChannel
2) Remove mostly-duplicate NPVariantToPPVar from MessageChannel.
3) Separate V8VarConverter::FromV8Value implementation from calling the callback (so I can have a sync path later, and this is shorter anyway).
4) Simplify NaCl in-process to out-of-process transition. Now, we just queue stuff for in-process (nobody but NaCl uses Messaging in-process), and if/when we switch to out-of-process (handing off to a NaCl app) we drain the queues then.
5) Some other renames/tweaks to how MessageChannel queues pending conversions.

BUG=367896
R=raymes@chromium.org

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

[raymes, 2014-05-20 01:54:41]

Looks a lot cleaner overall! 

The early message queue stuff combined with the plugin queue is a bit confusing,
but it's still an improvement from before.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (left):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:412: if
(instance_->module()->IsProxied()) {
Is it worth having a DCHECK to make sure we're OOP?

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:303: true);
Do we need to call DrainCompletePluginMessages in this case?

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:352: void MessageChannel::Start() {
I guess we could check that we're OOP here too.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:435: void
MessageChannel::PostMessageToNative(const NPVariant* message_data) {
We could check OOP here too.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/resource_converter.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/resource_converter.h:98:
std::vector<scoped_refptr<HostResourceVar> > browser_vars_;
Was this intended?

[dmichael (off chromium), 2014-05-20 18:09:11]

Comments addressed, I think. Thanks!

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (left):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:412: if
(instance_->module()->IsProxied()) {
On 2014/05/20 01:54:42, raymes wrote:
> Is it worth having a DCHECK to make sure we're OOP?
Not in this first case, because (for now) it could be the NaCl trusted plugin,
in which case it may try to send a message, and we queue it until the switchover
to the untrusted plugin. I added the DCHECK in the 2nd case and a comment to
hopefully explain a little bit.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:303: true);
On 2014/05/20 01:54:42, raymes wrote:
> Do we need to call DrainCompletePluginMessages in this case?
Yes, thanks, I think you just saved me some debugging. I somehow dropped that
when I did the last bit of code shuffling.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:352: void MessageChannel::Start() {
On 2014/05/20 01:54:42, raymes wrote:
> I guess we could check that we're OOP here too.

Done.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:435: void
MessageChannel::PostMessageToNative(const NPVariant* message_data) {
On 2014/05/20 01:54:42, raymes wrote:
> We could check OOP here too.
JavaScript should be allowed to send messages to a NaCl app before the NaCl app
is finished loading. In that case, the messages should sit in the queue until
Start() is called. (This is actually a slight change; I think the old code could
drop messages that are sent at the wrong time, which would explain this bug:
https://code.google.com/p/chromium/issues/detail?id=372630
)

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/resource_converter.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/resource_converter.h:98:
std::vector<scoped_refptr<HostResourceVar> > browser_vars_;
On 2014/05/20 01:54:42, raymes wrote:
> Was this intended?
Yes, sorry I forgot to mention it. It looked like a simple mistake that it
didn't have the underscore like the other members, and I thought I'd clean it up
while I was in the neighborhood.

[dmichael (off chromium), 2014-05-20 18:14:16]

I added the DrainCompletedPluginMessages call you suggested, then remembered
it's not necessary. So I'll take it back out in the next patch.

Separate issue, I just realized I broke all the in-process tests, since this
messes up postMessage in-process, on which the tests rely. So...  I'm going to
have to rework it I think, because we probably don't want to turn off all of
those tests just yet.

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:303: true);
On 2014/05/20 18:09:12, dmichael wrote:
> On 2014/05/20 01:54:42, raymes wrote:
> > Do we need to call DrainCompletePluginMessages in this case?
> Yes, thanks, I think you just saved me some debugging. I somehow dropped that
> when I did the last bit of code shuffling.
Oh, nevermind, I just realized my goal was to keep the responsibility here to
just enqueuing the message. Callers of this need to call
DrainCompletedPluginMessages() (see PostMessageToNative)

[raymes, 2014-05-21 01:27:35]

lgtm

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
File content/renderer/pepper/message_channel.cc (right):

https://chromiumcodereview.appspot.com/290553004/diff/60001/content/renderer/...
content/renderer/pepper/message_channel.cc:303: true);
Oh I see now, sorry I was confused!

[dmichael (off chromium), 2014-05-21 22:52:34]

jam: Please look w.r.t. content/public.

The story is, when used for NaCl, the Messaging API should queue up messages
until the trusted NaCl plugin hands off to the untrusted NaCl app. That ensures
that messages that JavaScript sends to the plugin early in plugin startup will
be queued up and eventually go to the right place. So PepperPluginInstanceImpl
needs to know if this plugin is an "external plugin host" (of which NaCl trusted
plugin is the only example) so that it knows to queue up messages.

Eventually, we'll remove the NaCl trusted plugin and in-process support and a
lot of this will go away.

If you can think of a cleaner way to plumb this through content, I'm happy to
change it.

[raymes, 2014-05-22 00:25:30]

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:129: // plugin module is a host
for "external plugins."
nit: "external plugins".

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:141: virtual void
SetToExternalPluginHost() = 0;
Can we do this without modifying the content API? It seems like we know when we
create an "external" module:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

I think we could store that in the plugin module and then just return it.

[dmichael (off chromium), 2014-05-22 20:18:02]

+teravest for NaClHelper.

I realized that putting the NaCl stuff in the PepperHelper was the wrong
approach, and that I really need to make a NaClHelper RenderFrameObserver that
can inject NaCl-specific stuff when necessary.

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:129: // plugin module is a host
for "external plugins."
On 2014/05/22 00:25:31, raymes wrote:
> nit: "external plugins".
Technically, the period should be inside the quotes...
http://blog.apastyle.org/apastyle/2011/08/punctuating-around-quotation-marks....

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:141: virtual void
SetToExternalPluginHost() = 0;
On 2014/05/22 00:25:31, raymes wrote:
> Can we do this without modifying the content API? It seems like we know when
we
> create an "external" module:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> 
> I think we could store that in the plugin module and then just return it.
Unfortunately, that wouldn't work. That function basically clones a Module to
make a new one, and this happens at the point that the instance switches from
the external plugin "host" (really, the trusted NaCl plugin) to the actual
external plugin (the untrusted NaCl app):
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
What I need to know is if the instance we're Initialize()ing is a host of
external plugins (i.e., the NaCl trusted plugin), so the module it looks at is
the one associated with that in-process trusted NaCl plugin.

It's admittedly gross and confusing in the short term (mostly because of the
trusted plugin which switches control to an untrusted plugin). But I think we're
ultimately going to want other hooks for NaCl, even when we get rid of the
trusted plugin.

[teravest, 2014-05-22 20:20:30]

Only one quick thing about NaClHelper; I didn't look at the rest of the change.

https://chromiumcodereview.appspot.com/290553004/diff/120001/components/nacl/...
File components/nacl/renderer/nacl_helper.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/120001/components/nacl/...
components/nacl/renderer/nacl_helper.h:11: // This class listens for Pepper
creation events from the RenderFrame. For the
I think this should go in the nacl namespace.

[dmichael (off chromium), 2014-05-22 20:34:59]

https://chromiumcodereview.appspot.com/290553004/diff/120001/components/nacl/...
File components/nacl/renderer/nacl_helper.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/120001/components/nacl/...
components/nacl/renderer/nacl_helper.h:11: // This class listens for Pepper
creation events from the RenderFrame. For the
On 2014/05/22 20:20:30, teravest wrote:
> I think this should go in the nacl namespace.
Good catch, thanks, done.

[teravest, 2014-05-22 20:36:06]

lgtm

NaClHelper looks good to me.

[raymes, 2014-05-22 23:42:33]

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:129: // plugin module is a host
for "external plugins."
On 2014/05/22 20:18:02, dmichael wrote:
> On 2014/05/22 00:25:31, raymes wrote:
> > nit: "external plugins".
> Technically, the period should be inside the quotes...
>
http://blog.apastyle.org/apastyle/2011/08/punctuating-around-quotation-marks....

Haha ok, I guess I'm used to the "British style" :) Sorry for the noise.

https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
content/public/renderer/renderer_ppapi_host.h:141: virtual void
SetToExternalPluginHost() = 0;
I see, thanks for the explanation.

[raymes, 2014-05-22 23:42:45]

On 2014/05/22 23:42:33, raymes wrote:
>
https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
> File content/public/renderer/renderer_ppapi_host.h (right):
> 
>
https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
> content/public/renderer/renderer_ppapi_host.h:129: // plugin module is a host
> for "external plugins."
> On 2014/05/22 20:18:02, dmichael wrote:
> > On 2014/05/22 00:25:31, raymes wrote:
> > > nit: "external plugins".
> > Technically, the period should be inside the quotes...
> >
>
http://blog.apastyle.org/apastyle/2011/08/punctuating-around-quotation-marks....
> 
> Haha ok, I guess I'm used to the "British style" :) Sorry for the noise.
> 
>
https://chromiumcodereview.appspot.com/290553004/diff/100001/content/public/r...
> content/public/renderer/renderer_ppapi_host.h:141: virtual void
> SetToExternalPluginHost() = 0;
> I see, thanks for the explanation.

lgtm

[jam, 2014-05-23 15:12:26]

content/public and chrome lgtm

[dmichael (off chromium), 2014-05-23 15:16:54]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-05-23 15:17:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/290553004/140001

[commit-bot: I haz the power, 2014-05-23 20:13:16]

Change committed as 272583