Work in progress for end-to-end bindings

Work in progress for end-to-end bindings

R=darin@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234975

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235379

[darin (slow to review), 2013-11-13 22:57:49]

https://codereview.chromium.org/72123002/diff/1/mojo/examples/hello_world_ser...
File mojo/examples/hello_world_service/hello_world_service.mojom (right):

https://codereview.chromium.org/72123002/diff/1/mojo/examples/hello_world_ser...
mojo/examples/hello_world_service/hello_world_service.mojom:9: void
Greeting(string greeting @0) @0;
note: ordinals are optional.

https://codereview.chromium.org/72123002/diff/1/mojo/examples/sample_app/samp...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/1/mojo/examples/sample_app/samp...
mojo/examples/sample_app/sample_app.cc:44: common::BindingsSupportImpl
bindings_support;
note: using common::BindingsSupportImpl implies having a MessageLoop instance on
the current thread. you can just use a base::MessageLoop of TYPE_DEFAULT. You'll
need to call MessageLoop::Run() somewhere, presumably after the call to
SayHello.

[DaveMoore, 2013-11-13 23:22:11]

Add MessageLoop instance to SampleApp and run it

[darin (slow to review), 2013-11-14 00:05:34]

LGTM

Would be good to update the CL description.

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
File mojo/examples/sample_app/hello_world_client_impl.h (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
mojo/examples/sample_app/hello_world_client_impl.h:17: ~HelloWorldClientImpl();
nit: virtual dtor (some clang bot will complain)

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:28: static HelloWorldClientImpl* client =
0;
nit: g_client?

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:45: base::MessageLoop loop;
since this is sample code, it might be nice to add some comments explaining what
these steps are all about.

https://codereview.chromium.org/72123002/diff/60001/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/shell/app_container....
mojo/shell/app_container.cc:97: // native_viewport_controller_->Close();
nit: commented out code should have a TODO

[DaveMoore, 2013-11-14 00:20:20]

Resolve merge comments

[DaveMoore, 2013-11-14 00:21:16]

Committed patchset #3 manually as r234975 (presubmit successful).

[mmenke, 2013-11-14 03:32:14]

On 2013/11/14 00:21:16, DaveMoore wrote:
> Committed patchset #3 manually as r234975 (presubmit successful).

This seems to have broken debug trybots on Win and Mac.  See, for example,
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/125707. 
Going to go ahead and revert.

[DaveMoore, 2013-11-14 23:41:46]

Use SimpleThread instead of Thread to avoid creating a MessageLoop

[DaveMoore, 2013-11-14 23:45:15]

Use SimpleThread instead of Thread to avoid creating a MessageLoop

[DaveMoore, 2013-11-14 23:46:53]

- Switched to base::SimpleThread
- Made HelloWorldClientImpl quit the current message loop, so we can exit
- Made HelloWorldClientImpl stack based so it would be destroyed before the
bindings impls to avoid DCHECK.

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
File mojo/examples/sample_app/hello_world_client_impl.h (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
mojo/examples/sample_app/hello_world_client_impl.h:17: ~HelloWorldClientImpl();
On 2013/11/14 00:05:34, darin wrote:
> nit: virtual dtor (some clang bot will complain)

Done.

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:45: base::MessageLoop loop;
On 2013/11/14 00:05:34, darin wrote:
> since this is sample code, it might be nice to add some comments explaining
what
> these steps are all about.

Done.

https://codereview.chromium.org/72123002/diff/60001/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/72123002/diff/60001/mojo/shell/app_container....
mojo/shell/app_container.cc:97: // native_viewport_controller_->Close();
On 2013/11/14 00:05:34, darin wrote:
> nit: commented out code should have a TODO

Done.

[abarth-chromium, 2013-11-14 23:59:04]

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
mojo/examples/sample_app/sample_app.cc:46: printf("here!\n");
We probably want to remove this before landing.  :)

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container...
mojo/shell/app_container.h:52: scoped_refptr<base::MessageLoopProxy>
main_message_loop_proxy_;
You don't need a main_message_loop_proxy_ member.  That loop is available on via
context_ above.

[darin (slow to review), 2013-11-15 00:01:37]

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
mojo/examples/sample_app/sample_app.cc:29: base::MessageLoop loop;
it would be rational for BindingsSupportImpl to assert that the current thread
has a MessageLoop since it doesn't work without one. you might want to allocate
the MessageLoop in MojoMain for that reason.

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container...
mojo/shell/app_container.cc:91: base::Bind(&AppContainer::AppCompleted,
weak_factory_.GetWeakPtr()));
WeakPtr is not thread-safe. you need to call GetWeakPtr() on the same thread
where you wish to de-reference it. This means, you need to pass the WeakPtr as a
parameter to Run() somehow, perhaps as a member variable of |this|.

But, why do we even need a WeakPtr? It seems like we already expect that |this|
cannot become invalid until after Run() returns since we are freely
dereferencing |this| inside of Run(). It seems like we already know that the
main thread is not going away until Run() returns (i.e., until AppCompleted is
called).

Oh, I see... the TODO in mojo::shell::Run() seems to indicate that we just
haven't invented how shutdown sequencing should be done.

[DaveMoore, 2013-11-15 00:03:44]

Address review concerns

[abarth-chromium, 2013-11-15 00:04:36]

On 2013/11/15 00:01:37, darin wrote:
> Oh, I see... the TODO in mojo::shell::Run() seems to indicate that we just
> haven't invented how shutdown sequencing should be done.

Yeah, we're probably going to need something to contain the app containers (in
the same way that the browser contains the tab contents).  I sort of punted on
that for now.

[DaveMoore, 2013-11-15 00:10:34]

Address review concerns

[DaveMoore, 2013-11-15 00:11:32]

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
File mojo/examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
mojo/examples/sample_app/sample_app.cc:29: base::MessageLoop loop;
On 2013/11/15 00:01:37, darin wrote:
> it would be rational for BindingsSupportImpl to assert that the current thread
> has a MessageLoop since it doesn't work without one. you might want to
allocate
> the MessageLoop in MojoMain for that reason.

Done.

https://codereview.chromium.org/72123002/diff/210001/mojo/examples/sample_app...
mojo/examples/sample_app/sample_app.cc:46: printf("here!\n");
On 2013/11/14 23:59:05, abarth wrote:
> We probably want to remove this before landing.  :)

Done.

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container...
mojo/shell/app_container.cc:91: base::Bind(&AppContainer::AppCompleted,
weak_factory_.GetWeakPtr()));
On 2013/11/15 00:01:37, darin wrote:
> WeakPtr is not thread-safe. you need to call GetWeakPtr() on the same thread
> where you wish to de-reference it. This means, you need to pass the WeakPtr as
a
> parameter to Run() somehow, perhaps as a member variable of |this|.
> 
> But, why do we even need a WeakPtr? It seems like we already expect that
|this|
> cannot become invalid until after Run() returns since we are freely
> dereferencing |this| inside of Run(). It seems like we already know that the
> main thread is not going away until Run() returns (i.e., until AppCompleted is
> called).
> 
> Oh, I see... the TODO in mojo::shell::Run() seems to indicate that we just
> haven't invented how shutdown sequencing should be done.

Done.

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/72123002/diff/210001/mojo/shell/app_container...
mojo/shell/app_container.h:52: scoped_refptr<base::MessageLoopProxy>
main_message_loop_proxy_;
On 2013/11/14 23:59:05, abarth wrote:
> You don't need a main_message_loop_proxy_ member.  That loop is available on
via
> context_ above.

Done.

[DaveMoore, 2013-11-15 03:08:30]

Fix bad merge

[DaveMoore, 2013-11-15 15:50:16]

Add base dep for sample_app

[sky, 2013-11-15 17:11:56]

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container...
mojo/shell/app_container.h:45: virtual void Run() OVERRIDE;
In hopes of making code easier to maintain and reason about is it possible to
move all the code/data that runs on the background into a separate object?

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container...
mojo/shell/app_container.h:55: scoped_ptr<examples::HelloWorldServiceImpl>
hello_world_service_;
How come the appcontainer is depending upon an example?

[abarth-chromium, 2013-11-15 17:14:33]

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container...
mojo/shell/app_container.h:55: scoped_ptr<examples::HelloWorldServiceImpl>
hello_world_service_;
On 2013/11/15 17:11:56, sky wrote:
> How come the appcontainer is depending upon an example?

It's just temporary while we bring up the service manager.

[DaveMoore, 2013-11-15 17:15:23]

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container...
mojo/shell/app_container.h:45: virtual void Run() OVERRIDE;
As said in comment, this is a WIP. When we make it real, we'll certainly do
that. This is basically to prove out an approach.

On 2013/11/15 17:11:56, sky wrote:
> In hopes of making code easier to maintain and reason about is it possible to
> move all the code/data that runs on the background into a separate object?

https://codereview.chromium.org/72123002/diff/470001/mojo/shell/app_container...
mojo/shell/app_container.h:55: scoped_ptr<examples::HelloWorldServiceImpl>
hello_world_service_;
WIP. Next we will generalize and no longer have that dependency.

On 2013/11/15 17:11:56, sky wrote:
> How come the appcontainer is depending upon an example?

[sky, 2013-11-15 18:08:26]

appcontainer LGTM

[DaveMoore, 2013-11-15 18:12:52]

Committed patchset #9 manually as r235379 (presubmit successful).