Blimp: make PacketReader interface return packet sizes as return vals.

* Change PacketReader interface to return packet sizes as return
values instead of buffer offsets. Nonzero offsets can create unwanted
behavior for callers that depend on the data() method for accessing
the result. It is also a better fit with established net/ conventions.
* Modify PacketWriter to take buffer refptrs by const ref, reducing refcounting churn.

BUG=
R=wez@chromium.org

Review URL: https://codereview.chromium.org/1831833003

Cr-Commit-Position: refs/heads/master@{#383244}

10 files changed, 37 insertions, 49 deletions

diff --git a/blimp/net/blimp_message_pump.cc b/blimp/net/blimp_message_pump.cc
index 37b084ee..82a2dc3 100644
--- a/blimp/net/blimp_message_pump.cc
+++ b/blimp/net/blimp_message_pump.cc
@@ -53,9 +53,9 @@ void BlimpMessagePump::OnReadPacketComplete(int result) {
   DVLOG(2) << "OnReadPacketComplete, result=" << result;
   DCHECK(read_inflight_);
   read_inflight_ = false;
-  if (result == net::OK) {
+  if (result >= 0) {
     scoped_ptr<BlimpMessage> message(new BlimpMessage);
-    if (message->ParseFromArray(buffer_->StartOfBuffer(), buffer_->offset())) {
+    if (message->ParseFromArray(buffer_->data(), result)) {
       DVLOG(2) << "OnReadPacketComplete, result=" << *message;
       processor_->ProcessMessage(
           std::move(message),
@@ -66,7 +66,7 @@ void BlimpMessagePump::OnReadPacketComplete(int result) {
     }
   }
 
-  if (result != net::OK) {
+  if (result < 0) {
     error_observer_->OnConnectionError(result);
   }
 }
@@ -74,7 +74,7 @@ void BlimpMessagePump::OnReadPacketComplete(int result) {
 void BlimpMessagePump::OnProcessMessageComplete(int result) {
   DVLOG(2) << "OnProcessMessageComplete, result=" << result;
 
-  if (result != net::OK) {
+  if (result < 0) {
     error_observer_->OnConnectionError(result);
     return;
   }
diff --git a/blimp/net/blimp_message_pump_unittest.cc b/blimp/net/blimp_message_pump_unittest.cc
index aede128..8d813e6 100644
--- a/blimp/net/blimp_message_pump_unittest.cc
+++ b/blimp/net/blimp_message_pump_unittest.cc
@@ -58,7 +58,6 @@ TEST_F(BlimpMessagePumpTest, ReadPacket) {
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _));
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message1_.get()),
-                      SetBufferOffset<0>(message1_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)))
       .RetiresOnSaturation();
   net::CompletionCallback process_msg_cb;
@@ -66,7 +65,7 @@ TEST_F(BlimpMessagePumpTest, ReadPacket) {
       .WillOnce(SaveArg<1>(&process_msg_cb));
   message_pump_->SetMessageProcessor(&receiver_);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message1_->ByteSize());
   process_msg_cb.Run(net::OK);
 }
 
@@ -75,10 +74,8 @@ TEST_F(BlimpMessagePumpTest, ReadTwoPackets) {
   net::CompletionCallback read_packet_cb;
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message1_.get()),
-                      SetBufferOffset<0>(message1_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message2_.get()),
-                      SetBufferOffset<0>(message2_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)));
   net::CompletionCallback process_msg_cb;
   {
@@ -90,12 +87,12 @@ TEST_F(BlimpMessagePumpTest, ReadTwoPackets) {
   }
   message_pump_->SetMessageProcessor(&receiver_);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message1_->ByteSize());
 
   // Trigger next packet read
   process_msg_cb.Run(net::OK);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message2_->ByteSize());
 }
 
 // Reader completes reading two packets asynchronously.
@@ -105,10 +102,8 @@ TEST_F(BlimpMessagePumpTest, ReadTwoPacketsWithError) {
   net::CompletionCallback read_packet_cb;
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message1_.get()),
-                      SetBufferOffset<0>(message1_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message2_.get()),
-                      SetBufferOffset<0>(message2_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)));
   EXPECT_CALL(receiver_, MockableProcessMessage(EqualsProto(*message1_), _))
       .WillOnce(SaveArg<1>(&process_msg_cb));
@@ -116,7 +111,7 @@ TEST_F(BlimpMessagePumpTest, ReadTwoPacketsWithError) {
 
   message_pump_->SetMessageProcessor(&receiver_);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message1_->ByteSize());
 
   // Trigger next packet read
   process_msg_cb.Run(net::OK);
@@ -130,13 +125,12 @@ TEST_F(BlimpMessagePumpTest, InvalidPacket) {
   std::string test_msg("msg");
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _))
       .WillOnce(DoAll(FillBufferFromString<0>(test_msg),
-                      SetBufferOffset<0>(test_msg.size()),
                       SaveArg<1>(&read_packet_cb)));
   EXPECT_CALL(error_observer_, OnConnectionError(net::ERR_FAILED));
 
   message_pump_->SetMessageProcessor(&receiver_);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message1_->ByteSize());
 }
 
 // Outgoing MessageProcessor can be set to NULL if no read is pending.
@@ -146,7 +140,6 @@ TEST_F(BlimpMessagePumpTest, NullMessageProcessor) {
   net::CompletionCallback read_packet_cb;
   EXPECT_CALL(reader_, ReadPacket(NotNull(), _))
       .WillOnce(DoAll(FillBufferFromMessage<0>(message1_.get()),
-                      SetBufferOffset<0>(message1_->ByteSize()),
                       SaveArg<1>(&read_packet_cb)))
       .RetiresOnSaturation();
 
@@ -160,7 +153,7 @@ TEST_F(BlimpMessagePumpTest, NullMessageProcessor) {
   // Set the outgoing processor to start the MessagePump.
   message_pump_->SetMessageProcessor(&receiver_);
   ASSERT_FALSE(read_packet_cb.is_null());
-  base::ResetAndReturn(&read_packet_cb).Run(net::OK);
+  base::ResetAndReturn(&read_packet_cb).Run(message1_->ByteSize());
   process_msg_cb.Run(net::OK);
   // Running |process_msg_cb| should NOT trigger another ReadPacket call.
 }
diff --git a/blimp/net/packet_reader.h b/blimp/net/packet_reader.h
index c000ab0..543351a 100644
--- a/blimp/net/packet_reader.h
+++ b/blimp/net/packet_reader.h
@@ -18,9 +18,9 @@ class BLIMP_NET_EXPORT PacketReader {
   virtual ~PacketReader() {}
 
   // Reads a packet from the connection.
-  // Passes net::OK to |cb| if the read operation executed successfully.
-  // Sets |buf.offset()| to the received message's size, and invokes |cb| with
-  // net::OK result on success.
+  // Passes the packet size to |cb| if the read operation executed
+  // successfully.
+  // Passes the error code to |cb| if an error occurred.
   // All other values indicate errors.
   // |callback| will not be invoked if |this| is deleted.
   virtual void ReadPacket(const scoped_refptr<net::GrowableIOBuffer>& buf,
diff --git a/blimp/net/packet_writer.h b/blimp/net/packet_writer.h
index 8c7b9ca..7f9b18b 100644
--- a/blimp/net/packet_writer.h
+++ b/blimp/net/packet_writer.h
@@ -24,7 +24,7 @@ class BLIMP_NET_EXPORT PacketWriter {
   // Invokes |cb| with net::OK if the write operation executed successfully.
   // All other values indicate unrecoverable errors.
   // |callback| must not be invoked if |this| is deleted.
-  virtual void WritePacket(scoped_refptr<net::DrainableIOBuffer> data,
+  virtual void WritePacket(const scoped_refptr<net::DrainableIOBuffer>& data,
                            const net::CompletionCallback& callback) = 0;
 };
 
diff --git a/blimp/net/stream_packet_reader.cc b/blimp/net/stream_packet_reader.cc
index 00e0c3e..da65ad9 100644
--- a/blimp/net/stream_packet_reader.cc
+++ b/blimp/net/stream_packet_reader.cc
@@ -61,8 +61,9 @@ void StreamPacketReader::ReadPacket(
     payload_buffer_ = nullptr;
 
     // Adapt synchronous completion to an asynchronous style.
-    base::MessageLoop::current()->PostTask(FROM_HERE,
-                                           base::Bind(callback, result));
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, result == net::OK ? payload_size_ : result));
   } else {
     callback_ = callback;
   }
@@ -136,7 +137,8 @@ int StreamPacketReader::DoReadPayload(int result) {
 
   // Finished reading the payload.
   read_state_ = ReadState::IDLE;
-  return net::OK;
+  payload_buffer_->set_offset(0);
+  return payload_size_;
 }
 
 void StreamPacketReader::OnReadComplete(int result) {
@@ -157,7 +159,8 @@ void StreamPacketReader::OnReadComplete(int result) {
   // caller.
   if (result != net::ERR_IO_PENDING) {
     payload_buffer_ = nullptr;
-    base::ResetAndReturn(&callback_).Run(result);
+    base::ResetAndReturn(&callback_)
+        .Run(result == net::OK ? payload_size_ : result);
   }
 }
 
diff --git a/blimp/net/stream_packet_reader_unittest.cc b/blimp/net/stream_packet_reader_unittest.cc
index 5777d4d..d8e5d59 100644
--- a/blimp/net/stream_packet_reader_unittest.cc
+++ b/blimp/net/stream_packet_reader_unittest.cc
@@ -68,8 +68,7 @@ TEST_F(StreamPacketReaderTest, ReadAsyncHeaderAsyncPayload) {
   ReadPacket();
   socket_cb.Run(kPacketHeaderSizeBytes);
   socket_cb.Run(test_msg_.size());
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
 }
 
@@ -92,8 +91,7 @@ TEST_F(StreamPacketReaderTest, ReadAsyncHeaderSyncPayload) {
   EXPECT_FALSE(callback_.have_result());
 
   socket_cb.Run(kPacketHeaderSizeBytes);
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
 }
 
@@ -110,8 +108,7 @@ TEST_F(StreamPacketReaderTest, ReadSyncHeaderAsyncPayload) {
 
   ReadPacket();
   socket_cb.Run(test_msg_.size());
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
 }
 
@@ -127,8 +124,7 @@ TEST_F(StreamPacketReaderTest, ReadSyncHeaderSyncPayload) {
           DoAll(FillBufferFromString<0>(test_msg_), Return(test_msg_.size())));
 
   ReadPacket();
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
 }
 
@@ -163,12 +159,10 @@ TEST_F(StreamPacketReaderTest, ReadMultipleMessagesSync) {
       .RetiresOnSaturation();
 
   ReadPacket();
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
 
   ReadPacket();
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg2.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg2.size()), callback_.WaitForResult());
 
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
   EXPECT_FALSE(callback_.have_result());
@@ -208,14 +202,12 @@ TEST_F(StreamPacketReaderTest, ReadMultipleMessagesAsync) {
   data_reader_.ReadPacket(buffer_, read_cb1.callback());
   socket_cb.Run(kPacketHeaderSizeBytes);
   socket_cb.Run(test_msg_.size());
-  EXPECT_EQ(net::OK, read_cb1.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), read_cb1.WaitForResult());
 
   data_reader_.ReadPacket(buffer_, read_cb2.callback());
   socket_cb.Run(kPacketHeaderSizeBytes);
   socket_cb.Run(test_msg_.size());
-  EXPECT_EQ(net::OK, read_cb2.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), read_cb2.WaitForResult());
   EXPECT_TRUE(BufferStartsWith(buffer_.get(), test_msg_));
 }
 
@@ -265,8 +257,7 @@ TEST_F(StreamPacketReaderTest, PartialPayloadReadAsync) {
 
   cb.Run(1);
   cb.Run(test_msg_.size() - 1);
-  EXPECT_EQ(net::OK, callback_.WaitForResult());
-  EXPECT_EQ(static_cast<int>(test_msg_.size()), buffer_->offset());
+  EXPECT_EQ(static_cast<int>(test_msg_.size()), callback_.WaitForResult());
 }
 
 // Verify that synchronous header read errors are reported correctly.
diff --git a/blimp/net/stream_packet_writer.cc b/blimp/net/stream_packet_writer.cc
index 0c01d76..7fe00d2 100644
--- a/blimp/net/stream_packet_writer.cc
+++ b/blimp/net/stream_packet_writer.cc
@@ -47,8 +47,9 @@ StreamPacketWriter::StreamPacketWriter(net::StreamSocket* socket)
 
 StreamPacketWriter::~StreamPacketWriter() {}
 
-void StreamPacketWriter::WritePacket(scoped_refptr<net::DrainableIOBuffer> data,
-                                     const net::CompletionCallback& callback) {
+void StreamPacketWriter::WritePacket(
+    const scoped_refptr<net::DrainableIOBuffer>& data,
+    const net::CompletionCallback& callback) {
   DCHECK_EQ(WriteState::IDLE, write_state_);
   DCHECK(data);
   CHECK(data->BytesRemaining());
diff --git a/blimp/net/stream_packet_writer.h b/blimp/net/stream_packet_writer.h
index 071a8f0..a13711b 100644
--- a/blimp/net/stream_packet_writer.h
+++ b/blimp/net/stream_packet_writer.h
@@ -35,7 +35,7 @@ class BLIMP_NET_EXPORT StreamPacketWriter : public PacketWriter {
   ~StreamPacketWriter() override;
 
   // PacketWriter implementation.
-  void WritePacket(scoped_refptr<net::DrainableIOBuffer> data,
+  void WritePacket(const scoped_refptr<net::DrainableIOBuffer>& data,
                    const net::CompletionCallback& callback) override;
 
  private:
diff --git a/blimp/net/tcp_transport_unittest.cc b/blimp/net/tcp_transport_unittest.cc
index 8f1525a..c5916c0 100644
--- a/blimp/net/tcp_transport_unittest.cc
+++ b/blimp/net/tcp_transport_unittest.cc
@@ -98,6 +98,7 @@ TEST_F(TCPTransportTest, ExchangeMessages) {
   net::CompletionCallback engine_process_message_cb;
   scoped_ptr<BlimpMessage> client_message1 =
       CreateStartConnectionMessage("", 0);
+  int client_message1_size = client_message1->ByteSize();
   scoped_ptr<BlimpMessage> client_message2 = CreateCheckpointAckMessage(5);
   scoped_ptr<BlimpMessage> engine_message = CreateCheckpointAckMessage(10);
   EXPECT_CALL(engine_incoming_processor,
@@ -124,7 +125,7 @@ TEST_F(TCPTransportTest, ExchangeMessages) {
 
   // Engine finishes processing the client message.
   EXPECT_FALSE(engine_process_message_cb.is_null());
-  engine_process_message_cb.Run(net::OK);
+  engine_process_message_cb.Run(client_message1_size);
 
   // Engine sends one message.
   net::TestCompletionCallback engine_send_callback;
diff --git a/blimp/net/test_common.h b/blimp/net/test_common.h
index 380465e..e86944b 100644
--- a/blimp/net/test_common.h
+++ b/blimp/net/test_common.h
@@ -84,7 +84,6 @@ ACTION_TEMPLATE(FillBufferFromMessage,
                 AND_1_VALUE_PARAMS(message)) {
   message->SerializeToArray(testing::get<buf_idx>(args)->data(),
                             message->ByteSize());
-  testing::get<buf_idx>(args)->set_offset(message->ByteSize());
 }
 
 // Calls |set_offset()| for a GrowableIOBuffer.
@@ -165,7 +164,7 @@ class MockPacketWriter : public PacketWriter {
   ~MockPacketWriter() override;
 
   MOCK_METHOD2(WritePacket,
-               void(scoped_refptr<net::DrainableIOBuffer>,
+               void(const scoped_refptr<net::DrainableIOBuffer>&,
                     const net::CompletionCallback&));
 };