Remote Assistance on Chrome OS Part II - Native Messaging renaming

Remote Assistance on Chrome OS Part II - Native Messaging renaming

This CL lays the ground work for implementing in process native messaging.

It extracts a common interface |NativeMessagingChannel| outs of
native_messaging_channel.h and renames the existing implementation
to pipe_messaging_channel.

This CL is best reviewed by diffing against patch set 1.

Committed: https://crrev.com/3a76af7b69d0b8b8cfe09a6fe264bde945364ab6
Cr-Commit-Position: refs/heads/master@{#295851}

[kelvinp, 2014-09-12 01:52:10]

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

[kelvinp, 2014-09-12 01:56:55]

PTAL

[weitao, 2014-09-12 16:37:35]

weitaosu@chromium.org changed reviewers:
+ weitaosu@chromium.org

[weitao, 2014-09-12 16:37:36]

https://codereview.chromium.org/558403002/diff/20001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/20001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_native_messaging_host.h:17: class File;
Why is this needed?

[kelvinp, 2014-09-12 17:58:32]

PTAL

https://codereview.chromium.org/558403002/diff/20001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/20001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_native_messaging_host.h:17: class File;
On 2014/09/12 16:37:36, weitaosu wrote:
> Why is this needed?

Done.

[Sergey Ulanov, 2014-09-15 19:53:39]

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
I think it's better to add this file under extensions/browser/api/messaging (and
ideally all native messaging code should be moved there)

Otherwise we would need to have circular dependencies between src/chrome and
src/remoting: chrome will depend on remoting (for the It2me host) and remoting
will depend on chrome for the interface.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:21: typedef
base::Callback<void(scoped_ptr<base::DictionaryValue> message)>
this should be base::Value instead of DictionaryValue. In remoting we send only
dictionaries, but NM allows sending any JSON values, not only dictionaries.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:31: virtual
void Start(const SendMessageCallback& received_message,
I suggest you replace these two callbacks with a callback interface.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:35: virtual
void SendMessage(scoped_ptr<base::DictionaryValue> message) = 0;
base::Value

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
File remoting/host/native_messaging/pipe_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:15: 
remove this empty line.

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:29: class
PipeMessagingChannel :
This class can also be used in chrome. We were previously not sharing any code
related to native messaging between chrome and remoting, and now we do
(NativeMessagingChannel interface). So it's probably better to move this class
to extensions/browser/api/messaging so it can be reused in chrome later

If you feel that moving it would take much time and then at least add a TODO
here.

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:30: public
extensions::NativeMessagingChannel,
indentation. : should be on this line.

[kelvinp, 2014-09-16 00:32:51]

PTAL

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> I think it's better to add this file under extensions/browser/api/messaging
(and
> ideally all native messaging code should be moved there)
> 
> Otherwise we would need to have circular dependencies between src/chrome and
> src/remoting: chrome will depend on remoting (for the It2me host) and remoting
> will depend on chrome for the interface.

Done.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:21: typedef
base::Callback<void(scoped_ptr<base::DictionaryValue> message)>
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> this should be base::Value instead of DictionaryValue. In remoting we send
only
> dictionaries, but NM allows sending any JSON values, not only dictionaries.

Done.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:31: virtual
void Start(const SendMessageCallback& received_message,
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> I suggest you replace these two callbacks with a callback interface.

Done.

https://codereview.chromium.org/558403002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_channel.h:35: virtual
void SendMessage(scoped_ptr<base::DictionaryValue> message) = 0;
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> base::Value

Done.

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
File remoting/host/native_messaging/pipe_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:15: 
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> remove this empty line.

Done.

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:29: class
PipeMessagingChannel :
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> This class can also be used in chrome. We were previously not sharing any code
> related to native messaging between chrome and remoting, and now we do
> (NativeMessagingChannel interface). So it's probably better to move this class
> to extensions/browser/api/messaging so it can be reused in chrome later
> 
> If you feel that moving it would take much time and then at least add a TODO
> here.

Todo statement added.

https://codereview.chromium.org/558403002/diff/40001/remoting/host/native_mes...
remoting/host/native_messaging/pipe_messaging_channel.h:30: public
extensions::NativeMessagingChannel,
On 2014/09/15 19:53:39, Sergey Ulanov wrote:
> indentation. : should be on this line.

Done.

[kelvinp, 2014-09-16 01:16:40]

Patchset #4 (id:60001) has been deleted

[kelvinp, 2014-09-16 02:22:02]

Patchset #4 (id:80001) has been deleted

[kelvinp, 2014-09-16 23:50:40]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:52:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/558403002/130001

[commit-bot: I haz the power, 2014-09-16 23:52:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:52:57]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sergey Ulanov, 2014-09-17 02:32:23]

Let's talk tomorrow about overall design for how the components should fit
together.

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
File extensions/browser/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:16: // Defines an
interface to read messages and send responses between a native
It's not clear which side of the channel is supposed to implement this
interface. Will it be used on both ends (then you need a connector) or is going
to be implemented by the chrome's side and passed to the host object?

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:20: // Callback
interface for the channel. |EventHandler| must be valid as long as
Normally |pipes| are not used for type names, only for variables.

https://codereview.chromium.org/558403002/diff/130001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/130001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:22: class
It2MeNativeMessagingHost
I'm not sure I understand how this will work in chrome. 

Does NativeMessagingChannel represent the chrome's end of the channel or the
host?

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
File remoting/host/native_messaging/pipe_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
year

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:25: // An implementation
of |extensions::NativeMessagingChannel| using a pipe. It
don't need | around type names

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:48: // Initiates
shutdown and runs |quit_closure| if there are no pending requests
there is no |quit_closure| anymore

[kelvinp, 2014-09-17 23:06:17]

PTAL

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
File extensions/browser/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:16: // Defines an
interface to read messages and send responses between a native
On 2014/09/17 02:32:22, Sergey Ulanov wrote:
> It's not clear which side of the channel is supposed to implement this
> interface. Will it be used on both ends (then you need a connector) or is
going
> to be implemented by the chrome's side and passed to the host object?

The native_messaging_channel is simply an object that supports sending and
receiving messages.  Both chrome and native messaging side shares the same
interface.  We can distinguish the side of the communication via variable
naming.  For instance, on the Chrome side, we can name the object as
native_channel, so that we know native_channel->Send will send message to the
native component.

https://codereview.chromium.org/558403002/diff/130001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:20: // Callback
interface for the channel. |EventHandler| must be valid as long as
On 2014/09/17 02:32:22, Sergey Ulanov wrote:
> Normally |pipes| are not used for type names, only for variables.

Done.

https://codereview.chromium.org/558403002/diff/130001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/130001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:22: class
It2MeNativeMessagingHost
On 2014/09/17 02:32:22, Sergey Ulanov wrote:
> I'm not sure I understand how this will work in chrome. 
> 
> Does NativeMessagingChannel represent the chrome's end of the channel or the
> host?

Neither, the Channel only representing the sending end and channel::Subscriber
representing the receiving end.  In most cases,
NativeMessageChannel->SendMessage, sends a message to the native component and
NativeMessageChannel::Subscriber receives message from the native component.

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
File remoting/host/native_messaging/pipe_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/09/17 02:32:23, Sergey Ulanov wrote:
> year

Done.

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:25: // An implementation
of |extensions::NativeMessagingChannel| using a pipe. It
On 2014/09/17 02:32:23, Sergey Ulanov wrote:
> don't need | around type names

Done.

https://codereview.chromium.org/558403002/diff/130001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.h:48: // Initiates
shutdown and runs |quit_closure| if there are no pending requests
On 2014/09/17 02:32:23, Sergey Ulanov wrote:
> there is no |quit_closure| anymore

Done.

[kelvinp, 2014-09-18 01:44:13]

Patchset #8 (id:170001) has been deleted

[kelvinp, 2014-09-18 03:04:37]

Patchset #8 (id:190001) has been deleted

[Sergey Ulanov, 2014-09-18 18:08:36]

Some nits about style and comments

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
File extensions/browser/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:16: // Defines an
interface to read messages and send responses between a native
nit: "Defines an " is redundant. Just "Interface ...".
Also it can be used to send any messages, not just responses. I suggest this
wording: "Interface for sending and receiving messages on
NativeMessagingChannel. Channel is disconnected when NativeMessagingChannel is
destroyed."

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:21: //
NativeMessagingChannel is valid.
"valid" is ambiguous. Suggest rewording second sentence "EventHandler must
outlive NativeMessagingChannel."

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:27: // Invoked when
there is an error in sending or receiving a message.
Suggest "Called when the channel is disconnected."

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:28: // The event
handler should shut down the channel on this event.
It's not clear what this means. Does it mean that the handler has to delete the
calling NativeMessagingChannel? Maybe "EventHandler is guaranteed not to called
after OnDisconnect()."?

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:120: if
(!quit_closure_.is_null()) {
nit: remove {} for consistency with other single-line ifs in this file

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:34: //
extensions::NativeMessagingChannel::EventHandler implementation
nit: add . at the end of the comment

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_main.cc:121:
scoped_ptr<PipeMessagingChannel> channel(
nit: define this as scoped_ptr<extensions::NativeMessagingChannel> then you
wouldn't need PassAs<>() below

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:434:
scoped_ptr<PipeMessagingChannel> channel(new PipeMessagingChannel(
scoped_ptr<extensions::NativeMessagingChannel>

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
File remoting/host/native_messaging/pipe_messaging_channel.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.cc:62: DCHECK(NULL ==
event_handler_);
DCHECK(!event_handler_);

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.cc:81: if (event_handler_)
{
remove {}

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:163: if
(!quit_closure_.is_null()) {
remove {}

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:689:
static_cast<extensions::NativeMessagingChannel*>(channel.release()));
Why static_cast<> instead of PassAs<>? But I think you don't need |channel| at
all. Just reset elevated_channel_ with the new PipeMessagingChannel().

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.h:14: 
remove this empty line

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.h:158:
scoped_ptr<ElevatedChannelEventHandler> elevated_channel_event_handler_;
I think this doesn't need to be a pointer.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host_main.cc:244:
scoped_ptr<PipeMessagingChannel> channel(
scoped_ptr<extensions::NativeMessagingChannel> and don't need PassAs below.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host_unittest.cc:326:
scoped_ptr<PipeMessagingChannel> channel(
scoped_ptr<extensions::NativeMessagingChannel>

[kelvinp, 2014-09-18 19:03:51]

PTAL

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
File extensions/browser/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:16: // Defines an
interface to read messages and send responses between a native
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> nit: "Defines an " is redundant. Just "Interface ...".
> Also it can be used to send any messages, not just responses. I suggest this
> wording: "Interface for sending and receiving messages on
> NativeMessagingChannel. Channel is disconnected when NativeMessagingChannel is
> destroyed." 

Done.

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:21: //
NativeMessagingChannel is valid.
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> "valid" is ambiguous. Suggest rewording second sentence "EventHandler must
> outlive NativeMessagingChannel."

Done.

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:27: // Invoked when
there is an error in sending or receiving a message.
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> Suggest "Called when the channel is disconnected."

Done.

https://codereview.chromium.org/558403002/diff/210001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:28: // The event
handler should shut down the channel on this event.
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> It's not clear what this means. Does it mean that the handler has to delete
the
> calling NativeMessagingChannel? Maybe "EventHandler is guaranteed not to
called
> after OnDisconnect()."?

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:120: if
(!quit_closure_.is_null()) {
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> nit: remove {} for consistency with other single-line ifs in this file

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:34: //
extensions::NativeMessagingChannel::EventHandler implementation
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> nit: add . at the end of the comment

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_main.cc:121:
scoped_ptr<PipeMessagingChannel> channel(
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> nit: define this as scoped_ptr<extensions::NativeMessagingChannel> then you
> wouldn't need PassAs<>() below

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:434:
scoped_ptr<PipeMessagingChannel> channel(new PipeMessagingChannel(
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> scoped_ptr<extensions::NativeMessagingChannel>

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
File remoting/host/native_messaging/pipe_messaging_channel.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.cc:62: DCHECK(NULL ==
event_handler_);
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> DCHECK(!event_handler_);

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/native_me...
remoting/host/native_messaging/pipe_messaging_channel.cc:81: if (event_handler_)
{
On 2014/09/18 18:08:35, Sergey Ulanov wrote:
> remove {}

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:163: if
(!quit_closure_.is_null()) {
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> remove {}

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:689:
static_cast<extensions::NativeMessagingChannel*>(channel.release()));
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> Why static_cast<> instead of PassAs<>? But I think you don't need |channel| at
> all. Just reset elevated_channel_ with the new PipeMessagingChannel().

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.h (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.h:14: 
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> remove this empty line

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.h:158:
scoped_ptr<ElevatedChannelEventHandler> elevated_channel_event_handler_;
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> I think this doesn't need to be a pointer.

|elevated_channel_event_handler| and Me2MeNativeMessaginHost has a different
slightly lifetime.  |elevated_channel_event_handler| is   constructed lazily
when elevation is required, with the stdio of the elevated child process as the
parameters.  As a result, it cannot be declared by a class member variable as it
won't be initialized in the constructor of Me2MeNativeMessaginHost.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host_main.cc:244:
scoped_ptr<PipeMessagingChannel> channel(
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> scoped_ptr<extensions::NativeMessagingChannel> and don't need PassAs below.

Done.

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/558403002/diff/210001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host_unittest.cc:326:
scoped_ptr<PipeMessagingChannel> channel(
On 2014/09/18 18:08:36, Sergey Ulanov wrote:
> scoped_ptr<extensions::NativeMessagingChannel>

Done.

[kelvinp, 2014-09-18 20:06:27]

Patchset #9 (id:230001) has been deleted

[Sergey Ulanov, 2014-09-19 21:22:48]

lgtm when my nits are addressed

https://codereview.chromium.org/558403002/diff/270001/extensions/browser/api/...
File extensions/browser/api/messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/558403002/diff/270001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:16: // An interface
to read messages and send responses between a native
s/read messages and send responses/receive and send messages/

https://codereview.chromium.org/558403002/diff/270001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:21: //
NativeMessagingChannel
add . at the end.

https://codereview.chromium.org/558403002/diff/270001/extensions/browser/api/...
extensions/browser/api/messaging/native_messaging_channel.h:28: // EventHandler
is guaranteed not to be called after OnDisconnect()..
remove extra dot at the end.

[kelvinp, 2014-09-19 21:33:27]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:34:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/558403002/290001

[commit-bot: I haz the power, 2014-09-19 23:04:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:04:51]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[commit-bot: I haz the power, 2014-09-19 23:05:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:05:56]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[kelvinp, 2014-09-19 23:21:12]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:23:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/558403002/290001

[commit-bot: I haz the power, 2014-09-19 23:40:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:41:00]

Failed to commit the patch.

[commit-bot: I haz the power, 2014-09-19 23:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:41:06]

Failed to apply the patch.

[kelvinp, 2014-09-19 23:45:43]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:46:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/558403002/310001

[commit-bot: I haz the power, 2014-09-20 02:45:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-20 02:45:27]

Failed to apply the patch.

[commit-bot: I haz the power, 2014-09-20 02:45:38]

Committed patchset #12 (id:310001) as 0d8743b898758b72e3bfbf38d35ee3f04d62cb4d

[commit-bot: I haz the power, 2014-09-20 02:46:07]

Patchset 12 (id:??) landed as
https://crrev.com/3a76af7b69d0b8b8cfe09a6fe264bde945364ab6
Cr-Commit-Position: refs/heads/master@{#295851}