Use ConnectionErrorObserver, not callbacks, for error handling.

blimp/net/blimp_message_output_buffer.cc

Index: blimp/net/blimp_message_output_buffer.cc
diff --git a/blimp/net/blimp_message_output_buffer.cc b/blimp/net/blimp_message_output_buffer.cc
index b5c5ec5612b0f8c0a45fe4860160369540325fda..0ad4a126a6b79a16f8f301cad97a8790452b18c1 100644
--- a/blimp/net/blimp_message_output_buffer.cc
+++ b/blimp/net/blimp_message_output_buffer.cc
@@ -144,18 +144,11 @@ void BlimpMessageOutputBuffer::WriteNextMessageIfReady() {
 }
 
 void BlimpMessageOutputBuffer::OnWriteComplete(int result) {
-  DCHECK_LE(result, net::OK);
-  VLOG(2) << "Write complete, result=" << result;
-
-  if (result == net::OK) {
-    ack_buffer_.push_back(std::move(write_buffer_.front()));
-    write_buffer_.pop_front();
-    WriteNextMessageIfReady();
-  } else {
-    // An error occurred while writing to the network connection.
-    // Stop writing more messages until a new connection is established.
-    DLOG(WARNING) << "Write error (result=" << result << ")";
-  }
+  DCHECK_EQ(result, net::OK);

[Wez, 2016/04/21 22:44:29]

It seems that you're changing the MessageProcessor contract such that processing
a message can never fail, so why do we still have |result| in the callback
signature?

nit: DCHECK_EQ(expected, actual) is equivalent but preferred, since it matches
the gmock EXPECT_EQ() required ordering.

nit: Functions like this are easier to read if you use blank lines to e.g.
separate the pre-condition checks (this DCHECK) from the body, and generally to
break up the body into logically blocks.

[Kevin M, 2016/04/22 22:10:41]

Done.

+  VLOG(2) << "Write complete.";

[Wez, 2016/04/21 22:44:29]

nit: None of our other log messages, above, seem to have terminating periods.

[Kevin M, 2016/04/22 22:10:41]

Done.

+  ack_buffer_.push_back(std::move(write_buffer_.front()));
+  write_buffer_.pop_front();
+  WriteNextMessageIfReady();
 }
 
 }  // namespace blimp