[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo, consuming the new Watch API instead. For convenience a new mojo::Watcher is added to the public Mojo C++ API library, and this is used by Connector. BUG=590495 R=yzshen@chromium.org TBR=blundell@chromium.org for rename affecting components/message_port.gypi TBR=jam@chromium.org - added a missing header to new url tests Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add Cr-Commit-Position: refs/heads/master@{#379402} Review URL: https://codereview.chromium.org/1759783003 Cr-Commit-Position: refs/heads/master@{#379463}
marcosholgado · Mar 5, 2016 · 2cdb2f6 · 2cdb2f6
1 parent 30abe5a
commit 2cdb2f6
Show file tree

Hide file tree

Showing 17 changed files with 483 additions and 43 deletions.
diff --git a/components/BUILD.gn b/components/BUILD.gn
@@ -69,7 +69,7 @@ test("components_unittests") {
     "//components/variations:unit_tests",
     "//components/variations/service:unit_tests",
     "//components/web_resource:unit_tests",
-    "//mojo/edk/embedder:headers",
+    "//mojo/edk/system",
     "//net",
     "//testing/gtest",
     "//ui/base",

diff --git a/components/message_port.gypi b/components/message_port.gypi
@@ -10,7 +10,7 @@
       'dependencies': [
         '../base/base.gyp:base',
         '../mojo/mojo_public.gyp:mojo_message_pump_lib',
-        '../mojo/mojo_public.gyp:mojo_system_cpp_headers',
+        '../mojo/mojo_public.gyp:mojo_cpp_system',
         '../third_party/WebKit/public/blink.gyp:blink',
       ],
       'include_dirs': [

diff --git a/mojo/mojo_edk_tests.gyp b/mojo/mojo_edk_tests.gyp
@@ -150,6 +150,7 @@
       'dependencies': [
         '../testing/gtest.gyp:gtest',
         'mojo_edk.gyp:mojo_run_all_unittests',
+        'mojo_public.gyp:mojo_cpp_system',
         'mojo_public.gyp:mojo_public_test_utils',
       ],
       'sources': [

diff --git a/mojo/mojo_public.gyp b/mojo/mojo_public.gyp
@@ -74,8 +74,8 @@
     },
     {
       # GN version: //mojo/public/cpp/system
-      'target_name': 'mojo_system_cpp_headers',
-      'type': 'none',
+      'target_name': 'mojo_cpp_system',
+      'type': 'static_library',
       'sources': [
         'public/cpp/system/buffer.h',
         'public/cpp/system/core.h',
@@ -84,8 +84,11 @@
         'public/cpp/system/handle.h',
         'public/cpp/system/macros.h',
         'public/cpp/system/message_pipe.h',
+        'public/cpp/system/watcher.cc',
+        'public/cpp/system/watcher.h',
       ],
       'dependencies': [
+        '../base/base.gyp:base',
         'mojo_system_headers',
       ],
     },
@@ -187,7 +190,7 @@
       ],
       'dependencies': [
         '../base/base.gyp:base',
-        'mojo_message_pump_lib',
+        'mojo_cpp_system',
         'mojo_interface_bindings_cpp_sources',
       ],
     },

diff --git a/mojo/mojo_variables.gypi b/mojo/mojo_variables.gypi
@@ -45,6 +45,7 @@
       '<(DEPTH)/mojo/public/c/system/tests/macros_unittest.cc',
       '<(DEPTH)/mojo/public/cpp/system/tests/core_unittest.cc',
       '<(DEPTH)/mojo/public/cpp/system/tests/macros_unittest.cc',
+      '<(DEPTH)/mojo/public/cpp/system/tests/watcher_unittest.cc',
     ],
   },
 }
diff --git a/mojo/public/cpp/bindings/BUILD.gn b/mojo/public/cpp/bindings/BUILD.gn
@@ -101,7 +101,6 @@ source_set("bindings") {
 
   deps = [
     "//base",
-    "//mojo/message_pump",
     "//mojo/public/interfaces/bindings:bindings_cpp_sources",
   ]
 }

diff --git a/mojo/public/cpp/bindings/lib/connector.cc b/mojo/public/cpp/bindings/lib/connector.cc
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_task_runner_handle.h"
 #include "mojo/public/cpp/bindings/lib/sync_handle_watcher.h"
 
 namespace mojo {
@@ -247,7 +248,7 @@ bool Connector::RunSyncHandleWatch(const bool* should_stop) {
   return SyncHandleWatcher::current()->WatchAllHandles(should_stop_array, 2);
 }
 
-void Connector::OnHandleWatcherHandleReady(MojoResult result) {
+void Connector::OnWatcherHandleReady(MojoResult result) {
   OnHandleReadyInternal(result);
 }
 
@@ -273,12 +274,21 @@ void Connector::OnHandleReadyInternal(MojoResult result) {
 }
 
 void Connector::WaitToReadMore() {
-  CHECK(!handle_watcher_.is_watching());
   CHECK(!paused_);
-  handle_watcher_.Start(message_pipe_.get(), MOJO_HANDLE_SIGNAL_READABLE,
-                        MOJO_DEADLINE_INDEFINITE,
-                        base::Bind(&Connector::OnHandleWatcherHandleReady,
-                                   base::Unretained(this)));
+  DCHECK(!handle_watcher_.IsWatching());
+
+  MojoResult rv = handle_watcher_.Start(
+      message_pipe_.get(), MOJO_HANDLE_SIGNAL_READABLE,
+      base::Bind(&Connector::OnWatcherHandleReady,
+                 base::Unretained(this)));
+
+  if (rv != MOJO_RESULT_OK) {
+    // If the watch failed because the handle is invalid or its conditions can
+    // no longer be met, we signal the error asynchronously to avoid reentry.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&Connector::OnWatcherHandleReady,
+                              weak_factory_.GetWeakPtr(), rv));
+  }
 
   if (register_sync_handle_watch_count_ > 0 &&
       !registered_with_sync_handle_watcher_) {
@@ -304,13 +314,6 @@ bool Connector::ReadSingleMessage(MojoResult* read_result) {
   *read_result = rv;
 
   if (rv == MOJO_RESULT_OK) {
-    // Dispatching the message may spin in a nested message loop. To ensure we
-    // continue dispatching messages when this happens start listening for
-    // messagse now.
-    if (!handle_watcher_.is_watching()) {
-      // TODO: Need to evaluate the perf impact of this.
-      WaitToReadMore();
-    }
     receiver_result =
         incoming_receiver_ && incoming_receiver_->Accept(&message);
   }
@@ -344,21 +347,13 @@ void Connector::ReadAllAvailableMessages() {
     if (paused_)
       return;
 
-    if (rv == MOJO_RESULT_SHOULD_WAIT) {
-      // ReadSingleMessage could end up calling HandleError which resets
-      // message_pipe_ to a dummy one that is closed. The old EDK will see the
-      // that the peer is closed immediately, while the new one is asynchronous
-      // because of thread hops. In that case, there'll still be an async
-      // waiter.
-      if (!handle_watcher_.is_watching())
-        WaitToReadMore();
+    if (rv == MOJO_RESULT_SHOULD_WAIT)
       break;
-    }
   }
 }
 
 void Connector::CancelWait() {
-  handle_watcher_.Stop();
+  handle_watcher_.Cancel();
 
   if (registered_with_sync_handle_watcher_) {
     SyncHandleWatcher::current()->UnregisterHandle(message_pipe_.get());
@@ -394,8 +389,6 @@ void Connector::HandleError(bool force_pipe_reset, bool force_async_handler) {
   }
 
   if (force_async_handler) {
-    // |dummy_pipe.handle1| has been destructed. Reading the pipe will
-    // eventually cause a read error on |message_pipe_| and set error state.
     if (!paused_)
       WaitToReadMore();
   } else {

diff --git a/mojo/public/cpp/bindings/lib/connector.h b/mojo/public/cpp/bindings/lib/connector.h
@@ -9,10 +9,10 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
-#include "mojo/message_pump/handle_watcher.h"
 #include "mojo/public/cpp/bindings/callback.h"
 #include "mojo/public/cpp/bindings/message.h"
 #include "mojo/public/cpp/system/core.h"
+#include "mojo/public/cpp/system/watcher.h"
 
 namespace base {
 class Lock;
@@ -141,8 +141,8 @@ class Connector : public MessageReceiver {
   }
 
  private:
-  // Callback of mojo::common::HandleWatcher.
-  void OnHandleWatcherHandleReady(MojoResult result);
+  // Callback of mojo::Watcher.
+  void OnWatcherHandleReady(MojoResult result);
   // Callback of SyncHandleWatcher.
   void OnSyncHandleWatcherHandleReady(MojoResult result);
   void OnHandleReadyInternal(MojoResult result);
@@ -169,7 +169,7 @@ class Connector : public MessageReceiver {
   ScopedMessagePipeHandle message_pipe_;
   MessageReceiver* incoming_receiver_;
 
-  common::HandleWatcher handle_watcher_;
+  Watcher handle_watcher_;
 
   bool error_;
   bool drop_writes_;

diff --git a/mojo/public/cpp/system/BUILD.gn b/mojo/public/cpp/system/BUILD.gn
@@ -11,6 +11,8 @@ source_set("system") {
     "handle.h",
     "macros.h",
     "message_pipe.h",
+    "watcher.cc",
+    "watcher.h",
   ]
 
   public_deps = [

diff --git a/mojo/public/cpp/system/tests/BUILD.gn b/mojo/public/cpp/system/tests/BUILD.gn
@@ -8,9 +8,11 @@ source_set("tests") {
   sources = [
     "core_unittest.cc",
     "macros_unittest.cc",
+    "watcher_unittest.cc",
   ]
 
   deps = [
+    "//base",
     "//mojo/public/c/system/tests",
     "//mojo/public/cpp/environment:standalone",
     "//mojo/public/cpp/system",