Native Messaging!

Native messaging now uses the MessageService back-end. 

BUG=142915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156678

[Aaron Boodman, 2012-07-23 22:30:17]

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/native_message/native_message_api.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.cc:53: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Can you also make this only work on the dev channel for now? See
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/f...
for how to check this.

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.cc:60: //
intended directory. Eg. '../../dangerous_something.exe'.
Good point. You can easily defend against that by using
file_util::ContainsPath().

[Matt Perry, 2012-07-23 23:47:03]

Here's some preliminary comments.

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/native_message/native_message_api.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.cc:22: const
char kNativeClientDir[] = "native-hosts";
Chrome's data directories are formatted like "Native Hosts"

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.cc:53: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/07/23 22:30:17, Aaron Boodman wrote:
> Can you also make this only work on the dev channel for now? See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/f...
> for how to check this.

Wouldn't it just be a matter of adding extension.sendNativeMessage to
_permission_features.json?

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.cc:75:
"test_prefix"));
Have you seen base::WorkerPool? It might be better than allocating a separate
dedicated thread for each message.

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/native_message/native_message_api.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_message_api.h:20: class
SendNativeMessageFunction : public AsyncExtensionFunction {
put this in the extensions namespace

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/native_message/native_thread_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_thread_delegate.cc:47:
file_util::ScopedFD read_pipe_write_fd(&read_pipe_fds[1]);
A lot of this stuff is posix only, in which case this won't compile on windows.
If you have posix-specific code, you'll need to put it in a _posix.cc file (or
if the implementation is mostly shared cross-platform, use #ifdefs).

Though, this is all pretty low level. I have to think there should be some
higher level cross-platform primitives for stuff like this in Chrome. Chrome
does a lot of spawning separate processes and sending messages back and forth
between them.

https://chromiumcodereview.appspot.com/10818013/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/native_message/native_thread_delegate.cc:106: if
(HANDLE_EINTR(write(fd, &type, 1)) < 1) {
base::Pickle might be a good cross-platform alternative to reading and writing
manually. Serialize the data to a Pickle, then write the bytes all at once. Our
IPC system is based on this.

[eaugusti, 2012-08-08 22:12:10]

This is still a bit hacky, and linux (posix?) only, but this shows the new
architecture in mind.

[Matt Perry, 2012-08-09 02:13:00]

Looks much cleaner! I like the idea of reusing the MessageService. Pretty cool
how nice that fits.

I'm still hesitant about rolling your own code to launch the process and
read/write pipes. Have you seen ChildProcessLauncher? ipc_channel_*.cc? Have you
talked to darin or jam to see if there's other good stuff in Chrome you can
reuse? If there's not, that's cool. Just want to make sure you're not
reinventing the wheel.

You also might want to get someone more low-level to review the
native_message_process stuff. My posix is rusty.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.cc (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:52: struct
MessageService::MessagePort {
maybe we should turn this into an interface that does the right thing for both
renderers and native processes. it could have the Dispatch* and
Increment/Decrement* methods too, so that you don't have to check is_native.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:268: MessagePort
receiver(GetExtensionProcess(profile, source_extension_id),
The receiver is the target of the connect event - in this case, a native app. So
you shouldn't be using the extension process here. I think it makes sense to
just make it NULL (or turn MessagePort into an interface like I mentioned above,
so there is no RenderProcessHost for the native case).

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:304: void
MessageService::OpenChannelToNativeAppOnFileThread(
This class is not threadsafe. Same goes for |channel|. They can only be accessed
on the FILE thread. This should be a static function. If you need to update
state after this runs, you should post a task from the FILE thread back to the
UI.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:318: native_app_name,
connect_message, this, receiver_port_id, type);
What if an extension opens 2 channels to the same process? Do we spawn 2
processes, or try to keep track of whether it's running?

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:333:
DispatchOnConnect(channel->receiver, receiver_port_id,
I don't think this call makes sense. You're dispatching a "connect" event to the
receiver, which is a native app. It's actually sent to the opener extension,
though, since that's what you used when initializing the receiver.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.cc (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:26: const uint32
kMaxMessageDataLength = 4096;
I imagine extensions will want much larger sizes, to pass things like image data
back and forth.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:157: void
NativeMessageProcess::Send(MessageType type, const std::string& json) {
What thread is this called on? Assuming the answer is anything but "FILE"
thread, there's some thread safety issues here. Touching any of these member
vars is not safe. You can probably just pass all this data over to the FILE
thread and do the updates there.

Have you seen ipc_channel_proxy.cc/h? That has to deal with issues like this. If
you can't use it directly, it may help to model it.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:183:
service_->PostMessageFromRenderer(dest_port_, data.data);
This must be called on the UI thread. Note you also have to deal with lifetime
issues (if you PostTask, |service| might be deleted by the time the Task runs).

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:226: char
data[kMaxMessageDataLength];
Better to heap-allocate this, especially if you up the max length. You can use a
std::string directly, and just resize it to the right size. That'll save you a
string copy down below, too.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:277: void
NativeMessageProcess::KillProcess(base::ProcessHandle handle) {
This is a little draconian. Why do we need to be so adamant about killing the
process quickly? Can we do this on a worker thread and make the timeout much
longer? What does ChildProcessLauncher do?

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.h (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:28: // message.
indenting is off here

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:31: TYPE_CONNECT_MESSAGE,  //
Used for messages after a connection has already
Do we need to expose the difference between connect and sendMessage to the app?
We could instead expose 4 primitives:
- connect (establishes the connection)
- message (message from extension -> app)
- response (message from app -> extension)
- close (closes the connection)

sendMessage then is just [connect,message,response,close] in sequence.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:38: // Must be called on the
FILE thread.
Is there a reason you chose the FILE thread (say, as opposed to the IO thread,
or a new worker thread)? I'm not fmailiar with these classes, but using
MessageLoopForIO::Watcher on the FILE thread seems weird to me.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:41: const std::string&
connection_message,
indent should align with first param after (

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:69: MessageService* service,
indent to 4 from the N

see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/10818013/diff/14001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/extension_custom_bindings.js (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/extension_custom_bindings.js:119: //
TODO(eriq): Properly handle the callback like in sendMessageImpl()
can you just use sendMessageImpl directly?

http://codereview.chromium.org/10818013/diff/14001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/extension_custom_bindings.js:184: throw new
Error('Need a message for native connect');
I think you can use the schema validation for these checks.

http://codereview.chromium.org/10818013/diff/14001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:187: if
(targetExtensionId == "native")
This shouldn't happen if you remove the DispatchOnConnect in
extension_message_service.cc

[Aaron Boodman, 2012-08-09 18:36:12]

Yes, we need to figure out the threading model.

I wonder if we can pull WatchFileDescriptor down into MessageLoop base class. If
so, we could use it with the UI message loop, and the collection of threads
involved in this CL would drop to just two: FILE and UI.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.cc (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:52: struct
MessageService::MessagePort {
On 2012/08/09 02:13:00, Matt Perry wrote:
> maybe we should turn this into an interface that does the right thing for both
> renderers and native processes. it could have the Dispatch* and
> Increment/Decrement* methods too, so that you don't have to check is_native.

/me is shocked to here mpcomplete say 'interface'. Agree.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:59:
linked_ptr<NativeMessageProcess> native_process;
linked_ptr here is unfortunate, as it makes the lifetime of this process less
clear. Can you make this a scoped_ptr and wherever we pass MessagePort by copy
today, pass scoped_ptr instead?

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:318: native_app_name,
connect_message, this, receiver_port_id, type);
On 2012/08/09 02:13:00, Matt Perry wrote:
> What if an extension opens 2 channels to the same process? Do we spawn 2
> processes, or try to keep track of whether it's running?

I think we should just create two. The extension should use connect/send if they
want to have a long-lived conversation with a single process.

Or if the app would like to share connections, it can implement that on its end,
too.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.cc (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:277: void
NativeMessageProcess::KillProcess(base::ProcessHandle handle) {
On 2012/08/09 02:13:00, Matt Perry wrote:
> This is a little draconian. Why do we need to be so adamant about killing the
> process quickly? Can we do this on a worker thread and make the timeout much
> longer? What does ChildProcessLauncher do?

I don't think we need to kill it all. We can just release our file descriptors
and let the other side shut down as it wants.

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.h (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:31: TYPE_CONNECT_MESSAGE,  //
Used for messages after a connection has already
On 2012/08/09 02:13:00, Matt Perry wrote:
> Do we need to expose the difference between connect and sendMessage to the
app?
> We could instead expose 4 primitives:
> - connect (establishes the connection)
> - message (message from extension -> app)
> - response (message from app -> extension)
> - close (closes the connection)
> 
> sendMessage then is just [connect,message,response,close] in sequence.

We could do that. I was just mirroring the API we have elsewhere for other
clients.

If we just have the four primitives, it makes it more work for simple clients
who know that they just want to send a single message. They have to read through
the connect message and discard it.

http://codereview.chromium.org/10818013/diff/14001/chrome/common/extensions/a...
File chrome/common/extensions/api/extension.json (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/common/extensions/a...
chrome/common/extensions/api/extension.json:80: "unprivileged": true,
"unprivileged" controls whether the api is available outside the extension
process (like in content scripts). We don't want that.

[eaugusti, 2012-08-13 23:22:34]

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/message_service.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:52: struct
MessageService::MessagePort {
On 2012/08/09 02:13:00, Matt Perry wrote:
> maybe we should turn this into an interface that does the right thing for both
> renderers and native processes. it could have the Dispatch* and
> Increment/Decrement* methods too, so that you don't have to check is_native.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:59:
linked_ptr<NativeMessageProcess> native_process;
On 2012/08/09 18:36:12, Aaron Boodman wrote:
> linked_ptr here is unfortunate, as it makes the lifetime of this process less
> clear. Can you make this a scoped_ptr and wherever we pass MessagePort by copy
> today, pass scoped_ptr instead?

Will now be a raw pointer.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:268: MessagePort
receiver(GetExtensionProcess(profile, source_extension_id),
On 2012/08/09 02:13:00, Matt Perry wrote:
> The receiver is the target of the connect event - in this case, a native app.
So
> you shouldn't be using the extension process here. I think it makes sense to
> just make it NULL (or turn MessagePort into an interface like I mentioned
above,
> so there is no RenderProcessHost for the native case).

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:304: void
MessageService::OpenChannelToNativeAppOnFileThread(
On 2012/08/09 02:13:00, Matt Perry wrote:
> This class is not threadsafe. Same goes for |channel|. They can only be
accessed
> on the FILE thread. This should be a static function. If you need to update
> state after this runs, you should post a task from the FILE thread back to the
> UI.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:318: native_app_name,
connect_message, this, receiver_port_id, type);
On 2012/08/09 18:36:12, Aaron Boodman wrote:
> On 2012/08/09 02:13:00, Matt Perry wrote:
> > What if an extension opens 2 channels to the same process? Do we spawn 2
> > processes, or try to keep track of whether it's running?
> 
> I think we should just create two. The extension should use connect/send if
they
> want to have a long-lived conversation with a single process.
> 
> Or if the app would like to share connections, it can implement that on its
end,
> too.

I agree with spawning multiple processes, the native application may be
stateful.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:333:
DispatchOnConnect(channel->receiver, receiver_port_id,
On 2012/08/09 02:13:00, Matt Perry wrote:
> I don't think this call makes sense. You're dispatching a "connect" event to
the
> receiver, which is a native app. It's actually sent to the opener extension,
> though, since that's what you used when initializing the receiver.

You're right, it doesn't make sense.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/native_message_process.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:26: const uint32
kMaxMessageDataLength = 4096;
On 2012/08/09 02:13:00, Matt Perry wrote:
> I imagine extensions will want much larger sizes, to pass things like image
data
> back and forth.

How about 10MB?

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:157: void
NativeMessageProcess::Send(MessageType type, const std::string& json) {
On 2012/08/09 02:13:00, Matt Perry wrote:
> What thread is this called on? Assuming the answer is anything but "FILE"
> thread, there's some thread safety issues here. Touching any of these member
> vars is not safe. You can probably just pass all this data over to the FILE
> thread and do the updates there.
> 
> Have you seen ipc_channel_proxy.cc/h? That has to deal with issues like this.
If
> you can't use it directly, it may help to model it.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:183:
service_->PostMessageFromRenderer(dest_port_, data.data);
On 2012/08/09 02:13:00, Matt Perry wrote:
> This must be called on the UI thread. Note you also have to deal with lifetime
> issues (if you PostTask, |service| might be deleted by the time the Task
runs).

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:226: char
data[kMaxMessageDataLength];
On 2012/08/09 02:13:00, Matt Perry wrote:
> Better to heap-allocate this, especially if you up the max length. You can use
a
> std::string directly, and just resize it to the right size. That'll save you a
> string copy down below, too.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:277: void
NativeMessageProcess::KillProcess(base::ProcessHandle handle) {
On 2012/08/09 18:36:12, Aaron Boodman wrote:
> On 2012/08/09 02:13:00, Matt Perry wrote:
> > This is a little draconian. Why do we need to be so adamant about killing
the
> > process quickly? Can we do this on a worker thread and make the timeout much
> > longer? What does ChildProcessLauncher do?
> 
> I don't think we need to kill it all. We can just release our file descriptors
> and let the other side shut down as it wants.

Aaron and I were talking about it, and how about we first close the pipes and
then wait ~5secs until we try and kill it.

FYI: ChildProcessLauncher does base::KillProcess() followed immediately by
base::EnsureProcessTerminated().

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/native_message_process.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:28: // message.
On 2012/08/09 02:13:00, Matt Perry wrote:
> indenting is off here

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:31: TYPE_CONNECT_MESSAGE,  //
Used for messages after a connection has already
On 2012/08/09 18:36:12, Aaron Boodman wrote:
> On 2012/08/09 02:13:00, Matt Perry wrote:
> > Do we need to expose the difference between connect and sendMessage to the
> app?
> > We could instead expose 4 primitives:
> > - connect (establishes the connection)
> > - message (message from extension -> app)
> > - response (message from app -> extension)
> > - close (closes the connection)
> > 
> > sendMessage then is just [connect,message,response,close] in sequence.
> 
> We could do that. I was just mirroring the API we have elsewhere for other
> clients.
> 
> If we just have the four primitives, it makes it more work for simple clients
> who know that they just want to send a single message. They have to read
through
> the connect message and discard it.

I think the app knowing the difference between sendMessage and connect could be
useful. Then again, the extension could always just send this information in its
message.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:38: // Must be called on the
FILE thread.
On 2012/08/09 02:13:00, Matt Perry wrote:
> Is there a reason you chose the FILE thread (say, as opposed to the IO thread,
> or a new worker thread)? I'm not fmailiar with these classes, but using
> MessageLoopForIO::Watcher on the FILE thread seems weird to me.

On Posix, the FILE and IO threads both have an IO type MessageLoop.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:41: const std::string&
connection_message,
On 2012/08/09 02:13:00, Matt Perry wrote:
> indent should align with first param after (

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:69: MessageService* service,
On 2012/08/09 02:13:00, Matt Perry wrote:
> indent to 4 from the N
> 
> see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/common/exte...
File chrome/common/extensions/api/extension.json (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/common/exte...
chrome/common/extensions/api/extension.json:80: "unprivileged": true,
On 2012/08/09 18:36:12, Aaron Boodman wrote:
> "unprivileged" controls whether the api is available outside the extension
> process (like in content scripts). We don't want that.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/renderer/re...
File chrome/renderer/resources/extensions/extension_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/renderer/re...
chrome/renderer/resources/extensions/extension_custom_bindings.js:119: //
TODO(eriq): Properly handle the callback like in sendMessageImpl()
On 2012/08/09 02:13:00, Matt Perry wrote:
> can you just use sendMessageImpl directly?

Yes, but it it counter-intuitive since we don't want to actually send another
message. I think the decision here is dependent upon the decision we make to
your earlier comment with the four primitives. For now, I will just use it.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/renderer/re...
chrome/renderer/resources/extensions/extension_custom_bindings.js:184: throw new
Error('Need a message for native connect');
On 2012/08/09 02:13:00, Matt Perry wrote:
> I think you can use the schema validation for these checks.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/renderer/re...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

https://chromiumcodereview.appspot.com/10818013/diff/14001/chrome/renderer/re...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:187: if
(targetExtensionId == "native")
On 2012/08/09 02:13:00, Matt Perry wrote:
> This shouldn't happen if you remove the DispatchOnConnect in
> extension_message_service.cc

Done.

[Aaron Boodman, 2012-08-15 03:39:21]

You'll also want to start thinking about how to test it. I think it might be
interesting to factor out the code that launches a process and returns file
descriptors. If you did that, in unit tests you could just return some fd's that
correspond to actual files and test it that way.

Then you'd also have a browser test that tests the whole stack launching a real
process (maybe a little python script or something).

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/message_port.h (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_port.h:8: #include
"chrome/browser/extensions/message_service.h"
Pull all of the messaging-related classes into c/b/e/api/messaging/*? Might want
to do that first as a purely mechanical change prior to this one.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_port.h:20: class MessagePort {
Also, I think it would be nicer to define this as a member of MessageService,
then have separate files for the two different implementations.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_port.h:34: virtual bool IsNative() { return
false; }
Ghetto-RTTI :(. It's better to tell the object what to do and rely on
polymorphism to have the right thing happen, rather than ask the object its type
and then do something based on that.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.cc (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:134:
weak_factory_.InvalidateWeakPtrs();
This happens automatically, no need to call explicitly.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:178: scoped_ptr<OpenChannelParams>
scoped_params(params);
Either define params as scoped_ptr from the start, or else just construct an
anon scoped_ptr to pass to this function. No need for the scoped_params
temporary in any case.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.h (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.h:83: int source_process_id, int
source_routing_id, int receiver_port_id,
Nit: Some of these lines could be combined. Either that or put each param on its
own line. Either way be consistent.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.h:88: // Should be called on the UI
thread.
Nit: insert blank line

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.cc (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:27: const char
kNativeClientDir[] = "Native Hosts";
kNativeHostsDirectoryName. Yeah it's longer. Life's rough.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:44: int dest_port,
Nit: avoid abbreviations; I would prefer destination_port.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:45: base::ProcessHandle
handle,
Nit: native_process_handle

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:69:
read_watcher_.StopWatchingFileDescriptor();
You probably don't need to do this manually.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:73: scoped_read_fd_.reset();
You probably don't need to do this manually.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:100: int read_pipe_fds[2];
zero init with = {0};

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:109: int write_pipe_fds[2];
initialize

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:118: FilePath client_path;
Suggestion: native_host_program

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:119: FilePath
registered_client_dir;
Naming suggestion: native_host_registry

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:127: LOG(ERROR) << "Paths
are not accepted as native app names.";
LOG(ERROR) << "Could not find native host: " << native_app_name;

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:151: base::Bind(callback,
process));
bad indent. This line should line up with 'content::...' above.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:159: if
(!MessageLoopForIO::current()->WatchFileDescriptor(
Do we even need the async stuff for writing? Writing can still block when we get
the callback if the pipe fills. And that's OK because we're on the file thread.
So why do the async bit at all? Why not just write to the fd directly in
SendImpl()?

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:230: if
(!file_util::ReadFromFD(fd, message_meta_data, 8)) {
What about incomplete messages? It seems like there'd be no guarantee that we
get OnFileCanReadWithoutBlocking at a time when we're guaranteed complete
messages.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.h (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_
Nit: Maybe call it 'native host process' to go along with the directory name and
other related names?

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:18: // TODO(eriq): Only posix
right now.
I suspect that the way this will likely shake out is one of either:

a) NativeMessageProcess interface, with NativeMessageProcessPosix and
NativeMessageProcessWin

b) native_message_process.cc only contains some of the implementation. Other
bits are in native_process_posix.cc and native_process_win.cc, which are
included conditionally, depending on platform.

It kinda depends on how much is shared between the two implementations though,
so the way you have it here makes sense until we know more.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:20: // This class ensures
that all pipes will be closed and the process will be
This class comment could use sprucing. Something along the lines of:

===
Manages the native side of a connection between an extension and a native
process.

This class must only be created, called, and deleted on the FILE thread. Public
methods typically accept callbacks which will be invoked on the UI thread.
===

The other details about ownership should be self-evident from the code.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:44: // Must be called on the
FILE thread.
These lines are not necessary everywhere since the class header should explain
that all methods need to be called on FILE thread.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:49: // |callback| will be
called on the UI thread and with a NULL on error.
The class header can explain that all callbacks are invoked on UI thread.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:50: static void
Create(base::WeakPtr<MessageService> weak_service,
The dependency upon MessageService should be factored out into a
NativeMessageProces::Client interface, which MessageService implements. This
will make testing easier, and is just generally a clean way to do things.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:64: virtual void
OnFileCanReadWithoutBlocking(int fd);
These can be private.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:68: struct MessageData {
A little header describing what this struct represents would be useful.

[Matt Perry, 2012-08-16 00:01:35]

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.cc (right):

http://codereview.chromium.org/10818013/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:26: const uint32
kMaxMessageDataLength = 4096;
On 2012/08/13 23:22:34, eaugusti wrote:
> On 2012/08/09 02:13:00, Matt Perry wrote:
> > I imagine extensions will want much larger sizes, to pass things like image
> data
> > back and forth.
> 
> How about 10MB?

SGTM

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.cc (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:288: OpenChannelParams* params =
new OpenChannelParams(source, tab_json,
nit: I prefer making this a scoped_ptr and using params.Pass below. (otherwise
there's no point to making it a scoped_ptr)

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:437: NULL :
current->second->receiver->AsExtensionMessagePort()->process();
Could replace this check and the IsNative method with a
MessagePort::IsSameProcess(MessagePort*).

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.cc:471: base::Unretained(this),
base::Passed(&scoped_params),
can you update the uses of base::Unretained to use the weak pointer factory, now
that it exists?

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/message_service.h (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/message_service.h:83: int source_process_id, int
source_routing_id, int receiver_port_id,
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Nit: Some of these lines could be combined. Either that or put each param on
its
> own line. Either way be consistent.

Chromium style is actually to put each arg on its own line (unless it all fits
on 1). See http://dev.chromium.org/developers/coding-style "For function
declarations and definitions"

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.cc (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:151: base::Bind(callback,
process));
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> bad indent. This line should line up with 'content::...' above.

I don't think the style guide actually requires this.

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.cc:267: if
(!file_util::ReadFromFD(fd, const_cast<char*>(str_data.data()),
use &str_data[0]

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
File chrome/browser/extensions/native_message_process.h (right):

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Nit: Maybe call it 'native host process' to go along with the directory name
and
> other related names?

NativeMessageProcessHost?

http://codereview.chromium.org/10818013/diff/15018/chrome/browser/extensions/...
chrome/browser/extensions/native_message_process.h:57: //
TYPE_SEND_MESSAGE_REQUEST will be sent via the connection messege in
message*

[eaugusti, 2012-08-21 23:08:34]

This one's a doozy.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
File chrome/browser/extensions/message_port.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_port.h:8: #include
"chrome/browser/extensions/message_service.h"
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Pull all of the messaging-related classes into c/b/e/api/messaging/*? Might
want
> to do that first as a purely mechanical change prior to this one.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_port.h:20: class MessagePort {
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Also, I think it would be nicer to define this as a member of MessageService,
> then have separate files for the two different implementations.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_port.h:34: virtual bool IsNative() { return
false; }
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Ghetto-RTTI :(. It's better to tell the object what to do and rely on
> polymorphism to have the right thing happen, rather than ask the object its
type
> and then do something based on that.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
File chrome/browser/extensions/message_service.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:134:
weak_factory_.InvalidateWeakPtrs();
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> This happens automatically, no need to call explicitly.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:178: scoped_ptr<OpenChannelParams>
scoped_params(params);
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Either define params as scoped_ptr from the start, or else just construct an
> anon scoped_ptr to pass to this function. No need for the scoped_params
> temporary in any case.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:288: OpenChannelParams* params =
new OpenChannelParams(source, tab_json,
On 2012/08/16 00:01:36, Matt Perry wrote:
> nit: I prefer making this a scoped_ptr and using params.Pass below. (otherwise
> there's no point to making it a scoped_ptr)

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:437: NULL :
current->second->receiver->AsExtensionMessagePort()->process();
On 2012/08/16 00:01:36, Matt Perry wrote:
> Could replace this check and the IsNative method with a
> MessagePort::IsSameProcess(MessagePort*).

Removed IsNative().

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.cc:471: base::Unretained(this),
base::Passed(&scoped_params),
On 2012/08/16 00:01:36, Matt Perry wrote:
> can you update the uses of base::Unretained to use the weak pointer factory,
now
> that it exists?

You got it!

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
File chrome/browser/extensions/message_service.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.h:83: int source_process_id, int
source_routing_id, int receiver_port_id,
On 2012/08/16 00:01:36, Matt Perry wrote:
> On 2012/08/15 03:39:21, Aaron Boodman wrote:
> > Nit: Some of these lines could be combined. Either that or put each param on
> its
> > own line. Either way be consistent.
> 
> Chromium style is actually to put each arg on its own line (unless it all fits
> on 1). See http://dev.chromium.org/developers/coding-style "For function
> declarations and definitions"

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/message_service.h:88: // Should be called on the UI
thread.
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Nit: insert blank line

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
File chrome/browser/extensions/native_message_process.cc (right):

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:27: const char
kNativeClientDir[] = "Native Hosts";
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> kNativeHostsDirectoryName. Yeah it's longer. Life's rough.

Ha, done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:44: int dest_port,
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Nit: avoid abbreviations; I would prefer destination_port.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:45: base::ProcessHandle
handle,
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Nit: native_process_handle

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:69:
read_watcher_.StopWatchingFileDescriptor();
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> You probably don't need to do this manually.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:73: scoped_read_fd_.reset();
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> You probably don't need to do this manually.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:100: int read_pipe_fds[2];
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> zero init with = {0};

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:109: int write_pipe_fds[2];
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> initialize

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:118: FilePath client_path;
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Suggestion: native_host_program

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:119: FilePath
registered_client_dir;
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Naming suggestion: native_host_registry

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:127: LOG(ERROR) << "Paths
are not accepted as native app names.";
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> LOG(ERROR) << "Could not find native host: " << native_app_name;

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:151: base::Bind(callback,
process));
On 2012/08/16 00:01:36, Matt Perry wrote:
> On 2012/08/15 03:39:21, Aaron Boodman wrote:
> > bad indent. This line should line up with 'content::...' above.
> 
> I don't think the style guide actually requires this.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:159: if
(!MessageLoopForIO::current()->WatchFileDescriptor(
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> Do we even need the async stuff for writing? Writing can still block when we
get
> the callback if the pipe fills. And that's OK because we're on the file
thread.
> So why do the async bit at all? Why not just write to the fd directly in
> SendImpl()?

I think we can get away without it. Looking at ipc_channel_posix.cc, it only
waits for the write in the case of a partial write (which WriteFileDescriptor()
protects against).

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:230: if
(!file_util::ReadFromFD(fd, message_meta_data, 8)) {
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> What about incomplete messages? It seems like there'd be no guarantee that we
> get OnFileCanReadWithoutBlocking at a time when we're guaranteed complete
> messages.

ReadFromFD protects against partial reads by just reading again. Since we always
know exactly how much we want to read, we should be fine.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.cc:267: if
(!file_util::ReadFromFD(fd, const_cast<char*>(str_data.data()),
On 2012/08/16 00:01:36, Matt Perry wrote:
> use &str_data[0]

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
File chrome/browser/extensions/native_message_process.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_
On 2012/08/16 00:01:36, Matt Perry wrote:
> On 2012/08/15 03:39:21, Aaron Boodman wrote:
> > Nit: Maybe call it 'native host process' to go along with the directory name
> and
> > other related names?
> 
> NativeMessageProcessHost?

NativeMessageProcessHost makes more sense to me.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:18: // TODO(eriq): Only posix
right now.
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> I suspect that the way this will likely shake out is one of either:
> 
> a) NativeMessageProcess interface, with NativeMessageProcessPosix and
> NativeMessageProcessWin
> 
> b) native_message_process.cc only contains some of the implementation. Other
> bits are in native_process_posix.cc and native_process_win.cc, which are
> included conditionally, depending on platform.
> 
> It kinda depends on how much is shared between the two implementations though,
> so the way you have it here makes sense until we know more.

Once I factored it out, there is actually not that much different
implementation. Option B seems pretty good.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:20: // This class ensures
that all pipes will be closed and the process will be
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> This class comment could use sprucing. Something along the lines of:
> 
> ===
> Manages the native side of a connection between an extension and a native
> process.
> 
> This class must only be created, called, and deleted on the FILE thread.
Public
> methods typically accept callbacks which will be invoked on the UI thread.
> ===
> 
> The other details about ownership should be self-evident from the code.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:44: // Must be called on the
FILE thread.
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> These lines are not necessary everywhere since the class header should explain
> that all methods need to be called on FILE thread.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:49: // |callback| will be
called on the UI thread and with a NULL on error.
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> The class header can explain that all callbacks are invoked on UI thread.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:50: static void
Create(base::WeakPtr<MessageService> weak_service,
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> The dependency upon MessageService should be factored out into a
> NativeMessageProces::Client interface, which MessageService implements. This
> will make testing easier, and is just generally a clean way to do things.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:57: //
TYPE_SEND_MESSAGE_REQUEST will be sent via the connection messege in
On 2012/08/16 00:01:36, Matt Perry wrote:
> message*

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:64: virtual void
OnFileCanReadWithoutBlocking(int fd);
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> These can be private.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/15018/chrome/browser/ext...
chrome/browser/extensions/native_message_process.h:68: struct MessageData {
On 2012/08/15 03:39:21, Aaron Boodman wrote:
> A little header describing what this struct represents would be useful.

Done.

[Matt Perry, 2012-08-22 00:02:42]

Looking good! You'll need to add _win.cc files, even if they just contain empty
stubs (have them call NOTREACHED()), to avoid breaking the win compile.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/message_service.h:74: virtual
content::RenderProcessHost* process() { return NULL; }
It's not really ideal to expose this method here, since it only makes sense for
ExtensionMessagePort. But I can't think of a better alternative (the other
option is ghetto RTTI which aa hated). At the least, it should be renamed
render_process_host() to avoid confusion with native_process_.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:116:
native_process_handle, read_handle,
indent either 4 spaces or to align with the (

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:123:
base::Bind(callback, process));
The good news is that since MessageService uses a weak pointer, callback won't
be called if MessageService dies before this runs. The bad news is that
|process| will leak if that happens. Use a scoped_ptr here and in the callback
interface to handle that case.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:133:
const std::string& message) {
bad indent

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:155:
std::string* message) {
bad indent

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:164:
pickle.WriteBytes(message_meta_data, 8);
Use "Pickle pickle(message_meta_data, 8)" to avoid copying.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:23: class
NativeMessageProcessHost : public MessageLoopForIO::IOHandler {
nit: restructure as
class Foo
#if defined(OS_WIN)
    : public A {
#else
    : public B {
#endif

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:54: class
Client {
Add a comment that this can only be accessed on the UI thread.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:63: //
|type|must be TYPE_CONNECT or TYPE_SEND_MESSAGE_REQUEST.
space after |

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:103: void
SendImpl(MessageType type, const std::string& json);
add some method comments for these.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:120:
base::WeakPtr<Client> weak_client_;
This can only be dereferenced on the UI thread. Please add a comment about that
(or maybe put in the name, like weak_client_ui_)

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:121: int
destination_port_;
comment this guy

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File
chrome/browser/extensions/api/messaging/native_message_process_host_posix.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host_posix.cc:51:
LOG(ERROR) << "Bad Read";
ReadMessage already spits out a LOG message on failure. You can remove this one.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:105:
TEST_F(NativeMessagingTest, SingleSendMessageRead) {
Can you add comments briefly explaining what these tests are testing?

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_process_launcher.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.h:23: };
DISALLOW_COPY_AND_ASSIGN

[Aaron Boodman, 2012-08-22 21:47:21]

I'm going to leave the technical bits to Matt until he goes on vacation.

At a higher level, here are the remaining pieces:

- This should only be enabled on the dev channel for now. See feature.cc for how
to do that.

- Please add at least one sample to
chrome/common/extensions/docs/examples/extensions/.

- Docs. For now (until the Windows bit is present), I think the methods should
be marked nodoc so that they don't show up in the generated doc.

http://codereview.chromium.org/10818013/diff/11038/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/message_service.h (right):

http://codereview.chromium.org/10818013/diff/11038/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/message_service.h:63: virtual void
DispatchOnConnect(int dest_port_id,
You should docment all these methods. You won't need to document them in the
implementation classes.

http://codereview.chromium.org/10818013/diff/11038/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/message_service.h:74: virtual
content::RenderProcessHost* process() { return NULL; }
I heard Darin say once that virtual methods should never be unix_hacker style
since they are never inlined.

[eaugusti, 2012-08-31 23:47:13]

Still no Windows, but fixed some issues and added an example extension. 

Is the change in _permission_features.json sufficient to enable this only in
dev?

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/message_service.h:74: virtual
content::RenderProcessHost* process() { return NULL; }
On 2012/08/22 00:02:42, Matt Perry wrote:
> It's not really ideal to expose this method here, since it only makes sense
for
> ExtensionMessagePort. But I can't think of a better alternative (the other
> option is ghetto RTTI which aa hated). At the least, it should be renamed
> render_process_host() to avoid confusion with native_process_.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/message_service.h:74: virtual
content::RenderProcessHost* process() { return NULL; }
On 2012/08/22 21:47:21, Aaron Boodman wrote:
> I heard Darin say once that virtual methods should never be unix_hacker style
> since they are never inlined.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:116:
native_process_handle, read_handle,
On 2012/08/22 00:02:42, Matt Perry wrote:
> indent either 4 spaces or to align with the (

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:123:
base::Bind(callback, process));
On 2012/08/22 00:02:42, Matt Perry wrote:
> The good news is that since MessageService uses a weak pointer, callback won't
> be called if MessageService dies before this runs. The bad news is that
> |process| will leak if that happens. Use a scoped_ptr here and in the callback
> interface to handle that case.

Nice catch, but we also have to make sure process gets deleted on the FILE
thread. Sounds like a job for scoped_malloc_ptr!

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:133:
const std::string& message) {
On 2012/08/22 00:02:42, Matt Perry wrote:
> bad indent

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:155:
std::string* message) {
On 2012/08/22 00:02:42, Matt Perry wrote:
> bad indent

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:164:
pickle.WriteBytes(message_meta_data, 8);
On 2012/08/22 00:02:42, Matt Perry wrote:
> Use "Pickle pickle(message_meta_data, 8)" to avoid copying.

We can't use that because then Pickle assumes that the header is included in the
passed in data.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:23: class
NativeMessageProcessHost : public MessageLoopForIO::IOHandler {
On 2012/08/22 00:02:42, Matt Perry wrote:
> nit: restructure as
> class Foo
> #if defined(OS_WIN)
>     : public A {
> #else
>     : public B {
> #endif

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:54: class
Client {
On 2012/08/22 00:02:42, Matt Perry wrote:
> Add a comment that this can only be accessed on the UI thread.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:63: //
|type|must be TYPE_CONNECT or TYPE_SEND_MESSAGE_REQUEST.
On 2012/08/22 00:02:42, Matt Perry wrote:
> space after |

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:103: void
SendImpl(MessageType type, const std::string& json);
On 2012/08/22 00:02:42, Matt Perry wrote:
> add some method comments for these.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:120:
base::WeakPtr<Client> weak_client_;
On 2012/08/22 00:02:42, Matt Perry wrote:
> This can only be dereferenced on the UI thread. Please add a comment about
that
> (or maybe put in the name, like weak_client_ui_)

... or both!

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:121: int
destination_port_;
On 2012/08/22 00:02:42, Matt Perry wrote:
> comment this guy

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File
chrome/browser/extensions/api/messaging/native_message_process_host_posix.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host_posix.cc:51:
LOG(ERROR) << "Bad Read";
On 2012/08/22 00:02:42, Matt Perry wrote:
> ReadMessage already spits out a LOG message on failure. You can remove this
one.

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:105:
TEST_F(NativeMessagingTest, SingleSendMessageRead) {
On 2012/08/22 00:02:42, Matt Perry wrote:
> Can you add comments briefly explaining what these tests are testing?

Done.

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_process_launcher.h (right):

https://chromiumcodereview.appspot.com/10818013/diff/11038/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.h:23: };
On 2012/08/22 00:02:42, Matt Perry wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Matt Perry, 2012-09-06 00:40:01]

Awesome. LGTM with one last change.

AFAIK, your entry in _permission_features.json is enough. You may want to verify
by hacking VersionInfo to return BETA or STABLE.

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:42:
ScopedNativeMessageProcessHost;
nit: since this typedef is contained in the class, you could shorten its name to
something like "ScopedInstance" or "ScopedHost"

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/test/data/n...
File chrome/test/data/native_messaging/Native Hosts/echo.rb (right):

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/test/data/n...
chrome/test/data/native_messaging/Native Hosts/echo.rb:1: #!/usr/bin/ruby
Using ruby in Chrome code is discouraged. Can you write this in python instead?

[eaugusti, 2012-09-13 17:24:28]

+jam For c/b/renderer_host/chrome_render_message_filter.*

Native app converted to python.

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://chromiumcodereview.appspot.com/10818013/diff/39001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_message_process_host.h:42:
ScopedNativeMessageProcessHost;
On 2012/09/06 00:40:01, Matt Perry wrote:
> nit: since this typedef is contained in the class, you could shorten its name
to
> something like "ScopedInstance" or "ScopedHost"

Done.

[jam, 2012-09-13 21:27:27]

On 2012/09/13 17:24:28, eaugusti wrote:
> +jam For c/b/renderer_host/chrome_render_message_filter.*

lgtm. for future reference, please see the comment in src/chrome/OWNERS

[Isaac (away), 2012-09-26 18:40:28]

This was reverted because it broke the build (i.e. http://goo.gl/OrILM)

Error below:


In file included from chrome/browser/extensions/message_handler.cc:8:0:
./chrome/browser/extensions/api/messaging/message_service.h:15:81: fatal error:
chrome/browser/extensions/api/messaging/native_message_process_host.h: No such
file or directory
compilation terminated.
make: ***
[out/Debug/obj.target/browser_extensions/chrome/browser/extensions/message_handler.o]
Error 1
In file included from chrome/browser/extensions/extension_system.cc:28:0:
./chrome/browser/extensions/api/messaging/message_service.h:15:81: fatal error:
chrome/browser/extensions/api/messaging/native_message_process_host.h: No such
file or directory
compilation terminated.
make: ***
[out/Debug/obj.target/browser_extensions/chrome/browser/extensions/extension_system.o]
Error 1
In file included from chrome/browser/extensions/test_extension_system.cc:9:0:
./chrome/browser/extensions/api/messaging/message_service.h:15:81: fatal error:
chrome/browser/extensions/api/messaging/native_message_process_host.h: No such
file or directory
compilation terminated.
make: ***
[out/Debug/obj.target/test_support_common/chrome/browser/extensions/test_extension_system.o]
Error 1

[Isaac (away), 2012-09-26 18:41:29]

Arggg sorry.  This is the wrong review too.  We reverted the revert.