Remote Assistance on Chrome OS Part III - NativeMessageHost

Remote Assistance on Chrome OS Part III - NativeMessageHost

This CL extracts the NativeMessageHost interface from the NativeMessageProcessHost
A NativeMessageHost hosts a native component, which can be
running in the same process as chrome or in a separate
process.

Committed: https://crrev.com/ba722a2a8ddf967dcadb483ea32d5ad31fc4bdf6
Cr-Commit-Position: refs/heads/master@{#298366}

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

[kelvinp, 2014-09-19 20:48:21]

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

[kelvinp, 2014-09-19 20:48:22]

Proof of concept only.   The code does not compile.

[kelvinp, 2014-09-19 23:39:28]

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

[kelvinp, 2014-09-19 23:39:42]

Patchset #1 (id:1) has been deleted

[kelvinp, 2014-09-19 23:40:54]

On 2014/09/19 20:48:22, kelvinp wrote:
> Proof of concept only.   The code does not compile.

The CL is now ready for review.  PTAL.

[kelvinp, 2014-09-22 23:16:05]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2014-09-22 23:42:59]

In case you plan to land this before M39 branch you also need to add a flag in
about:flags and keep the host disabled by default

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.h:157: virtual void
PostMessageFromNative(int port_id,
I think it's better to keep the old name.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:9:
#include "base/bind.h"
empty line above this one

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:68:
typedef std::map<std::string, HostDescriptor*> HostMap;
This all seems to be more complicated than it needs to be.
Wouldn't something like this be sufficient?

struct BuiltInHost {
  const char * const name;
  const char * const * const allowed_origins;
  int allowed_origins_count;
  scoped_ptr<NativeMessageHost> (*create_function)(base::WeakPtr<Client>);
};
const char* const kRemotingIt2MeOrigins[] = {
    "chrome-extension://ljacajndfccfgnfohlgkdphmbnpkjflk/",
    ....
};
static const BuiltInHost kBuiltInHosts[] = {
 {"com.google.chrome.remote_assistance",
  kRemotingIt2MeOrigins, arraysize(It2MeOrigins),
  &CreateIt2MeHost},
};

That way you wouldn't need NativeMessageHostFactory

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:86:
NativeMessageHostFactory* g_host_factory;
As mentioned above I don't think you need this. But in case you do this should
be initialized to NULL.
Also in general it's preferred to use base::Singleton for this kind of objects
(see base/memory/singleton.h) because it avoids possible races during
initialization. (not a problem here because NativeMessageHost::Create() is
called on the same thread)

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:106:
"chrome-extension://ljacajndfccfgnfohlgkdphmbnpkjflk/");
You need to whitelist all chromoting IDs. See
remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2 for the full
list.
Also since this list is essentially duplicated here these two files need to
cross-reference each other so we don't forget to update then when new ID added
or removed.

Or ideally we should build the manifest file as resource and load it here. That
way this info would need to be duplicated here.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_port.h:16: // Takes
ownership of |native_message_host|.
Please use scoped_ptr<> to pass ownership and remove this comment

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.h:20: class
PrefService;
this can be removed

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.h:57: //
Send a message with the specified payload.
Replace this comment with // NativeMessageHost implementation.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.cc (right):

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.cc:15:
NativeMessageHost::IsHostAllowed(const PrefService* pref_service,
I don't think this function belongs in this interface. Maybe move it in
message_service.cc?

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:9: #include
"base/memory/scoped_ptr.h"
Empty line above this one.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:37: virtual void
PostMessageFromNative(int port_id,
Maybe PostMessageFromNativeHost()?

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:46: base::WeakPtr<Client>
weak_client_ui,
I think this should be called |client|. It has nothing to do with the UI and the
type shouldn't be part of the name.

[kelvinp, 2014-09-23 20:16:03]

Patchset #2 (id:60001) has been deleted

[kelvinp, 2014-09-23 20:16:42]

PTAL

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.h:157: virtual void
PostMessageFromNative(int port_id,
On 2014/09/22 23:42:58, Sergey Ulanov wrote:
> I think it's better to keep the old name.

Renamed to PostMessageFromNative.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:9:
#include "base/bind.h"
On 2014/09/22 23:42:58, Sergey Ulanov wrote:
> empty line above this one

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:68:
typedef std::map<std::string, HostDescriptor*> HostMap;
On 2014/09/22 23:42:58, Sergey Ulanov wrote:
> This all seems to be more complicated than it needs to be.
> Wouldn't something like this be sufficient?
> 
> struct BuiltInHost {
>   const char * const name;
>   const char * const * const allowed_origins;
>   int allowed_origins_count;
>   scoped_ptr<NativeMessageHost> (*create_function)(base::WeakPtr<Client>);
> };
> const char* const kRemotingIt2MeOrigins[] = {
>     "chrome-extension://ljacajndfccfgnfohlgkdphmbnpkjflk/",
>     ....
> };
> static const BuiltInHost kBuiltInHosts[] = {
>  {"com.google.chrome.remote_assistance",
>   kRemotingIt2MeOrigins, arraysize(It2MeOrigins),
>   &CreateIt2MeHost},
> };
> 
> That way you wouldn't need NativeMessageHostFactory

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:86:
NativeMessageHostFactory* g_host_factory;
On 2014/09/22 23:42:58, Sergey Ulanov wrote:
> As mentioned above I don't think you need this. But in case you do this should
> be initialized to NULL.
> Also in general it's preferred to use base::Singleton for this kind of objects
> (see base/memory/singleton.h) because it avoids possible races during
> initialization. (not a problem here because NativeMessageHost::Create() is
> called on the same thread)

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:106:
"chrome-extension://ljacajndfccfgnfohlgkdphmbnpkjflk/");
On 2014/09/22 23:42:58, Sergey Ulanov wrote:
> You need to whitelist all chromoting IDs. See
> remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2 for the
full
> list.
> Also since this list is essentially duplicated here these two files need to
> cross-reference each other so we don't forget to update then when new ID added
> or removed.
> 
> Or ideally we should build the manifest file as resource and load it here.
That
> way this info would need to be duplicated here.

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_port.h:16: // Takes
ownership of |native_message_host|.
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> Please use scoped_ptr<> to pass ownership and remove this comment

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.h:20: class
PrefService;
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> this can be removed

Done.

https://codereview.chromium.org/591463003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.h:57: //
Send a message with the specified payload.
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> Replace this comment with // NativeMessageHost implementation.

Done.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.cc (right):

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.cc:15:
NativeMessageHost::IsHostAllowed(const PrefService* pref_service,
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> I don't think this function belongs in this interface. Maybe move it in
> message_service.cc? 

Done.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:9: #include
"base/memory/scoped_ptr.h"
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> Empty line above this one.

Done.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:37: virtual void
PostMessageFromNative(int port_id,
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> Maybe PostMessageFromNativeHost()?

Done.

https://codereview.chromium.org/591463003/diff/20001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:46: base::WeakPtr<Client>
weak_client_ui,
On 2014/09/22 23:42:59, Sergey Ulanov wrote:
> I think this should be called |client|. It has nothing to do with the UI and
the
> type shouldn't be part of the name.

Done.  Copy and paste from existing code does not induce good behavior O_O

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

kelvinp@chromium.org changed reviewers:
+ yoz@chromium.org
- darin@chromium.org

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

PTAL

@yoz, can you help me to take a look at the changes on
extensions/common/switches.cc/h ?

[Yoyo Zhou, 2014-09-23 23:35:59]

switches LGTM

[Sergey Ulanov, 2014-09-25 01:45:22]

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.h:169: static
PolicyPermission IsHostAllowed(const PrefService* pref_service,
This doesn't really need to be part of the MessageService class. Make it a
static function in message_service.cc . Also this name is confusing in this
context. I suggest IsNativeMessagingHostAllowed().

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:74:
"chrome-extension://dhdboenlgbbeohfjeboeooomjmgbejoh/"};
This list is different what we have in
com.google.chrome.remote_assistance.json.jinja2

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:80:
&EchoHost::CreateEchoHost}};
Given that this CL contains changes for the It2Me host, why can't we use it here
instead of the fake Echo?
Would it make sense adding separate echo host for tests?

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:20: // which can run in
the browser-process or in a separate process (SEe
typo: SEe

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:43: int destination_port,
It looks wrong that the host implementation have to care about port ids, which
is an implementation detail of the MessageService. I suggest replacing Client
interface with two base::Callback<> parameters. This would allow you to curry
the port_id when creating the callbacks in MessagingService will not need to
expose them outside.

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:49: virtual
~NativeMessageHost() {}
destructor should be declared between static methods and all other methods, i.e.
between Create() and Send().

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
File remoting/host/native_messaging/native_messaging_pipe.h (right):

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
remoting/host/native_messaging/native_messaging_pipe.h:14: class Value;
nit: this line doesn't need to be indented.

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
remoting/host/native_messaging/native_messaging_pipe.h:19: // Connects a
extensions::NativeMessageHost to a PipeMessagingChannel
nit: add . at the end of the sentence.

[kelvinp, 2014-09-26 18:12:27]

PTAL

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.h:169: static
PolicyPermission IsHostAllowed(const PrefService* pref_service,
On 2014/09/25 01:45:21, Sergey Ulanov wrote:
> This doesn't really need to be part of the MessageService class. Make it a
> static function in message_service.cc . Also this name is confusing in this
> context. I suggest IsNativeMessagingHostAllowed().

Done.

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:74:
"chrome-extension://dhdboenlgbbeohfjeboeooomjmgbejoh/"};
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> This list is different what we have in
> com.google.chrome.remote_assistance.json.jinja2

Done.

https://codereview.chromium.org/591463003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:80:
&EchoHost::CreateEchoHost}};
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> Given that this CL contains changes for the It2Me host, why can't we use it
here
> instead of the fake Echo?
> Would it make sense adding separate echo host for tests?

It2Me host would crash on launching now.  We still need to make some changes in
the ChromotingHostContext in order to launch It2Me host safely.  I can include a
separating echo host for testing in the change when I enable the It2me host.

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:20: // which can run in
the browser-process or in a separate process (SEe
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> typo: SEe

Done.

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:43: int destination_port,
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> It looks wrong that the host implementation have to care about port ids, which
> is an implementation detail of the MessageService. I suggest replacing Client
> interface with two base::Callback<> parameters. This would allow you to curry
> the port_id when creating the callbacks in MessagingService will not need to
> expose them outside.

Done.

https://codereview.chromium.org/591463003/diff/80001/extensions/browser/api/m...
extensions/browser/api/messaging/native_message_host.h:49: virtual
~NativeMessageHost() {}
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> destructor should be declared between static methods and all other methods,
i.e.
> between Create() and Send().

Done.

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
File remoting/host/native_messaging/native_messaging_pipe.h (right):

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
remoting/host/native_messaging/native_messaging_pipe.h:14: class Value;
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> nit: this line doesn't need to be indented.

Done.

https://codereview.chromium.org/591463003/diff/80001/remoting/host/native_mes...
remoting/host/native_messaging/native_messaging_pipe.h:19: // Connects a
extensions::NativeMessageHost to a PipeMessagingChannel
On 2014/09/25 01:45:22, Sergey Ulanov wrote:
> nit: add . at the end of the sentence.

Done.

[Sergey Ulanov, 2014-09-27 00:24:10]

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1930: #endif
Add //  defined(OS_CHROMEOS)

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/OWNERS (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/OWNERS:6: kelvinp@chromium.org
It's better to move this to a separate CL.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:23:
typedef NativeMessageHost::Client Client;
Don't need this typedef. EchoHost inherits NativeMessageHost and you use Client
only inside EchoHost.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:103:
->HasSwitch(switches::kEnableRemoteAssistance)) {
I think it makes more sense to put this in the function that actually creates
the It2Me host. Otherwise it looks confusing here since the flag per se is not
related to native messaging.

Also without this check you can remove |not_found|.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:30: // Calling
CloseChannel will delete the current object.
don't need this comment.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port_chromeos.cc:11:
NativeMessagePort::NativeMessagePort(
Why do we need chrome-os specific version of this file? It looks almost exactly
like native_message_port.cc and there is nothing in the logic of this file
that's OS specific. It's just a connector between MessageService and
NativeMessageHost.

https://codereview.chromium.org/591463003/diff/100001/extensions/browser/api/...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/100001/extensions/browser/api/...
extensions/browser/api/messaging/native_message_host.h:44: virtual void
Send(const std::string& json) = 0;
Rename this to ProcessMessage()? Send() doesn't make sense for 
NativeMessageHost implementations that process the message themselves instead of
forwarding it somewhere else.

Also rename |json| to |message|.

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

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:31: // Called when a message
is received from the client.
This comment wouldn't be necessary if you rename Send() to OnMessage().

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

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:213:
scoped_ptr<remoting::NativeMessagingPipe> host_;
rename this, e.g. pipe_?

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:441:
scoped_ptr<extensions::NativeMessageHost> it2meHost(
it2me_host

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:447:
host_->Start(base::Bind(&It2MeNativeMessagingHostTest::StopHost,
I think you also want to call Init() here. (but it's better to merge Init() and
Start() to a single method).

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
File remoting/host/native_messaging/native_messaging_pipe.h (right):

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
remoting/host/native_messaging/native_messaging_pipe.h:32: void Start(const
base::Closure& quit_closure);
Do we need separate Init() and Start()? Merge them to a single method?

[kelvinp, 2014-09-29 22:59:40]

PTAL

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1930: #endif
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> Add //  defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/OWNERS (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/OWNERS:6: kelvinp@chromium.org
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> It's better to move this to a separate CL.

Done.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:23:
typedef NativeMessageHost::Client Client;
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> Don't need this typedef. EchoHost inherits NativeMessageHost and you use
Client
> only inside EchoHost.

Done.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:103:
->HasSwitch(switches::kEnableRemoteAssistance)) {
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> I think it makes more sense to put this in the function that actually creates
> the It2Me host. Otherwise it looks confusing here since the flag per se is not
> related to native messaging.
> 
> Also without this check you can remove |not_found|.

Done.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:30: // Calling
CloseChannel will delete the current object.
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> don't need this comment.

Done.

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port_chromeos.cc:11:
NativeMessagePort::NativeMessagePort(
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> Why do we need chrome-os specific version of this file? It looks almost
exactly
> like native_message_port.cc and there is nothing in the logic of this file
> that's OS specific. It's just a connector between MessageService and
> NativeMessageHost.

Done.

https://codereview.chromium.org/591463003/diff/100001/extensions/browser/api/...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/100001/extensions/browser/api/...
extensions/browser/api/messaging/native_message_host.h:44: virtual void
Send(const std::string& json) = 0;
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> Rename this to ProcessMessage()? Send() doesn't make sense for 
> NativeMessageHost implementations that process the message themselves instead
of
> forwarding it somewhere else.
> 
> Also rename |json| to |message|.

Done.

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

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:31: // Called when a message
is received from the client.
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> This comment wouldn't be necessary if you rename Send() to OnMessage().

Done.

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

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:213:
scoped_ptr<remoting::NativeMessagingPipe> host_;
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> rename this, e.g. pipe_?

Done.

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:441:
scoped_ptr<extensions::NativeMessageHost> it2meHost(
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> it2me_host

Done.

https://codereview.chromium.org/591463003/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:447:
host_->Start(base::Bind(&It2MeNativeMessagingHostTest::StopHost,
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> I think you also want to call Init() here. (but it's better to merge Init()
and
> Start() to a single method).

Good catch.
I think it is better to keep the construction of the object (init) and starting
the object (which has the side effect of receiving messages) separate.

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
File remoting/host/native_messaging/native_messaging_pipe.h (right):

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
remoting/host/native_messaging/native_messaging_pipe.h:32: void Start(const
base::Closure& quit_closure);
On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> Do we need separate Init() and Start()? Merge them to a single method?

I think it is better to keep the construction of the object (init) and starting
the object (which has the side effect of start receiving messages) separate. 
This will make the code more flexible if we decided to assemble the object in
and different place than starting it in the future.

[Sergey Ulanov, 2014-09-30 20:33:40]

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
File remoting/host/native_messaging/native_messaging_pipe.h (right):

https://codereview.chromium.org/591463003/diff/100001/remoting/host/native_me...
remoting/host/native_messaging/native_messaging_pipe.h:32: void Start(const
base::Closure& quit_closure);
On 2014/09/29 22:59:40, kelvinp wrote:
> On 2014/09/27 00:24:10, Sergey Ulanov wrote:
> > Do we need separate Init() and Start()? Merge them to a single method?
> 
> I think it is better to keep the construction of the object (init) and
starting
> the object (which has the side effect of start receiving messages) separate. 
> This will make the code more flexible if we decided to assemble the object in
> and different place than starting it in the future.

I still think they shouldn't be separate unless you have a usecase for that.
E.g. how do you know that you want to pass NativeMessagingChannel to Init()
instead of Start(), if at some point you actually need them to be separate? Why
|quit_closure| is passed to Start() and not Init()?
This adds code complexity for no benefits. If at point we find that we need
these methods to be separate it would be trivial to separate them.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.cc (right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:23:
native_message_host_->set_client(weak_factory_.GetWeakPtr());
See my comment in native_message_process_host.cc. This class should be
responsible for jumping from UI to IO thread and _back_. Ideally
NativeMessageHost implementations should call the Client  interface on the same
thread it does everything else. This means that you want to call set_client() on
the IO thread. Take a look at remoting/host/host_window_proxy.h - that's a good
example of a similar object that forwards calls between two threads. Internally
it has Core object internally that works on the target thread and is destroyed
on the target thread. I think you want something similar here:
 - NativeMessagePort::Core would own native_message_host and call it on the
host_task_runner_.
 - NativeMessagePort owns instance of the Core and posts tasks to
host_task_runner_.
 - Core handles Client callbacks from NativeMessageHost and posts tasks on UI
thread for NativeMessagePort.

The goal here is to isolate threading logic in this class instead of having it
sprinkled all around (e.g. in NativeMessageProcessHost).

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:36:
base::WeakPtrFactory<NativeMessagePort> weak_factory_;
add empty line above this one.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:293:
content::BrowserThread::PostTask(
Since it's NativeMessagingHost's responsibility to jump from UI to IO thread it
should also do it in the opposite direction. Call weak_client_ here on IO thread
and let NativeMessagePort and jump to UI thread.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:43:
typedef NativeMessageHost::Client Client;
don't need this typedef.

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

https://codereview.chromium.org/591463003/diff/120001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:32: virtual void
set_client(base::WeakPtr<Client> client) OVERRIDE;
This can be Client* instead of WeakPtr<> if you take my suggestions in
NativeMessagePort

[kelvinp, 2014-10-01 06:06:55]

Patchset #5 (id:140001) has been deleted

[kelvinp, 2014-10-01 06:08:03]

PTAL

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.cc (right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:23:
native_message_host_->set_client(weak_factory_.GetWeakPtr());
On 2014/09/30 20:33:39, Sergey Ulanov wrote:
> See my comment in native_message_process_host.cc. This class should be
> responsible for jumping from UI to IO thread and _back_. Ideally
> NativeMessageHost implementations should call the Client  interface on the
same
> thread it does everything else. This means that you want to call set_client()
on
> the IO thread. Take a look at remoting/host/host_window_proxy.h - that's a
good
> example of a similar object that forwards calls between two threads.
Internally
> it has Core object internally that works on the target thread and is destroyed
> on the target thread. I think you want something similar here:
>  - NativeMessagePort::Core would own native_message_host and call it on the
> host_task_runner_.
>  - NativeMessagePort owns instance of the Core and posts tasks to
> host_task_runner_.
>  - Core handles Client callbacks from NativeMessageHost and posts tasks on UI
> thread for NativeMessagePort.
> 
> The goal here is to isolate threading logic in this class instead of having it
> sprinkled all around (e.g. in NativeMessageProcessHost). 

Done.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:36:
base::WeakPtrFactory<NativeMessagePort> weak_factory_;
On 2014/09/30 20:33:39, Sergey Ulanov wrote:
> add empty line above this one.

Done.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:293:
content::BrowserThread::PostTask(
On 2014/09/30 20:33:39, Sergey Ulanov wrote:
> Since it's NativeMessagingHost's responsibility to jump from UI to IO thread
it
> should also do it in the opposite direction. Call weak_client_ here on IO
thread
> and let NativeMessagePort and jump to UI thread.

Done.

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:43:
typedef NativeMessageHost::Client Client;
On 2014/09/30 20:33:39, Sergey Ulanov wrote:
> don't need this typedef.

Done.

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

https://codereview.chromium.org/591463003/diff/120001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:32: virtual void
set_client(base::WeakPtr<Client> client) OVERRIDE;
On 2014/09/30 20:33:39, Sergey Ulanov wrote:
> This can be Client* instead of WeakPtr<> if you take my suggestions in
> NativeMessagePort

Done.

[kelvinp, 2014-10-01 06:08:19]

Patchset #5 (id:160001) has been deleted

[Sergey Ulanov, 2014-10-01 23:23:59]

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:33:
EchoHost() {}
initialize client_ to NULL.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.cc (right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:14: typedef
NativeMessageHost::Client Client;
Don't need this typedef: Client is used in only one place in this file. It also
makes code less readable.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:25:
base::WeakPtr<NativeMessagePort> weak_port,
don't need 'weak_' prefix.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:37:
base::WeakPtr<NativeMessagePort> weak_port_;
port_

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:58:
NativeMessagePort::Core::~Core() {
This object must always be destroyed on |host_task_runner_|. Otherwise the host
may call PostMessageFromNativeHost() after it's destroyed. Call DeleteSoon() in
~NativeMessagePort() to delete the core_ and add DCHECK() here. Don't need
DeleteSoon() for host_.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:28: //
NativeMessageHost::Client implementation.
This class doesn't really need to implement NativeMessageHost::Client interface.
Make these two methods private and make Core class a friend so it can call them.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:185:
client_ = client;
add DCHECK(task_runner_->BelongsToCurrentThread());

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:13:
#include "base/memory/weak_ptr.h"
No longer need this.

https://codereview.chromium.org/591463003/diff/180001/extensions/browser/api/...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/180001/extensions/browser/api/...
extensions/browser/api/messaging/native_message_host.h:23: // Callback interface
for receiving messages from the native component.
s/the native component/the host/

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

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:109: client_ = client;
DCHECK that we are on the right thread.

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:114: std::string
message_json;
same here

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:230: // Trigger a host
shutdown by sending a NULL message.
update the comment

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:231:
client_->CloseChannel("");
Use std::string() instead of "" for empty string. (otherwise the compiler
generates suboptimal code).

https://codereview.chromium.org/591463003/diff/180001/remoting/host/native_me...
File remoting/host/native_messaging/native_messaging_pipe.cc (right):

https://codereview.chromium.org/591463003/diff/180001/remoting/host/native_me...
remoting/host/native_messaging/native_messaging_pipe.cc:44:
channel_->SendMessage(json.Pass());
nit: use make_scoped_ptr() to construct scoped_ptr:
  channel_->SendMessage(make_scoped_ptr(base::JSONReader::Read(message)));

[kelvinp, 2014-10-02 03:10:34]

Patchset #6 (id:200001) has been deleted

[kelvinp, 2014-10-02 03:12:17]

PTAL

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:33:
EchoHost() {}
On 2014/10/01 23:23:58, Sergey Ulanov wrote:
> initialize client_ to NULL.

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.cc (right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:14: typedef
NativeMessageHost::Client Client;
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> Don't need this typedef: Client is used in only one place in this file. It
also
> makes code less readable.

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:25:
base::WeakPtr<NativeMessagePort> weak_port,
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> don't need 'weak_' prefix.

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:37:
base::WeakPtr<NativeMessagePort> weak_port_;
On 2014/10/01 23:23:58, Sergey Ulanov wrote:
> port_

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.cc:58:
NativeMessagePort::Core::~Core() {
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> This object must always be destroyed on |host_task_runner_|. Otherwise the
host
> may call PostMessageFromNativeHost() after it's destroyed. Call DeleteSoon()
in
> ~NativeMessagePort() to delete the core_ and add DCHECK() here. Don't need
> DeleteSoon() for host_.

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_port.h (right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_port.h:28: //
NativeMessageHost::Client implementation.
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> This class doesn't really need to implement NativeMessageHost::Client
interface.
> Make these two methods private and make Core class a friend so it can call
them.

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:185:
client_ = client;
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> add DCHECK(task_runner_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:13:
#include "base/memory/weak_ptr.h"
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> No longer need this.

Done.

https://codereview.chromium.org/591463003/diff/180001/extensions/browser/api/...
File extensions/browser/api/messaging/native_message_host.h (right):

https://codereview.chromium.org/591463003/diff/180001/extensions/browser/api/...
extensions/browser/api/messaging/native_message_host.h:23: // Callback interface
for receiving messages from the native component.
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> s/the native component/the host/

Done.

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

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:109: client_ = client;
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> DCHECK that we are on the right thread.

Done.

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:114: std::string
message_json;
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> same here

Done.

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:230: // Trigger a host
shutdown by sending a NULL message.
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> update the comment

Done.

https://codereview.chromium.org/591463003/diff/180001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:231:
client_->CloseChannel("");
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> Use std::string() instead of "" for empty string. (otherwise the compiler
> generates suboptimal code).

Done.

https://codereview.chromium.org/591463003/diff/180001/remoting/host/native_me...
File remoting/host/native_messaging/native_messaging_pipe.cc (right):

https://codereview.chromium.org/591463003/diff/180001/remoting/host/native_me...
remoting/host/native_messaging/native_messaging_pipe.cc:44:
channel_->SendMessage(json.Pass());
On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> nit: use make_scoped_ptr() to construct scoped_ptr:
>   channel_->SendMessage(make_scoped_ptr(base::JSONReader::Read(message)));

Done.

[Sergey Ulanov, 2014-10-02 23:09:59]

lgtm when my comments are addressed

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/591463003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:185:
client_ = client;
On 2014/10/02 03:12:17, kelvinp wrote:
> On 2014/10/01 23:23:59, Sergey Ulanov wrote:
> > add DCHECK(task_runner_->BelongsToCurrentThread());
> 
> Done.

I still don't see DCHECK here :)

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

https://codereview.chromium.org/591463003/diff/210001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:80:
static_cast<base::DictionaryValue*>(base::JSONReader::Read(message)));
Verify that the message is a dictionary before casting it to DictionaryValue
here.
You probably want to merge my recent changes in this file:
https://codereview.chromium.org/615543004/diff/1/remoting/host/it2me/it2me_na...

[kelvinp, 2014-10-03 01:18:58]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-03 01:20:22]

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

[commit-bot: I haz the power, 2014-10-03 01:54:47]

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

[commit-bot: I haz the power, 2014-10-03 01:54:48]

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

[Sergey Ulanov, 2014-10-03 17:55:47]

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

https://codereview.chromium.org/591463003/diff/230001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:80: base::Value*
message_value = base::JSONReader::Read(message);
This won't be deleted if it's not a dictionary. Please use scoped_ptr<> here.

https://codereview.chromium.org/591463003/diff/230001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:81: if
(message_value->GetType() != base::Value::TYPE_DICTIONARY) {
IsType(base::Value::TYPE_DICTIONARY)

[kelvinp, 2014-10-03 19:06:20]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2014-10-03 19:06:36]

The CQ bit was unchecked by kelvinp@chromium.org

[kelvinp, 2014-10-03 19:10:36]

Patchset #8 (id:250001) has been deleted

[kelvinp, 2014-10-03 19:12:08]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-03 19:13:04]

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

[Sergey Ulanov, 2014-10-03 19:21:25]

The CQ bit was unchecked by sergeyu@chromium.org

[Sergey Ulanov, 2014-10-03 19:22:10]

Unchecked CQ bit because there is a bug.

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

https://codereview.chromium.org/591463003/diff/270001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:81:
make_scoped_ptr(base::JSONReader::Read(message)));
don't need make_scoped_ptr() here. scoped_ptr<> can be initialized with a raw
pointer.

https://codereview.chromium.org/591463003/diff/270001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:89:
static_cast<base::DictionaryValue*>(message_value.get()));
You need release() here instead of get(). With get() here ownership the object
is owned by both scoped_ptr<>, which will cause crash when they are deleted.

[kelvinp, 2014-10-06 04:34:21]

Patchset #9 (id:290001) has been deleted

[kelvinp, 2014-10-06 04:37:25]

kelvinp@chromium.org changed reviewers:
+ asvitkine@chromium.org, pastarmovj@chromium.org

[kelvinp, 2014-10-06 04:37:26]

PTAL
pastarmov@ can you help me review the change on
chrome/browser/policy/policy_browsertest.cc

asvitkine@ can you help me review the change on
tools/metrics/histograms/histograms.xml

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

https://codereview.chromium.org/591463003/diff/230001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:80: base::Value*
message_value = base::JSONReader::Read(message);
On 2014/10/03 17:55:47, Sergey Ulanov wrote:
> This won't be deleted if it's not a dictionary. Please use scoped_ptr<> here.

Done.

https://codereview.chromium.org/591463003/diff/230001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:81: if
(message_value->GetType() != base::Value::TYPE_DICTIONARY) {
On 2014/10/03 17:55:47, Sergey Ulanov wrote:
> IsType(base::Value::TYPE_DICTIONARY)

Done.

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

https://codereview.chromium.org/591463003/diff/270001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:81:
make_scoped_ptr(base::JSONReader::Read(message)));
On 2014/10/03 19:22:10, Sergey Ulanov wrote:
> don't need make_scoped_ptr() here. scoped_ptr<> can be initialized with a raw
> pointer.

Done.

https://codereview.chromium.org/591463003/diff/270001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:89:
static_cast<base::DictionaryValue*>(message_value.get()));
On 2014/10/03 19:22:10, Sergey Ulanov wrote:
> You need release() here instead of get(). With get() here ownership the object
> is owned by both scoped_ptr<>, which will cause crash when they are deleted.

Done.

[pastarmovj, 2014-10-06 09:15:39]

chrome/browser/policy/policy_browsertest.cc - LGTM

[Alexei Svitkine (slow), 2014-10-06 15:27:17]

histograms lgtm

[kelvinp, 2014-10-06 17:05:14]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-06 17:05:56]

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

[commit-bot: I haz the power, 2014-10-06 17:33:00]

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

[commit-bot: I haz the power, 2014-10-06 17:33:03]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kelvinp, 2014-10-06 18:27:12]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-06 18:28:25]

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

[commit-bot: I haz the power, 2014-10-06 19:05:43]

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

[commit-bot: I haz the power, 2014-10-06 19:05:44]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[kelvinp, 2014-10-06 19:58:31]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-06 19:59:58]

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

[commit-bot: I haz the power, 2014-10-06 23:23:16]

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

[commit-bot: I haz the power, 2014-10-06 23:23:17]

Exceeded time limit waiting for builds to trigger.

[kelvinp, 2014-10-07 00:15:53]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-07 00:20:31]

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

[kelvinp, 2014-10-07 00:33:26]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-07 00:33:39]

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

[commit-bot: I haz the power, 2014-10-07 02:42:09]

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

[commit-bot: I haz the power, 2014-10-07 02:42:10]

Exceeded time limit waiting for builds to trigger.

[kelvinp, 2014-10-07 03:04:22]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-07 03:06:19]

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

[commit-bot: I haz the power, 2014-10-07 04:18:02]

Committed patchset #12 (id:370001) as 10e583165e5a45d032977cc25a2d11eb6f5f4dd0

[commit-bot: I haz the power, 2014-10-07 04:19:05]

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

[hayato, 2014-10-07 05:57:21]

It seems that ExtensionApiTest.NativeMessagingBasic and
ExtensionApiTest.UserLevelNativeMessaging started to fail.

http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

Could you fix or revert this?
If you can't soon, let me revert this patch as a gardener.

[kelvinp, 2014-10-07 06:29:24]

Not sure why the tests broke as it passed the trybots which run ChromeOS tests.
I won't be able to look at them now.  Please revert them.

[hayato, 2014-10-07 06:34:23]

Okay. Let me revert that. The revert patch is here:
https://codereview.chromium.org/637463002

[hayato, 2014-10-07 08:40:35]

FYI. It seems the revert has fixed the failures.

http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

[kelvinp, 2014-10-07 19:04:20]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-10-07 19:07:15]

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

[commit-bot: I haz the power, 2014-10-07 21:50:00]

Committed patchset #13 (id:390001) as 41c42f80e66aad80a7006742b8fdef29190decc3

[commit-bot: I haz the power, 2014-10-07 21:50:46]

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

[kelvinp1, 2016-03-16 01:25:30]

Patchset #15 (id:430001) has been deleted

[kelvinp1, 2016-03-16 01:25:40]

Patchset #15 (id:450001) has been deleted