A proposal for an initial postMessage interface. This will allow JavaScript ...

Based on Neb's suggestion to take these functions out of the interface, I started a new CL: http://codereview.chromium.org/6716005/

A proposal and implementation for an initial postMessage interface.  This will allow JavaScript to
send data asynchronously to a module, and the module to asynchronously send
data to a JavaScript message handler.  This also adds (in an admittedly unsophisticated way) backwards compatibility so that existing apps & plugins should still function with ToT Chrome, and should also compile & work without code changes when DEPSing the latest in.

Overview:
  From JavaScript, you can invoke 'postMessage' on the embedded module.  That
  results in a call to 'PPP_Instance::HandleMessage'.

  From Native Code, you can invoke 'PPB_Instance::PostMessage', which results
  in a call to an 'onmessage' function on the embedded module in the JavaScript
  code (if one has been registered).

  Please see the examples in PPP_Instance and PPB_Instance.

Restrictions:
The first implementation will support only intrinsic values and strings (all
types that PP_Var supports except for objects).  Sending more complex data will
require packaging it in to a string first (e.g., via JSON).  In a future
version, I would like to add support for structured data via the structured
clone algorithm.  I have ideas about how the data would be sent, but it will
be a lot more complex than this initial proposal.

This also does not support MessagePorts.  Those might be desirable in the
future, but they are not necessary for a first implementation.

BUG=None
TEST=test_post_message.h/.cc, manually testing of existing NaCl SDK & Gallery examples to check backwards compatibility.

[Mark Seaborn, 2011-02-17 19:18:47]

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
File ppapi/c/dev/pp_message_event_dev.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:19: *  instrinsic type or string (but not
structured data such as an object or
"instrinsic" -> "intrinsic"

Does "intrinsic type or string" mean "number or string"?  (AFAIK, "intrinsic
type" is not a technical term in Javascript.)

I wouldn't have thought sending a single number as a message is very useful.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:31: struct PP_Var data;
I thought the purpose of this async messaging was to get rid of PP_Var?  I think
it would be better to support just strings (preferably byte strings) initially.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:34: *  strongly recommended that message
handlers check @a origin before doing
This is contradictory.  On the one hand you say this field should be checked. 
On the other hand you say it can only have one value.  What is a NaCl process
supposed to check this against?

I think you should omit this field.  If you want to extend the message format
later you can add more callback types.

[darin (slow to review), 2011-02-17 19:20:52]

Thanks for putting this together.  I've dumped some counter proposal ideas
below.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
File ppapi/c/dev/pp_message_event_dev.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:21: *  DESIGN ISSUE:  I want to support
structured data and the structured
SGTM2

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:37: *  DESIGN ISSUE:  There was discussion of
omitting this parameter.  For now,
OK, yeah...

While NaCl has a same-origin restriction, other pepper plugins do not.  We
should design with that in mind.

Unlike the postMessage API exposed to JS, we don't really know the origin of the
target--the plugin.  NaCl knows this, but the pepper plugin host in Chrome does
not.  All Chrome knows is the origin of the Document embedding the plugin.

So, I think this data structure should include both source_origin and
target_origin fields.  That will enable trusted pepper plugins that are loading
restricted content (think flash) to perform origin checks because only the
plugin may know the origin of the content it has loaded into an instance.

This may impact the PostMessage function call too.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:61: struct PP_Var source;
it is an interesting idea to include this here.  i'm OK adding it in later as we
can just rev the structure once we know how we would use this.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance, struct
PP_MessageEvent_Dev message);
nit: pass message by const pointer.

Stepping back, I thought that David Sehr made a good suggestion the other day. 
Instead of this model for PostMessage (which I know I had suggested), maybe we
could have the notion of PPB_MessagePort that you can operate on.  Then the
plugin can assign a message port to the instance.

PPB_MessagePort would have methods to post and pop messages.  The pop function
would take a PP_CompletionCallback as we could say that post always completes
quickly by placing the message in an outbound queue.

struct PPB_MessagePort {
  PP_Resource (*Create)(PP_Instance, struct PP_Var origin);

  int32_t (*PostMessage)(PP_Resource message_port,
                         const struct PP_Var message,
                         const struct PP_Var target_origin);

  int32_t (*PopMessage)(PP_Resource message_port, 
                        struct PP_Message_Dev* message,
                        PP_CompletionCallback callback);

  void (*Close)(PP_Resource message_port);
};

The Close function is needed so that the caller of PopMessage has a way to
interrupt the call, allowing them to free the memory associated with the given
PP_Message_Dev struct.

(I'm not sure that the "Event" suffix to PP_MessageEvent_Dev is necessary.  A
message is a kind of event, right?)

Here's what PPB_Instance could look like for binding a message port:

struct PPB_Instance {
  ...
  void (*BindMessagePort)(PP_Instance instance,
                          PP_Resource message_port);
  ...
};

There is one thing I'm not sure of, which is what happens if JS calls
postMessage on the plugin element before the plugin has called BindMessagePort. 
Should we just queue messages on behalf of the plugin, or should we drop them on
the floor?  We can say that BindMessagePort should be called during DidCreate,
but I'm not sure if DidCreate runs synchronously to plugin construction or not.

Note:  I've added an origin parameter to the PPB_MessagePort::Create function as
a way of identifying the origin of the message port.  Normally, this is derived
from the script context, but we don't have that in Pepper.  This origin will be
used as the "origin" field for MessageEvent seen by JS.  We can also go one step
further and say that PopMessage will not return any messages that do not match
the origin of the message port.  This forces message ports to be
origin-restricted, which is certainly fine for NaCl, and probably fine for
others too.

[darin (slow to review), 2011-02-17 19:27:30]

On Thu, Feb 17, 2011 at 11:20 AM, <darin@chromium.org> wrote:

> Thanks for putting this together.  I've dumped some counter proposal ideas
> below.
>
>
>
>
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> File ppapi/c/dev/pp_message_event_dev.h (right):
>
>
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> ppapi/c/dev/pp_message_event_dev.h:21: *  DESIGN ISSUE:  I want to
> support structured data and the structured
> SGTM2
>
>
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> ppapi/c/dev/pp_message_event_dev.h:37: *  DESIGN ISSUE:  There was
> discussion of omitting this parameter.  For now,
> OK, yeah...
>
> While NaCl has a same-origin restriction, other pepper plugins do not.
> We should design with that in mind.
>
> Unlike the postMessage API exposed to JS, we don't really know the
> origin of the target--the plugin.  NaCl knows this, but the pepper
> plugin host in Chrome does not.  All Chrome knows is the origin of the
> Document embedding the plugin.
>
> So, I think this data structure should include both source_origin and
> target_origin fields.  That will enable trusted pepper plugins that are
> loading restricted content (think flash) to perform origin checks
> because only the plugin may know the origin of the content it has loaded
> into an instance.
>
> This may impact the PostMessage function call too.
>
>
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> ppapi/c/dev/pp_message_event_dev.h:61: struct PP_Var source;
> it is an interesting idea to include this here.  i'm OK adding it in
> later as we can just rev the structure once we know how we would use
> this.
>
> http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
> File ppapi/c/ppp_instance.h (right):
>
>
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
> ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance,
> struct PP_MessageEvent_Dev message);
> nit: pass message by const pointer.
>
> Stepping back, I thought that David Sehr made a good suggestion the
> other day.


David's reasons for this proposal were (as I recall):

1- This would enable reading of messages from a background thread.
2- This would be compatible with future directions where we might support
multiple message ports.
3- Related to #1, this would prevent the main thread from necessarily being
a bottle neck for messaging.

Those reasons convinced me!  Plus, I think the implementation costs would be
similar.  My only concern is that we might not converge on API quickly
enough b/c calling this a MessagePort opens up a whole can of possibilities
:-P

-Darin



>  Instead of this model for PostMessage (which I know I had
> suggested), maybe we could have the notion of PPB_MessagePort that you
> can operate on.  Then the plugin can assign a message port to the
> instance.
>
> PPB_MessagePort would have methods to post and pop messages.  The pop
> function would take a PP_CompletionCallback as we could say that post
> always completes quickly by placing the message in an outbound queue.
>
> struct PPB_MessagePort {
>  PP_Resource (*Create)(PP_Instance, struct PP_Var origin);
>
>  int32_t (*PostMessage)(PP_Resource message_port,
>                         const struct PP_Var message,
>                         const struct PP_Var target_origin);
>
>  int32_t (*PopMessage)(PP_Resource message_port,
>                        struct PP_Message_Dev* message,
>                        PP_CompletionCallback callback);
>
>  void (*Close)(PP_Resource message_port);
> };
>
> The Close function is needed so that the caller of PopMessage has a way
> to interrupt the call, allowing them to free the memory associated with
> the given PP_Message_Dev struct.
>
> (I'm not sure that the "Event" suffix to PP_MessageEvent_Dev is
> necessary.  A message is a kind of event, right?)
>
> Here's what PPB_Instance could look like for binding a message port:
>
> struct PPB_Instance {
>  ...
>  void (*BindMessagePort)(PP_Instance instance,
>                          PP_Resource message_port);
>  ...
> };
>
> There is one thing I'm not sure of, which is what happens if JS calls
> postMessage on the plugin element before the plugin has called
> BindMessagePort.  Should we just queue messages on behalf of the plugin,
> or should we drop them on the floor?  We can say that BindMessagePort
> should be called during DidCreate, but I'm not sure if DidCreate runs
> synchronously to plugin construction or not.
>
> Note:  I've added an origin parameter to the PPB_MessagePort::Create
> function as a way of identifying the origin of the message port.
> Normally, this is derived from the script context, but we don't have
> that in Pepper.  This origin will be used as the "origin" field for
> MessageEvent seen by JS.  We can also go one step further and say that
> PopMessage will not return any messages that do not match the origin of
> the message port.  This forces message ports to be origin-restricted,
> which is certainly fine for NaCl, and probably fine for others too.
>
>
> http://codereview.chromium.org/6538028/
>

[dmichael(do not use this one), 2011-02-17 20:06:42]

Some counterpoint.  I'm not averse to doing MessagePort, I was just trying to
defer it :-)
> 
> 
> David's reasons for this proposal were (as I recall):
> 
> 1- This would enable reading of messages from a background thread.
True (when one of the proxies supports doing so, which neither the Chrome IPC
one nor the SRPC does right now).  I do want to provide that.
> 2- This would be compatible with future directions where we might support
> multiple message ports.
I don't think my design precludes it;  the idea is basically doing what exists
in JavaScript & the DOM...  sprinkle postMessage where it is needed, with
appropriate parameters in each context.  
> 3- Related to #1, this would prevent the main thread from necessarily being
> a bottle neck for messaging.
> 
> Those reasons convinced me!  Plus, I think the implementation costs would be
> similar.  My only concern is that we might not converge on API quickly
> enough b/c calling this a MessagePort opens up a whole can of possibilities
> :-P
I think those were what I was trying to avoid:
-how do you expose a MessagePort?  (your proposal for having 1 for the instance
is good, though)
-How do you pass a MessagePort (if we allow that)?
-How do you discover a MessagePort so you can post messages to it?
Granted, we could still defer those even if we make something named
'MessagePort'.
> 
> -Darin
> 
> 
> 
> >  Instead of this model for PostMessage (which I know I had
> > suggested), maybe we could have the notion of PPB_MessagePort that you
> > can operate on.  Then the plugin can assign a message port to the
> > instance.
> >
> > PPB_MessagePort would have methods to post and pop messages.  The pop
> > function would take a PP_CompletionCallback as we could say that post
> > always completes quickly by placing the message in an outbound queue.
> >
> > struct PPB_MessagePort {
> >  PP_Resource (*Create)(PP_Instance, struct PP_Var origin);
> >
> >  int32_t (*PostMessage)(PP_Resource message_port,
> >                         const struct PP_Var message,
> >                         const struct PP_Var target_origin);
> >
> >  int32_t (*PopMessage)(PP_Resource message_port,
> >                        struct PP_Message_Dev* message,
> >                        PP_CompletionCallback callback);
> >
> >  void (*Close)(PP_Resource message_port);
> > };
> >
> > The Close function is needed so that the caller of PopMessage has a way
> > to interrupt the call, allowing them to free the memory associated with
> > the given PP_Message_Dev struct.
> >
> > (I'm not sure that the "Event" suffix to PP_MessageEvent_Dev is
> > necessary.  A message is a kind of event, right?)
> >
> > Here's what PPB_Instance could look like for binding a message port:
> >
> > struct PPB_Instance {
> >  ...
> >  void (*BindMessagePort)(PP_Instance instance,
> >                          PP_Resource message_port);
> >  ...
> > };
> >
> > There is one thing I'm not sure of, which is what happens if JS calls
> > postMessage on the plugin element before the plugin has called
> > BindMessagePort.  Should we just queue messages on behalf of the plugin,
> > or should we drop them on the floor?  We can say that BindMessagePort
> > should be called during DidCreate, but I'm not sure if DidCreate runs
> > synchronously to plugin construction or not.
> >
> > Note:  I've added an origin parameter to the PPB_MessagePort::Create
> > function as a way of identifying the origin of the message port.
> > Normally, this is derived from the script context, but we don't have
> > that in Pepper.  This origin will be used as the "origin" field for
> > MessageEvent seen by JS.  We can also go one step further and say that
> > PopMessage will not return any messages that do not match the origin of
> > the message port.  This forces message ports to be origin-restricted,
> > which is certainly fine for NaCl, and probably fine for others too.
> >
> >
> > http://codereview.chromium.org/6538028/
> >

[dmichael(do not use this one), 2011-02-17 20:07:03]

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
File ppapi/c/dev/pp_message_event_dev.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:19: *  instrinsic type or string (but not
structured data such as an object or
On 2011/02/17 19:18:47, Mark Seaborn wrote:
> "instrinsic" -> "intrinsic"
> 
> Does "intrinsic type or string" mean "number or string"?  (AFAIK, "intrinsic
> type" is not a technical term in Javascript.)
> 
> I wouldn't have thought sending a single number as a message is very useful.
I agree, it's not.  But the postMessage spec allows for it, and it's easy to
provide.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:31: struct PP_Var data;
On 2011/02/17 19:18:47, Mark Seaborn wrote:
> I thought the purpose of this async messaging was to get rid of PP_Var?  I
think
> it would be better to support just strings (preferably byte strings)
initially.
The proposal is more about getting rid of PPB_Var, PPB_Var_Deprecated,
PPP_Class_Deprecated, and PPB_Class.  PP_Var will remain as a variant data type.
 Its ability to reference an object may or may not go away.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:34: *  strongly recommended that message
handlers check @a origin before doing
On 2011/02/17 19:18:47, Mark Seaborn wrote:
> This is contradictory.  On the one hand you say this field should be checked. 
> On the other hand you say it can only have one value.  What is a NaCl process
> supposed to check this against?
> 
> I think you should omit this field.  If you want to extend the message format
> later you can add more callback types.
PP_MessageEvent_Dev is based around the spec for MessageEvent (see above link),
which applies not only to postMessage, and not only to this one case of
postMessage.  For NaCl usage of postMessage, same-origin will always be true,
but not for all possible uses of PP_MessageEvent_Dev.  Taking Darin's comment
below in to account, I think we need this now (not just for future-proofing,
which was my original intent.)

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:37: *  DESIGN ISSUE:  There was discussion of
omitting this parameter.  For now,
On 2011/02/17 19:20:52, darin wrote:
> OK, yeah...
> 
> While NaCl has a same-origin restriction, other pepper plugins do not.  We
> should design with that in mind.
> 
> Unlike the postMessage API exposed to JS, we don't really know the origin of
the
> target--the plugin.  NaCl knows this, but the pepper plugin host in Chrome
does
> not.  All Chrome knows is the origin of the Document embedding the plugin.
> 
Ah, thanks for pointing that out!

> So, I think this data structure should include both source_origin and
> target_origin fields.  That will enable trusted pepper plugins that are
loading
> restricted content (think flash) to perform origin checks because only the
> plugin may know the origin of the content it has loaded into an instance.
The spec doesn't met a target origin...  I'm not sure what it would be used for.
 In the Flash plugin, what would it be?  A file URL?
> 
> This may impact the PostMessage function call too.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance, struct
PP_MessageEvent_Dev message);
On 2011/02/17 19:20:52, darin wrote:
> nit: pass message by const pointer.
> 
> Stepping back, I thought that David Sehr made a good suggestion the other day.

> Instead of this model for PostMessage (which I know I had suggested), maybe we
> could have the notion of PPB_MessagePort that you can operate on.  Then the
> plugin can assign a message port to the instance.
I thought about doing MessagePort based on his suggestion, and I want to support
that model eventually.  But after seeing how it's done in JS, I realized there
are postMessage functions sprinkled all over the place (Workers, iframes,
MessagePorts), and so I thought we could go with a simple approach for now and
expand later with an appropriate implementation of MessagePort.

> 
> PPB_MessagePort would have methods to post and pop messages.  The pop function
> would take a PP_CompletionCallback as we could say that post always completes
> quickly by placing the message in an outbound queue.
Interesting...  I wasn't thinking of a polling model.  That would certainly make
it easier to define.
> 
> struct PPB_MessagePort {
>   PP_Resource (*Create)(PP_Instance, struct PP_Var origin);
> 
>   int32_t (*PostMessage)(PP_Resource message_port,
>                          const struct PP_Var message,
>                          const struct PP_Var target_origin);
> 
>   int32_t (*PopMessage)(PP_Resource message_port, 
>                         struct PP_Message_Dev* message,
>                         PP_CompletionCallback callback);
> 
>   void (*Close)(PP_Resource message_port);
> };
> 
> The Close function is needed so that the caller of PopMessage has a way to
> interrupt the call, allowing them to free the memory associated with the given
> PP_Message_Dev struct.
> 
> (I'm not sure that the "Event" suffix to PP_MessageEvent_Dev is necessary.  A
> message is a kind of event, right?)
> 
I'm mirroring the spec for MessageEvent to make it more familiar to those who
know the spec.
> Here's what PPB_Instance could look like for binding a message port:
> 
> struct PPB_Instance {
>   ...
>   void (*BindMessagePort)(PP_Instance instance,
>                           PP_Resource message_port);
>   ...
> };
> 
> There is one thing I'm not sure of, which is what happens if JS calls
> postMessage on the plugin element before the plugin has called
BindMessagePort. 
> Should we just queue messages on behalf of the plugin,
If so, when do we stop?
> or should we drop them on
> the floor?  We can say that BindMessagePort should be called during DidCreate,
> but I'm not sure if DidCreate runs synchronously to plugin construction or
not.
> 
I think these problems go away if we're willing to do OnMessage directly on
PPP_Instance for now and add separate MessagePorts (and a way to expose them)
later.
> Note:  I've added an origin parameter to the PPB_MessagePort::Create function
as
> a way of identifying the origin of the message port.  Normally, this is
derived
> from the script context, but we don't have that in Pepper.  This origin will
be
> used as the "origin" field for MessageEvent seen by JS.  We can also go one
step
> further and say that PopMessage will not return any messages that do not match
> the origin of the message port.  This forces message ports to be
> origin-restricted, which is certainly fine for NaCl, and probably fine for
> others too.

[brettw, 2011-02-17 20:40:01]

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
File ppapi/c/dev/pp_message_event_dev.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:61: struct PP_Var source;
Yeah, I would probably just delete this.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance, struct
PP_MessageEvent_Dev message);
In reality, 99% of people will use the simplest postMessage between JS and the
plugin and won't need to talk between plugins or do other message port stuff. It
would be nice if this was easy to explain and difficult to mess up. I tried to
do a minimal implementation both ways using a hypothetical C++ wrapper:

class MyInstance : public pp::Instance {
  void OnMessage(const PP_MessageEvent_Dev& msg) {
    if (msg.data.AsString() == "doit")
      DoIt();
  }
};

class MyInstance : public pp::Instance {
  MyInstance() : factory_(this), port_(origin) {
    BindMessagePort(port_);
    if (port_.PopMessage(factory.NewCompletionCallback(&OnMessage)) ==
        PP_OK)
      OnMessage();  // I bet people will forget this synchronous case.
  }
  ~MyInstance() {
    port_.Close();  // In case you passed the port_ ref anywhere else.
  }
  void OnMessage(int32_t result) {
    if (result != PP_OK)
      return;  // I bet people will forget this check.
    if (message_.data.AsString() == "doit")
      DoIt();
  }
 private:
  pp::CompletionCallbackFactory factory_;
  PP_Message_Dev message_;
  pp::MessagePort port_;
};

I found 2 bugs in my initial implementation of the MessagePort way as I was
writing this, and 0 bugs in the OnMessage way. And if you're writing to the C
API, the difference will be even greater. I think I prefer the simple way for
now as long as we're not going to be in a place later where we wished it was
more expressive.

[dmichael(do not use this one), 2011-02-17 22:18:22]

I fixed a couple little things based on the feedback so far.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
File ppapi/c/dev/pp_message_event_dev.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:19: *  instrinsic type or string (but not
structured data such as an object or
On 2011/02/17 19:18:47, Mark Seaborn wrote:
> "instrinsic" -> "intrinsic"
> 
> Does "intrinsic type or string" mean "number or string"?  (AFAIK, "intrinsic
> type" is not a technical term in Javascript.)
> 
> I wouldn't have thought sending a single number as a message is very useful.
I reworded it.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
ppapi/c/dev/pp_message_event_dev.h:61: struct PP_Var source;
On 2011/02/17 20:40:01, brettw wrote:
> Yeah, I would probably just delete this.
Done.

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance, struct
PP_MessageEvent_Dev message);
On 2011/02/17 19:20:52, darin wrote:
> nit: pass message by const pointer.
> 
Done.

[brettw, 2011-02-21 04:58:29]

LGTM. We should probably check that Darin is happy with the overall direction
before checking anything in, but this is fine with me.

One note: it's kind of painful to reflect the structures that "own" a PP_Var
into C++ since you have to do some extra gymnastics for the ownership. If we're
really only going to have two things in the message structure, we may want to
consider just making them parameters. I realize we may add more stuff and the
current approach gives more flexibility in that case.

[dmichael(do not use this one), 2011-02-22 15:24:20]

I'll wait for Darin to comment, and I'll look in to the struct-owning-PP_Var
thing.  I'd like to keep the flexibility if possible.

A quick question on process:  would I check in the API before there is an
implementation backing it?  I thought this CL was just a proposal and I'd
fill in the details of the Chrome side (+tests and review) before checking
in.  But if it's typical to put the interface in first, I can do that.

Another question...  I made PP_MessageEvent_Dev under /dev/.  If I end up
keeping it, should I leave it there, or optimistically move it up to
ppapi/c?  I realized that in my current proposal PPP_Instance uses a 'Dev'
struct, and no other 'stable' interfaces do that so far as I can tell.

On Sun, Feb 20, 2011 at 9:58 PM, <brettw@chromium.org> wrote:

> LGTM. We should probably check that Darin is happy with the overall
> direction
> before checking anything in, but this is fine with me.
>
> One note: it's kind of painful to reflect the structures that "own" a
> PP_Var
> into C++ since you have to do some extra gymnastics for the ownership. If
> we're
> really only going to have two things in the message structure, we may want
> to
> consider just making them parameters. I realize we may add more stuff and
> the
> current approach gives more flexibility in that case.
>
>
> http://codereview.chromium.org/6538028/
>

[brettw, 2011-02-22 16:10:37]

On Tue, Feb 22, 2011 at 7:22 AM, David Michael <dmichael@google.com> wrote:
> I'll wait for Darin to comment, and I'll look in to the struct-owning-PP_Var
> thing.  I'd like to keep the flexibility if possible.
> A quick question on process:  would I check in the API before there is an
> implementation backing it?  I thought this CL was just a proposal and I'd
> fill in the details of the Chrome side (+tests and review) before checking
> in.  But if it's typical to put the interface in first, I can do that.

Yes, it's common to just check in the headers first. But this is more
of a convenience. You can do whatever works better for you.

> Another question...  I made PP_MessageEvent_Dev under /dev/.  If I end up
> keeping it, should I leave it there, or optimistically move it up to
> ppapi/c?  I realized that in my current proposal PPP_Instance uses a 'Dev'
> struct, and no other 'stable' interfaces do that so far as I can tell.

Good point, let's optimistically put it in "c".

Brett

[darin (slow to review), 2011-02-23 06:32:27]

I'm concerned that PostMessage needs to pass both target and source origins
since the non-NaCl host does not know the source origin.  The same is true of
OnMessage.  The non-NaCl host needs to see the source origin as well as the
target origin since the host cannot perform origin checks itself.

The problem with the above is that it presents a far less friendly API to NaCl
users since NaCl does know the origin of the NEXE.  It would be nice if NaCl
users did not need to specify source origin as it would be redundant, and it
would be nice if NaCl users did not need to see target origin in OnMessage. 
Just like JavaScript!  I'm just not sure how to make this happen.

My message port proposal addressed this issue slightly as NaCl users only needed
to redundantly mention their source origin once when constructing the message
port.  That could be hidden away in the C++ bindings though.

nit: OnMessage should be renamed to HandleMessageEvent (for consistency w/
HandleInputEvent).

[darin (slow to review), 2011-02-23 06:39:03]

Please keep in mind that we want to allow NEXEs to be loaded cross-origin
(subject to CORS restriction).  It is therefore important that we also support
CORS for messaging between JS and the NEXE.

Here's a proposal:

Just make PP_MessageEvent have both source_origin and target_origin fields. 
Make PostMessage also take a PP_MessageEvent.  Make it so that you have to
specify both target_origin and source_origin when calling PostMessage, even when
calling PostMessage from untrusted code.  Perhaps we can do something at the C++
layer to make this less annoying, although I'm not sure what.  Maybe we can
support "/" just as JavaScript does, but only support that when the host has
some way to interpret it (applies only to the NaCl host).

The trusted NaCl host would audit the PP_MessageEvent passed to it from
JavaScript before passing it along to the untrusted code.

[darin (slow to review), 2011-02-23 06:42:04]

On 2011/02/17 20:40:01, brettw wrote:
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> File ppapi/c/dev/pp_message_event_dev.h (right):
> 
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/dev/pp_message_event...
> ppapi/c/dev/pp_message_event_dev.h:61: struct PP_Var source;
> Yeah, I would probably just delete this.
> 
> http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h
> File ppapi/c/ppp_instance.h (right):
> 
>
http://codereview.chromium.org/6538028/diff/2002/ppapi/c/ppp_instance.h#newco...
> ppapi/c/ppp_instance.h:149: void (*OnMessage)(PP_Instance instance, struct
> PP_MessageEvent_Dev message);
> In reality, 99% of people will use the simplest postMessage between JS and the
> plugin and won't need to talk between plugins or do other message port stuff.
It
> would be nice if this was easy to explain and difficult to mess up. I tried to
> do a minimal implementation both ways using a hypothetical C++ wrapper:
> 
> class MyInstance : public pp::Instance {
>   void OnMessage(const PP_MessageEvent_Dev& msg) {
>     if (msg.data.AsString() == "doit")
>       DoIt();
>   }
> };
> 
> class MyInstance : public pp::Instance {
>   MyInstance() : factory_(this), port_(origin) {
>     BindMessagePort(port_);
>     if (port_.PopMessage(factory.NewCompletionCallback(&OnMessage)) ==
>         PP_OK)
>       OnMessage();  // I bet people will forget this synchronous case.
>   }
>   ~MyInstance() {
>     port_.Close();  // In case you passed the port_ ref anywhere else.
>   }
>   void OnMessage(int32_t result) {
>     if (result != PP_OK)
>       return;  // I bet people will forget this check.
>     if (message_.data.AsString() == "doit")
>       DoIt();
>   }
>  private:
>   pp::CompletionCallbackFactory factory_;
>   PP_Message_Dev message_;
>   pp::MessagePort port_;
> };
> 
> I found 2 bugs in my initial implementation of the MessagePort way as I was
> writing this, and 0 bugs in the OnMessage way. And if you're writing to the C
> API, the difference will be even greater. I think I prefer the simple way for
> now as long as we're not going to be in a place later where we wished it was
> more expressive.

I agree this is more complicated for the simple case.  However, that doesn't
really worry me that much.  People can build the simpler API on top of this one.

However, the thing I like the most about OnMessage is that it addresses the race
condition issue.

[Mark Seaborn, 2011-02-23 07:04:56]

On 22 February 2011 22:39, <darin@chromium.org> wrote:

> Please keep in mind that we want to allow NEXEs to be loaded cross-origin
> (subject to CORS restriction).  It is therefore important that we also
> support
> CORS for messaging between JS and the NEXE.
>

I don't see how the second sentence follows from the first.

If a page from a.com fetches NaCl executables or libraries from b.com, the
purpose of using CORS or UMP for the request is so that a.com can see the
data returned from b.com, so that the executable/libraries can be run in the
context of the a.com page.

That is what I understand by "subject to CORS restriction", but I don't know
what you mean by "CORS for messaging between JS and the NEXE".  CORS is an
extension to HTTP but you surely don't mean to use HTTP to communicate
between Javascript and a NaCl process.

Are you suggesting frame+postMessage()-style origin checks between
Javascript and NaCl?  Do you mean that a NaCl executable fetched from
b.comwould run in the context of
b.com?  If so, this would be quite dangerous, because it would mean that no
domain could host an executable with MIME type of application/x-nacl (or
similar) without trusting that executable with all the domain's ambient
authority.

Cheers,
Mark

[darin (slow to review), 2011-02-23 08:22:46]

On Tue, Feb 22, 2011 at 11:04 PM, Mark Seaborn <mseaborn@chromium.org>wrote:

> On 22 February 2011 22:39, <darin@chromium.org> wrote:
>
>> Please keep in mind that we want to allow NEXEs to be loaded cross-origin
>> (subject to CORS restriction).  It is therefore important that we also
>> support
>> CORS for messaging between JS and the NEXE.
>>
>
> I don't see how the second sentence follows from the first.
>
> If a page from a.com fetches NaCl executables or libraries from b.com, the
> purpose of using CORS or UMP for the request is so that a.com can see the
> data returned from b.com, so that the executable/libraries can be run in
> the context of the a.com page.
>

Oh, right.  I got confused :-(  Sorry for the noise.


>
> That is what I understand by "subject to CORS restriction", but I don't
> know what you mean by "CORS for messaging between JS and the NEXE".  CORS is
> an extension to HTTP but you surely don't mean to use HTTP to communicate
> between Javascript and a NaCl process.
>

Sure, CORS is technically an extension to HTTP.  What I was referring to is
the origin checking employed by the JS postMessage API, which enables the
cross-origin exchange of data.  It is CORS-like.  CORS is also a
cross-origin exchange of data.



>
> Are you suggesting frame+postMessage()-style origin checks between
> Javascript and NaCl?
>

Yes, I had confused myself into thinking that this is what it would mean to
load a cross-origin NEXE.  Thanks for setting me straight.  Clearly, I
should step away from the keyboard at this late hour ;-)

-Darin



>   Do you mean that a NaCl executable fetched from b.com would run in the
> context of b.com?  If so, this would be quite dangerous, because it would
> mean that no domain could host an executable with MIME type of
> application/x-nacl (or similar) without trusting that executable with all
> the domain's ambient authority.
>
> Cheers,
> Mark
>
>

[darin (slow to review), 2011-02-23 18:09:28]

OK, I've convinced myself that the argument I was making is bogus :-)

Here's the deal:  URLLoader and FileSystem APIs already assume that the
origin of the Pepper plugin is the origin of the Document.  The FileSystem
API exposes the origin specific file system!

So, it should also hold true that the Pepper host knows the origin of the
plugin using PostMessage and receiving messages.

This means that the plugin should not need to specify his own origin when
calling PostMessage.  I will go one step further though and argue that we
don't need to exchange any origin information.  The only reason we would is
if it were somehow possible for JS not in our origin to call the postMessage
exposed on our <embed> element.  That doesn't seem possible given the
same-origin restrictions of JS.

Thus, it seems like we can ditch the origin stuff entirely, which is nice.
 The only case where it might make a return is if we support MessagePorts
and support passing them via postMessage to JS running with an IFRAME in
another origin, say.  But, we aren't there yet.

Ditching origin info suggests that we can just get by making PostMessage
take a PP_Var and OnMessage receive a PP_Var.  However, perhaps we should
still keep the PP_MessageEvent structure so that we can have the ability to
extend it in the future?  We might want to expose message ports or maybe we
would want to express the message differently if it is used to pass a binary
array efficiently.  Just like PP_InputEvent, it seems like PP_MessageEvent
would then need a type field, so that we can extend it in the future.

-Darin



On Tue, Feb 22, 2011 at 10:32 PM, <darin@chromium.org> wrote:

> I'm concerned that PostMessage needs to pass both target and source origins
> since the non-NaCl host does not know the source origin.  The same is true
> of
> OnMessage.  The non-NaCl host needs to see the source origin as well as the
> target origin since the host cannot perform origin checks itself.
>
> The problem with the above is that it presents a far less friendly API to
> NaCl
> users since NaCl does know the origin of the NEXE.  It would be nice if
> NaCl
> users did not need to specify source origin as it would be redundant, and
> it
> would be nice if NaCl users did not need to see target origin in OnMessage.
> Just like JavaScript!  I'm just not sure how to make this happen.
>
> My message port proposal addressed this issue slightly as NaCl users only
> needed
> to redundantly mention their source origin once when constructing the
> message
> port.  That could be hidden away in the C++ bindings though.
>
> nit: OnMessage should be renamed to HandleMessageEvent (for consistency w/
> HandleInputEvent).
>
>
> http://codereview.chromium.org/6538028/
>

[dmichael(do not use this one), 2011-02-23 18:36:27]

On Wed, Feb 23, 2011 at 11:09 AM, Darin Fisher <darin@chromium.org> wrote:

> OK, I've convinced myself that the argument I was making is bogus :-)
>
> Here's the deal:  URLLoader and FileSystem APIs already assume that the
> origin of the Pepper plugin is the origin of the Document.  The FileSystem
> API exposes the origin specific file system!
>
> So, it should also hold true that the Pepper host knows the origin of the
> plugin using PostMessage and receiving messages.
>
> This means that the plugin should not need to specify his own origin when
> calling PostMessage.  I will go one step further though and argue that we
> don't need to exchange any origin information.  The only reason we would is
> if it were somehow possible for JS not in our origin to call the postMessage
> exposed on our <embed> element.  That doesn't seem possible given the
> same-origin restrictions of JS.
>
> Thus, it seems like we can ditch the origin stuff entirely, which is nice.
>  The only case where it might make a return is if we support MessagePorts
> and support passing them via postMessage to JS running with an IFRAME in
> another origin, say.  But, we aren't there yet.
>
> Ditching origin info suggests that we can just get by making PostMessage
> take a PP_Var and OnMessage receive a PP_Var.  However, perhaps we should
> still keep the PP_MessageEvent structure so that we can have the ability to
> extend it in the future?  We might want to expose message ports or maybe we
> would want to express the message differently if it is used to pass a binary
> array efficiently.  Just like PP_InputEvent, it seems like PP_MessageEvent
> would then need a type field, so that we can extend it in the future.
>
Given what Brett said about difficulties with having structs own Vars, it
might be nice to ditch PP_MessageEvent for now, and reintroduce it later if
necessary.  We can rely on PPP_Instance's version string for supporting
backwards compatibility if we change PostMessage to take a PP_MessageEvent
(I think we'd have to do that anyway).


>> nit: OnMessage should be renamed to HandleMessageEvent (for consistency w/
>> HandleInputEvent).
>>
>>
>> http://codereview.chromium.org/6538028/
>>
>
> I was going for consistency with the postMessage in the spec (for
MessagePorts here
http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html and
for Workers here http://www.whatwg.org/specs/web-workers/current-work/).
 But I'm happy to go with HandleMessageEvent, if internal consistency trumps
consistency with the spec.  Obviously, we don't need to conform to the
spec., but I was trying to make NaCl look as much like a Worker as was
practical.

Thoughts?

(Thanks again for all the feedback)

[darin (slow to review), 2011-02-23 21:00:54]

On Wed, Feb 23, 2011 at 10:33 AM, David Michael <dmichael@google.com> wrote:

>
>
> On Wed, Feb 23, 2011 at 11:09 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> OK, I've convinced myself that the argument I was making is bogus :-)
>>
>> Here's the deal:  URLLoader and FileSystem APIs already assume that the
>> origin of the Pepper plugin is the origin of the Document.  The FileSystem
>> API exposes the origin specific file system!
>>
>> So, it should also hold true that the Pepper host knows the origin of the
>> plugin using PostMessage and receiving messages.
>>
>> This means that the plugin should not need to specify his own origin when
>> calling PostMessage.  I will go one step further though and argue that we
>> don't need to exchange any origin information.  The only reason we would is
>> if it were somehow possible for JS not in our origin to call the postMessage
>> exposed on our <embed> element.  That doesn't seem possible given the
>> same-origin restrictions of JS.
>>
>> Thus, it seems like we can ditch the origin stuff entirely, which is nice.
>>  The only case where it might make a return is if we support MessagePorts
>> and support passing them via postMessage to JS running with an IFRAME in
>> another origin, say.  But, we aren't there yet.
>>
>> Ditching origin info suggests that we can just get by making PostMessage
>> take a PP_Var and OnMessage receive a PP_Var.  However, perhaps we should
>> still keep the PP_MessageEvent structure so that we can have the ability to
>> extend it in the future?  We might want to expose message ports or maybe we
>> would want to express the message differently if it is used to pass a binary
>> array efficiently.  Just like PP_InputEvent, it seems like PP_MessageEvent
>> would then need a type field, so that we can extend it in the future.
>>
> Given what Brett said about difficulties with having structs own Vars, it
> might be nice to ditch PP_MessageEvent for now, and reintroduce it later if
> necessary.  We can rely on PPP_Instance's version string for supporting
> backwards compatibility if we change PostMessage to take a PP_MessageEvent
> (I think we'd have to do that anyway).
>

I think we can change PP_InputEvent without changing PPP_Instance's version,
provided we retain backwards compatibility.  The same could be true of
PP_MessageEvent.



>
>
>>> nit: OnMessage should be renamed to HandleMessageEvent (for consistency
>>> w/
>>> HandleInputEvent).
>>>
>>>
>>> http://codereview.chromium.org/6538028/
>>>
>>
>> I was going for consistency with the postMessage in the spec (for
> MessagePorts here
> http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html and
> for Workers here http://www.whatwg.org/specs/web-workers/current-work/).
>  But I'm happy to go with HandleMessageEvent, if internal consistency trumps
> consistency with the spec.  Obviously, we don't need to conform to the
> spec., but I was trying to make NaCl look as much like a Worker as was
> practical.


Right... I think we are already deviating from the spec a lot.  If the
parameter is not PP_MessageEvent, then I'd just call the method
HandleMessage.

But as you say, I think it is very important for Pepper to be
self-consistent, so I think we should put greater emphasis on internal
consistency than consistency with HTML APIs, when it comes to naming
conventions.  (We don't use the "On" prefix anywhere.)

-Darin



>
> Thoughts?
>
> (Thanks again for all the feedback)
>

[dmichael(do not use this one), 2011-02-23 22:16:49]

Okay, one more try.  This changes the PP_MessageEvent argument to just a PP_Var,
since we don't need origin yet.  I checked with Darin, and he's okay with this.

OnMessage has been renamed HandleMessage to be consistent with other PPAPI
functions (but the JS handler will still be called onmessage to mirror the
Worker spec).

If this looks okay, I can start working on the browser implementation and tests.
 I think I'll wait to check in the interface until there's a backing
implementation, in case I find some important detail we're missing while
implementing it.

[darin (slow to review), 2011-02-23 22:36:46]

LGTM

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:175: struct PP_Var (*GetInstanceObject)(PP_Instance
instance);
I think now is the time to delete GetInstanceObject and the corresponding APIs
on PPB_Instance:  GetWindowObject, GetOwnerElementObject, and ExecuteScript. 
Those can be moved to some other private interfaces to help Flash and NaCl
implementations.

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, PP_Var
message);
I think you need to stay "struct PP_Var" here.

[dmichael(do not use this one), 2011-02-23 22:45:00]

Terrific, thank you all for the great comments!

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:175: struct PP_Var (*GetInstanceObject)(PP_Instance
instance);
On 2011/02/23 22:36:46, darin wrote:
> I think now is the time to delete GetInstanceObject and the corresponding APIs
> on PPB_Instance:  GetWindowObject, GetOwnerElementObject, and ExecuteScript. 
> Those can be moved to some other private interfaces to help Flash and NaCl
> implementations.

I'm concerned that will make it really difficult to put the initial change in. 
I'd like to leave backwards-compatibility (at LEAST for compilation, probably
for behavior if it's not too impractical) until NaCl can adapt to the new
interface.  Otherwise, I'll be breaking bots left and right.

http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, PP_Var
message);
On 2011/02/23 22:36:46, darin wrote:
> I think you need to stay "struct PP_Var" here.
Nice catch, thanks!  If I'd actually *built* ppapi_tests, I would've caught it
:-p

[darin (slow to review), 2011-02-23 22:51:06]

On Wed, Feb 23, 2011 at 2:45 PM, <dmichael@google.com> wrote:

> Terrific, thank you all for the great comments!
>
>
>
> http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h
> File ppapi/c/ppp_instance.h (right):
>
>
>
http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
> ppapi/c/ppp_instance.h:175: struct PP_Var
> (*GetInstanceObject)(PP_Instance instance);
> On 2011/02/23 22:36:46, darin wrote:
>
>> I think now is the time to delete GetInstanceObject and the
>>
> corresponding APIs
>
>> on PPB_Instance:  GetWindowObject, GetOwnerElementObject, and
>>
> ExecuteScript.
>
>> Those can be moved to some other private interfaces to help Flash and
>>
> NaCl
>
>> implementations.
>>
>
> I'm concerned that will make it really difficult to put the initial
> change in.  I'd like to leave backwards-compatibility (at LEAST for
> compilation, probably for behavior if it's not too impractical) until
> NaCl can adapt to the new interface.  Otherwise, I'll be breaking bots
> left and right.


Yeah, I figured as much.  Please go with whatever allows you to make
progress most readily, so long as it doesn't mean leaving these entry
points in the function tables forever.

I think at least in this CL you should be able to annotate the methods
as DEPRECATED.

-Darin



>
>
>
>
http://codereview.chromium.org/6538028/diff/19001/ppapi/c/ppp_instance.h#newc...
> ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance,
> PP_Var message);
> On 2011/02/23 22:36:46, darin wrote:
>
>> I think you need to stay "struct PP_Var" here.
>>
> Nice catch, thanks!  If I'd actually *built* ppapi_tests, I would've
> caught it :-p
>
>
> http://codereview.chromium.org/6538028/
>

[polina, 2011-02-26 08:00:08]

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h
File ppapi/c/ppb_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:112: * PostMessage asynchronously invokes the onmessage
handler of the 'embed'
s/'embed' instance/module instance/

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:121: *   <object id="module"
s/module/plugin/
(You are also misusing the word "module" in the CL description. It should be
plugin or module instance or just instance.)

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:125: *       alert(message.data);
It would be a good idea to extend the header comment to mention what kind of
object "message" is and how the function args will map to its properties.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:130: * If the module then invokes PostMessage as follows:
PostMessage()

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:139: void (*PostMessage)(PP_Instance instance, struct
PP_Var message);
Can this be any type of PP_Var?

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:8: #include "ppapi/c/dev/pp_message_event_dev.h"
this file is no longer in the CL

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:178: * This method gets called when postMessage is
invoked on the DOM object for
postMessage()

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:178: * This method gets called when postMessage is
invoked on the DOM object for
called asynchronously?
(you clarified this in the other interface)

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:192: * message being a string PP_Var containing "Hello
world!".
In the other direction you pass an object with a property set to the original
message and possibly other properties (e.g. port?). Why do you only send a
string PP_Var in this direction?

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, struct
PP_Var message);
Why is this not called OnMessage?

[brettw, 2011-02-26 17:11:17]

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, struct
PP_Var message);
We decided not to use "On" since it can be ambiguous whether it's telling you to
do something or telling you that something happened. The other functions on this
class are named "Get" or "Handle"

[polina, 2011-02-26 20:06:36]

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, struct
PP_Var message);
On 2011/02/26 17:11:17, brettw wrote:
> We decided not to use "On" since it can be ambiguous whether it's telling you
to
> do something or telling you that something happened. The other functions on
this
> class are named "Get" or "Handle"

I agree that "HandleMessage" is a better name in isolation. But since we use
onmessage in the other direction to match the Web Workers analogy, it would be
nice to have naming consistency in both directions.

What about renaming the other "onmessage" to "handleMessage"?

[dmichael(do not use this one), 2011-02-27 02:10:39]

I really want to keep the 'onmessage' name in the JavaScript side, at least.
 If we say we are implementing postMessage, it should look and feel like
postMessage does in JavaScript on other DOM elements.  I don't care much
what we call it on the native side.

On Sat, Feb 26, 2011 at 8:06 PM, <polina@google.com> wrote:

>
> http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
> File ppapi/c/ppp_instance.h (right):
>
>
>
http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
> ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance,
> struct PP_Var message);
> On 2011/02/26 17:11:17, brettw wrote:
>
>> We decided not to use "On" since it can be ambiguous whether it's
>>
> telling you to
>
>> do something or telling you that something happened. The other
>>
> functions on this
>
>> class are named "Get" or "Handle"
>>
>
> I agree that "HandleMessage" is a better name in isolation. But since we
> use onmessage in the other direction to match the Web Workers analogy,
> it would be nice to have naming consistency in both directions.
>
> What about renaming the other "onmessage" to "handleMessage"?
>
>
> http://codereview.chromium.org/6538028/
>

[darin (slow to review), 2011-02-27 03:27:26]

Ditto.
-Darin

On Sat, Feb 26, 2011 at 6:10 PM, David Michael <dmichael@google.com> wrote:

> I really want to keep the 'onmessage' name in the JavaScript side, at
> least.  If we say we are implementing postMessage, it should look and feel
> like postMessage does in JavaScript on other DOM elements.  I don't care
> much what we call it on the native side.
>
>
> On Sat, Feb 26, 2011 at 8:06 PM, <polina@google.com> wrote:
>
>>
>> http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
>> File ppapi/c/ppp_instance.h (right):
>>
>>
>>
http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
>> ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance,
>> struct PP_Var message);
>> On 2011/02/26 17:11:17, brettw wrote:
>>
>>> We decided not to use "On" since it can be ambiguous whether it's
>>>
>> telling you to
>>
>>> do something or telling you that something happened. The other
>>>
>> functions on this
>>
>>> class are named "Get" or "Handle"
>>>
>>
>> I agree that "HandleMessage" is a better name in isolation. But since we
>> use onmessage in the other direction to match the Web Workers analogy,
>> it would be nice to have naming consistency in both directions.
>>
>> What about renaming the other "onmessage" to "handleMessage"?
>>
>>
>> http://codereview.chromium.org/6538028/
>>
>
>

[darin (slow to review), 2011-02-27 03:28:19]

Er, I mean... I agree with your argument for onmessage, but I also feel
strongly about consistency on the native side as I wrote in a previous
review comment.

On Sat, Feb 26, 2011 at 7:27 PM, Darin Fisher <darin@chromium.org> wrote:

> Ditto.
> -Darin
>
>
> On Sat, Feb 26, 2011 at 6:10 PM, David Michael <dmichael@google.com>wrote:
>
>> I really want to keep the 'onmessage' name in the JavaScript side, at
>> least.  If we say we are implementing postMessage, it should look and feel
>> like postMessage does in JavaScript on other DOM elements.  I don't care
>> much what we call it on the native side.
>>
>>
>> On Sat, Feb 26, 2011 at 8:06 PM, <polina@google.com> wrote:
>>
>>>
>>> http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
>>> File ppapi/c/ppp_instance.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
>>> ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance,
>>> struct PP_Var message);
>>> On 2011/02/26 17:11:17, brettw wrote:
>>>
>>>> We decided not to use "On" since it can be ambiguous whether it's
>>>>
>>> telling you to
>>>
>>>> do something or telling you that something happened. The other
>>>>
>>> functions on this
>>>
>>>> class are named "Get" or "Handle"
>>>>
>>>
>>> I agree that "HandleMessage" is a better name in isolation. But since we
>>> use onmessage in the other direction to match the Web Workers analogy,
>>> it would be nice to have naming consistency in both directions.
>>>
>>> What about renaming the other "onmessage" to "handleMessage"?
>>>
>>>
>>> http://codereview.chromium.org/6538028/
>>>
>>
>>
>

[dmichael(do not use this one), 2011-02-28 15:59:28]

Tweaked to address some of Polina's comments.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h
File ppapi/c/ppb_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:112: * PostMessage asynchronously invokes the onmessage
handler of the 'embed'
On 2011/02/26 08:00:08, polina wrote:
> s/'embed' instance/module instance/
That's not quite what I was trying (badly) to convey; I want it clear that it's
a JS handler on the DOM element.  I reworded it.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:121: *   <object id="module"
On 2011/02/26 08:00:08, polina wrote:
> s/module/plugin/
> (You are also misusing the word "module" in the CL description. It should be
> plugin or module instance or just instance.)
Done.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:125: *       alert(message.data);
On 2011/02/26 08:00:08, polina wrote:
> It would be a good idea to extend the header comment to mention what kind of
> object "message" is and how the function args will map to its properties.
Done.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:130: * If the module then invokes PostMessage as follows:
On 2011/02/26 08:00:08, polina wrote:
> PostMessage()
Done.  I also threw in some @a doxygen annotations for good measure.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:139: void (*PostMessage)(PP_Instance instance, struct
PP_Var message);
On 2011/02/26 08:00:08, polina wrote:
> Can this be any type of PP_Var?
I added more information to the top-level comment.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h
File ppapi/c/ppp_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:8: #include "ppapi/c/dev/pp_message_event_dev.h"
On 2011/02/26 08:00:08, polina wrote:
> this file is no longer in the CL
Done.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:178: * This method gets called when postMessage is
invoked on the DOM object for
On 2011/02/26 08:00:08, polina wrote:
> postMessage()
Done.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:178: * This method gets called when postMessage is
invoked on the DOM object for
On 2011/02/26 08:00:08, polina wrote:
> called asynchronously?
> (you clarified this in the other interface)
Done.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:192: * message being a string PP_Var containing "Hello
world!".
On 2011/02/26 08:00:08, polina wrote:
> In the other direction you pass an object with a property set to the original
> message and possibly other properties (e.g. port?). Why do you only send a
> string PP_Var in this direction?
See previous review comments.  I was originally going with a PP_MessageEvent
that was a rough translation of the JavaScript MessageEvent object.  It turned
out we didn't need anything other than the data on the native side (at least for
now), so I changed to PP_Var to keep it simple.  But I want the JS side to match
up with the existing spec for Workers, so the JS onmessage handler gets a
MessageData.

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppp_instance.h#newc...
ppapi/c/ppp_instance.h:194: void (*HandleMessage)(PP_Instance instance, struct
PP_Var message);
On 2011/02/26 20:06:36, polina wrote:
> On 2011/02/26 17:11:17, brettw wrote:
> > We decided not to use "On" since it can be ambiguous whether it's telling
you
> to
> > do something or telling you that something happened. The other functions on
> this
> > class are named "Get" or "Handle"
> 
> I agree that "HandleMessage" is a better name in isolation. But since we use
> onmessage in the other direction to match the Web Workers analogy, it would be
> nice to have naming consistency in both directions.
> 
> What about renaming the other "onmessage" to "handleMessage"?
See other comments.  I want to keep the JS side 'onmessage' for consistency with
JS, and the native side HandleMessage for consistency with other PPAPI
functions.  Unfortunately, that means that the two endpoints of the interface
look different, but there doesn't seem to be any approach that is free of
compromises.

[polina, 2011-03-03 00:22:15]

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h
File ppapi/c/ppb_instance.h (right):

http://codereview.chromium.org/6538028/diff/17003/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:130: * If the module then invokes PostMessage as follows:
On 2011/02/28 15:59:28, dmichael wrote:
> On 2011/02/26 08:00:08, polina wrote:
> > PostMessage()
> Done.  I also threw in some @a doxygen annotations for good measure.

You should be consistent with whatever annotations our tech writers are
introducing.

http://codereview.chromium.org/6538028/diff/30002/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:112: * PostMessage asynchronously invokes the onmessage
handler of the 'embed'
Btw, note that our tech writers are changing all the headers to say "Foo is a
pointer to a function".

http://codereview.chromium.org/6538028/diff/30002/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:115: * See:
Thanks for the helpful clarification.

I would say "PP_Var representing a boolean, string or numeric value". Otherwise
one would have to remember to update this description if PP_Var header changes.

[Mark Seaborn, 2011-03-04 19:59:09]

dmichael wrote:

> A proposal for an initial postMessage interface. This will allow JavaScript
> to
> send data asynchronously to a module, and the module to asynchronously send
> data to a JavaScript message handler.
>
> Overview:
> From JavaScript, you can invoke 'postMessage' on the embedded module. That
> results in a call to 'PPP_Instance::HandleMessage'.
>
> From Native Code, you can invoke 'PPB_Instance::PostMessage', which results
> in a call to an 'onmessage' function on the embedded module in the
> JavaScript
> code (if one has been registered).


What would the buffering semantics of this interface be?  Specifically, what
happens if Javascript repeatedly sends messages with postMessage() and the
NaCl process does not unqueue them?  Does the message queue have an
unlimited-size buffer?  If not, would postMessage() block or raise an
exception when the buffer is full?

I am not sure whether this would need to be covered by a spec.  This issue
presumably arises for Web Workers' MessagePorts, but I don't see any mention
of it in the spec (http://www.whatwg.org/specs/web-workers/current-work/).

This is less of an issue with Javascript and Web Workers than with NaCl,
though.  As I understand it, with JS/workers, the only way for a worker to
prevent its messages from being unqueued is for it to busy-loop inside an
event loop turn.  With NaCl, though, we have blocking syscalls, and so there
is more latitude leaving an incoming message queue to build up without
consuming CPU.

Mark

[sehr (please use chromium), 2011-03-14 23:47:46]

One typo, one question.

http://codereview.chromium.org/6538028/diff/45005/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

http://codereview.chromium.org/6538028/diff/45005/ppapi/cpp/instance.h#newcode75
ppapi/cpp/instance.h:75: /** See PPP_Instance.HandleEvent. */
s/Event/Message/

http://codereview.chromium.org/6538028/diff/45005/ppapi/tests/test_post_messa...
File ppapi/tests/test_post_message.cc (right):

http://codereview.chromium.org/6538028/diff/45005/ppapi/tests/test_post_messa...
ppapi/tests/test_post_message.cc:66: std::string
TestPostMessage::TestSendingData() {
I thought that postMessage only passed strings?

[dmichael(do not use this one), 2011-03-16 21:08:48]

I think the implementation is ready for review.  Thanks for looking!

http://codereview.chromium.org/6538028/diff/45005/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

http://codereview.chromium.org/6538028/diff/45005/ppapi/cpp/instance.h#newcode75
ppapi/cpp/instance.h:75: /** See PPP_Instance.HandleEvent. */
On 2011/03/14 23:47:46, sehr wrote:
> s/Event/Message/
Done.

http://codereview.chromium.org/6538028/diff/45005/ppapi/tests/test_post_messa...
File ppapi/tests/test_post_message.cc (right):

http://codereview.chromium.org/6538028/diff/45005/ppapi/tests/test_post_messa...
ppapi/tests/test_post_message.cc:66: std::string
TestPostMessage::TestSendingData() {
On 2011/03/14 23:47:46, sehr wrote:
> I thought that postMessage only passed strings?
Who said that?  I never said that ;-)

Eventually, I want it to send the same set of things postMessage in pure JS can
send.  That includes all the intrinsic types as well as arrays and objects
(albeit as copies).  I'm leaving out object & array support for now, but passing
bools and numbers is trivial, so I'm throwing it in, no extra charge.

[brettw, 2011-03-16 21:34:11]

I didn't check all the bindings, neb may be the best person do to that.

http://codereview.chromium.org/6538028/diff/54012/ppapi/c/ppb_instance.h
File ppapi/c/ppb_instance.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:115: * @a PostMessage is a pointer to a function which
asynchronously invokes th
th -> the

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/testing_instance.h
File ppapi/tests/testing_instance.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/testing_instanc...
ppapi/tests/testing_instance.h:48: bool nacl_mode() { return nacl_mode_; }
This should be const.

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

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/messag...
webkit/plugins/ppapi/message_channel.cc:120: if (IdentifierIsPostMessage(name))
Can't you just compare identifiers? I thought that's what the point of
identifiers was, that a certain ID represents a certain function name in all
cases.

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:460: instance = new
PluginInstance(delegate, this, plugin_instance_interface,
What about just passing the instance interface object by value? It should be
fairly small and reasonable to copy, and then you don't need to worry about the
ownership complexity.

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:95: instance_->
I'd probably have wrapped this like:
instance_->message_channel().set_passthrough_object(
    object->np_object());
to avoid breaking after the ->

[neb, 2011-03-16 21:41:22]

Jumping in late, my main concern is with sticking this in the catch-all
PPB_Instance.

This set of functions is related to message posting, it should go into separate
PPB_Messaging/PPP_Messaging. Sticking scripting in PPB_Instance was wrong, now
we have to rev it just because we're changing scripting.

Specifically, there are a bunch of functions that don't have a class to operate
on, but take at least the instance, it's tempting to stick them all into
PPB_Instance since they look like they operate on the instance. So we get this
mess of unrelated functions which is PPB_Instance.

If you think of interfaces as groups of related functionality, this makes more
sense. Interface != class/vtable.

[polina, 2011-03-17 02:40:26]

I agree with Neb. I have also been wondering why we were extending
*_Instance interfaces. I suspect it's because people did not think it was
worth creating a new interface just for 1 function. But if we want to be
flexible and had an option to extend the interface in the future, having it
in one header will make it only easier.

Thanks,
P.

On Wed, Mar 16, 2011 at 2:41 PM, <neb@chromium.org> wrote:

> Jumping in late, my main concern is with sticking this in the catch-all
> PPB_Instance.
>
> This set of functions is related to message posting, it should go into
> separate
> PPB_Messaging/PPP_Messaging. Sticking scripting in PPB_Instance was wrong,
> now
> we have to rev it just because we're changing scripting.
>
> Specifically, there are a bunch of functions that don't have a class to
> operate
> on, but take at least the instance, it's tempting to stick them all into
> PPB_Instance since they look like they operate on the instance. So we get
> this
> mess of unrelated functions which is PPB_Instance.
>
> If you think of interfaces as groups of related functionality, this makes
> more
> sense. Interface != class/vtable.
>
>
> http://codereview.chromium.org/6538028/
>

[polina, 2011-03-17 02:59:28]

http://codereview.chromium.org/6538028/diff/54012/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/cpp/instance.h#newcode76
ppapi/cpp/instance.h:76: virtual void HandleMessage(const Var& message_data);
This is called |message| in the C header. Would be nice to be consistent.

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/test_case.h
File ppapi/tests/test_case.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/test_case.h#new...
ppapi/tests/test_case.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
I don't know about open source code, but in other parts of google we were
explicitly asked not to update dates after file creation.

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/test_post_messa...
File ppapi/tests/test_post_message.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/test_post_messa...
ppapi/tests/test_post_message.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
2011

[dmichael(do not use this one), 2011-03-17 14:41:57]

On 2011/03/17 02:40:26, polina wrote:
> I agree with Neb. I have also been wondering why we were extending
> *_Instance interfaces. I suspect it's because people did not think it was
> worth creating a new interface just for 1 function. But if we want to be
> flexible and had an option to extend the interface in the future, having it
> in one header will make it only easier.
> 
> Thanks,
> P.
> 
I thought about it a bit, and I agree with Neb too.  It would delay this a
little bit while I refactor the interface, but most of my implementation code
will still be valid.  It would also make the backwards-compatibility easier in
this case (as discussed, PPP_Instance is singularly difficult to support given
the way the proxy has to work currently).  I'm curious what Brett or Darin
thinks...
> On Wed, Mar 16, 2011 at 2:41 PM, <mailto:neb@chromium.org> wrote:
> 
> > Jumping in late, my main concern is with sticking this in the catch-all
> > PPB_Instance.
> >
> > This set of functions is related to message posting, it should go into
> > separate
> > PPB_Messaging/PPP_Messaging. Sticking scripting in PPB_Instance was wrong,
> > now
> > we have to rev it just because we're changing scripting.
> >
> > Specifically, there are a bunch of functions that don't have a class to
> > operate
> > on, but take at least the instance, it's tempting to stick them all into
> > PPB_Instance since they look like they operate on the instance. So we get
> > this
> > mess of unrelated functions which is PPB_Instance.
> >
> > If you think of interfaces as groups of related functionality, this makes
> > more
> > sense. Interface != class/vtable.
> >
> >
> > http://codereview.chromium.org/6538028/
> >

[brettw, 2011-03-17 16:13:46]

On Thu, Mar 17, 2011 at 7:41 AM,  <dmichael@google.com> wrote:
> On 2011/03/17 02:40:26, polina wrote:
>>
>> I agree with Neb. I have also been wondering why we were extending
>> *_Instance interfaces. I suspect it's because people did not think it was
>> worth creating a new interface just for 1 function. But if we want to be
>> flexible and had an option to extend the interface in the future, having
>> it
>> in one header will make it only easier.
>
>> Thanks,
>> P.
>
> I thought about it a bit, and I agree with Neb too.  It would delay this a
> little bit while I refactor the interface, but most of my implementation
> code
> will still be valid.  It would also make the backwards-compatibility easier
> in
> this case (as discussed, PPP_Instance is singularly difficult to support
> given
> the way the proxy has to work currently).  I'm curious what Brett or Darin
> thinks...

I'm fine with changing this.

Brett

[dmichael(do not use this one), 2011-03-21 21:43:35]

Closed this CL, started a new one at:  http://codereview.chromium.org/6716005/
to account for Neb's suggestion of leaving PPB_Instance and PPP_Instance alone.

http://codereview.chromium.org/6538028/diff/54012/ppapi/c/ppb_instance.h
File ppapi/c/ppb_instance.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/c/ppb_instance.h#newc...
ppapi/c/ppb_instance.h:115: * @a PostMessage is a pointer to a function which
asynchronously invokes th
On 2011/03/16 21:34:11, brettw wrote:
> th -> the
Done.

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/testing_instance.h
File ppapi/tests/testing_instance.h (right):

http://codereview.chromium.org/6538028/diff/54012/ppapi/tests/testing_instanc...
ppapi/tests/testing_instance.h:48: bool nacl_mode() { return nacl_mode_; }
On 2011/03/16 21:34:11, brettw wrote:
> This should be const.
Nice catch.  Turns out I didn't end up using it, so I just cut it.

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/6538028/diff/54012/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:460: instance = new
PluginInstance(delegate, this, plugin_instance_interface,
On 2011/03/16 21:34:11, brettw wrote:
> What about just passing the instance interface object by value? It should be
> fairly small and reasonable to copy, and then you don't need to worry about
the
> ownership complexity.
Good idea.  But since I'm going to follow Neb's suggestion, I'm not going to
have to worry about backwards compatibility.  Somebody (maybe Neb or I?) will
have to deal with it later when we hack functions out of PPP_Instance.