Revert "Remote Assistance on Chrome OS Part III - NativeMessageHost"

This reverts commit ba722a2a8ddf967dcadb483ea32d5ad31fc4bdf6.

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

http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%281%29
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/4549
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/4549/steps/browser_tests/logs/NativeMessagingBasic
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/4549/steps/browser_tests/logs/UserLevelNativeMessaging

TBR=kelvinp@chromium.org

Review URL: https://codereview.chromium.org/637463002

Cr-Commit-Position: refs/heads/master@{#298404}

8 files changed, 69 insertions, 185 deletions

diff --git a/remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2 b/remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2
index 5545dc8..9c90357 100644
--- a/remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2
+++ b/remoting/host/it2me/com.google.chrome.remote_assistance.json.jinja2
@@ -3,9 +3,6 @@
   "description": "{{ IT2ME_HOST_DESCRIPTION }}",
   "type": "stdio",
   "path": "{{ IT2ME_HOST_PATH }}",
-  // If you modify the list of allowed_origins, don't forget to update
-  // chrome/browser/extensions/messaging/native_message_host_chromeos.cc
-  // to keep the two lists in sync.
   "allowed_origins": [
     "chrome-extension://ljacajndfccfgnfohlgkdphmbnpkjflk/",
     "chrome-extension://gbchcmhmhahfdphkhkmpfmihenigjmpp/",
diff --git a/remoting/host/it2me/it2me_native_messaging_host.cc b/remoting/host/it2me/it2me_native_messaging_host.cc
index 24ddc1b..8d0d131 100644
--- a/remoting/host/it2me/it2me_native_messaging_host.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host.cc
@@ -9,8 +9,9 @@
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/json/json_reader.h"
-#include "base/json/json_writer.h"
+#include "base/callback_helpers.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringize_macros.h"
 #include "base/threading/thread.h"
@@ -41,8 +42,9 @@ const remoting::protocol::NameMapElement<It2MeHostState> kIt2MeHostStates[] = {
 
 It2MeNativeMessagingHost::It2MeNativeMessagingHost(
     scoped_refptr<AutoThreadTaskRunner> task_runner,
+    scoped_ptr<extensions::NativeMessagingChannel> channel,
     scoped_ptr<It2MeHostFactory> factory)
-    : client_(NULL),
+    : channel_(channel.Pass()),
       factory_(factory.Pass()),
       weak_factory_(this) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
@@ -73,19 +75,27 @@ It2MeNativeMessagingHost::~It2MeNativeMessagingHost() {
   }
 }
 
-void It2MeNativeMessagingHost::OnMessage(const std::string& message) {
+void It2MeNativeMessagingHost::Start(const base::Closure& quit_closure) {
   DCHECK(task_runner()->BelongsToCurrentThread());
+  DCHECK(!quit_closure.is_null());
 
-  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
-  scoped_ptr<base::Value> message_value(base::JSONReader::Read(message));
-  if (!message_value->IsType(base::Value::TYPE_DICTIONARY)) {
+  quit_closure_ = quit_closure;
+
+  channel_->Start(this);
+}
+
+void It2MeNativeMessagingHost::OnMessage(scoped_ptr<base::Value> message) {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+
+  if (!message->IsType(base::Value::TYPE_DICTIONARY)) {
     LOG(ERROR) << "Received a message that's not a dictionary.";
-    client_->CloseChannel(std::string());
+    channel_->SendMessage(nullptr);
     return;
   }
 
   scoped_ptr<base::DictionaryValue> message_dict(
-      static_cast<base::DictionaryValue*>(message_value.release()));
+      static_cast<base::DictionaryValue*>(message.release()));
+  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
 
   // If the client supplies an ID, it will expect it in the response. This
   // might be a string or a number, so cope with both.
@@ -112,17 +122,9 @@ void It2MeNativeMessagingHost::OnMessage(const std::string& message) {
   }
 }
 
-void It2MeNativeMessagingHost::Start(Client* client) {
-  DCHECK(task_runner()->BelongsToCurrentThread());
-  client_ = client;
-}
-
-void It2MeNativeMessagingHost::SendMessageToClient(
-    scoped_ptr<base::DictionaryValue> message) const {
-  DCHECK(task_runner()->BelongsToCurrentThread());
-  std::string message_json;
-  base::JSONWriter::Write(message.get(), &message_json);
-  client_->PostMessageFromNativeHost(message_json);
+void It2MeNativeMessagingHost::OnDisconnect() {
+  if (!quit_closure_.is_null())
+    base::ResetAndReturn(&quit_closure_).Run();
 }
 
 void It2MeNativeMessagingHost::ProcessHello(
@@ -136,7 +138,7 @@ void It2MeNativeMessagingHost::ProcessHello(
   scoped_ptr<base::ListValue> supported_features_list(new base::ListValue());
   response->Set("supportedFeatures", supported_features_list.release());
 
-  SendMessageToClient(response.Pass());
+  channel_->SendMessage(response.Pass());
 }
 
 void It2MeNativeMessagingHost::ProcessConnect(
@@ -210,7 +212,7 @@ void It2MeNativeMessagingHost::ProcessConnect(
                                           directory_bot_jid_);
   it2me_host_->Connect();
 
-  SendMessageToClient(response.Pass());
+  channel_->SendMessage(response.Pass());
 }
 
 void It2MeNativeMessagingHost::ProcessDisconnect(
@@ -222,7 +224,7 @@ void It2MeNativeMessagingHost::ProcessDisconnect(
     it2me_host_->Disconnect();
     it2me_host_ = NULL;
   }
-  SendMessageToClient(response.Pass());
+  channel_->SendMessage(response.Pass());
 }
 
 void It2MeNativeMessagingHost::SendErrorAndExit(
@@ -234,10 +236,10 @@ void It2MeNativeMessagingHost::SendErrorAndExit(
 
   response->SetString("type", "error");
   response->SetString("description", description);
-  SendMessageToClient(response.Pass());
+  channel_->SendMessage(response.Pass());
 
-  // Trigger a host shutdown by sending an empty message.
-  client_->CloseChannel(std::string());
+  // Trigger a host shutdown by sending a NULL message.
+  channel_->SendMessage(nullptr);
 }
 
 void It2MeNativeMessagingHost::OnStateChanged(It2MeHostState state) {
@@ -270,7 +272,7 @@ void It2MeNativeMessagingHost::OnStateChanged(It2MeHostState state) {
       ;
   }
 
-  SendMessageToClient(message.Pass());
+  channel_->SendMessage(message.Pass());
 }
 
 void It2MeNativeMessagingHost::OnNatPolicyChanged(bool nat_traversal_enabled) {
@@ -280,7 +282,7 @@ void It2MeNativeMessagingHost::OnNatPolicyChanged(bool nat_traversal_enabled) {
 
   message->SetString("type", "natPolicyChanged");
   message->SetBoolean("natTraversalEnabled", nat_traversal_enabled);
-  SendMessageToClient(message.Pass());
+  channel_->SendMessage(message.Pass());
 }
 
 // Stores the Access Code for the web-app to query.
@@ -301,7 +303,7 @@ void It2MeNativeMessagingHost::OnClientAuthenticated(
   client_username_ = client_username;
 }
 
-scoped_refptr<base::SingleThreadTaskRunner>
+scoped_refptr<AutoThreadTaskRunner>
 It2MeNativeMessagingHost::task_runner() const {
   return host_context_->ui_task_runner();
 }
diff --git a/remoting/host/it2me/it2me_native_messaging_host.h b/remoting/host/it2me/it2me_native_messaging_host.h
index 18e738b..f9d8ac9 100644
--- a/remoting/host/it2me/it2me_native_messaging_host.h
+++ b/remoting/host/it2me/it2me_native_messaging_host.h
@@ -8,38 +8,40 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "extensions/browser/api/messaging/native_message_host.h"
+#include "extensions/browser/api/messaging/native_messaging_channel.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/host/it2me/it2me_host.h"
 
 namespace base {
 class DictionaryValue;
-class Value;
 }  // namespace base
 
 namespace remoting {
 
 // Implementation of the native messaging host process.
-class It2MeNativeMessagingHost : public It2MeHost::Observer,
-                                 public extensions::NativeMessageHost {
+class It2MeNativeMessagingHost
+    : public It2MeHost::Observer,
+      public extensions::NativeMessagingChannel::EventHandler {
  public:
-  It2MeNativeMessagingHost(scoped_refptr<AutoThreadTaskRunner> task_runner,
-                           scoped_ptr<It2MeHostFactory> factory);
+  It2MeNativeMessagingHost(
+      scoped_refptr<AutoThreadTaskRunner> task_runner,
+      scoped_ptr<extensions::NativeMessagingChannel> channel,
+      scoped_ptr<It2MeHostFactory> factory);
   virtual ~It2MeNativeMessagingHost();
 
-  // extensions::NativeMessageHost implementation.
-  virtual void OnMessage(const std::string& message) override;
-  virtual void Start(Client* client) override;
-  virtual scoped_refptr<base::SingleThreadTaskRunner> task_runner()
-      const override;
+  void Start(const base::Closure& quit_closure);
+
+  // extensions::NativeMessagingChannel::EventHandler implementation.
+  virtual void OnMessage(scoped_ptr<base::Value> message) OVERRIDE;
+  virtual void OnDisconnect() OVERRIDE;
 
   // It2MeHost::Observer implementation.
   virtual void OnClientAuthenticated(const std::string& client_username)
-      override;
+      OVERRIDE;
   virtual void OnStoreAccessCode(const std::string& access_code,
-                                 base::TimeDelta access_code_lifetime) override;
-  virtual void OnNatPolicyChanged(bool nat_traversal_enabled) override;
-  virtual void OnStateChanged(It2MeHostState state) override;
+                                 base::TimeDelta access_code_lifetime) OVERRIDE;
+  virtual void OnNatPolicyChanged(bool nat_traversal_enabled) OVERRIDE;
+  virtual void OnStateChanged(It2MeHostState state) OVERRIDE;
 
   static std::string HostStateToString(It2MeHostState host_state);
 
@@ -55,9 +57,12 @@ class It2MeNativeMessagingHost : public It2MeHost::Observer,
                          scoped_ptr<base::DictionaryValue> response);
   void SendErrorAndExit(scoped_ptr<base::DictionaryValue> response,
                         const std::string& description) const;
-  void SendMessageToClient(scoped_ptr<base::DictionaryValue> message) const;
 
-  Client* client_;
+  base::Closure quit_closure_;
+
+  scoped_refptr<AutoThreadTaskRunner> task_runner() const;
+
+  scoped_ptr<extensions::NativeMessagingChannel> channel_;
   scoped_ptr<It2MeHostFactory> factory_;
   scoped_ptr<ChromotingHostContext> host_context_;
   scoped_refptr<It2MeHost> it2me_host_;
diff --git a/remoting/host/it2me/it2me_native_messaging_host_main.cc b/remoting/host/it2me/it2me_native_messaging_host_main.cc
index 7d27d19..25f5021 100644
--- a/remoting/host/it2me/it2me_native_messaging_host_main.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host_main.cc
@@ -16,7 +16,6 @@
 #include "remoting/host/host_exit_codes.h"
 #include "remoting/host/it2me/it2me_native_messaging_host.h"
 #include "remoting/host/logging.h"
-#include "remoting/host/native_messaging/native_messaging_pipe.h"
 #include "remoting/host/native_messaging/pipe_messaging_channel.h"
 #include "remoting/host/usage_stats_consent.h"
 
@@ -118,21 +117,13 @@ int StartIt2MeNativeMessagingHost() {
 
   scoped_ptr<It2MeHostFactory> factory(new It2MeHostFactory());
 
-  scoped_ptr<NativeMessagingPipe> native_messaging_pipe(
-      new NativeMessagingPipe());
-
   // Set up the native messaging channel.
   scoped_ptr<extensions::NativeMessagingChannel> channel(
       new PipeMessagingChannel(read_file.Pass(), write_file.Pass()));
 
-  scoped_ptr<extensions::NativeMessageHost> host(new It2MeNativeMessagingHost(
-      task_runner,
-      factory.Pass()));
-
-  host->Start(native_messaging_pipe.get());
-
-  native_messaging_pipe->Start(
-      host.Pass(), channel.Pass(), run_loop.QuitClosure());
+  scoped_ptr<It2MeNativeMessagingHost> host(new It2MeNativeMessagingHost(
+      task_runner, channel.Pass(), factory.Pass()));
+  host->Start(run_loop.QuitClosure());
 
   // Run the loop until channel is alive.
   run_loop.Run();
diff --git a/remoting/host/it2me/it2me_native_messaging_host_unittest.cc b/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
index 61334cf..56515a9 100644
--- a/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
@@ -17,7 +17,6 @@
 #include "net/base/net_util.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/host/chromoting_host_context.h"
-#include "remoting/host/native_messaging/native_messaging_pipe.h"
 #include "remoting/host/native_messaging/pipe_messaging_channel.h"
 #include "remoting/host/setup/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -79,9 +78,9 @@ class MockIt2MeHost : public It2MeHost {
                   directory_bot_jid) {}
 
   // It2MeHost overrides
-  virtual void Connect() override;
-  virtual void Disconnect() override;
-  virtual void RequestNatPolicy() override;
+  virtual void Connect() OVERRIDE;
+  virtual void Disconnect() OVERRIDE;
+  virtual void RequestNatPolicy() OVERRIDE;
 
  private:
   virtual ~MockIt2MeHost() {}
@@ -154,7 +153,7 @@ class MockIt2MeHostFactory : public It2MeHostFactory {
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
       base::WeakPtr<It2MeHost::Observer> observer,
       const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
-      const std::string& directory_bot_jid) override {
+      const std::string& directory_bot_jid) OVERRIDE {
     return new MockIt2MeHost(
         context, task_runner, observer, xmpp_server_config, directory_bot_jid);
   }
@@ -168,8 +167,8 @@ class It2MeNativeMessagingHostTest : public testing::Test {
   It2MeNativeMessagingHostTest() {}
   virtual ~It2MeNativeMessagingHostTest() {}
 
-  virtual void SetUp() override;
-  virtual void TearDown() override;
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
 
  protected:
   scoped_ptr<base::DictionaryValue> ReadMessageFromOutputPipe();
@@ -210,7 +209,7 @@ class It2MeNativeMessagingHostTest : public testing::Test {
 
   // Task runner of the host thread.
   scoped_refptr<AutoThreadTaskRunner> host_task_runner_;
-  scoped_ptr<remoting::NativeMessagingPipe> pipe_;
+  scoped_ptr<remoting::It2MeNativeMessagingHost> host_;
 
   DISALLOW_COPY_AND_ASSIGN(It2MeNativeMessagingHostTest);
 };
@@ -432,21 +431,15 @@ void It2MeNativeMessagingHostTest::StartHost() {
   // Creating a native messaging host with a mock It2MeHostFactory.
   scoped_ptr<It2MeHostFactory> factory(new MockIt2MeHostFactory());
 
-  pipe_.reset(new NativeMessagingPipe());
-
   scoped_ptr<extensions::NativeMessagingChannel> channel(
       new PipeMessagingChannel(input_read_file.Pass(),
                                output_write_file.Pass()));
 
-  scoped_ptr<extensions::NativeMessageHost> it2me_host(
-      new It2MeNativeMessagingHost(
-          host_task_runner_,
-          factory.Pass()));
-  it2me_host->Start(pipe_.get());
-
-  pipe_->Start(it2me_host.Pass(),
-               channel.Pass(),
-               base::Bind(&It2MeNativeMessagingHostTest::StopHost,
+  host_.reset(new It2MeNativeMessagingHost(
+      host_task_runner_,
+      channel.Pass(),
+      factory.Pass()));
+  host_->Start(base::Bind(&It2MeNativeMessagingHostTest::StopHost,
                           base::Unretained(this)));
 
   // Notify the test that the host has finished starting up.
@@ -457,7 +450,7 @@ void It2MeNativeMessagingHostTest::StartHost() {
 void It2MeNativeMessagingHostTest::StopHost() {
   DCHECK(host_task_runner_->RunsTasksOnCurrentThread());
 
-  pipe_.reset();
+  host_.reset();
 
   // Wait till all shutdown tasks have completed.
   base::RunLoop().RunUntilIdle();
diff --git a/remoting/host/native_messaging/native_messaging_pipe.cc b/remoting/host/native_messaging/native_messaging_pipe.cc
deleted file mode 100644
index 13abc35..0000000
--- a/remoting/host/native_messaging/native_messaging_pipe.cc
+++ /dev/null
@@ -1,52 +0,0 @@
-// Copyright 2014 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.
-
-#include "remoting/host/native_messaging/native_messaging_pipe.h"
-
-#include "base/callback_helpers.h"
-#include "base/json/json_reader.h"
-#include "base/json/json_writer.h"
-#include "base/values.h"
-
-namespace remoting {
-
-NativeMessagingPipe::NativeMessagingPipe() {
-}
-
-NativeMessagingPipe::~NativeMessagingPipe() {
-}
-
-void NativeMessagingPipe::Start(
-    scoped_ptr<extensions::NativeMessageHost> host,
-    scoped_ptr<extensions::NativeMessagingChannel> channel,
-    const base::Closure& quit_closure) {
-  host_ = host.Pass();
-  channel_ = channel.Pass();
-  quit_closure_ = quit_closure;
-  channel_->Start(this);
-}
-
-void NativeMessagingPipe::OnMessage(scoped_ptr<base::Value> message) {
-  std::string message_json;
-  base::JSONWriter::Write(message.get(), &message_json);
-  host_->OnMessage(message_json);
-}
-
-void NativeMessagingPipe::OnDisconnect() {
-  if (!quit_closure_.is_null())
-    base::ResetAndReturn(&quit_closure_).Run();
-}
-
-void NativeMessagingPipe::PostMessageFromNativeHost(
-    const std::string& message) {
-  scoped_ptr<base::Value> json(base::JSONReader::Read(message));
-  channel_->SendMessage(json.Pass());
-}
-
-void NativeMessagingPipe::CloseChannel(const std::string& error_message) {
-  if (!quit_closure_.is_null())
-    base::ResetAndReturn(&quit_closure_).Run();
-}
-
-}  // namespace remoting
diff --git a/remoting/host/native_messaging/native_messaging_pipe.h b/remoting/host/native_messaging/native_messaging_pipe.h
deleted file mode 100644
index 24a16f7..0000000
--- a/remoting/host/native_messaging/native_messaging_pipe.h
+++ /dev/null
@@ -1,50 +0,0 @@
-// Copyright 2014 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 REMOTING_HOST_NATIVE_MESSAGING_NATIVE_MESSAGING_PIPE_H_
-#define REMOTING_HOST_NATIVE_MESSAGING_NATIVE_MESSAGING_PIPE_H_
-
-#include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
-#include "extensions/browser/api/messaging/native_message_host.h"
-#include "extensions/browser/api/messaging/native_messaging_channel.h"
-
-namespace base {
-class Value;
-}
-
-namespace remoting {
-
-// Connects a extensions::NativeMessageHost to a PipeMessagingChannel.
-class NativeMessagingPipe
-    : public extensions::NativeMessagingChannel::EventHandler,
-      public extensions::NativeMessageHost::Client {
- public:
-  NativeMessagingPipe();
-  virtual ~NativeMessagingPipe();
-
-  // Starts processing messages from the pipe.
-  void Start(scoped_ptr<extensions::NativeMessageHost> host,
-             scoped_ptr<extensions::NativeMessagingChannel> channel,
-             const base::Closure& quit_closure);
-
-  // extensions::NativeMessageHost::Client implementation.
-  virtual void PostMessageFromNativeHost(const std::string& message) override;
-  virtual void CloseChannel(const std::string& error_message) override;
-
-  // extensions::NativeMessagingChannel::EventHandler implementation.
-  virtual void OnMessage(scoped_ptr<base::Value> message) override;
-  virtual void OnDisconnect() override;
-
- private:
-  base::Closure quit_closure_;
-  scoped_ptr<extensions::NativeMessagingChannel> channel_;
-  scoped_ptr<extensions::NativeMessageHost> host_;
-
-  DISALLOW_COPY_AND_ASSIGN(NativeMessagingPipe);
-};
-
-}  // namespace remoting
-
-#endif  // REMOTING_HOST_NATIVE_MESSAGING_NATIVE_MESSAGING_PIPE_H_
diff --git a/remoting/remoting_host.gypi b/remoting/remoting_host.gypi
index 5538700..eca5646 100644
--- a/remoting/remoting_host.gypi
+++ b/remoting/remoting_host.gypi
@@ -431,8 +431,6 @@
           'sources': [
             'host/native_messaging/pipe_messaging_channel.cc',
             'host/native_messaging/pipe_messaging_channel.h',
-            'host/native_messaging/native_messaging_pipe.cc',
-            'host/native_messaging/native_messaging_pipe.h',
             'host/native_messaging/native_messaging_reader.cc',
             'host/native_messaging/native_messaging_reader.h',
             'host/native_messaging/native_messaging_writer.cc',