[NaCl SDK] nacl_io: Add initial implementations of kill and signal

[NaCl SDK] nacl_io: Add initial implementations of kill and signal.

These functions are needed to make window resize events
using SIGWINCH work for nacl terminal apps.

Also add the windows size ioctl's to the TTY device and
have it send SIGWINCH when the size changes.

R=binji@chromium.org

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

[Sam Clegg, 2013-08-21 01:01:05]

PTAL.

[binji, 2013-08-21 17:42:00]

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:726: switch (sig) {
is there danger in calling default handlers if the user has sigset them and then
kill'ed them? Why not just call whatever handlers you have available?

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:746: return
sigset(signum, handler);
if sigset and signal are identical, I'd rather see only one implementation in
kernel_proxy, and have the each wrapper to the one implementation.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:753: sighandler_t
old_value = sigwinch_handler_;
I know you're only handling SIGWINCH, but you might as well handle this with an
array of signal handlers rather than a switch. Are there any signal types that
require special behavior?

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:756:
//dbgprintf("registering handler: %p\n", handler);
remove

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:187: sighandler_t
sigwinch_handler_;
kernel_proxy is a bit of a god class, so can very easily become a dumping
ground... I'd like to see stuff like this moved into its own class, and
kernel_proxy just delegates to it.

Maybe a SignalHandler class?

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:36: rows_(10),
use consts. BTW, why 10?

[Sam Clegg, 2013-08-22 03:32:45]

This change is fairly big now.

I could split it into a separate plumbing only CL if you like?

PTAL

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:726: switch (sig) {
On 2013/08/21 17:42:01, binji wrote:
> is there danger in calling default handlers if the user has sigset them and
then
> kill'ed them? Why not just call whatever handlers you have available?

I'm not sure what you mean.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:746: return
sigset(signum, handler);
On 2013/08/21 17:42:01, binji wrote:
> if sigset and signal are identical, I'd rather see only one implementation in
> kernel_proxy, and have the each wrapper to the one implementation.

They are not quite identical.  But the differences seem pretty subtle.
sigset has an additional special value of SIG_HOLD, which we currently
don't support.

Done.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:753: sighandler_t
old_value = sigwinch_handler_;
On 2013/08/21 17:42:01, binji wrote:
> I know you're only handling SIGWINCH, but you might as well handle this with
an
> array of signal handlers rather than a switch. Are there any signal types that
> require special behavior?

KILL and STOP can never be handled, aside from that you are correctly.  I
was just lazy and didn't want to create the signal -> handler mapping datatype.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:756:
//dbgprintf("registering handler: %p\n", handler);
On 2013/08/21 17:42:01, binji wrote:
> remove

Done.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:36: rows_(10),
On 2013/08/21 17:42:01, binji wrote:
> use consts. BTW, why 10?

Done.

Changed default rows to 30.  Is there a more logical default
value?

[binji, 2013-08-22 18:09:35]

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:726: switch (sig) {
On 2013/08/22 03:32:45, Sam Clegg wrote:
> On 2013/08/21 17:42:01, binji wrote:
> > is there danger in calling default handlers if the user has sigset them and
> then
> > kill'ed them? Why not just call whatever handlers you have available?
> 
> I'm not sure what you mean.

I just mean, if we had a generic SignalHandler class, where the caller can
arbitrarily set handlers, is there any issue with just calling them here?

I suppose it is irrelevant unless you refactor the logic here to be more
generic--I'd prefer it, but I'm not going to hold up the CL for it.

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23005005/diff/20025/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:36: rows_(10),
On 2013/08/22 03:32:45, Sam Clegg wrote:
> On 2013/08/21 17:42:01, binji wrote:
> > use consts. BTW, why 10?
> 
> Done.
> 
> Changed default rows to 30.  Is there a more logical default
> value?

25 or 50, is what I remember, but I guess it probably doesn't matter.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/ioctl.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:17: * with EALREADY if a handler
is already registered.  Call with NULL
Why fail if already set? It could just overwrite it.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:18: * to unregister handler.
document the thread that gets the callback: the thread that writes to the tty
node.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
size_t for consistency with tioc_nacl_input_tstring

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
tioc prefix?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
document that the return value is the number of bytes written.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
Make this a struct containing the function pointer and user_data?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:876: listener->Track(-1,
ScopedKernelProxy(this), POLLERR, -1);
OK, I see why you want to use the kernel proxy for this, but I'd prefer if you
created a dummy EventEmitter class that the kernel proxy owns instead. That will
keep you from having to make the kernel proxy refcounted, an event emitter, etc.
which is pretty ugly, IMO.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:96: int rtn =
output_handler_(reinterpret_cast<const char*>(buf), count);
static_cast

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:96: int rtn =
output_handler_(reinterpret_cast<const char*>(buf), count);
Not sure I like holding the node lock while calling a callback -- seems pretty
easy to accidentally deadlock. OTOH, we can't cache callback pointer, unlock,
and call through it, because the user may have changed the callback pointer (and
would not expect a call from the previous pointer).

I think we may need to add another lock, specifically for output_handler_.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:98: return EIO;
hm, we could delegate the errno return to the output_handler as well. What do
you think?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:100: tty_fd_(-1) {
init tty_prefix_ and tty_resize_ to NULL?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:217: if
(tty_resize_)
tty_resize_ is only used in this function, does it need to be a member?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:358: void
PSInstance::MessageHandlerResizeStatic(const pp::Var& key,
nit: the ordering of functions here is strange, I'd expect either grouping the
handler types:

MessageHandlerInput
MessageHandlerInputStatic

TtyOutputHandler
TtyOutputHandlerStatic

MessageHandlerResize
MessageHandlerResizeStatic

or grouping the statics and non-statics:

MessageHandlerInput
TtyOutputHandler
MessageHandlerResize

MessageHandlerInputStatic
TtyOutputHandlerStatic
MessageHandlerResizeStatic

and keeping this consistent with the class definitions in the header.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:366:
assert(message.is_array());
document this message somewhere in the header?

value needs to be [col, row], where col and row are ints.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:394: // ioctl()
rather then adding them to the even queue.
sp: event

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:415:
std::map<std::string, MessageHandler>::iterator iter =
I find that this is a bit more readable if you make a typedef in the class
definition:

  ...
  typedef std::map<...> MessageHandlerMap;
  MessageHandlerMap message_handlers_;
  ...

Then this is just MessageHandlerMap::iterator

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:29: typedef void
(*MessageHandler_t)(const pp::Var& key,
ps_main.h defines PsMainFunc_t for a function typedef, maybe call this
MessageHandlerFunc_t for consistency?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:84: // Register a
message handler for messages that are arrive
Register a message handler for messages that arrive from JavaScript with a given
name.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:86: //  'name' {
<var> }
{message_name: value}

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:90: // removed.
Probably worthwhile to have an example:

JavaScript:
  nacl_module.postMessage({'foo': 123});

C++:
  instance_->RegisterMessageHandler("foo", &MyMessageHandler, NULL);
  ...

  void MyMessageHandler(const pp::Var& key, const pp::Var& value, void*
user_data) {
    assert(key.is_string());
    assert(key.AsString() == "foo");
    assert(value.is_int());
    assert(value.AsInt() == 123);
  }

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc:216: /*
remove?

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:96: g_output_buf
= buf;
won't need a global if you add user_data to the output function callback

[Sam Clegg, 2013-08-22 19:52:25]

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/ioctl.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:17: * with EALREADY if a handler
is already registered.  Call with NULL
On 2013/08/22 18:09:36, binji wrote:
> Why fail if already set? It could just overwrite it.

Just so the user knows when they do that.  Its probably
a coding error to do this.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:18: * to unregister handler.
On 2013/08/22 18:09:36, binji wrote:
> document the thread that gets the callback: the thread that writes to the tty
> node.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
On 2013/08/22 18:09:36, binji wrote:
> size_t for consistency with tioc_nacl_input_tstring

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
On 2013/08/22 18:09:36, binji wrote:
> tioc prefix?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
On 2013/08/22 18:09:36, binji wrote:
> document that the return value is the number of bytes written.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/ioctl.h:28: typedef int
(*nacl_io_tty_output_handler_t)(const char* buf, int count);
On 2013/08/22 18:09:36, binji wrote:
> Make this a struct containing the function pointer and user_data?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:876: listener->Track(-1,
ScopedKernelProxy(this), POLLERR, -1);
On 2013/08/22 18:09:36, binji wrote:
> OK, I see why you want to use the kernel proxy for this, but I'd prefer if you
> created a dummy EventEmitter class that the kernel proxy owns instead. That
will
> keep you from having to make the kernel proxy refcounted, an event emitter,
etc.
> which is pretty ugly, IMO.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:96: int rtn =
output_handler_(reinterpret_cast<const char*>(buf), count);
On 2013/08/22 18:09:36, binji wrote:
> static_cast

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:96: int rtn =
output_handler_(reinterpret_cast<const char*>(buf), count);
On 2013/08/22 18:09:36, binji wrote:
> Not sure I like holding the node lock while calling a callback -- seems pretty
> easy to accidentally deadlock. OTOH, we can't cache callback pointer, unlock,
> and call through it, because the user may have changed the callback pointer
(and
> would not expect a call from the previous pointer).
> 
> I think we may need to add another lock, specifically for output_handler_.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:98: return EIO;
On 2013/08/22 18:09:36, binji wrote:
> hm, we could delegate the errno return to the output_handler as well. What do
> you think?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:100: tty_fd_(-1) {
On 2013/08/22 18:09:36, binji wrote:
> init tty_prefix_ and tty_resize_ to NULL?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:217: if
(tty_resize_)
On 2013/08/22 18:09:36, binji wrote:
> tty_resize_ is only used in this function, does it need to be a member?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:358: void
PSInstance::MessageHandlerResizeStatic(const pp::Var& key,
On 2013/08/22 18:09:36, binji wrote:
> nit: the ordering of functions here is strange, I'd expect either grouping the
> handler types:
> 
> MessageHandlerInput
> MessageHandlerInputStatic
> 
> TtyOutputHandler
> TtyOutputHandlerStatic
> 
> MessageHandlerResize
> MessageHandlerResizeStatic
> 
> or grouping the statics and non-statics:
> 
> MessageHandlerInput
> TtyOutputHandler
> MessageHandlerResize
> 
> MessageHandlerInputStatic
> TtyOutputHandlerStatic
> MessageHandlerResizeStatic
> 
> and keeping this consistent with the class definitions in the header.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:366:
assert(message.is_array());
On 2013/08/22 18:09:36, binji wrote:
> document this message somewhere in the header?
> 
> value needs to be [col, row], where col and row are ints.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:394: // ioctl()
rather then adding them to the even queue.
On 2013/08/22 18:09:36, binji wrote:
> sp: event

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:415:
std::map<std::string, MessageHandler>::iterator iter =
On 2013/08/22 18:09:36, binji wrote:
> I find that this is a bit more readable if you make a typedef in the class
> definition:
> 
>   ...
>   typedef std::map<...> MessageHandlerMap;
>   MessageHandlerMap message_handlers_;
>   ...
> 
> Then this is just MessageHandlerMap::iterator

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:29: typedef void
(*MessageHandler_t)(const pp::Var& key,
On 2013/08/22 18:09:36, binji wrote:
> ps_main.h defines PsMainFunc_t for a function typedef, maybe call this
> MessageHandlerFunc_t for consistency?

I think the handler part implied its a function... but will
change it if you like.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:84: // Register a
message handler for messages that are arrive
On 2013/08/22 18:09:36, binji wrote:
> Register a message handler for messages that arrive from JavaScript with a
given
> name.

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:86: //  'name' {
<var> }
On 2013/08/22 18:09:36, binji wrote:
> {message_name: value}

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc:216: /*
On 2013/08/22 18:09:36, binji wrote:
> remove?

Done.

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:96: g_output_buf
= buf;
On 2013/08/22 18:09:36, binji wrote:
> won't need a global if you add user_data to the output function callback

Done.

[binji, 2013-08-22 20:10:54]

lgtm

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:90: // removed.
On 2013/08/22 18:09:36, binji wrote:
> Probably worthwhile to have an example:
> 
> JavaScript:
>   nacl_module.postMessage({'foo': 123});
> 
> C++:
>   instance_->RegisterMessageHandler("foo", &MyMessageHandler, NULL);
>   ...
> 
>   void MyMessageHandler(const pp::Var& key, const pp::Var& value, void*
> user_data) {
>     assert(key.is_string());
>     assert(key.AsString() == "foo");
>     assert(value.is_int());
>     assert(value.AsInt() == 123);
>   }

WDYT? This would make the connection a lot clearer for me, personally.

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:38: typedef
std::map<std::string, MessageHandler> MessageHandlerMap;
put this inside the PSInstance

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:89: //  message_name
{ <value> }
this still looks weird to me. You'd never write

foo { bar }

It isn't legal in javascript

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:90: // PSInstance wht
then not generate events but instead
s/wht/will/

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:145: /// Handle input
message from JavaScript.  The valu is
value

[Sam Clegg, 2013-08-22 20:19:02]

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/51001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:90: // removed.
On 2013/08/22 18:09:36, binji wrote:
> Probably worthwhile to have an example:
> 
> JavaScript:
>   nacl_module.postMessage({'foo': 123});
> 
> C++:
>   instance_->RegisterMessageHandler("foo", &MyMessageHandler, NULL);
>   ...
> 
>   void MyMessageHandler(const pp::Var& key, const pp::Var& value, void*
> user_data) {
>     assert(key.is_string());
>     assert(key.AsString() == "foo");
>     assert(value.is_int());
>     assert(value.AsInt() == 123);
>   }

Done.

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:38: typedef
std::map<std::string, MessageHandler> MessageHandlerMap;
On 2013/08/22 20:10:55, binji wrote:
> put this inside the PSInstance

Done.

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:89: //  message_name
{ <value> }
On 2013/08/22 20:10:55, binji wrote:
> this still looks weird to me. You'd never write
> 
> foo { bar }
> 
> It isn't legal in javascript

Oops.  Fixed.  (do you object to the <> too?)

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:90: // PSInstance wht
then not generate events but instead
On 2013/08/22 20:10:55, binji wrote:
> s/wht/will/

Done.

https://codereview.chromium.org/23005005/diff/73001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:145: /// Handle input
message from JavaScript.  The valu is
On 2013/08/22 20:10:55, binji wrote:
> value

Done.

[Sam Clegg, 2013-08-22 21:10:55]

Committed patchset #8 manually as r219103 (presubmit successful).