Refactor the python system_impl module.

mojo/public/python/src/common.cc

Index: mojo/public/python/src/common.cc
diff --git a/mojo/public/python/src/python_system_helper.cc b/mojo/public/python/src/common.cc
similarity index 58%
copy from mojo/public/python/src/python_system_helper.cc
copy to mojo/public/python/src/common.cc
index 22b6bbb4a5d47eff3e459168b387df4e58b3ddf9..43d7f509bf385c19557ac3180819b501a783742b 100644
--- a/mojo/public/python/src/python_system_helper.cc
+++ b/mojo/public/python/src/common.cc
@@ -1,105 +1,104 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
+// Copyright 2013 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 "mojo/public/python/src/python_system_helper.h"
+#include "common.h"
 
-#include "Python.h"
+#include <Python.h>
 
-#include "mojo/public/cpp/environment/environment.h"
+#include "mojo/public/c/environment/async_waiter.h"
+#include "mojo/public/cpp/bindings/callback.h"
+#include "mojo/public/cpp/bindings/lib/shared_ptr.h"
 #include "mojo/public/cpp/environment/logging.h"
+#include "mojo/public/cpp/system/core.h"
 #include "mojo/public/cpp/system/macros.h"
 #include "mojo/public/cpp/utility/run_loop.h"
 
 namespace {
 
-class ScopedGIL {
- public:
-  ScopedGIL() { state_ = PyGILState_Ensure(); }
+void AsyncCallbackForwarder(void* closure, MojoResult result) {
+  mojo::Callback<void(MojoResult)>* callback =
+      static_cast<mojo::Callback<void(MojoResult)>*>(closure);
+  // callback will be deleted when it is run.
+  callback->Run(result);
+}
 
-  ~ScopedGIL() { PyGILState_Release(state_); }
+}  // namespace
 
- private:
-  PyGILState_STATE state_;
+namespace mojo {
+namespace python {
 
-  MOJO_DISALLOW_COPY_AND_ASSIGN(ScopedGIL);
-};
+ScopedGIL::ScopedGIL() {
+  state_ = PyGILState_Ensure();
+}
 
-enum ScopedPyRefAcquire {
-  kAcquire,
-};
+ScopedGIL::~ScopedGIL() {
+  PyGILState_Release(state_);
+}
 
-class ScopedPyRef {
- public:
-  ScopedPyRef(PyObject* object) : object_(object) {}
-  ScopedPyRef(PyObject* object, ScopedPyRefAcquire) : object_(object) {
-    Py_XINCREF(object_);
-  }
+ScopedPyRef::ScopedPyRef(PyObject* object) : object_(object) {
+}
 
-  ~ScopedPyRef() {
-    if (object_) {
-      ScopedGIL acquire_gil;
-      Py_DECREF(object_);
-    }
+ScopedPyRef::ScopedPyRef(PyObject* object, ScopedPyRefAcquire)
+    : object_(object) {
+  Py_XINCREF(object_);
+}
+
+ScopedPyRef::~ScopedPyRef() {
+  if (object_) {
+    ScopedGIL acquire_gil;
+    Py_DECREF(object_);
   }
+}
 
-  operator PyObject*() const { return object_; }
+ScopedPyRef::operator PyObject*() const {
+  return object_;
+}
 
- private:
-  PyObject* object_;
+PythonClosure::PythonClosure(PyObject* callable, const mojo::Closure& quit)
+    : callable_(callable, kAcquire), quit_(quit) {
+  MOJO_DCHECK(callable);
+}
 
-  MOJO_DISALLOW_COPY_AND_ASSIGN(ScopedPyRef);
-};
+PythonClosure::~PythonClosure() {}
 
-class PythonClosure : public mojo::Closure::Runnable {
- public:
-  PythonClosure(PyObject* callable) : callable_(callable, kAcquire) {
-    MOJO_DCHECK(callable);
+void PythonClosure::Run() const {
+  ScopedGIL acquire_gil;
+  ScopedPyRef empty_tuple(PyTuple_New(0));
+  if (!empty_tuple) {
+    quit_.Run();
+    return;
   }
 
-  void Run() const override {
-    ScopedGIL acquire_gil;
-    ScopedPyRef empty_tuple(PyTuple_New(0));
-    if (!empty_tuple) {
-      mojo::RunLoop::current()->Quit();
-      return;
-    }
-
-    ScopedPyRef result(PyObject_CallObject(callable_, empty_tuple));
-    if (!result) {
-      mojo::RunLoop::current()->Quit();
-      return;
-    }
+  ScopedPyRef result(PyObject_CallObject(callable_, empty_tuple));
+  if (!result) {
+    quit_.Run();
+    return;
   }
-
- private:
-  ScopedPyRef callable_;
-
-  MOJO_DISALLOW_COPY_AND_ASSIGN(PythonClosure);
-};
-
-void AsyncCallbackForwarder(void* closure, MojoResult result) {
-  mojo::Callback<void(MojoResult)>* callback =
-      static_cast<mojo::Callback<void(MojoResult)>*>(closure);
-  // callback will be deleted when it is run.
-  callback->Run(result);
 }
 
-}  // namespace
+Closure::Runnable* NewRunnable(PyObject* callable,
+                               const mojo::Closure& quit_closure) {
+  MOJO_DCHECK(PyCallable_Check(callable));
 
-namespace mojo {
-namespace python {
+  return new PythonClosure(callable, quit_closure);
+}
 
 class PythonAsyncWaiter::AsyncWaiterRunnable
     : public mojo::Callback<void(MojoResult)>::Runnable {
  public:
-  AsyncWaiterRunnable(PyObject* callable, CallbackMap* callbacks)
-      : wait_id_(0), callable_(callable, kAcquire), callbacks_(callbacks) {
+  AsyncWaiterRunnable(PyObject* callable,
+                      CallbackMap* callbacks,
+                      const mojo::Closure& quit)
+      : wait_id_(0),
+        callable_(callable, kAcquire),
+        callbacks_(callbacks),
+        quit_(quit) {
     MOJO_DCHECK(callable);
     MOJO_DCHECK(callbacks_);

[qsr, 2014/12/10 15:38:57]

Was there before your code, but no reason to check for the private field once,
and for the parameter the other time, just always check for the same thing.

[etiennej, 2014/12/11 09:52:52]

Done.

   }
 
-  void set_wait_id(int wait_id) { wait_id_ = wait_id; }
+  void set_wait_id(MojoAsyncWaitID wait_id) { wait_id_ = wait_id; }
 
   void Run(MojoResult mojo_result) const override {
     MOJO_DCHECK(wait_id_);
@@ -114,13 +113,13 @@ class PythonAsyncWaiter::AsyncWaiterRunnable
     ScopedGIL acquire_gil;
     ScopedPyRef args_tuple(Py_BuildValue("(i)", mojo_result));
     if (!args_tuple) {
-      mojo::RunLoop::current()->Quit();
+      quit_.Run();
       return;
     }
 
     ScopedPyRef result(PyObject_CallObject(callable_, args_tuple));
     if (!result) {
-      mojo::RunLoop::current()->Quit();
+      quit_.Run();
       return;
     }
   }
@@ -129,19 +128,13 @@ class PythonAsyncWaiter::AsyncWaiterRunnable
   MojoAsyncWaitID wait_id_;
   ScopedPyRef callable_;
   CallbackMap* callbacks_;
+  const mojo::Closure& quit_;
 
   MOJO_DISALLOW_COPY_AND_ASSIGN(AsyncWaiterRunnable);
 };
 
-Closure BuildClosure(PyObject* callable) {
-  if (!PyCallable_Check(callable))
-    return Closure();
-
-  return Closure(
-      static_cast<mojo::Closure::Runnable*>(new PythonClosure(callable)));
-}
-
-PythonAsyncWaiter::PythonAsyncWaiter() {
+PythonAsyncWaiter::PythonAsyncWaiter(const mojo::Closure& quit_closure)
+    : quit_(quit_closure) {
   async_waiter_ = Environment::GetDefaultAsyncWaiter();
 }
 
@@ -157,7 +150,8 @@ MojoAsyncWaitID PythonAsyncWaiter::AsyncWait(MojoHandle handle,
                                              MojoHandleSignals signals,
                                              MojoDeadline deadline,
                                              PyObject* callable) {
-  AsyncWaiterRunnable* runner = new AsyncWaiterRunnable(callable, &callbacks_);
+  AsyncWaiterRunnable* runner =
+      new AsyncWaiterRunnable(callable, &callbacks_, quit_);
   internal::SharedPtr<mojo::Callback<void(MojoResult)>> callback(
       new mojo::Callback<void(MojoResult)>(
           static_cast<mojo::Callback<void(MojoResult)>::Runnable*>(runner)));
@@ -174,6 +168,5 @@ void PythonAsyncWaiter::CancelWait(MojoAsyncWaitID wait_id) {
     callbacks_.erase(wait_id);
   }
 }
-
 }  // namespace python
 }  // namespace mojo