Adding the message reading class used for remote_security_key STDIN communication.

Adding the message reading class used for remote_security_key STDIN communication.

This class is used to read remote security key messages via the STDIN pipe.
This CL does not include its usage to reduce the overall CR size as that will
occur in a future CL.

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

[joedow, 2016-03-24 21:48:05]

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

[joedow, 2016-03-24 21:48:06]

PTAL!

[Sergey Ulanov, 2016-03-28 18:52:46]

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

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:139: }
Thread destructor will join the thread and it will block while the thread is
still running ReadMessage(), which may be forever. Native Messaging works the
same, but it may be less of an issue there because chrome normally closes the
pipe when NM host should be stopped and it's the correct behavior there is
arguable, see
https://chromiumcodereview.appspot.com/12852005/diff2/67001:95001/remoting/ho...
Are the requirements different here? Do we want to be able to stop the process
while the pipe is still open?

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader.h (right):

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.h:8: #include
<stdint.h>
don't need this include

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.h:49:
scoped_ptr<Core> core_;
This class owns the target thread, so you don't really need the core class here.

[joedow, 2016-03-30 19:39:00]

Addressing feedback and integrating security message changes.

PTAL!

Thanks,
Joe

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

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:139: }
On 2016/03/28 18:52:45, Sergey Ulanov wrote:
> Thread destructor will join the thread and it will block while the thread is
> still running ReadMessage(), which may be forever. Native Messaging works the
> same, but it may be less of an issue there because chrome normally closes the
> pipe when NM host should be stopped and it's the correct behavior there is
> arguable, see
>
https://chromiumcodereview.appspot.com/12852005/diff2/67001:95001/remoting/ho...
> Are the requirements different here? Do we want to be able to stop the process
> while the pipe is still open?

I don't think I have the same problem here.  I close the read_stream_ handle in
the D'tor which breaks the ReadMessage() loop on the reader thread.  Does that
alleviate the concern or is there another scenario I'm missing?  I've added a
test to make sure this scenario works correctly and does not regress.

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader.h (right):

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.h:8: #include
<stdint.h>
On 2016/03/28 18:52:46, Sergey Ulanov wrote:
> don't need this include

Done.

https://codereview.chromium.org/1818233005/diff/20001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.h:49:
scoped_ptr<Core> core_;
On 2016/03/28 18:52:46, Sergey Ulanov wrote:
> This class owns the target thread, so you don't really need the core class
here.

Done.

[Sergey Ulanov, 2016-03-30 21:34:28]

lgtm

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader.cc (right):

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:16: #include
"base/stl_util.h"
Don't need this

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:18: #include
"base/threading/thread_checker.h"
Don't need this include.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:34: reader_ =
weak_factory_.GetWeakPtr();
I don't think you really need reader_. Just use weak_factory_.GetWeakPtr() when
posting tasks from read_thread_ to main_task_runner_.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:43:
reader_thread_.Stop();
nit: Thread destructor calls stop, so you don't need to call it explicitly here,
but feel free to keep it here if you think it makes the code clearer.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_unittest.cc
(right):

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_unittest.cc:195:
for (size_t i = 0; i < payloads.size(); i++) {
for (auto& payload : payloads)

[joedow, 2016-03-31 01:45:13]

Thanks!

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader.cc (right):

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:16: #include
"base/stl_util.h"
On 2016/03/30 21:34:28, Sergey Ulanov wrote:
> Don't need this

The 'string_as_array' function I use below is defined there.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:18: #include
"base/threading/thread_checker.h"
On 2016/03/30 21:34:28, Sergey Ulanov wrote:
> Don't need this include.

Done.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:34: reader_ =
weak_factory_.GetWeakPtr();
On 2016/03/30 21:34:28, Sergey Ulanov wrote:
> I don't think you really need reader_. Just use weak_factory_.GetWeakPtr()
when
> posting tasks from read_thread_ to main_task_runner_.

Done.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader.cc:43:
reader_thread_.Stop();
On 2016/03/30 21:34:28, Sergey Ulanov wrote:
> nit: Thread destructor calls stop, so you don't need to call it explicitly
here,
> but feel free to keep it here if you think it makes the code clearer.

Acknowledged.

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
File remoting/host/security_key/remote_security_key_message_reader_unittest.cc
(right):

https://codereview.chromium.org/1818233005/diff/80001/remoting/host/security_...
remoting/host/security_key/remote_security_key_message_reader_unittest.cc:195:
for (size_t i = 0; i < payloads.size(); i++) {
On 2016/03/30 21:34:28, Sergey Ulanov wrote:
> for (auto& payload : payloads)

Done.

[joedow, 2016-03-31 01:45:20]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-03-31 01:45:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1818233005/#ps100001
(title: "Addressing feedback")

[commit-bot: I haz the power, 2016-03-31 01:45:37]

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

[commit-bot: I haz the power, 2016-03-31 02:00:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-31 02:01:37]

Description was changed from

==========
Adding the message reading class used for remote_security_key STDIN
communication.

This class is used to read remote security key messages via the STDIN pipe.
This CL does not include its usage to reduce the overall CR size as that will
occur in a future CL.

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

to

==========
Adding the message reading class used for remote_security_key STDIN
communication.

This class is used to read remote security key messages via the STDIN pipe.
This CL does not include its usage to reduce the overall CR size as that will
occur in a future CL.

BUG=584463

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

[commit-bot: I haz the power, 2016-03-31 02:01:39]

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