[NaCl SDK] nacl_io: Add flow control the JavaScript pipes.

[NaCl SDK] nacl_io: Add flow control the JavaScript pipes.

Add unit tests.

Split unit tests for jspipe and tty nodes into those that
test the internal directly and those that require that higher
level ki_ functions withing the test code.

R=binji@chromium.org

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

[Sam Clegg, 2014-04-29 22:35:11]

Sending this out for initial feedback.

I advice looking at the example and tests first.

Unfortunately these is a bunch of duplication now between pipe nodes and jspipe
node.  I think there is some re-factoring to be done here but I'd like to land
this (along with more tests) first.

[Sam Clegg, 2014-05-01 18:48:38]

ptal

[binji, 2014-05-01 20:22:30]

lgtm.

I think the message format could be better, but in general this looks fine.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io/example.js (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:47: var onChange =
function(e) {
this function is small enough to inline, IMO

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:309: return
String.fromCharCode.apply(null, new Uint16Array(buf));
probably not an issue here, but this fails for large array buffers.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:318: var channelName =
Object.keys(data)[0]
This seems clunky to me. It would be nicer to have a fixed key that you could
check to get the channel name:
{"channel": "jspipe1", "data": <ArrayBuffer>}

In any case, you can reverse this check and look for "cmd". It is always defined
in a response message.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:319: if (channelName ==
'jspipe1' ||
nit: === here and everywhere

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:301: fprintf(stderr,
"only wrote %d/%d bytes\n", wrote, rtn);
might be nice to send these errors to JavaScript, rather than the console.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:411: if (message.type
== PP_VARTYPE_DICTIONARY) {
this won't work anymore because all messages are dictionaries.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:417:
sprintf(pipe_name, "/dev/%s", key_string);
snprintf

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:422:
g_ppb_messaging->PostMessage(g_instance, var);
Use PostMessageVar to cleanup the newly created string variable.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:423: return;
release PP_Vars on failure

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:36:
messaging_iface_ = ppapi->GetMessagingInterface();
initialize to NULL if ppapi is NULL

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:61:
SendAckMessage(bytes_read_);
Why ack total count instead of the amount read?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:61:
SendAckMessage(bytes_read_);
check error

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:78:
SendMessageToJS("write", buffer);
check error

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:105:
PP_Var message_type_var = var_iface_->VarFromUtf8(message_type,
are you expecting arbitrary message types? If not, maybe just an int/enum. write
= 0, ack = 1, etc.

Even if you don't go that route, you can cache "write" and "ack" vars.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:107:
array_iface_->Set(array, 0, message_type_var);
check errors on these pepper functions

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:113:
PP_Var pipe_name = var_iface_->VarFromUtf8(name_.c_str(), name_.size());
cache this in SetName because it never changes?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:154:
return -1;
should this return ssize_t then?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:163:
SendWriteMessage(data, len);
check error

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:12:
#include "nacl_io/devfs/fifo_post_message.h"
file doesn't exist?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:41: size_t
GetOSpace() { return post_message_buffer_size_ - BytesOutstanding(); };
these are called {Read,Write}Available in the fifo

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:59: int64_t
bytes_sent_;
why not use size_t?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc:62: int wrote =
GetEventEmitter()->HandleJSWrite(buffer, length);
returns size_t

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc:65: LOG_ERROR("Only
wrote %d of %d bytes to pipe", wrote, length);
ERROR instead of LOG_ERROR?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc:77:
GetEventEmitter()->HandleJSAck(message.value.as_int);
This will never be able to ack above 2**31

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h:24: * via calls to
PostMessage.  In order to use these some ammount of logic on
amount

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h:36: * Messages
comming from JavaScript must be delivered using the
coming

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/fifo_char.h (left):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:24: virtual bool IsEmpty();
I prefer when virtual overrides are labelled as such. (ReadAvailable and
WriteAvailable should be too)

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/ioctl.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/ioctl.h:26: #define TIOCNACLOUTPUT
0xadcd03
rename these too (or add a TODO?)

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/library.dsc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/library.dsc:17:
"devfs/jspipe_event_emitter.cc",
alphabetize

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/pipe/pipe_node.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/pipe/pipe_node.cc:38: if (err ==
ETIMEDOUT)
Do we want to do this everywhere?

If not, it still might be nice to have a flag to handle this in WaitOnEvent.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:457: int ret =
ioctl(fd, NACL_IOC_HANDLEMESSAGE, &message.pp_var());
did you add this functionality to the ttynode?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:162: void
MessageHandlerInput(const pp::Var& target, const pp::Var& message);
"target" is called "key" in the definition below

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc:107: //
These types don't have any var data as thier data
their

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:68: PP_Var ack_count;
PP_MakeInt32(...) is a bit nicer

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:257: // Verify that
exaclty one message sent.
exactly

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:375:
TEST_F(JSPipeNodeTest, JSPipeOverrun) {
not implemented

[Sam Clegg, 2014-05-01 22:16:54]

Fixed all the nits, still need to potentially re-think the message format stuff.
 PTAL.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io/example.js (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:47: var onChange =
function(e) {
On 2014/05/01 20:22:31, binji wrote:
> this function is small enough to inline, IMO

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:318: var channelName =
Object.keys(data)[0]
On 2014/05/01 20:22:31, binji wrote:
> This seems clunky to me. It would be nicer to have a fixed key that you could
> check to get the channel name:
> {"channel": "jspipe1", "data": <ArrayBuffer>}
> 
> In any case, you can reverse this check and look for "cmd". It is always
defined
> in a response message.

I guess it will make the code slightly large and the objects slightly larger..
but I think I agree with you.  I'll give it a go.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:319: if (channelName ==
'jspipe1' ||
On 2014/05/01 20:22:31, binji wrote:
> nit: === here and everywhere

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:301: fprintf(stderr,
"only wrote %d/%d bytes\n", wrote, rtn);
On 2014/05/01 20:22:31, binji wrote:
> might be nice to send these errors to JavaScript, rather than the console.

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:411: if (message.type
== PP_VARTYPE_DICTIONARY) {
On 2014/05/01 20:22:31, binji wrote:
> this won't work anymore because all messages are dictionaries.

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:422:
g_ppb_messaging->PostMessage(g_instance, var);
On 2014/05/01 20:22:31, binji wrote:
> Use PostMessageVar to cleanup the newly created string variable.

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/nacl_io_demo.c:423: return;
On 2014/05/01 20:22:31, binji wrote:
> release PP_Vars on failure

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:36:
messaging_iface_ = ppapi->GetMessagingInterface();
On 2014/05/01 20:22:31, binji wrote:
> initialize to NULL if ppapi is NULL

Done, and added a test.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:61:
SendAckMessage(bytes_read_);
On 2014/05/01 20:22:31, binji wrote:
> Why ack total count instead of the amount read?

The ack messages always contain the running total.  Makes debugging much easier.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:61:
SendAckMessage(bytes_read_);
On 2014/05/01 20:22:31, binji wrote:
> check error

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:78:
SendMessageToJS("write", buffer);
On 2014/05/01 20:22:31, binji wrote:
> check error

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:154:
return -1;
On 2014/05/01 20:22:31, binji wrote:
> should this return ssize_t then?

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:163:
SendWriteMessage(data, len);
On 2014/05/01 20:22:31, binji wrote:
> check error

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/05/01 20:22:31, binji wrote:
> 2014

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:41: size_t
GetOSpace() { return post_message_buffer_size_ - BytesOutstanding(); };
On 2014/05/01 20:22:31, binji wrote:
> these are called {Read,Write}Available in the fifo

These are slightly different. They correspond to the GETOSPACE and GETISPACE
ioctl calls.

GETOSPACE does correspond to WriteAvailable() (i.e. how much can you write()
before you block). 

GITISPACE is different as it tells you how much data can can be injected with
ioctl() before it will fail (i.e. the "input" space available to the JavaScript
side).

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.h:59: int64_t
bytes_sent_;
On 2014/05/01 20:22:31, binji wrote:
> why not use size_t?

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc:65: LOG_ERROR("Only
wrote %d of %d bytes to pipe", wrote, length);
On 2014/05/01 20:22:31, binji wrote:
> ERROR instead of LOG_ERROR?

File local macro that also print the name of the pipe in the error message.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.cc:77:
GetEventEmitter()->HandleJSAck(message.value.as_int);
On 2014/05/01 20:22:31, binji wrote:
> This will never be able to ack above 2**31

True, this is a current know issue that also effect nassh.

We can probably work around it by using two integers on the JS side if it causes
a problem.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h:24: * via calls to
PostMessage.  In order to use these some ammount of logic on
On 2014/05/01 20:22:31, binji wrote:
> amount

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_node.h:36: * Messages
comming from JavaScript must be delivered using the
On 2014/05/01 20:22:31, binji wrote:
> coming

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/fifo_char.h (left):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:24: virtual bool IsEmpty();
On 2014/05/01 20:22:31, binji wrote:
> I prefer when virtual overrides are labelled as such. (ReadAvailable and
> WriteAvailable should be too)

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/ioctl.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/ioctl.h:26: #define TIOCNACLOUTPUT
0xadcd03
On 2014/05/01 20:22:31, binji wrote:
> rename these too (or add a TODO?)

Yes, in a future CL I want to make the tty node inherit from the jspipe, at
which point these ictls might go away completely.  Note sure this is the best
please to document that though.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/library.dsc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/library.dsc:17:
"devfs/jspipe_event_emitter.cc",
On 2014/05/01 20:22:31, binji wrote:
> alphabetize

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/pipe/pipe_node.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/pipe/pipe_node.cc:38: if (err ==
ETIMEDOUT)
On 2014/05/01 20:22:31, binji wrote:
> Do we want to do this everywhere?
> 
> If not, it still might be nice to have a flag to handle this in WaitOnEvent.

There may be test code that expects otherwise but in practice we
want to do this everywhere I think yes.

Might leave that for a separate CL.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:457: int ret =
ioctl(fd, NACL_IOC_HANDLEMESSAGE, &message.pp_var());
On 2014/05/01 20:22:31, binji wrote:
> did you add this functionality to the ttynode?

No.  TTY still uses the legacy code above for now.  Should I extent this CL to
include TTY?

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:162: void
MessageHandlerInput(const pp::Var& target, const pp::Var& message);
On 2014/05/01 20:22:31, binji wrote:
> "target" is called "key" in the definition below

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc
(right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc:107: //
These types don't have any var data as thier data
On 2014/05/01 20:22:31, binji wrote:
> their

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
File native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc (right):

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:68: PP_Var ack_count;
On 2014/05/01 20:22:31, binji wrote:
> PP_MakeInt32(...) is a bit nicer

Thanks! Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:257: // Verify that
exaclty one message sent.
On 2014/05/01 20:22:31, binji wrote:
> exactly

Done.

https://codereview.chromium.org/242533005/diff/100001/native_client_sdk/src/t...
native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc:375:
TEST_F(JSPipeNodeTest, JSPipeOverrun) {
On 2014/05/01 20:22:31, binji wrote:
> not implemented

Done.

[Sam Clegg, 2014-05-07 23:30:36]

Updated to new message format.

Still want to add an example of piping all the way from one native FD to anther.

[binji, 2014-05-08 18:18:33]

lgtm

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/e...
File native_client_sdk/src/examples/demo/nacl_io/example.js (right):

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/e...
native_client_sdk/src/examples/demo/nacl_io/example.js:319: var pipeName =
data['pipe']
you can just do data.pipe in JavaScript.
neat, huh?

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc
(right):

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:155:
dict_iface_->Set(dict, pipe_key_, pipe_name_var_);
nice

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:189:
TRACE("HandleAck: %" SCNuS "/%" PRIuS, bytes_acked_, bytes_sent_);
PRIuS, SCNuS is for scanf

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:209:
LOG_ERROR("Only wrote %d of %d bytes to pipe", (int)wrote, (int)length);
why cast?

https://codereview.chromium.org/242533005/diff/160001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/devfs/jspipe_event_emitter.cc:230:
std::string string_a(cstring_a, length_a);
seems a bit cheesy to create a std::string just to compare. Why not memcmp
(since there could be embedded nulls)?

[Sam Clegg, 2014-05-22 18:10:20]

Committed patchset #12 manually as r272235 (presubmit successful).