Adding the message writing class used for remote_security_key STDOUT communication.

remoting/host/security_key/remote_security_key_message_writer.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/security_key/remote_security_key_message_writer.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <string>
+#include <utility>
+
+#include "remoting/host/security_key/security_key_message.h"
+
+namespace remoting {
+
+RemoteSecurityKeyMessageWriter::RemoteSecurityKeyMessageWriter(
+    base::File output_file)
+    : output_stream_(std::move(output_file)) {}
+
+RemoteSecurityKeyMessageWriter::~RemoteSecurityKeyMessageWriter() {}
+
+bool RemoteSecurityKeyMessageWriter::WriteMessage(

[Sergey Ulanov, 2016/03/28 18:20:07]

Maybe call this WriteEmptyMessage()? Style guide is iffy about function
overloading:
https://google.github.io/styleguide/cppguide.html#Function_Overloading

[joedow, 2016/03/29 22:14:14]

Done.

+    RemoteSecurityKeyMessageType message_type) {
+  return WriteMessage(message_type, std::string());
+}
+
+bool RemoteSecurityKeyMessageWriter::WriteMessage(
+    RemoteSecurityKeyMessageType message_type,
+    const std::string& message_payload) {
+  if (write_failed_) {
+    return false;
+  }
+
+  if (!output_stream_.IsValid()) {

[Sergey Ulanov, 2016/03/28 18:20:07]

Why would it be invalid here? I think you can just DCHECK in the constructor
that the output_file is valid.

[joedow, 2016/03/29 22:14:14]

The file representing output_stream could be valid when the object is
constructed and subsequently closed (due to an error for instance).  I'm trying
to detect the condition and fail gracefully in that case.

+    LOG(ERROR) << "Cannot write to invalid stream.";
+    write_failed_ = true;
+    return false;
+  }
+
+  uint32_t message_payload_size_bytes = message_payload.size();
+  uint32_t total_message_size_bytes =
+      sizeof(message_type) + message_payload_size_bytes;

[Sergey Ulanov, 2016/03/28 18:20:07]

In remoting/host/security_key/security_key_message.cc you have
kRemoteSecurityKeyMessageControlCodeByteCount and here you use
sizeof(message_type). Please be consistent either use sizeof() in both places or
share a constant.

[joedow, 2016/03/29 22:14:14]

Done.

+  CHECK(SecurityKeyMessage::IsValidMessageSize(total_message_size_bytes));
+
+  // First we send the message header which is the length of the message_type
+  // and message_payload in bytes.
+  int result = output_stream_.WriteAtCurrentPos(
+      reinterpret_cast<char*>(&total_message_size_bytes),
+      SecurityKeyMessage::kHeaderSizeBytes);
+  if (result != SecurityKeyMessage::kHeaderSizeBytes) {
+    LOG(ERROR) << "Failed to send message header: " << result;

[Sergey Ulanov, 2016/03/28 18:20:07]

result will be -1 on error. Print error_details() too?

[joedow, 2016/03/29 22:14:14]

Done.

+    write_failed_ = true;
+    return false;
+  }
+
+  // Next we send the message_type.
+  result = output_stream_.WriteAtCurrentPos(

[Sergey Ulanov, 2016/03/28 18:20:08]

In this function WriteAtCurrentPos is called in 3 places and the errors handled
the same way every time. It might be better to assemble the message in memory
and write it whole after that. It may make this code shorter and it would be
slightly more efficient (though that doesn't matter here).

[joedow, 2016/03/29 22:14:14]

Done.

+      reinterpret_cast<char*>(&message_type), sizeof(message_type));
+  if (result != sizeof(message_type)) {
+    LOG(ERROR) << "Failed to send message type: " << result;
+    write_failed_ = true;
+    return false;
+  }
+
+  // Lastly, send the message data if appropriate.
+  if (!message_payload.empty()) {
+    result = output_stream_.WriteAtCurrentPos(message_payload.data(),
+                                              message_payload_size_bytes);
+    // The static cast is safe as we know the value is smaller than max int.
+    if (result != static_cast<int>(message_payload_size_bytes)) {

[Sergey Ulanov, 2016/03/28 18:20:07]

nit: You wouldn't need this static_cast<> if message_payload_size_bytes was int.

[joedow, 2016/03/29 22:14:14]

Done.

+      LOG(ERROR) << "Failed to send message payload:" << result;
+      write_failed_ = true;
+      return false;
+    }
+  }
+
+  return true;
+}
+
+}  // namespace remoting