Add data pipe wrappers to be used to implement serial receive.

Add data pipe wrappers to be used to implement serial receive.

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

[Ken Rockot(use gerrit already), 2014-08-04 17:01:04]

This generally looks fine to me and I'll be happy to rubber stamp for
//device/serial pending a mojo review, BUT:

It seems like these wrappers (as well as the Buffer stuff from your previous CL)
could be useful for other data pipe consumers. Should they have a home in //mojo
instead?

[raymes, 2014-08-05 06:26:45]

I've only reviewed half of this but here are some initial comments. Sorry it has
taken me a bit to get my head around things :)

https://codereview.chromium.org/437933002/diff/80001/device/serial/async_wait...
File device/serial/async_waiter.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/async_wait...
device/serial/async_waiter.cc:35: callback.Run(result);
Why not just make the member non-const and call callback_.Run(result)?

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe....
File device/serial/data_pipe.mojom (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe....
device/serial/data_pipe.mojom:8: interface DataPipeProducer {
DataStreamSender/Client

DataStreamReceiver/Client?

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_producer.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_producer.cc:48: return;
It might be good to DCHECK at the start of every function that state_ == one of
the possible valid states to call that function in.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_producer.h (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_producer.h:25: // Constructs a DataPipeProducer.
Whenever the pipe as ready for writing, the
as->is

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:45: options.struct_size = sizeof(options);
is this necessary?

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:54: const ReceiveErrorCallback&
error_callback) {
Similarly for these functions it would be good to DCHECK that we are in one of
the valid states at the entrypoint to each function.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:59: if (state_ != STATE_IDLE ||
callback.is_null() || error_callback.is_null()) {
Might as well just DCHECK null callbacks rather than have this test?
Also no need for {}

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:122: switch (result) {
consider just using if/else rather than switch since we're only ever considering
2 cases

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:129:
base::Bind(&DataPipeReceiver::OnDoneWaiting,
Could you bind directly to RetryReceive?

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_receiver.h (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.h:28: // pipe with a buffer size of
|buffer_size|, with connections errors reported
connections->connection

[Sam McNally, 2014-08-05 07:26:33]

https://codereview.chromium.org/437933002/diff/80001/device/serial/async_wait...
File device/serial/async_waiter.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/async_wait...
device/serial/async_waiter.cc:35: callback.Run(result);
On 2014/08/05 06:26:44, raymes wrote:
> Why not just make the member non-const and call callback_.Run(result)?

Done.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe....
File device/serial/data_pipe.mojom (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe....
device/serial/data_pipe.mojom:8: interface DataPipeProducer {
On 2014/08/05 06:26:44, raymes wrote:
> DataStreamSender/Client
> 
> DataStreamReceiver/Client?

Done with names as discussed.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_producer.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_producer.cc:48: return;
On 2014/08/05 06:26:44, raymes wrote:
> It might be good to DCHECK at the start of every function that state_ == one
of
> the possible valid states to call that function in.

Init and Resume are in response to client calls so we can't trust them to only
occur in valid states. Done elsewhere.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_producer.h (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_producer.h:25: // Constructs a DataPipeProducer.
Whenever the pipe as ready for writing, the
On 2014/08/05 06:26:44, raymes wrote:
> as->is

Done.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:45: options.struct_size = sizeof(options);
On 2014/08/05 06:26:44, raymes wrote:
> is this necessary?

Yes.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:54: const ReceiveErrorCallback&
error_callback) {
On 2014/08/05 06:26:44, raymes wrote:
> Similarly for these functions it would be good to DCHECK that we are in one of
> the valid states at the entrypoint to each function.

Done.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:59: if (state_ != STATE_IDLE ||
callback.is_null() || error_callback.is_null()) {
On 2014/08/05 06:26:44, raymes wrote:
> Might as well just DCHECK null callbacks rather than have this test?
> Also no need for {}

Done.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:122: switch (result) {
On 2014/08/05 06:26:44, raymes wrote:
> consider just using if/else rather than switch since we're only ever
considering
> 2 cases

Done.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.cc:129:
base::Bind(&DataPipeReceiver::OnDoneWaiting,
On 2014/08/05 06:26:44, raymes wrote:
> Could you bind directly to RetryReceive?

OnDoneWaiting should be doing something with the wait result.

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
File device/serial/data_pipe_receiver.h (right):

https://codereview.chromium.org/437933002/diff/80001/device/serial/data_pipe_...
device/serial/data_pipe_receiver.h:28: // pipe with a buffer size of
|buffer_size|, with connections errors reported
On 2014/08/05 06:26:45, raymes wrote:
> connections->connection

Done.

[raymes, 2014-08-05 08:10:21]

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:89: void DataReceiver::OnDoneWaiting(MojoResult
result) {
I think we can only be in a subset of states here, maybe DCHECK which ones.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:91: OnConnectionError();
Consider inlining OnConnectionError here as I also think it will make the state
transitions clearer.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:97: void DataReceiver::OnError(uint32_t
bytes_since_last_error, int32_t error) {
Consider putting comments in all of the entrypoints where we could be in any
state, just so it is clear.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:106: if (!receive_callback_.is_null())
Maybe rather than checking whether callbacks are null, check that we are in a
valid state?

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:110: void DataReceiver::RetryReceive() {
Inline RetryReceive to reduce indirection

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:119: bytes_since_last_error_ -= error_offset_;
As we discussed consider always just counting all the bytes transmitted to make
the computation simpler.

[Sam McNally, 2014-08-05 08:33:09]

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:89: void DataReceiver::OnDoneWaiting(MojoResult
result) {
On 2014/08/05 08:10:20, raymes wrote:
> I think we can only be in a subset of states here, maybe DCHECK which ones.

Done.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:91: OnConnectionError();
On 2014/08/05 08:10:20, raymes wrote:
> Consider inlining OnConnectionError here as I also think it will make the
state
> transitions clearer.

Done.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:97: void DataReceiver::OnError(uint32_t
bytes_since_last_error, int32_t error) {
On 2014/08/05 08:10:20, raymes wrote:
> Consider putting comments in all of the entrypoints where we could be in any
> state, just so it is clear.

Done.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:106: if (!receive_callback_.is_null())
On 2014/08/05 08:10:20, raymes wrote:
> Maybe rather than checking whether callbacks are null, check that we are in a
> valid state?

Done.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:110: void DataReceiver::RetryReceive() {
On 2014/08/05 08:10:20, raymes wrote:
> Inline RetryReceive to reduce indirection

Done.

https://codereview.chromium.org/437933002/diff/120001/device/serial/data_rece...
device/serial/data_receiver.cc:119: bytes_since_last_error_ -= error_offset_;
On 2014/08/05 08:10:20, raymes wrote:
> As we discussed consider always just counting all the bytes transmitted to
make
> the computation simpler.

Done.

[raymes, 2014-08-06 00:13:36]

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:90: bytes_received_ += bytes_consumed;
We should make sure that this never exceeds error_offset_ under normal
operation.

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:95: DCHECK_EQ(state_, STATE_WAITING_FOR_DATA);
could we be shutdown here? I'm not sure?

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:106: void DataReceiver::OnError(uint32_t
bytes_since_last_error, int32_t error) {
We should know that we aren't in the paused state here and also that
pending_error isn't already true. This would only happen if the other side of
the connection didn't behave as expected, so we could bail in that case.

[Sam McNally, 2014-08-06 03:51:31]

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:90: bytes_received_ += bytes_consumed;
On 2014/08/06 00:13:36, raymes wrote:
> We should make sure that this never exceeds error_offset_ under normal
> operation. 

Done.

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:95: DCHECK_EQ(state_, STATE_WAITING_FOR_DATA);
On 2014/08/06 00:13:36, raymes wrote:
> could we be shutdown here? I'm not sure?

No, when waiting is cancelled when shutting down.

https://codereview.chromium.org/437933002/diff/140001/device/serial/data_rece...
device/serial/data_receiver.cc:106: void DataReceiver::OnError(uint32_t
bytes_since_last_error, int32_t error) {
On 2014/08/06 00:13:36, raymes wrote:
> We should know that we aren't in the paused state here and also that
> pending_error isn't already true. This would only happen if the other side of
> the connection didn't behave as expected, so we could bail in that case.

Done.

[raymes, 2014-08-06 05:46:43]

This looks way better! Thanks!

Please make sure you add comments for things. It's good to give a high level
description of the architecture in headers and to point out subtleties in the
implementation. I've given some examples in my comments here.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:15: class DataReceiver::PendingReceive {
Please add some comments for these helper classes too, including on the methods.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:27: class Buffer;
since this class is only used in the .cc file, you don't have to make it a
nested class, just declare it in an anonymous namespace.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:86: if (receive_ || shut_down_)
probably pending_receive_ is a more descriptive name

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:88: if (error_ && error_->dispatched) {
similarly, pending_error_

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:89: source_->Resume();
A comment here describing what's going on would be helpful. Something like "When
an error is received from the DataSource, we pause transmission. When the user
has been notified (via |error_callback|) of the error we can tell the DataSource
to resume transmission of data."

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:110: OnConnectionError();
Change this to call ShutDown instead of OnConnectionError

Add a comment here. "When OnError is called by the DataSource, transmission of
data is suspended. Thus we shouldn't receive another call to OnError until we
have fully dealt with the error and called Resume to resume transmission (see
Receive()). Under normal operation we should never get here, but if we do (e.g.
in the case of a hijacked service process) just shut down.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:136: DCHECK(receive_ && !shut_down_);
we can DCHECK waiter_ and cal waiter_.reset() here too.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:159: if (error_ &&
!CheckBytesReceived(num_bytes)) {
Maybe rename this function to something like CheckErrorNotInReadRange. You can
make the function accept a PendingError* and do the NULL test inside of it.

Also make sure that function is documented because this is subtle. Something
like "We may have been notified of an error that occured at some future point in
the stream. We should never be able to read past the point at which the error
occurred until we have dealt with the error and called Resume() on the
DataSource. If this has occurred, something bad has happened on the service
side, so we shut down."

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:188: }
Mention the fact that this is complicated because we check for overflow.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:222: if (buffer_in_use_ || bytes_received !=
error->offset || error->dispatched)
Probably if error->dispatched is already true, we want to return true here
otherwise I don't think the callsites will behave correctly.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
File device/serial/data_receiver.h (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.h:54: void Done(uint32_t bytes_consumed);
Please add comments for each of these methods and the member variables below.

[raymes, 2014-08-06 06:41:54]

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
File device/serial/data_source_sender.cc (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:60: waiter_.reset();
I guess we could reset send_ here too?
Also consider resetting the callbacks to ensure we never run them again.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:68: if (send_ || handle_.is_valid())
send_ -> pending_send_

I guess this case should never happen under normal operation, should we
shutdown? A comment explaining this could be good.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:77: if (send_ || !handle_.is_valid())
-Can the handle_ ever go from being valid to invalid during operation? Can we
just test this once in Init()?

-Similarly in this case we can probably shut down here because this isn't
expected under normal operation.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:89: DCHECK(send_);
Also !waiter_ should be true here?

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:99: return;
I think you can get rid of the HandleMojoResult function and it's not much more
verbose. We already DCHECK send_ at all the call sites, so it boils down to:

if (result != MOJO_RESULT_OK)
  DispatchFatalError();
  return;
}

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:115: void
DataSourceSender::DoneWithError(uint32_t bytes_produced, int32_t error) {
Make a note in here that we don't call StartWaiting intentionally so that we
don't send any additional data until Resume() is called.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:122: void
DataSourceSender::DoneInternal(uint32_t bytes_produced) {
Should we bail if we are shut_down_ on entry to this function?

[Sam McNally, 2014-08-06 08:28:15]

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
File device/serial/data_receiver.cc (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:15: class DataReceiver::PendingReceive {
On 2014/08/06 05:46:43, raymes wrote:
> Please add some comments for these helper classes too, including on the
methods.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:27: class Buffer;
On 2014/08/06 05:46:43, raymes wrote:
> since this class is only used in the .cc file, you don't have to make it a
> nested class, just declare it in an anonymous namespace.

It's nested so it can see DataReceiver::PendingReceive, which is private.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:86: if (receive_ || shut_down_)
On 2014/08/06 05:46:43, raymes wrote:
> probably pending_receive_ is a more descriptive name

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:88: if (error_ && error_->dispatched) {
On 2014/08/06 05:46:43, raymes wrote:
> similarly, pending_error_

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:89: source_->Resume();
On 2014/08/06 05:46:43, raymes wrote:
> A comment here describing what's going on would be helpful. Something like
"When
> an error is received from the DataSource, we pause transmission. When the user
> has been notified (via |error_callback|) of the error we can tell the
DataSource
> to resume transmission of data."

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:110: OnConnectionError();
On 2014/08/06 05:46:43, raymes wrote:
> Change this to call ShutDown instead of OnConnectionError
> 
> Add a comment here. "When OnError is called by the DataSource, transmission of
> data is suspended. Thus we shouldn't receive another call to OnError until we
> have fully dealt with the error and called Resume to resume transmission (see
> Receive()). Under normal operation we should never get here, but if we do
(e.g.
> in the case of a hijacked service process) just shut down.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:136: DCHECK(receive_ && !shut_down_);
On 2014/08/06 05:46:43, raymes wrote:
> we can DCHECK waiter_ and cal waiter_.reset() here too.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:159: if (error_ &&
!CheckBytesReceived(num_bytes)) {
On 2014/08/06 05:46:43, raymes wrote:
> Maybe rename this function to something like CheckErrorNotInReadRange. You can
> make the function accept a PendingError* and do the NULL test inside of it.
> 
> Also make sure that function is documented because this is subtle. Something
> like "We may have been notified of an error that occured at some future point
in
> the stream. We should never be able to read past the point at which the error
> occurred until we have dealt with the error and called Resume() on the
> DataSource. If this has occurred, something bad has happened on the service
> side, so we shut down."

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:188: }
On 2014/08/06 05:46:43, raymes wrote:
> Mention the fact that this is complicated because we check for overflow.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.cc:222: if (buffer_in_use_ || bytes_received !=
error->offset || error->dispatched)
On 2014/08/06 05:46:43, raymes wrote:
> Probably if error->dispatched is already true, we want to return true here
> otherwise I don't think the callsites will behave correctly.

I don't think this should be called if error->dispatched is already true.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
File device/serial/data_receiver.h (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_rece...
device/serial/data_receiver.h:54: void Done(uint32_t bytes_consumed);
On 2014/08/06 05:46:43, raymes wrote:
> Please add comments for each of these methods and the member variables below.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
File device/serial/data_source_sender.cc (right):

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:60: waiter_.reset();
On 2014/08/06 06:41:53, raymes wrote:
> I guess we could reset send_ here too?
> Also consider resetting the callbacks to ensure we never run them again.

We can't reset pending_send_ in case it has buffer_in_use_

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:68: if (send_ || handle_.is_valid())
On 2014/08/06 06:41:53, raymes wrote:
> send_ -> pending_send_
> 
> I guess this case should never happen under normal operation, should we
> shutdown? A comment explaining this could be good.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:77: if (send_ || !handle_.is_valid())
On 2014/08/06 06:41:53, raymes wrote:
> -Can the handle_ ever go from being valid to invalid during operation? Can we
> just test this once in Init()?
> 
> -Similarly in this case we can probably shut down here because this isn't
> expected under normal operation.

It could get called before Init().

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:89: DCHECK(send_);
On 2014/08/06 06:41:53, raymes wrote:
> Also !waiter_ should be true here?

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:99: return;
On 2014/08/06 06:41:53, raymes wrote:
> I think you can get rid of the HandleMojoResult function and it's not much
more
> verbose. We already DCHECK send_ at all the call sites, so it boils down to:
> 
> if (result != MOJO_RESULT_OK)
>   DispatchFatalError();
>   return;
> }

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:115: void
DataSourceSender::DoneWithError(uint32_t bytes_produced, int32_t error) {
On 2014/08/06 06:41:53, raymes wrote:
> Make a note in here that we don't call StartWaiting intentionally so that we
> don't send any additional data until Resume() is called.

Done.

https://codereview.chromium.org/437933002/diff/160001/device/serial/data_sour...
device/serial/data_source_sender.cc:122: void
DataSourceSender::DoneInternal(uint32_t bytes_produced) {
On 2014/08/06 06:41:53, raymes wrote:
> Should we bail if we are shut_down_ on entry to this function?

Done.

[raymes, 2014-08-11 04:04:51]

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
File device/serial/data_source_unittest.cc (right):

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:64: RunMessageLoop();
-Add a comment // Run the message loop until CanWriteData is called.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:66: return false;
We never check the return value, so you could just DCHECK here. and below and
make it a void function.

DCHECK(write_buffer_)

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:67: if (write_buffer_->GetSize() <
static_cast<uint32_t>(data.size())) {
DCHECK(write_buffer_->GetSize() >= static_cast<uint32_t>(data.size()));

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:85: RunMessageLoop();
Add a comment here, // Run the message loop until OnDataReceived is called.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:126: TEST_F(DataSourceTest, Basic) {
Add a comment above each test explaining what it is testing.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:167: EXPECT_EQ("b", buffer_contents_);
You might as well call Done again and check that there is nothing else to
receive?

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:193: FillWriteBuffer("d", -2);
Maybe test a non-error transmission is successful before testing the error case
again. This will ensure that we can recover ok from errors.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:221: TEST_F(DataSourceTest,
ProducerShutdown) {
Maybe call this "SourceShutDown" since that matches our naming scheme more
closely.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:226: EXPECT_EQ(-10, error_);
Make this a global constant to clarify what's happening.

[raymes, 2014-08-11 04:07:44]

lgtm

[Sam McNally, 2014-08-11 04:56:43]

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
File device/serial/data_source_unittest.cc (right):

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:64: RunMessageLoop();
On 2014/08/11 04:04:51, raymes wrote:
> -Add a comment // Run the message loop until CanWriteData is called.

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:66: return false;
On 2014/08/11 04:04:51, raymes wrote:
> We never check the return value, so you could just DCHECK here. and below and
> make it a void function.
> 
> DCHECK(write_buffer_)

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:67: if (write_buffer_->GetSize() <
static_cast<uint32_t>(data.size())) {
On 2014/08/11 04:04:51, raymes wrote:
> DCHECK(write_buffer_->GetSize() >= static_cast<uint32_t>(data.size()));

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:85: RunMessageLoop();
On 2014/08/11 04:04:51, raymes wrote:
> Add a comment here, // Run the message loop until OnDataReceived is called.

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:126: TEST_F(DataSourceTest, Basic) {
On 2014/08/11 04:04:51, raymes wrote:
> Add a comment above each test explaining what it is testing.

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:167: EXPECT_EQ("b", buffer_contents_);
On 2014/08/11 04:04:51, raymes wrote:
> You might as well call Done again and check that there is nothing else to
> receive?

As discussed, the API doesn't support this.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:193: FillWriteBuffer("d", -2);
On 2014/08/11 04:04:51, raymes wrote:
> Maybe test a non-error transmission is successful before testing the error
case
> again. This will ensure that we can recover ok from errors.

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:221: TEST_F(DataSourceTest,
ProducerShutdown) {
On 2014/08/11 04:04:51, raymes wrote:
> Maybe call this "SourceShutDown" since that matches our naming scheme more
> closely.

Done.

https://codereview.chromium.org/437933002/diff/180001/device/serial/data_sour...
device/serial/data_source_unittest.cc:226: EXPECT_EQ(-10, error_);
On 2014/08/11 04:04:51, raymes wrote:
> Make this a global constant to clarify what's happening.

Done.

[Ken Rockot(use gerrit already), 2014-08-11 06:02:05]

lgtm

[Sam McNally, 2014-08-11 06:54:46]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-08-11 06:55:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/437933002/240001

[Sam McNally, 2014-08-11 07:36:44]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-08-11 07:37:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/437933002/260001

[Sam McNally, 2014-08-11 07:57:57]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-08-11 08:01:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/437933002/280001

[commit-bot: I haz the power, 2014-08-11 12:18:56]

Change committed as 288692