A proposal and implementation for an initial postMessage interface. These in...

A proposal and implementation for an initial postMessage interface.  These interfaces will allow JavaScript to send data asynchronously to a module instance, and the module instance to asynchronously send data to a JavaScript message handler.

Note, I did something differently from other per-instance interfaces.  While the C interface has 'PPB_Messaging' and 'PPP_Messaging' separate from the Instance interfaces, I stuck the per-instance messaging in to pp::Instance.  It seems more intuitive to me, and doesn't have the drawbacks of having too many functions in the C layer instance interfaces.  Happy to back off of that position, but it's worth a shot.

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

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

  Please see the included example or the examples in the comments of
  PPB_Messaging and PPP_Messaging.

Restrictions:
- This implementation is synchronous. A later CL will make it asynchronous.
- This implementation supports only intrinsic values and strings (all types that PP_Var supports except for objects). Object & array support will come later.
- This implementation only allows for 1 channel per instance. You can not expose other 'channels' or 'ports'.  Future CLs will add support for MessagePorts.

BUG=None
TEST=test_post_message.h/.cc

(This CL replaces http://codereview.chromium.org/6538028/ )
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79178

[brettw, 2011-03-22 05:59:11]

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
File ppapi/tests/test_post_message.cc (right):

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:16: namespace {
Blank lines on both sides of namespace begins & ends.

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:24: const std::string kTestString("Hello
world!");
Even though this is just test code, it would be nice to keep the "no static
objects" rule. In this case, I would just declar this as a const char[], I don't
even think you have to change the code below

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:29: }  // namespace
Ditto

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

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:31: MessageChannel&
ToMessageChannel(NPObject* object) {
Your code below checks for a NULL here. Can this return NULL? It seems like the
comment should say so if it can, or the checking code below should be removed.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:116: if (passthrough) {
Remove the {} to be consistent.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:125: if (NULL == np_obj)
We almost never write things in this order (the compiler warns if you do
assignment so the consensus has been to use the more "familiar" ordering). In
this case, though, I'd just do
  if (!np_obj)...
Same below.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:215: // add postMessage to the list and
return it.
Capital A

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:217: std::malloc(sizeof(NPIdentifier) *
(*count + 1)));
Why std:: on malloc & not free? Generally I haven't seen us use std:: for this
type of function.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:248: }  // namespace
Blank line before this.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:312: if (!success) return;
I'd avoid the single-line form, we almost never use this so it looks weird.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:328: if
(!PPVarToNPVariantNoCopy(message_data, &npvariant_args[1])) {
No {}

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:877: if
(!plugin_messaging_interface_) {
This will query the plugin every single time if it doesn't support the
interface. Do you think we should remember whether we've previously asked so we
can avoid re-querying if it returns NULL?

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.h:218: // Implementation of
PPB_Messaging.
I'd just use one comment for both of these functions. I don't think
"Implementation" is too misleading here, even though one of the functions is an
override. If this offends you, sometimes some people just write "//
PPP_Messanging."

[dmichael(do not use this one), 2011-03-22 15:39:00]

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
File ppapi/tests/test_post_message.cc (right):

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:16: namespace {
On 2011/03/22 05:59:11, brettw wrote:
> Blank lines on both sides of namespace begins & ends.
Done.

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:24: const std::string kTestString("Hello
world!");
On 2011/03/22 05:59:11, brettw wrote:
> Even though this is just test code, it would be nice to keep the "no static
> objects" rule. In this case, I would just declar this as a const char[], I
don't
> even think you have to change the code below
Done.

http://codereview.chromium.org/6716005/diff/8001/ppapi/tests/test_post_messag...
ppapi/tests/test_post_message.cc:29: }  // namespace
On 2011/03/22 05:59:11, brettw wrote:
> Ditto
Done.

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

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:31: MessageChannel&
ToMessageChannel(NPObject* object) {
On 2011/03/22 05:59:11, brettw wrote:
> Your code below checks for a NULL here. Can this return NULL? It seems like
the
> comment should say so if it can, or the checking code below should be removed.
This can't return NULL, but the passthrough object may be null, if there is no
'Instance' object.  I added a comment to MessageChannel::passthrough_object().

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:116: if (passthrough) {
On 2011/03/22 05:59:11, brettw wrote:
> Remove the {} to be consistent.
Done.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:125: if (NULL == np_obj)
Done

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:217: std::malloc(sizeof(NPIdentifier) *
(*count + 1)));
On 2011/03/22 05:59:11, brettw wrote:
> Why std:: on malloc & not free? Generally I haven't seen us use std:: for this
> type of function.
free was an oversight.  Most people don't use std:: on C functions, but I like
to always include the C++ versions of C headers (e.g. cstdlib instead of
stdlib.h) which put them in the std namespace, to avoid cluttering the global
namespace.  (Technically the C versions are deprecated in C++, but I'm sure
they'll never go away.)  I also think std:: makes it clearer that you're using a
standard library function.  I'm apparently getting stdlib.h for free somewhere
else in the translation unit, or the compiler would have complained.

Anyway, that's why I prefer using std:: everywhere, but I can change it if it's
too unusual.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:312: if (!success) return;
On 2011/03/22 05:59:11, brettw wrote:
> I'd avoid the single-line form, we almost never use this so it looks weird.
Done.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/message...
webkit/plugins/ppapi/message_channel.cc:328: if
(!PPVarToNPVariantNoCopy(message_data, &npvariant_args[1])) {
On 2011/03/22 05:59:11, brettw wrote:
> No {}
Done.

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/8001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:877: if
(!plugin_messaging_interface_) {
On 2011/03/22 05:59:11, brettw wrote:
> This will query the plugin every single time if it doesn't support the
> interface. Do you think we should remember whether we've previously asked so
we
> can avoid re-querying if it returns NULL?
I'm fine either way...  it only happens when the JS calls postMessage on a
plugin which doesn't support it;  I'm okay with exceptional cases being slow. 
And this is almost a straight copy of other functions.

But I took a stab at it anyway.  Are function-local static variables okay? 
Since all of this code already assumes it's only run on the main thread, it
should be fine;  I just feel dirty any time I use them.

Is this approach okay for you, and if so, do you want me to throw it in for
Find, Zoom, et al?

[brettw, 2011-03-22 21:39:06]

http://codereview.chromium.org/6716005/diff/118/webkit/plugins/ppapi/ppapi_pl...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/118/webkit/plugins/ppapi/ppapi_pl...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:877: static bool
checked_interface(false);
This won't work since there can be many PluginInstances for different plugins
that may support different interfaces. Just put this on the class.

I'm less concerned about find and zoom since these are instantiated by unusual
user actions. Potentially postMessage might be called a lot by script.

[dmichael(do not use this one), 2011-03-22 21:59:01]

http://codereview.chromium.org/6716005/diff/118/webkit/plugins/ppapi/ppapi_pl...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/118/webkit/plugins/ppapi/ppapi_pl...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:877: static bool
checked_interface(false);
On 2011/03/22 21:39:06, brettw wrote:
> This won't work since there can be many PluginInstances for different plugins
> that may support different interfaces. Just put this on the class.
Of course, thanks for catching that.
> 
> I'm less concerned about find and zoom since these are instantiated by unusual
> user actions. Potentially postMessage might be called a lot by script.
I tend to think people won't bother writing JS that calls postMessage on plugins
that don't support it, since presumably they know what plugin they are
developing for. But it's not a big deal to me; it's easy to optimize for.

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

Thanks for moving this to a separate interface.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev.h
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
Is this necessary at this point?
Other headers don't have this, do they? And I was under the impression that we
are still working out the details of how versioning will work.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:27: * sending messages for a specific module
instance.
"for" is ambiguous as it can mean "to" or "from". Also it is not clear which
direction this one is for.

How about  "functions for passing messages to the module instance on a web
page"?

Please also use the same description in the file comment.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:31: * @a PostMessage is a pointer to a function
which asynchronously invokes the
I would suggest being more specific than just saying "asynchronously" and note
that the onmessage handler won't be invoked until the user call stack is popped.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:35: * value (objects are not currently
supported.)
you have "currently" twice in the same sentence

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev.h
File ppapi/c/dev/ppp_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:21: * the DOM element associated with the module
instance.
You have "module instance" 3 times in the same sentence.  Please use "it" and
(optionally) drop the 1st reference the way you have it in the next comment.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:38: * @a postMessage() is invoked on the DOM
object for the module instance in
DOM element

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:40: * asynchronous, meaning JavaScript execution
will not be blocked while
I think it's more important to note that HandleMessage won't be called until
JavaScript call stack is popped. It's not clear from your description.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:57: * This will result in HandleMessage being
invoked, passing the module
@a  HandleMessage?

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:58: * instance on which it was invoked, with
message being a string PP_Var
@a message?

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:61: void (*HandleMessage)(PP_Instance instance,
struct PP_Var message);
I still think that it is unfortunate that we have onmessage and HandleMessage.
In our email/verbal discussions I noticed how inconvenient it was that I could
not say postmessage/onmessage to refer to both directions.

Since this is now in a separate interface and does not have to match other
functions, would it make sense to reconsider this name?

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

http://codereview.chromium.org/6716005/diff/118/ppapi/cpp/instance.h#newcode75
ppapi/cpp/instance.h:75: /** See PPP_Messaging.HandleMessage. */
These deserve their own section if not a separate API. Right now you are mixing
them with the Instance methods under a single sub-header comment.

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
File ppapi/examples/scripting/post_message.html (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:1: <body>
no html, title, etc?

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:10: // PPB_Instance::PostMessage in
C).  In this case, we're expecting a bool to
PPB_Messaging

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:25: // to Instance::HandleMessage in
C++ (or PPP_Instance::HandleMessage in C).
PPP_Messaging

[brettw, 2011-03-23 01:59:17]

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev.h
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:31: * @a PostMessage is a pointer to a function
which asynchronously invokes the
Personally I think asynchronously is more clear here than "popping the stack".
Popping the stack is also incorrect for a totally asynchronous proxy. The spec
also says "asynchronously" and I don't think it's at all ambiguous in this
context.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev.h
File ppapi/c/dev/ppp_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:40: * asynchronous, meaning JavaScript execution
will not be blocked while
I think the current description is fine for the same reasons I listed above.

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

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev.h
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
On 2011/03/22 22:00:59, polina wrote:
> Is this necessary at this point?
> Other headers don't have this, do they? And I was under the impression that we
> are still working out the details of how versioning will work.
Ah yes, I meant to bring this up.  I found it convenient to have this when I was
trying to do backwards compatibility.

I was thinking of proposing we start doing this.  It may only matter for
PPP_Instance.  However, if the NaCl proxy (which is in a separate repo) needs to
refer to a _particular_ version of an interface, it can only do it with a raw
string literal right now.  I definitely wanted something like this when I was
trying to stick backwards compatibility in for PPP_Instance.

I think it might be useful when we start putting in backwards compatibility
(which I'll try doing as I rev these interfaces), but I can cut it if it's
objectionable.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:27: * sending messages for a specific module
instance.
On 2011/03/22 22:00:59, polina wrote:
> "for" is ambiguous as it can mean "to" or "from". Also it is not clear which
> direction this one is for.
> 
> How about  "functions for passing messages to the module instance on a web
> page"?
> 
> Please also use the same description in the file comment.
Reworded.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:31: * @a PostMessage is a pointer to a function
which asynchronously invokes the
On 2011/03/23 01:59:17, brettw wrote:
> Personally I think asynchronously is more clear here than "popping the stack".
> Popping the stack is also incorrect for a totally asynchronous proxy. The spec
> also says "asynchronously" and I don't think it's at all ambiguous in this
> context.
I tried to add a little wording to make it clearer what I mean, but I agree with
Brett that it's not worth talking about the stack (etc).  I think it would be
more confusing than helpful.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:35: * value (objects are not currently
supported.)
On 2011/03/22 22:00:59, polina wrote:
> you have "currently" twice in the same sentence
Fixed.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev.h
File ppapi/c/dev/ppp_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:21: * the DOM element associated with the module
instance.
On 2011/03/22 22:00:59, polina wrote:
> You have "module instance" 3 times in the same sentence.  Please use "it" and
> (optionally) drop the 1st reference the way you have it in the next comment.
Done.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:40: * asynchronous, meaning JavaScript execution
will not be blocked while
On 2011/03/23 01:59:17, brettw wrote:
> I think the current description is fine for the same reasons I listed above.
I agree with Brett here.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:57: * This will result in HandleMessage being
invoked, passing the module
On 2011/03/22 22:00:59, polina wrote:
> @a  HandleMessage?
Done.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:58: * instance on which it was invoked, with
message being a string PP_Var
On 2011/03/22 22:00:59, polina wrote:
> @a message?
Done.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:61: void (*HandleMessage)(PP_Instance instance,
struct PP_Var message);
On 2011/03/22 22:00:59, polina wrote:
> I still think that it is unfortunate that we have onmessage and HandleMessage.
> In our email/verbal discussions I noticed how inconvenient it was that I could
> not say postmessage/onmessage to refer to both directions.
> 
> Since this is now in a separate interface and does not have to match other
> functions, would it make sense to reconsider this name?
The way I have things organized currently, though, I'm still putting
HandleMessage on the C++ interface alongside HandleInputEvent etc, so if I can
stick with that design, we should probably leave it alone.

If having PostMessage/HandleMessage in pp::Instance is not OK, then it might be
worth changing.

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

http://codereview.chromium.org/6716005/diff/118/ppapi/cpp/instance.h#newcode75
ppapi/cpp/instance.h:75: /** See PPP_Messaging.HandleMessage. */
On 2011/03/22 22:00:59, polina wrote:
> These deserve their own section if not a separate API. Right now you are
mixing
> them with the Instance methods under a single sub-header comment.
I moved them together with a sub-header comment.  I like having them here for
the convenience it will provide to C++.  I think a separate class will be
confusing when I later add support for MessagePorts.  In C, it's worth
separating them so we avoid changing the Instance interfaces, but we don't have
to be so concerned with API/ABI stability and backwards compatibility here.

If there's opposition, I'll cave and make a pp::Messaging class, but I think the
way I have it will be better for developers.

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
File ppapi/examples/scripting/post_message.html (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:1: <body>
On 2011/03/22 22:00:59, polina wrote:
> no html, title, etc?
Sorry, was just copying the ppapi/example/example.html
Done.

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:10: // PPB_Instance::PostMessage in
C).  In this case, we're expecting a bool to
On 2011/03/22 22:00:59, polina wrote:
> PPB_Messaging
Done.

http://codereview.chromium.org/6716005/diff/118/ppapi/examples/scripting/post...
ppapi/examples/scripting/post_message.html:25: // to Instance::HandleMessage in
C++ (or PPP_Instance::HandleMessage in C).
On 2011/03/22 22:00:59, polina wrote:
> PPP_Messaging
Done.

[neb, 2011-03-23 17:45:30]

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
I agree with Polina - I'm actually against doing this in this versioning in this
CL, on only one file. We have plenty of cases in the sources where we tried to
start a convention and then backed off. I'd prefer to see this done in one CL
for all the interfaces. IDL generator is very close to being done, and making
the change there would be even simpler.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:27: * The PPB_Messaging_Dev interface contains
pointers to functions related to
Since all our interfaces are pointers to functions, I'd omit this from the
description. I think just 'Functions related to...' is better.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:33: * @a PostMessage is a pointer to a function
which asynchronously invokes the
Same as above, I think it's obvious that this is a pointer to a function, I'd
just say 'Asynchronously invoke...'.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:38: * @a message is a PP_Var containing the data
to be sent
@param?

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:52: * \verbatim
We're using @ elsewhere, so please use @verbratim

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:64: * \endverbatim
... and @endverbatim here.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:67: * <code>
You used @verbatim above, now <code>?

http://codereview.chromium.org/6716005/diff/20015/ppapi/examples/scripting/po...
File ppapi/examples/scripting/post_message.html (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/examples/scripting/po...
ppapi/examples/scripting/post_message.html:4: Copyright (c) 2011 The Native
Client Authors. All rights reserved.
Chromium Authors?

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

http://codereview.chromium.org/6716005/diff/20015/ppapi/tests/test_post_messa...
ppapi/tests/test_post_message.cc:145: return std::string();
You can use PASS()

http://codereview.chromium.org/6716005/diff/20015/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/20015/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:106:
COMPILE_ASSERT(static_cast<int>(WebCursorInfo::webkit_name) \
What was wrong with this?

[brettw, 2011-03-23 17:51:39]

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
Let's change this in a separate pass for every file.

[dmichael(do not use this one), 2011-03-23 18:48:04]

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
On 2011/03/23 17:51:39, brettw wrote:
> Let's change this in a separate pass for every file.
Okay, I made it match other interfaces.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:27: * The PPB_Messaging_Dev interface contains
pointers to functions related to
On 2011/03/23 17:45:30, neb wrote:
> Since all our interfaces are pointers to functions, I'd omit this from the
> description. I think just 'Functions related to...' is better.
I agree...  but this is what docs folks have decided was appropriate for
interfaces, so I was matching it.  We can revisit that, but it's a broader
change.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:33: * @a PostMessage is a pointer to a function
which asynchronously invokes the
On 2011/03/23 17:45:30, neb wrote:
> Same as above, I think it's obvious that this is a pointer to a function, I'd
> just say 'Asynchronously invoke...'.
Agreed, but I'm following what I think is established convention. Happy to
revisit it...  in another CL?

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:38: * @a message is a PP_Var containing the data
to be sent
On 2011/03/23 17:45:30, neb wrote:
> @param?
Done.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:52: * \verbatim
On 2011/03/23 17:45:30, neb wrote:
> We're using @ elsewhere, so please use @verbratim
Done.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:64: * \endverbatim
On 2011/03/23 17:45:30, neb wrote:
> ... and @endverbatim here.
Done.

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:67: * <code>
On 2011/03/23 17:45:30, neb wrote:
> You used @verbatim above, now <code>?
I'm pretty new to doxygen stuff, so I'm making my best guesses.  According to
Josie, <code> is the right way to do code samples.  But I was afraid my example
HTML wouldn't render properly using <code>.  I'm not sure if it will even work
with verbatim.  I'm happy to do whatever works.

http://codereview.chromium.org/6716005/diff/20015/ppapi/examples/scripting/po...
File ppapi/examples/scripting/post_message.html (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/examples/scripting/po...
ppapi/examples/scripting/post_message.html:4: Copyright (c) 2011 The Native
Client Authors. All rights reserved.
On 2011/03/23 17:45:30, neb wrote:
> Chromium Authors?
Done.

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

http://codereview.chromium.org/6716005/diff/20015/ppapi/tests/test_post_messa...
ppapi/tests/test_post_message.cc:145: return std::string();
On 2011/03/23 17:45:30, neb wrote:
> You can use PASS()
That's better, thanks.

http://codereview.chromium.org/6716005/diff/20015/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6716005/diff/20015/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:106:
COMPILE_ASSERT(static_cast<int>(WebCursorInfo::webkit_name) \
On 2011/03/23 17:45:30, neb wrote:
> What was wrong with this?
It was using C-style casts.  I think gcl lint found it.

[neb, 2011-03-23 20:14:30]

LGTM. I'll take my documentation beef up with the doc team, it's in shambles as
it is.

[polina, 2011-03-24 05:42:33]

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev.h
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
On 2011/03/23 17:03:18, dmichael wrote:
> On 2011/03/22 22:00:59, polina wrote:
> > Is this necessary at this point?
> > Other headers don't have this, do they? And I was under the impression that
we
> > are still working out the details of how versioning will work.
> Ah yes, I meant to bring this up.  I found it convenient to have this when I
was
> trying to do backwards compatibility.
> 
> I was thinking of proposing we start doing this.  It may only matter for
> PPP_Instance.  However, if the NaCl proxy (which is in a separate repo) needs
to
> refer to a _particular_ version of an interface, it can only do it with a raw
> string literal right now.  I definitely wanted something like this when I was
> trying to stick backwards compatibility in for PPP_Instance.
> 
> I think it might be useful when we start putting in backwards compatibility
> (which I'll try doing as I rev these interfaces), but I can cut it if it's
> objectionable.

Before we start adding any such hooks to the code, we should first reach a
global consensus on what interface changes we will allow (adding methods?
removing methods? changing signatures? all of the above?) and how multiple
versions will be delivered by the browser and the proxy.

NaCl can't ship without this, so I am happy to draft a proposal for this if
nobody is working on one already.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:31: * @a PostMessage is a pointer to a function
which asynchronously invokes the
On 2011/03/23 17:03:18, dmichael wrote:
> On 2011/03/23 01:59:17, brettw wrote:
> > Personally I think asynchronously is more clear here than "popping the
stack".
> > Popping the stack is also incorrect for a totally asynchronous proxy. The
spec
> > also says "asynchronously" and I don't think it's at all ambiguous in this
> > context.
> I tried to add a little wording to make it clearer what I mean, but I agree
with
> Brett that it's not worth talking about the stack (etc).  I think it would be
> more confusing than helpful.

We don't have to talk about  the call stack and reentrancy (although it might be
nice to tell users not to worry about it in some overview doc) in header
comments, but we do need to make it clear that posting a message and handling
that message are guaranteed to be two disjoint events. If I learned anything
from all of the recent internal discussions, it's that using a single magic
keyword (like "asynchronous") is likely to result in misinterpretation.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev.h
File ppapi/c/dev/ppp_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppp_messaging_dev...
ppapi/c/dev/ppp_messaging_dev.h:40: * asynchronous, meaning JavaScript execution
will not be blocked while
On 2011/03/23 17:03:18, dmichael wrote:
> On 2011/03/23 01:59:17, brettw wrote:
> > I think the current description is fine for the same reasons I listed above.
> I agree with Brett here.

Won't JavaScript thread be blocked once the proxy takes over HandleMessage()
with a synchronous proxy?

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/20015/ppapi/c/dev/ppb_messaging_d...
ppapi/c/dev/ppb_messaging_dev.h:27: * The PPB_Messaging_Dev interface contains
pointers to functions related to
On 2011/03/23 18:48:04, dmichael wrote:
> On 2011/03/23 17:45:30, neb wrote:
> > Since all our interfaces are pointers to functions, I'd omit this from the
> > description. I think just 'Functions related to...' is better.
> I agree...  but this is what docs folks have decided was appropriate for
> interfaces, so I was matching it.  We can revisit that, but it's a broader
> change.

I had the same concern in a code review I was doing for one of the doc writers.
He confirmed that this was a deliberate conscious decision and that all headers
are being converted to this format.

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

http://codereview.chromium.org/6716005/diff/21015/ppapi/cpp/instance.h#newcod...
ppapi/cpp/instance.h:105: virtual void HandleMessage(const Var& message_data);
Seeing "message_data" next to "message" in this section is confusing. If I post
a message to JS handler that just echos it back, then HandleMessage will get the
message in the exact same format. And this does not match the C API. Please
rename.

[dmichael(do not use this one), 2011-03-25 20:21:05]

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev.h
File ppapi/c/dev/ppb_messaging_dev.h (right):

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:13: #define PPB_MESSAGING_DEV_INTERFACE
PPB_MESSAGING_DEV_INTERFACE_0_1
On 2011/03/24 05:42:33, polina wrote:
> On 2011/03/23 17:03:18, dmichael wrote:
> > On 2011/03/22 22:00:59, polina wrote:
> > > Is this necessary at this point?
> > > Other headers don't have this, do they? And I was under the impression
that
> we
> > > are still working out the details of how versioning will work.
> > Ah yes, I meant to bring this up.  I found it convenient to have this when I
> was
> > trying to do backwards compatibility.
> > 
> > I was thinking of proposing we start doing this.  It may only matter for
> > PPP_Instance.  However, if the NaCl proxy (which is in a separate repo)
needs
> to
> > refer to a _particular_ version of an interface, it can only do it with a
raw
> > string literal right now.  I definitely wanted something like this when I
was
> > trying to stick backwards compatibility in for PPP_Instance.
> > 
> > I think it might be useful when we start putting in backwards compatibility
> > (which I'll try doing as I rev these interfaces), but I can cut it if it's
> > objectionable.
> 
> Before we start adding any such hooks to the code, we should first reach a
> global consensus on what interface changes we will allow (adding methods?
> removing methods? changing signatures? all of the above?) and how multiple
> versions will be delivered by the browser and the proxy.
> 
> NaCl can't ship without this, so I am happy to draft a proposal for this if
> nobody is working on one already.
Sure, that would be helpful.

http://codereview.chromium.org/6716005/diff/118/ppapi/c/dev/ppb_messaging_dev...
ppapi/c/dev/ppb_messaging_dev.h:31: * @a PostMessage is a pointer to a function
which asynchronously invokes the
On 2011/03/24 05:42:33, polina wrote:
> On 2011/03/23 17:03:18, dmichael wrote:
> > On 2011/03/23 01:59:17, brettw wrote:
> > > Personally I think asynchronously is more clear here than "popping the
> stack".
> > > Popping the stack is also incorrect for a totally asynchronous proxy. The
> spec
> > > also says "asynchronously" and I don't think it's at all ambiguous in this
> > > context.
> > I tried to add a little wording to make it clearer what I mean, but I agree
> with
> > Brett that it's not worth talking about the stack (etc).  I think it would
be
> > more confusing than helpful.
> 
> We don't have to talk about  the call stack and reentrancy (although it might
be
> nice to tell users not to worry about it in some overview doc) in header
> comments, but we do need to make it clear that posting a message and handling
> that message are guaranteed to be two disjoint events.
It's not just handling a response message that won't happen;  the thread won't
handle any other events (e.g. assuming it's called from the main thread,
completion callbacks, input events, etc won't happen).
> If I learned anything
> from all of the recent internal discussions, it's that using a single magic
> keyword (like "asynchronous") is likely to result in misinterpretation.
I actually think that in this case saying something like "the calling thread
will not handle other events while in PostMessage" would be more confusing than
helpful.  I don't think developers would expect it to do anything like that, so
reading a comment about what *won't* happen is probably just going to make it
harder to understand the description of what *will* happen.

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

http://codereview.chromium.org/6716005/diff/21015/ppapi/cpp/instance.h#newcod...
ppapi/cpp/instance.h:105: virtual void HandleMessage(const Var& message_data);
On 2011/03/24 05:42:33, polina wrote:
> Seeing "message_data" next to "message" in this section is confusing. If I
post
> a message to JS handler that just echos it back, then HandleMessage will get
the
> message in the exact same format. And this does not match the C API. Please
> rename.
Good point.  Renamed in:  http://codereview.chromium.org/6748006

[piman, 2011-03-29 04:40:22]

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

http://codereview.chromium.org/6716005/diff/21015/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:101: return
instance_->message_channel().np_object();
Mmmh.... This CL is causing
http://code.google.com/p/chromium-os/issues/detail?id=13605 to appear.

WebPluginImpl::scriptableObject is supposed to add a refcount to the npobject
(see the NPAPI implementation).

The previous code was wrong, but worked because of 2 bugs and luck:
PluginObject::Create leaks a reference, and this function failed to add one, and
is called only once. It all cancels out.
Unfortunately the new code passes an object on which a reference is never added,
so when the JS engine is done with it and garbage collects it, it destroys it
keeping garbage pointers all around.
Similarly, the instance's object's npobject doesn't get released any more.

[dmichael(do not use this one), 2011-03-29 16:07:03]

On Mon, Mar 28, 2011 at 10:40 PM, <piman@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6716005/diff/21015/webkit/plugins/ppapi/ppapi_...
> File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):
>
>
>
http://codereview.chromium.org/6716005/diff/21015/webkit/plugins/ppapi/ppapi_...
> webkit/plugins/ppapi/ppapi_webplugin_impl.cc:101: return
> instance_->message_channel().np_object();
> Mmmh.... This CL is causing
> http://code.google.com/p/chromium-os/issues/detail?id=13605 to appear.
>
> WebPluginImpl::scriptableObject is supposed to add a refcount to the
> npobject (see the NPAPI implementation).
>
> The previous code was wrong, but worked because of 2 bugs and luck:
> PluginObject::Create leaks a reference, and this function failed to add
> one, and is called only once. It all cancels out.
> Unfortunately the new code passes an object on which a reference is
> never added,

See MessageChannel's constructor, where WebBindings::createObject is called.
 That initializes the reference count to 1.  So we need to also increment it
when scriptableObject is called, to get the ref count up to 2?  My intent
was for MessageChannel's np_object_ to be retained as long as the plugin
instance is around.

so when the JS engine is done with it and garbage collects
> it, it destroys it keeping garbage pointers all around.

I didn't realize the JS engine could decide it was done with it while the
plugin instance was still around.  Do you have any suggestions on how to
reproduce this in an automated test?

Similarly, the instance's object's npobject doesn't get released any
> more.

Yeah, I fixed that in http://codereview.chromium.org/6733026/


>
> http://codereview.chromium.org/6716005/
>

While I do think it's my mistake, is it possible that the problem is
actually in how I'm dealing with the passthrough_object_?  That's the object
to which MessageChannel forwards all non-'postMessage' calls.  It occurs to
me looking at the code that at the end of WebPluginImpl::scriptableObject,
the scoped_refptr 'object' goes out of scope, so I think MessageChannel's
passthrough_object_ is invalid.

In any case, sorry for the rookie mistakes :-(

I'll work on an automated test to reproduce, to make sure I'm fixing it.  Or
if you have really high confidence that it's just the need to retain
MessageChannel's np_object(), I have a CL ready:
http://codereview.chromium.org/6731051/

[piman, 2011-03-29 16:28:19]

On Tue, Mar 29, 2011 at 9:06 AM, David Michael <dmichael@google.com> wrote:

>
>
> On Mon, Mar 28, 2011 at 10:40 PM, <piman@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6716005/diff/21015/webkit/plugins/ppapi/ppapi_...
>> File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):
>>
>>
>>
http://codereview.chromium.org/6716005/diff/21015/webkit/plugins/ppapi/ppapi_...
>> webkit/plugins/ppapi/ppapi_webplugin_impl.cc:101: return
>> instance_->message_channel().np_object();
>> Mmmh.... This CL is causing
>> http://code.google.com/p/chromium-os/issues/detail?id=13605 to appear.
>>
>> WebPluginImpl::scriptableObject is supposed to add a refcount to the
>> npobject (see the NPAPI implementation).
>>
>> The previous code was wrong, but worked because of 2 bugs and luck:
>> PluginObject::Create leaks a reference, and this function failed to add
>> one, and is called only once. It all cancels out.
>> Unfortunately the new code passes an object on which a reference is
>> never added,
>
> See MessageChannel's constructor, where WebBindings::createObject is
> called.  That initializes the reference count to 1.  So we need to also
> increment it when scriptableObject is called, to get the ref count up to 2?
>
>

Yes. WebKit will release() the np object you return.


> My intent was for MessageChannel's np_object_ to be retained as long as the
> plugin instance is around.
>

Understood, and that's perfectly fine.

>
> so when the JS engine is done with it and garbage collects
>> it, it destroys it keeping garbage pointers all around.
>
> I didn't realize the JS engine could decide it was done with it while the
> plugin instance was still around.  Do you have any suggestions on how to
> reproduce this in an automated test?
>

Well it won't if the refcount is correct. Right now it is incorrect (missing
one ref) so when the JS engine releases its reference, the object goes away.

>
> Similarly, the instance's object's npobject doesn't get released any
>> more.
>
> Yeah, I fixed that in http://codereview.chromium.org/6733026/
>
> I meant what you call the "passthrough object"

>
>>
>> http://codereview.chromium.org/6716005/
>>
>
> While I do think it's my mistake, is it possible that the problem is
> actually in how I'm dealing with the passthrough_object_?  That's the object
> to which MessageChannel forwards all non-'postMessage' calls.  It occurs to
> me looking at the code that at the end of WebPluginImpl::scriptableObject,
> the scoped_refptr 'object' goes out of scope, so I think MessageChannel's
> passthrough_object_ is invalid.
>

It is not invalid because of the separate bug that leaks a reference in
PluginObject::Create. That needs to be fixed too, at which point you'll
indeed also have to retain/release the passthrough object.

>
> In any case, sorry for the rookie mistakes :-(
>

No worries, the code that was there before was also incorrect, it only
worked because of the combination of 2 bugs and luck.

>
> I'll work on an automated test to reproduce, to make sure I'm fixing it.
>

I don't know of any test. Writing one would be good. However it's slightly
tricky because it depends on the JS garbage collector to make sure WebKit
drops its own reference.


>   Or if you have really high confidence that it's just the need to retain
> MessageChannel's np_object(), I have a CL ready:
> http://codereview.chromium.org/6731051/
>

I added some comments there.

Antoine