Native Messaging!

chrome/browser/extensions/native_message_process.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_

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

Nit: Maybe call it 'native host process' to go along with the directory name and
other related names?

[Matt Perry, 2012/08/16 00:01:36]

NativeMessageProcessHost?

[eaugusti, 2012/08/21 23:08:35]

NativeMessageProcessHost makes more sense to me.

+#define CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_
+
+#include <deque>
+
+#include "base/file_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/process.h"
+
+namespace extensions {
+class MessageService;
+
+// TODO(eriq): Only posix right now.

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

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.

[eaugusti, 2012/08/21 23:08:35]

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

+// This class lives on the FILE thread.
+// This class ensures that all pipes will be closed and the process will be

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

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.

[eaugusti, 2012/08/21 23:08:35]

Done.

+// killed upon deconstruction.
+//
+// Lifetime Management:
+// This class is owned by a MessageService::MessagePort which lives on the UI
+// thread. This class however lives on the FILE thread.
+// The MessagePort will hold a raw pointer to this class, and post a task for
+// the deletion of this class on the FILE thread from its destructor.
+class NativeMessageProcess : public MessageLoopForIO::Watcher {
+ public:
+  // Append any new types to the end. Changing the ordering will break native
+  // apps.
+  enum MessageType {
+    TYPE_SEND_MESSAGE_REQUEST,  // Used when an extension is sending a one-off
+                                // message to a native app.
+    TYPE_SEND_MESSAGE_RESPONSE,  // Used by a native app to respond to a one-off
+                                 // message.
+    TYPE_CONNECT,  // Used when an extension wants to establish a persistent
+                   // connection with a native app.
+    TYPE_CONNECT_MESSAGE,  // Used for messages after a connection has already
+                           // been established.
+    NUM_MESSAGE_TYPES  // The number of types of messages.
+  };
+
+  // Must be called on the FILE thread.

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

These lines are not necessary everywhere since the class header should explain
that all methods need to be called on FILE thread.

[eaugusti, 2012/08/21 23:08:35]

Done.

+  ~NativeMessageProcess();
+
+  // Must be called on the FILE thread.
+  // |type|must be TYPE_CONNECT or TYPE_SEND_MESSAGE_REQUEST.
+  // |callback| will be called on the UI thread and with a NULL on error.

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

The class header can explain that all callbacks are invoked on UI thread.

[eaugusti, 2012/08/21 23:08:35]

Done.

+  static void Create(base::WeakPtr<MessageService> weak_service,

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

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.

[eaugusti, 2012/08/21 23:08:35]

Done.

+                     const std::string& native_app_name,
+                     const std::string& connection_message,
+                     int dest_port,
+                     MessageType type,
+                     base::Callback<void(NativeMessageProcess*)> callback);
+
+  // TYPE_SEND_MESSAGE_REQUEST will be sent via the connection messege in

[Matt Perry, 2012/08/16 00:01:36]

message*

[eaugusti, 2012/08/21 23:08:35]

Done.

+  // NativeMessageProcess::Create, so only TYPE_CONNECT_MESSAGE is expected.
+  void Send(const std::string& json) {
+    SendImpl(TYPE_CONNECT_MESSAGE, json);
+  }
+
+  // MessageLoopForIO::Watcher
+  virtual void OnFileCanReadWithoutBlocking(int fd);

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

These can be private.

[eaugusti, 2012/08/21 23:08:35]

Done.

+  virtual void OnFileCanWriteWithoutBlocking(int fd);
+
+ private:
+  struct MessageData {

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

A little header describing what this struct represents would be useful.

[eaugusti, 2012/08/21 23:08:35]

Done.

+    MessageData();
+    MessageData(MessageType message_type, std::string message_data);
+
+    MessageType type;
+    std::string data;
+  };
+
+  NativeMessageProcess(
+      base::WeakPtr<MessageService> weak_service,
+      int dest_port,
+      base::ProcessHandle handle,
+      int read_fd,
+      int write_fd,
+      bool is_send_message);
+
+  void SendImpl(MessageType type, const std::string& json);
+
+  bool WriteMessage(int fd, const MessageData& data);
+  bool ReadMessage(int fd, MessageData* data);
+
+  base::WeakPtr<MessageService> weak_service_;
+  int dest_port_;
+  std::deque<MessageData> pending_messages_;
+  base::ProcessHandle handle_;
+
+  MessageLoopForIO::FileDescriptorWatcher read_watcher_;
+  MessageLoopForIO::FileDescriptorWatcher write_watcher_;
+
+  bool watching_write_;
+
+  int read_fd_;
+  int write_fd_;
+  file_util::ScopedFD scoped_read_fd_;
+  file_util::ScopedFD scoped_write_fd_;
+
+  // Only looking for one response.
+  bool is_send_message_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeMessageProcess);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_NATIVE_MESSAGE_PROCESS_H_