Adding the message writing class used for remote_security_key STDOUT communication.

Adding the message writing class used for remote_security_key STDOUT communication.

This class is used to write properly formatted remote security key messages
to STDOUT.  This CL does not include its usage to reduce the overall CR size.
That will be done in a future CL.

BUG=584463
Committed: https://crrev.com/f11e663b69324c70fdab07a30f7fe556833aa968
Cr-Commit-Position: refs/heads/master@{#384143}

[joedow, 2016-03-24 21:47:51]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2016-03-24 21:47:51]

PTAL!

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

lgtm when my nits are addressed

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_writer.cc (right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:23: bool
RemoteSecurityKeyMessageWriter::WriteMessage(
Maybe call this WriteEmptyMessage()? Style guide is iffy about function
overloading:
https://google.github.io/styleguide/cppguide.html#Function_Overloading

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:35: if
(!output_stream_.IsValid()) {
Why would it be invalid here? I think you can just DCHECK in the constructor
that the output_file is valid.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:43:
sizeof(message_type) + message_payload_size_bytes;
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.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:52: LOG(ERROR)
<< "Failed to send message header: " << result;
result will be -1 on error. Print error_details() too?

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:58: result =
output_stream_.WriteAtCurrentPos(
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).

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:71: if (result
!= static_cast<int>(message_payload_size_bytes)) {
nit: You wouldn't need this static_cast<> if message_payload_size_bytes was int.

[Sergey Ulanov, 2016-03-28 18:26:43]

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_writer_unittest.cc
(right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer_unittest.cc:48:
ASSERT_TRUE(MakePipe(&read_file_, &write_file_, kBufferSizeBytes));
Why do you need to set buffer size for this pipe?

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer_unittest.cc:115:
SecurityKeyMessage::kHeaderSizeBytes,
What if the message is larger than the buffer? I think the writer should still
be able to handle this case.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/setup/tes...
File remoting/host/setup/test_util.cc (right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/setup/tes...
remoting/host/setup/test_util.cc:23: unsigned int buffer_size_bytes) {
This parameter is windows-specific. I suggest wrapping this function in ifdef,
so that't it's not defined on other platforms. Also my comment about function
overloading applies here as well.

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

Addressed feedback and merged security message changes.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_writer.cc (right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:23: bool
RemoteSecurityKeyMessageWriter::WriteMessage(
On 2016/03/28 18:20:07, Sergey Ulanov wrote:
> Maybe call this WriteEmptyMessage()? Style guide is iffy about function
> overloading:
> https://google.github.io/styleguide/cppguide.html#Function_Overloading

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:35: if
(!output_stream_.IsValid()) {
On 2016/03/28 18:20:07, Sergey Ulanov wrote:
> Why would it be invalid here? I think you can just DCHECK in the constructor
> that the output_file is valid.

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.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:43:
sizeof(message_type) + message_payload_size_bytes;
On 2016/03/28 18:20:07, Sergey Ulanov wrote:
> 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.

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:52: LOG(ERROR)
<< "Failed to send message header: " << result;
On 2016/03/28 18:20:07, Sergey Ulanov wrote:
> result will be -1 on error. Print error_details() too?

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:58: result =
output_stream_.WriteAtCurrentPos(
On 2016/03/28 18:20:08, Sergey Ulanov wrote:
> 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).

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer.cc:71: if (result
!= static_cast<int>(message_payload_size_bytes)) {
On 2016/03/28 18:20:07, Sergey Ulanov wrote:
> nit: You wouldn't need this static_cast<> if message_payload_size_bytes was
int.

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_writer_unittest.cc
(right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer_unittest.cc:48:
ASSERT_TRUE(MakePipe(&read_file_, &write_file_, kBufferSizeBytes));
On 2016/03/28 18:26:43, Sergey Ulanov wrote:
> Why do you need to set buffer size for this pipe?

The short version is that the buffer size can vary and the writer blocks when
the pipe is full.  I initially wanted to avoid introducing a thread to use for
the read operation and a message loop/run loop mechanism for the writer, but it
seems like the better long term approach and is closer to how the writer will be
used IRL.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_writer_unittest.cc:115:
SecurityKeyMessage::kHeaderSizeBytes,
On 2016/03/28 18:26:43, Sergey Ulanov wrote:
> What if the message is larger than the buffer? I think the writer should still
> be able to handle this case.

Done.

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/setup/tes...
File remoting/host/setup/test_util.cc (right):

https://codereview.chromium.org/1830433002/diff/20001/remoting/host/setup/tes...
remoting/host/setup/test_util.cc:23: unsigned int buffer_size_bytes) {
On 2016/03/28 18:26:43, Sergey Ulanov wrote:
> This parameter is windows-specific. I suggest wrapping this function in ifdef,
> so that't it's not defined on other platforms. Also my comment about function
> overloading applies here as well.

Acknowledged.

[joedow, 2016-03-30 17:35:06]

FYI, addressed feedback and integrated security key message changes.  Although
this has been LGTM'd, please take another look if you are interested.

[Sergey Ulanov, 2016-03-30 21:23:56]

still lgtm

[joedow, 2016-03-30 23:13:27]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2016-03-30 23:14:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830433002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830433002/80001

[commit-bot: I haz the power, 2016-03-30 23:19:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 23:20:57]

Description was changed from

==========
Adding the message writing class used for remote_security_key STDOUT
communication.

This class is used to write properly formatted remote security key messages
to STDOUT.  This CL does not include its usage to reduce the overall CR size.
That will be done in a future CL.

BUG=584463
==========

to

==========
Adding the message writing class used for remote_security_key STDOUT
communication.

This class is used to write properly formatted remote security key messages
to STDOUT.  This CL does not include its usage to reduce the overall CR size.
That will be done in a future CL.

BUG=584463

Committed: https://crrev.com/f11e663b69324c70fdab07a30f7fe556833aa968
Cr-Commit-Position: refs/heads/master@{#384143}
==========

[commit-bot: I haz the power, 2016-03-30 23:20:58]

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