Refactor the python system_impl module.

Refactor the python system_impl module.

Extract reusable code into a common library and isolate
environment-specific logic. This allows the upcoming python content
handler to implement only the base::-specific logic around RunLoop and
AsyncWaiter before injecting it.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/0e776c2f77c489164b512bb7e33640088caadc55

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

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (left):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:23: configs = [ "../build/config:mojo_sdk" ]
You need to keep those.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:35: python_binary_module_sources("system_src") {
why are the dependencies different for this one and for system? If you can
remove dependency here, you can probably remove those for system.

Once this is the case, you might want to have those files and dependencies in
variables, and using those for both targets. And maybe, rework with the gni so
that you can not have any sources at all, and just depend on the source module
in the binary module.

Also, maybe rename system_src to system_embedded

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:85: include_dirs = [ "/usr/include/python2.7" ]
That's incorrect. You need to work with the rules.gni in cython, and use some
configuration from this so that this could work on other platform than linux or
with distribution that have python at some other place.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/mojo/syst...
File mojo/public/python/mojo/system_impl.pyx (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/mojo/syst...
mojo/public/python/mojo/system_impl.pyx:63: self._cenvironment = new
c_environment_common.CEnvironment()
You might not want to put Environment in c_environment_common (and maybe rename
it then), because instantiating an environment in chromium will not compile.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
File mojo/public/python/src/common.cc (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
mojo/public/python/src/common.cc:98: MOJO_DCHECK(callbacks_);
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.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/common.h
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
mojo/public/python/src/common.h:66: Closure::Runnable* NewRunnable(PyObject*
callable, const Closure& quit_closure);
I'm at fault, but this name is not very good -> way too generic. What about
NewRunnableFromCallable?

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/pytho...
File mojo/public/python/src/python_system_helper.cc (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/pytho...
mojo/public/python/src/python_system_helper.cc:39:
mojo::Closure(QuitCurrentRunLoop::NewInstance()));
You always create a mojo::Closure from the runnable, I guess you could change
NewInstance to be NewQuitClosure and directly return the closure.

[etiennej, 2014-12-11 09:52:53]

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (left):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:23: configs = [ "../build/config:mojo_sdk" ]
On 2014/12/10 15:38:57, qsr wrote:
> You need to keep those.

Done.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:35: python_binary_module_sources("system_src") {
On 2014/12/10 15:38:57, qsr wrote:
> why are the dependencies different for this one and for system? If you can
> remove dependency here, you can probably remove those for system.
> 
> Once this is the case, you might want to have those files and dependencies in
> variables, and using those for both targets. And maybe, rework with the gni so
> that you can not have any sources at all, and just depend on the source module
> in the binary module.
> 
> Also, maybe rename system_src to system_embedded

Done.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/BUILD.gn#...
mojo/public/python/BUILD.gn:85: include_dirs = [ "/usr/include/python2.7" ]
On 2014/12/10 15:38:57, qsr wrote:
> That's incorrect. You need to work with the rules.gni in cython, and use some
> configuration from this so that this could work on other platform than linux
or
> with distribution that have python at some other place.

Done.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/mojo/syst...
File mojo/public/python/mojo/system_impl.pyx (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/mojo/syst...
mojo/public/python/mojo/system_impl.pyx:63: self._cenvironment = new
c_environment_common.CEnvironment()
On 2014/12/10 15:38:57, qsr wrote:
> You might not want to put Environment in c_environment_common (and maybe
rename
> it then), because instantiating an environment in chromium will not compile.

Done (not sure about the renaming part).

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
File mojo/public/python/src/common.cc (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
mojo/public/python/src/common.cc:98: MOJO_DCHECK(callbacks_);
On 2014/12/10 15:38:57, qsr wrote:
> 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.

Done.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/common.h
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/commo...
mojo/public/python/src/common.h:66: Closure::Runnable* NewRunnable(PyObject*
callable, const Closure& quit_closure);
On 2014/12/10 15:38:57, qsr wrote:
> I'm at fault, but this name is not very good -> way too generic. What about
> NewRunnableFromCallable?

Done.

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/pytho...
File mojo/public/python/src/python_system_helper.cc (right):

https://codereview.chromium.org/792843002/diff/1/mojo/public/python/src/pytho...
mojo/public/python/src/python_system_helper.cc:39:
mojo::Closure(QuitCurrentRunLoop::NewInstance()));
On 2014/12/10 15:38:57, qsr wrote:
> You always create a mojo::Closure from the runnable, I guess you could change
> NewInstance to be NewQuitClosure and directly return the closure.

Done.

[etiennej, 2014-12-11 10:59:14]

Reworked rules.gni

[qsr, 2014-12-12 09:09:54]

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.cc (left):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:177: 
Please keep this blank line.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.cc (right):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:97: MOJO_DCHECK(callable_);
MOJO_DCHECK(callbacks_);
Did you run git cl format? Having 2 calls on the same line doesn't seem to fit
the C++ style.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:130: const mojo::Closure& quit_;
This seems wrong. What ensures you that the reference you keep will be kept
alive. You probably wants a copy of the closure here.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.h:18: namespace python {
Blank line after this.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.h:66: Closure::Runnable*
NewRunnableFromClosure(PyObject* callable,
Can you call this NewRunnableFromCallable instead of FromClosure? Closure really
makes me think to either base::Closure to mojo::Closure.

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
File third_party/cython/rules.gni (right):

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
third_party/cython/rules.gni:51: deps = []
Could you run gn format, I'm pretty sure deps should be the last thing in the
target.

[etiennej, 2014-12-12 10:58:25]

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.cc (left):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:177: 
On 2014/12/12 09:09:53, qsr wrote:
> Please keep this blank line.

Done.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.cc (right):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:97: MOJO_DCHECK(callable_);
MOJO_DCHECK(callbacks_);
On 2014/12/12 09:09:53, qsr wrote:
> Did you run git cl format? Having 2 calls on the same line doesn't seem to fit
> the C++ style.

Done.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.cc:130: const mojo::Closure& quit_;
On 2014/12/12 09:09:53, qsr wrote:
> This seems wrong. What ensures you that the reference you keep will be kept
> alive. You probably wants a copy of the closure here.

I went overboard on the copy/paste. Fixed.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.h:18: namespace python {
On 2014/12/12 09:09:53, qsr wrote:
> Blank line after this.

Done.

https://codereview.chromium.org/792843002/diff/60001/mojo/public/python/src/c...
mojo/public/python/src/common.h:66: Closure::Runnable*
NewRunnableFromClosure(PyObject* callable,
On 2014/12/12 09:09:53, qsr wrote:
> Can you call this NewRunnableFromCallable instead of FromClosure? Closure
really
> makes me think to either base::Closure to mojo::Closure.

Done.

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
File third_party/cython/rules.gni (right):

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
third_party/cython/rules.gni:51: deps = []
On 2014/12/12 09:09:54, qsr wrote:
> Could you run gn format, I'm pretty sure deps should be the last thing in the
> target.

I do not mind moving it, but I ran gn format (both now, and as a presubmit
before sending the CL) and there's no change.

[etiennej, 2014-12-12 12:23:28]

I just rebase this CL on HEAD, and there seems to be issues. I'll investigate
and ping the thread once it is done.

[qsr, 2014-12-12 12:27:46]

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
File third_party/cython/rules.gni (right):

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
third_party/cython/rules.gni:51: deps = []
On 2014/12/12 10:58:25, etiennej wrote:
> On 2014/12/12 09:09:54, qsr wrote:
> > Could you run gn format, I'm pretty sure deps should be the last thing in
the
> > target.
> 
> I do not mind moving it, but I ran gn format (both now, and as a presubmit
> before sending the CL) and there's no change.

 Hum, weird. Can you take a look at
https://code.google.com/p/chromium/wiki/GNStyleGuide and make sure you follow
the syle, in particular for the ordering of the variables. Thanks.

 Also, this still should be pushed in the chromium repo and not the mojo repo.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD...
mojo/public/python/BUILD.gn:27: "mojo/c_core.pxd",
Alphabetize, here and below.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD...
mojo/public/python/BUILD.gn:66: "src/common.h",
Alphabetize.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/mojo/...
File mojo/public/python/mojo/c_thunks.pxd (right):

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/mojo/...
mojo/public/python/mojo/c_thunks.pxd:8: cdef extern from
"third_party/cython/python_export.h":
I don't think you need this one.

[etiennej, 2014-12-12 13:39:46]

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
File third_party/cython/rules.gni (right):

https://codereview.chromium.org/792843002/diff/60001/third_party/cython/rules...
third_party/cython/rules.gni:51: deps = []
On 2014/12/12 12:27:45, qsr wrote:
> On 2014/12/12 10:58:25, etiennej wrote:
> > On 2014/12/12 09:09:54, qsr wrote:
> > > Could you run gn format, I'm pretty sure deps should be the last thing in
> the
> > > target.
> > 
> > I do not mind moving it, but I ran gn format (both now, and as a presubmit
> > before sending the CL) and there's no change.
> 
>  Hum, weird. Can you take a look at
> https://code.google.com/p/chromium/wiki/GNStyleGuide and make sure you follow
> the syle, in particular for the ordering of the variables. Thanks.
> 
>  Also, this still should be pushed in the chromium repo and not the mojo repo.

Made the fixes. I'll prepare a CL in chromium with the same changes.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD.gn
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD...
mojo/public/python/BUILD.gn:27: "mojo/c_core.pxd",
On 2014/12/12 12:27:46, qsr wrote:
> Alphabetize, here and below.

Done.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/BUILD...
mojo/public/python/BUILD.gn:66: "src/common.h",
On 2014/12/12 12:27:46, qsr wrote:
> Alphabetize.

Done.

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/mojo/...
File mojo/public/python/mojo/c_thunks.pxd (right):

https://codereview.chromium.org/792843002/diff/80001/mojo/public/python/mojo/...
mojo/public/python/mojo/c_thunks.pxd:8: cdef extern from
"third_party/cython/python_export.h":
On 2014/12/12 12:27:46, qsr wrote:
> I don't think you need this one.

I do need it. Added a comment.

[qsr, 2014-12-12 14:27:06]

LGTM with nits.

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/BUIL...
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/BUIL...
mojo/public/python/BUILD.gn:38: ":base",
Local dependencies must be first. Here and below

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/mojo...
File mojo/public/python/mojo/system.pyx (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/mojo...
mojo/public/python/mojo/system.pyx:34:
mojo.system_impl.SetSystemThunks(system_thunks_as_object)
Rethinking about this. This should be done in the embedder. Sorry about the
misdirection.

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/src/...
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/src/...
mojo/public/python/src/common.h:94: const mojo::Closure quit_;
 Adds  MOJO_DISALLOW_COPY_AND_ASSIGN(PythonAsyncWaiter);

[etiennej, 2014-12-12 15:29:36]

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/BUIL...
File mojo/public/python/BUILD.gn (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/BUIL...
mojo/public/python/BUILD.gn:38: ":base",
On 2014/12/12 14:27:06, qsr wrote:
> Local dependencies must be first. Here and below

Done.

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/mojo...
File mojo/public/python/mojo/system.pyx (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/mojo...
mojo/public/python/mojo/system.pyx:34:
mojo.system_impl.SetSystemThunks(system_thunks_as_object)
On 2014/12/12 14:27:06, qsr wrote:
> Rethinking about this. This should be done in the embedder. Sorry about the
> misdirection.

Done.

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/src/...
File mojo/public/python/src/common.h (right):

https://codereview.chromium.org/792843002/diff/120001/mojo/public/python/src/...
mojo/public/python/src/common.h:94: const mojo::Closure quit_;
On 2014/12/12 14:27:06, qsr wrote:
>  Adds  MOJO_DISALLOW_COPY_AND_ASSIGN(PythonAsyncWaiter);

Done.

[etiennej, 2014-12-17 10:42:49]

Patchset #10 (id:180001) has been deleted

[etiennej, 2014-12-17 10:44:52]

You can take another look. The main differences are:
 - merging with the current tree
 - importing c_export in the .pyx files, otherwise the initsystem* symbol is not
exported

[qsr, 2014-12-17 12:17:39]

https://codereview.chromium.org/792843002/diff/220001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/792843002/diff/220001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:205: """Tests that a source_set within an SDK buildfile
is flagged."""
Comment seems to be off.

[etiennej, 2014-12-17 12:19:39]

https://codereview.chromium.org/792843002/diff/220001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/792843002/diff/220001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:205: """Tests that a source_set within an SDK buildfile
is flagged."""
On 2014/12/17 12:17:39, qsr wrote:
> Comment seems to be off.

Sorry, fixed.

[qsr, 2014-12-17 12:22:53]

https://codereview.chromium.org/792843002/diff/240001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/792843002/diff/240001/mojo/PRESUBMIT.py#newco...
mojo/PRESUBMIT.py:24: "python_binary_source_set"],
Did you write a bug to figure out what to do with those? If yes, can you
reference it here?

[etiennej, 2014-12-17 13:01:06]

https://codereview.chromium.org/792843002/diff/240001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/792843002/diff/240001/mojo/PRESUBMIT.py#newco...
mojo/PRESUBMIT.py:24: "python_binary_source_set"],
On 2014/12/17 12:22:53, qsr wrote:
> Did you write a bug to figure out what to do with those? If yes, can you
> reference it here?

Done.

[qsr, 2014-12-17 13:02:51]

LGTM with nits.

https://codereview.chromium.org/792843002/diff/260001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/792843002/diff/260001/mojo/PRESUBMIT.py#newco...
mojo/PRESUBMIT.py:24: # python_binary_source_set added due to crbug.com/443147
Please move this comment on the previous line. And add a TODO(etiennej): at the
statr of it.

[etiennej, 2014-12-17 13:04:31]

https://codereview.chromium.org/792843002/diff/260001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/792843002/diff/260001/mojo/PRESUBMIT.py#newco...
mojo/PRESUBMIT.py:24: # python_binary_source_set added due to crbug.com/443147
On 2014/12/17 13:02:51, qsr wrote:
> Please move this comment on the previous line. And add a TODO(etiennej): at
the
> statr of it.

Done.

[etiennej, 2014-12-17 13:26:32]

Committed patchset #14 (id:280001) manually as
0e776c2f77c489164b512bb7e33640088caadc55 (presubmit successful).