Change data pipe wrappers used by SerialConnection to use message pipe.

Change data pipe wrappers used by SerialConnection to use message pipe.

Interprocess mojo data pipe is not ready yet. This converts the data
pipe wrappers to send the data via their control message pipes until
interprocess data pipe is ready.

BUG=389016
Committed: https://crrev.com/f43db66685654de91010ee4feda62d7b7880d4b4
Cr-Commit-Position: refs/heads/master@{#301582}

[Sam McNally, 2014-10-10 23:58:40]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-10-13 17:47:08]

Patchset #1 (id:170001) has been deleted

[Sam McNally, 2014-10-14 17:18:52]

Patchset #1 (id:370001) has been deleted

[Sam McNally, 2014-10-14 17:28:45]

Patchset #1 (id:520001) has been deleted

[Sam McNally, 2014-10-14 17:33:14]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2014-10-17 01:55:42]

Reviewed DataSender and DataReceiver

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

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:32: bool dispatched() { return buffer_in_use_; }
why not just call this function "buffer_in_use()"

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:97: struct DataReceiver::PendingError {
Could we combine PendingData and PendingError? It would simplify ordering logic
a bit. I think it seems natural have the error in the queue of received data.
Having a couple of unused fields seems ok. We could call it a DataFrame or
something? This seems like it would simplify DataReceiver::ReceiveInternal quite
a bit as well.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:183: if (pending_data.offset ==
pending_data.data.size()) {
Maybe we should at least DCHECK here that pending_data.offset <=
pending_data.data.size().

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
File device/serial/data_sender.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.cc:234: uint32_t* capacity) {
I think calling this available_buffer_size to match the member makes it clearer
that is what it it actually is.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
File device/serial/data_sender.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.h:36: bool use_data_pipe);
Looks like this was added before changing to support message pipe only, I think
we can remove.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.h:97: // acknowledged.
It looks like the description might be slightly wrong here? Isn't this the
buffer size minus the unacknowledged bytes?

[raymes, 2014-10-17 02:11:10]

A few more comments.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:16: AckData(uint32 bytes_received);
Please add a comment here. Does it make more sense to have this as a callback on
OnData?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:39: AcceptData(array<uint8> data);
nit, optional: should we name this consistently with DataSourceClient::OnData. I
realize you named them differently on purpose but both serve a similar purpose.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:48: // |bytes_to_flush|, the number of bytes
enqueued in the data pipe but not yet
This comment is out of date now. I'm curious why is it still useful to receive
the callback?

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
File device/serial/serial_connection_unittest.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
device/serial/serial_connection_unittest.cc:279:
EXPECT_TRUE(io_handler_->HasOneRef());
Hmm, what does this check?

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
device/serial/serial_connection_unittest.cc:283: connection_.reset();
And how come this is needed now?

https://codereview.chromium.org/646063003/diff/540001/extensions/renderer/api...
File extensions/renderer/api/serial/data_receiver_unittest.cc (left):

https://codereview.chromium.org/646063003/diff/540001/extensions/renderer/api...
extensions/renderer/api/serial/data_receiver_unittest.cc:143: 
Why is this removed now?

https://codereview.chromium.org/646063003/diff/540001/extensions/test/data/da...
File extensions/test/data/data_receiver_unittest.js (left):

https://codereview.chromium.org/646063003/diff/540001/extensions/test/data/da...
extensions/test/data/data_receiver_unittest.js:183: },
And why is this deleted? (sorry this is probably my ignorance)

[raymes, 2014-10-17 03:10:12]

DataSinkReceiver

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
File device/serial/data_sink_receiver.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:104: return;
Should this ever happen in normal operation? We should probably make a comment
about that. This line seems a little weird to me.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:112: if (waiting_for_data_ &&
!buffer_in_use_)
Hmm I can't work out what waiting_for_data_ is for?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:120: void DataSinkReceiver::CheckForData() {
I feel like there might be a more descriptive name for this. RunReadyCallback?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:158: pending_data_offset_ += bytes_read;
We could use a small struct for pending data buffers and have a member for
pending_data_offset_. This might simplify things a little bit e.g. no need to
remember to reset pending_data_buffer_, you could add a convenience function to
compute the amount remaining in the buffer which could be used here and in
DoFlush()

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:168: // When we encounter an error, we must
discard the data from any sends
sends->send ?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
File device/serial/data_sink_receiver.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.h:50: virtual void Init(uint32_t error)
override;
is the parameter meant to be the buffer size?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.h:103: uint32_t pending_data_offset_;
Please add comments for these.

[raymes, 2014-10-17 03:16:24]

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:48: // |bytes_to_flush|, the number of bytes
enqueued in the data pipe but not yet
On 2014/10/17 02:11:10, raymes wrote:
> This comment is out of date now. I'm curious why is it still useful to receive
> the callback? 

I guess the callback basically ensures that all the sends in the pipe have been
transmitted before it is received and that the DataSender isn't going to send
anymore. Is that correct?

[raymes, 2014-10-17 04:04:45]

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

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:20: // Asynchronously fills |data| with up
to |num_bytes| of data. Following this,
|data| is gone!

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:24: void
DispatchData(serial::DataSourceClient* client);
Please add a comment here.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:120:
pending_send_->GetData(available_buffer_capacity_);
What if the available buffer capacity is 0? Don't we want to wait in that case?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:147: pending_send_->DispatchData(client());
Rather than passing the client to the pending send, how about just sending the
client the data in pending send here?

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:169: new Buffer(sender_, this, &data_[0],
num_bytes)));
We should be careful of a 0 buffer size here.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:195: buffer_in_use_ = false;
nit: ordering of these 3 lines should be the same as above in Done()

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
File device/serial/data_source_sender.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.h:75: // acknowledged.
Please see my other note on the similar comment in data_sender.cc.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:16: AckData(uint32 bytes_received);
On 2014/10/17 02:11:09, raymes wrote:
> Please add a comment here. Does it make more sense to have this as a callback
on
> OnData?

Also should we name this consistently with ReportBytesSent(uint32 bytes_sent)?

[Sam McNally, 2014-10-20 05:12:04]

Patchset #2 (id:560001) has been deleted

[Sam McNally, 2014-10-20 05:13:00]

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

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:32: bool dispatched() { return buffer_in_use_; }
On 2014/10/17 01:55:42, raymes wrote:
> why not just call this function "buffer_in_use()"

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:97: struct DataReceiver::PendingError {
On 2014/10/17 01:55:42, raymes wrote:
> Could we combine PendingData and PendingError? It would simplify ordering
logic
> a bit. I think it seems natural have the error in the queue of received data.
> Having a couple of unused fields seems ok. We could call it a DataFrame or
> something? This seems like it would simplify DataReceiver::ReceiveInternal
quite
> a bit as well.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_rece...
device/serial/data_receiver.cc:183: if (pending_data.offset ==
pending_data.data.size()) {
On 2014/10/17 01:55:42, raymes wrote:
> Maybe we should at least DCHECK here that pending_data.offset <=
> pending_data.data.size(). 

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
File device/serial/data_sender.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.cc:234: uint32_t* capacity) {
On 2014/10/17 01:55:42, raymes wrote:
> I think calling this available_buffer_size to match the member makes it
clearer
> that is what it it actually is.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
File device/serial/data_sender.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.h:36: bool use_data_pipe);
On 2014/10/17 01:55:42, raymes wrote:
> Looks like this was added before changing to support message pipe only, I
think
> we can remove.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_send...
device/serial/data_sender.h:97: // acknowledged.
On 2014/10/17 01:55:42, raymes wrote:
> It looks like the description might be slightly wrong here? Isn't this the
> buffer size minus the unacknowledged bytes?

Done. It originally counted up.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
File device/serial/data_sink_receiver.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:104: return;
On 2014/10/17 03:10:12, raymes wrote:
> Should this ever happen in normal operation? We should probably make a comment
> about that. This line seems a little weird to me.

It can occur if the error notification and some data happen to cross paths. It
doesn't need to handled specially though.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:112: if (waiting_for_data_ &&
!buffer_in_use_)
On 2014/10/17 03:10:12, raymes wrote:
> Hmm I can't work out what waiting_for_data_ is for?

Removed it.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:120: void DataSinkReceiver::CheckForData() {
On 2014/10/17 03:10:12, raymes wrote:
> I feel like there might be a more descriptive name for this. RunReadyCallback?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:158: pending_data_offset_ += bytes_read;
On 2014/10/17 03:10:12, raymes wrote:
> We could use a small struct for pending data buffers and have a member for
> pending_data_offset_. This might simplify things a little bit e.g. no need to
> remember to reset pending_data_buffer_, you could add a convenience function
to
> compute the amount remaining in the buffer which could be used here and in
> DoFlush()

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:168: // When we encounter an error, we must
discard the data from any sends
On 2014/10/17 03:10:12, raymes wrote:
> sends->send ?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
File device/serial/data_sink_receiver.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.h:50: virtual void Init(uint32_t error)
override;
On 2014/10/17 03:10:12, raymes wrote:
> is the parameter meant to be the buffer size?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sink...
device/serial/data_sink_receiver.h:103: uint32_t pending_data_offset_;
On 2014/10/17 03:10:12, raymes wrote:
> Please add comments for these.

Done.

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

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:20: // Asynchronously fills |data| with up
to |num_bytes| of data. Following this,
On 2014/10/17 04:04:45, raymes wrote:
> |data| is gone!

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:24: void
DispatchData(serial::DataSourceClient* client);
On 2014/10/17 04:04:45, raymes wrote:
> Please add a comment here.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:120:
pending_send_->GetData(available_buffer_capacity_);
On 2014/10/17 04:04:45, raymes wrote:
> What if the available buffer capacity is 0? Don't we want to wait in that
case?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:147: pending_send_->DispatchData(client());
On 2014/10/17 04:04:45, raymes wrote:
> Rather than passing the client to the pending send, how about just sending the
> client the data in pending send here?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:169: new Buffer(sender_, this, &data_[0],
num_bytes)));
On 2014/10/17 04:04:45, raymes wrote:
> We should be careful of a 0 buffer size here.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.cc:195: buffer_in_use_ = false;
On 2014/10/17 04:04:45, raymes wrote:
> nit: ordering of these 3 lines should be the same as above in Done()

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
File device/serial/data_source_sender.h (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_sour...
device/serial/data_source_sender.h:75: // acknowledged.
On 2014/10/17 04:04:45, raymes wrote:
> Please see my other note on the similar comment in data_sender.cc.

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:16: AckData(uint32 bytes_received);
On 2014/10/17 02:11:09, raymes wrote:
> Please add a comment here. Does it make more sense to have this as a callback
on
> OnData?

It does, but there could be a stash before the response would be sent.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:16: AckData(uint32 bytes_received);
On 2014/10/17 04:04:45, raymes wrote:
> On 2014/10/17 02:11:09, raymes wrote:
> > Please add a comment here. Does it make more sense to have this as a
callback
> on
> > OnData?
> 
> Also should we name this consistently with ReportBytesSent(uint32 bytes_sent)?

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:39: AcceptData(array<uint8> data);
On 2014/10/17 02:11:10, raymes wrote:
> nit, optional: should we name this consistently with DataSourceClient::OnData.
I
> realize you named them differently on purpose but both serve a similar
purpose. 

Done.

https://codereview.chromium.org/646063003/diff/540001/device/serial/data_stre...
device/serial/data_stream.mojom:48: // |bytes_to_flush|, the number of bytes
enqueued in the data pipe but not yet
On 2014/10/17 03:16:24, raymes wrote:
> On 2014/10/17 02:11:10, raymes wrote:
> > This comment is out of date now. I'm curious why is it still useful to
receive
> > the callback? 
> 
> I guess the callback basically ensures that all the sends in the pipe have
been
> transmitted before it is received and that the DataSender isn't going to send
> anymore. Is that correct?

The sends in the pipeline are all cancelled. Otherwise, you're correct. It
ensures the sink knows which OnData messages were sent before the error and
which were after.

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
File device/serial/serial_connection_unittest.cc (right):

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
device/serial/serial_connection_unittest.cc:279:
EXPECT_TRUE(io_handler_->HasOneRef());
On 2014/10/17 02:11:10, raymes wrote:
> Hmm, what does this check?

This checks that the connection object has actually been deleted or at least
isn't holding onto |io_handler_|.

https://codereview.chromium.org/646063003/diff/540001/device/serial/serial_co...
device/serial/serial_connection_unittest.cc:283: connection_.reset();
On 2014/10/17 02:11:10, raymes wrote:
> And how come this is needed now?

Timing is different when data goes over the control pipe.

https://codereview.chromium.org/646063003/diff/540001/extensions/renderer/api...
File extensions/renderer/api/serial/data_receiver_unittest.cc (left):

https://codereview.chromium.org/646063003/diff/540001/extensions/renderer/api...
extensions/renderer/api/serial/data_receiver_unittest.cc:143: 
On 2014/10/17 02:11:10, raymes wrote:
> Why is this removed now?

Message pipe close isn't reported in the JS API unless waiting for a response.

https://codereview.chromium.org/646063003/diff/540001/extensions/test/data/da...
File extensions/test/data/data_receiver_unittest.js (left):

https://codereview.chromium.org/646063003/diff/540001/extensions/test/data/da...
extensions/test/data/data_receiver_unittest.js:183: },
On 2014/10/17 02:11:10, raymes wrote:
> And why is this deleted? (sorry this is probably my ignorance)

Corresponding JS test for the other deletion.

[Sam McNally, 2014-10-22 03:38:14]

Patchset #3 (id:600001) has been deleted

[raymes, 2014-10-27 03:02:23]

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

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_rece...
device/serial/data_receiver.h:86: std::queue<linked_ptr<DataFrame>>
pending_data_buffers_;
Should we call it pending_data_frames_? And update the comment?

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
File device/serial/data_sink_receiver.cc (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:59: }
nit: since we're not inlining OnDataConsumed we might as well not inline the
above either.

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_stre...
device/serial/data_stream.mojom:18: ReportBytesSent(uint32 bytes_sent);
Hmm maybe ReportBytesReceived is clearer?

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:133: pendingData,
Please update the comments.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:154: PendingSend.prototype.sendData
= function(sink, availableBufferCapacity) {
Please comment availableBufferCapacity

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:162: };
This no longer returns a number either.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:283: if (this.pendingSends_.length
|| this.sendsAwaitingAck_.length) {
Was this a bug before?

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:398: this.availableBufferCapacity_
+= numBytes;
Should this be -=? I'm not sure if we should test this because when we go back
to data pipe it will be implicit.

[Sam McNally, 2014-10-27 05:26:23]

Patchset #3 (id:620001) has been deleted

[Sam McNally, 2014-10-27 05:39:14]

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

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_rece...
device/serial/data_receiver.h:86: std::queue<linked_ptr<DataFrame>>
pending_data_buffers_;
On 2014/10/27 03:02:22, raymes wrote:
> Should we call it pending_data_frames_? And update the comment?

Done.

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
File device/serial/data_sink_receiver.cc (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
device/serial/data_sink_receiver.cc:59: }
On 2014/10/27 03:02:22, raymes wrote:
> nit: since we're not inlining OnDataConsumed we might as well not inline the
> above either.

Done.

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_stre...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_stre...
device/serial/data_stream.mojom:18: ReportBytesSent(uint32 bytes_sent);
On 2014/10/27 03:02:23, raymes wrote:
> Hmm maybe ReportBytesReceived is clearer?

Done.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:133: pendingData,
On 2014/10/27 03:02:23, raymes wrote:
> Please update the comments.

Done.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:154: PendingSend.prototype.sendData
= function(sink, availableBufferCapacity) {
On 2014/10/27 03:02:23, raymes wrote:
> Please comment availableBufferCapacity

Done.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:162: };
On 2014/10/27 03:02:23, raymes wrote:
> This no longer returns a number either.

Done.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:283: if (this.pendingSends_.length
|| this.sendsAwaitingAck_.length) {
On 2014/10/27 03:02:23, raymes wrote:
> Was this a bug before?

Yes.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:398: this.availableBufferCapacity_
+= numBytes;
On 2014/10/27 03:02:23, raymes wrote:
> Should this be -=? I'm not sure if we should test this because when we go back
> to data pipe it will be implicit.

No. Buffer space is freed up as the data is passed to the sink's client. This is
implicitly tested by the large sends test.

[raymes, 2014-10-27 22:57:29]

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
File device/serial/data_sink_receiver.h (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
device/serial/data_sink_receiver.h:50: virtual void Init(uint32_t buffer_size)
override;
Is removing virtual a part of a C++11 cleanup?

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:283: if (this.pendingSends_.length
|| this.sendsAwaitingAck_.length) {
We need to make sure it gets merged when we revert! How can we be reminded? How
about landing the change in a separate CL before you land this? I didn't spot
any other bug fixes in this CL - do you remember anything else that we could put
in that CL?

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:398: this.availableBufferCapacity_
+= numBytes;
I don't see where the availableBufferCapacity_ is reduced?

[Sam McNally, 2014-10-27 23:32:48]

Patchset #5 (id:680001) has been deleted

[Sam McNally, 2014-10-27 23:33:57]

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
File device/serial/data_sink_receiver.h (right):

https://codereview.chromium.org/646063003/diff/580001/device/serial/data_sink...
device/serial/data_sink_receiver.h:50: virtual void Init(uint32_t buffer_size)
override;
On 2014/10/27 22:57:29, raymes wrote:
> Is removing virtual a part of a C++11 cleanup?

I think so, but it isn't part of this CL.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:283: if (this.pendingSends_.length
|| this.sendsAwaitingAck_.length) {
On 2014/10/27 22:57:29, raymes wrote:
> We need to make sure it gets merged when we revert! How can we be reminded?
How
> about landing the change in a separate CL before you land this? I didn't spot
> any other bug fixes in this CL - do you remember anything else that we could
put
> in that CL?

Done.

https://codereview.chromium.org/646063003/diff/580001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:398: this.availableBufferCapacity_
+= numBytes;
On 2014/10/27 22:57:29, raymes wrote:
> I don't see where the availableBufferCapacity_ is reduced?

It's set in sendInternal_().

[raymes, 2014-10-27 23:36:36]

lgtm

[Sam McNally, 2014-10-27 23:41:59]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org

[Sam McNally, 2014-10-27 23:41:59]

+rockot

[Ken Rockot(use gerrit already), 2014-10-28 04:59:33]

lgtm

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

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_rece...
device/serial/data_receiver.cc:23: bool
DispatchDataFrame(DataReceiver::DataFrame* data);
nit: Please document the return value meaning - it's not immediately clear (at
least it wasn't to me)

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
File device/serial/data_sender.cc (right):

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
device/serial/data_sender.cc:239: memcpy(&bytes[0], data_.data() + bytes_sent_,
num_bytes_to_send);
I actually think the preferred spelling of &bytes[0] may be

&bytes.storage()[0]

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:126: * @param {ArrayBuffer[]}
pendingData Data received from the DataSource not
At least the current version of closure compiler doesn't seem to support this
syntax for array type specs. Also it's ambiguous about nullability (unless maybe
you said !(!ArrayBuffer[]) which is possibly also illegal). In any case, an
unambiguous and legal form would be:

{!Array.<!ArrayBuffer>}

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:149: * @return {object} result The
send result.
nit: !Object

[Sam McNally, 2014-10-28 05:01:53]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2014-10-28 05:02:06]

The CQ bit was unchecked by sammc@chromium.org

[Sam McNally, 2014-10-28 05:11:11]

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

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_rece...
device/serial/data_receiver.cc:23: bool
DispatchDataFrame(DataReceiver::DataFrame* data);
On 2014/10/28 04:59:33, Ken Rockot wrote:
> nit: Please document the return value meaning - it's not immediately clear (at
> least it wasn't to me)

Done.

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
File device/serial/data_sender.cc (right):

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
device/serial/data_sender.cc:239: memcpy(&bytes[0], data_.data() + bytes_sent_,
num_bytes_to_send);
On 2014/10/28 04:59:33, Ken Rockot wrote:
> I actually think the preferred spelling of &bytes[0] may be
> 
> &bytes.storage()[0]
> 

Done.

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:126: * @param {ArrayBuffer[]}
pendingData Data received from the DataSource not
On 2014/10/28 04:59:33, Ken Rockot wrote:
> At least the current version of closure compiler doesn't seem to support this
> syntax for array type specs. Also it's ambiguous about nullability (unless
maybe
> you said !(!ArrayBuffer[]) which is possibly also illegal). In any case, an
> unambiguous and legal form would be:
> 
> {!Array.<!ArrayBuffer>}

Done.

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/646063003/diff/700001/extensions/renderer/res...
extensions/renderer/resources/data_sender.js:149: * @return {object} result The
send result.
On 2014/10/28 04:59:33, Ken Rockot wrote:
> nit: !Object

Done.

[Sam McNally, 2014-10-28 05:11:44]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 05:13:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646063003/720001

[commit-bot: I haz the power, 2014-10-28 05:24:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-28 05:24:59]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2014-10-28 05:37:22]

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
File device/serial/data_sender.cc (right):

https://codereview.chromium.org/646063003/diff/700001/device/serial/data_send...
device/serial/data_sender.cc:239: memcpy(&bytes[0], data_.data() + bytes_sent_,
num_bytes_to_send);
On 2014/10/28 05:11:10, Sam McNally wrote:
> On 2014/10/28 04:59:33, Ken Rockot wrote:
> > I actually think the preferred spelling of &bytes[0] may be
> > 
> > &bytes.storage()[0]
> > 
> 
> Done.

Or not. storage() returns a const ref to the underlying vector.

[Sam McNally, 2014-10-28 05:37:37]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 05:38:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646063003/740001

[Sam McNally, 2014-10-28 06:03:30]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 06:03:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646063003/760001

[commit-bot: I haz the power, 2014-10-28 07:04:43]

Committed patchset #8 (id:760001)

[commit-bot: I haz the power, 2014-10-28 07:05:34]

Patchset 8 (id:??) landed as
https://crrev.com/f43db66685654de91010ee4feda62d7b7880d4b4
Cr-Commit-Position: refs/heads/master@{#301582}