NativeViewport

Adds the skeleton of a NativeViewport concept.

This is a platform-specific View (output surface and event sink). We listen for events and other disposition changes and forward them to app code. In this case I just do something ad-hoc (stringify some event metadata and post to our existing sample app). Once we have bindings, we can surface a better API here. I would also like for the app to request the NativeViewportService from the Service Service, and get a separate pipe for that, rather than reusing the one that is passed through MojoMain (which I think is supposed to only be for surfacing the ServiceService).

Anyway WDYT? If we agree on this I can figure out how to wrap the org.chromium.mojo_shell_apk.MojoView in a NativeViewport.

BUG=
R=abarth@chromium.org

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

[Ben Goodger (Google), 2013-10-29 21:34:41]

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

https://codereview.chromium.org/51373002/diff/50001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:8: #include "mojo/public/system/macros.h"
I noticed that this app need not depend on base at all. Darin mentioned it might
be good to have a Mojo app that didn't depend on any Chrome code at all. It
could be this one, though if we want to have something that works with our other
base-reliant services, we would have to write one that does.

https://codereview.chromium.org/51373002/diff/50001/mojo/shell/DEPS
File mojo/shell/DEPS (right):

https://codereview.chromium.org/51373002/diff/50001/mojo/shell/DEPS#newcode3
mojo/shell/DEPS:3: "+ui/events",
This is only necessary as long as there is only one pipe to sample app. Perhaps
I should work on that next.

[abarth-chromium, 2013-10-29 21:48:44]

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

https://codereview.chromium.org/51373002/diff/50001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:56: running_ = true;
Do we want to set running_ to false at some point?

https://codereview.chromium.org/51373002/diff/50001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport.h (right):

https://codereview.chromium.org/51373002/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.h:11: class
NativeViewportDelegate;
You don't want to make this an inner class?

https://codereview.chromium.org/51373002/diff/50001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport.h:19: NativeViewportDelegate*
delegate);
Sould this return a scoped_ptr<NativeViewport> to force the caller to take
ownership of the returned object?

Also, you can just call this function |Create|.  The "NativeViewport" part of
the name is redundant.

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

https://codereview.chromium.org/51373002/diff/50001/mojo/shell/app_container....
mojo/shell/app_container.cc:28: class ShellNativeViewportDelegate : public
services::NativeViewportDelegate {
Yeah, it's kind of lame to have this here.  Can we put this class in
mojo/services/native_viewport and hand it a Handle for now?  In the future, we'd
want to hand it a Pipe or something.

https://codereview.chromium.org/51373002/diff/50001/mojo/shell/app_container....
mojo/shell/app_container.cc:139:
services::NativeViewport::CreateNativeViewport(delegate);
These leak, right?  Maybe we should store them in scoped_ptrs in the
AppContainer?

I wonder if we should do something like we do in pepper where every object is
associated with an instance so that they can all be freed when the instance
dies.

[abarth-chromium, 2013-10-29 21:51:41]

> Anyway WDYT? If we agree on this I can figure out how to wrap the
org.chromium.mojo_shell_apk.MojoView in a NativeViewport. 

This seems genearlly fine.  I don't fully understand what putting this code in
the mojo/services directory buys us.  I had the same instinct for the loader,
but I eventually just moved it into the shell.  I feel like we need more of the
IPC system fleshed out to make proper services that are encapsulated behind IPC.

I'd like us clean up the lifetimes a bit before landing, if that's ok with you.

[Ben Goodger (Google), 2013-10-29 22:33:13]

On 2013/10/29 21:48:44, abarth wrote:
>
https://codereview.chromium.org/51373002/diff/50001/mojo/examples/sample_app/...
> mojo/examples/sample_app/sample_app.cc:56: running_ = true;
> Do we want to set running_ to false at some point?

Probably we just want MessageLoop. I just replaced this with while(1). I can try
and do something clever-er like have it quit when we receive a gesture up (touch
release/mouse up) if you like.

>
https://codereview.chromium.org/51373002/diff/50001/mojo/services/native_view...
> File mojo/services/native_viewport/native_viewport.h (right):
> 
>
https://codereview.chromium.org/51373002/diff/50001/mojo/services/native_view...
> mojo/services/native_viewport/native_viewport.h:11: class
> NativeViewportDelegate;
> You don't want to make this an inner class?

Makes it impossible to forward declare in headers. I did move it into
native_viewport.h however to reduce #including tho.

> Yeah, it's kind of lame to have this here.  Can we put this class in
> mojo/services/native_viewport and hand it a Handle for now?  In the future,
we'd
> want to hand it a Pipe or something.

Made simple_native_viewport_delegate.cc/h.

>
https://codereview.chromium.org/51373002/diff/50001/mojo/shell/app_container....
> mojo/shell/app_container.cc:139:
> services::NativeViewport::CreateNativeViewport(delegate);
> These leak, right?  Maybe we should store them in scoped_ptrs in the
> AppContainer?
> 
> I wonder if we should do something like we do in pepper where every object is
> associated with an instance so that they can all be freed when the instance
> dies.

I think AppContainer is probably good enough for now. Note that this is actually
wrong in some cases that may only be relevant on Windows - e.g. the user can
close your window and we would need to notify back out to reset these. I figure
we can just crash in this case for now since this is all very hand wavy.

[Ben Goodger (Google), 2013-10-29 22:35:52]

On 2013/10/29 21:51:41, abarth wrote:
> > Anyway WDYT? If we agree on this I can figure out how to wrap the
> org.chromium.mojo_shell_apk.MojoView in a NativeViewport. 
> 
> This seems genearlly fine.  I don't fully understand what putting this code in
> the mojo/services directory buys us.  I had the same instinct for the loader,
> but I eventually just moved it into the shell.  I feel like we need more of
the
> IPC system fleshed out to make proper services that are encapsulated behind
IPC.
> 
> I'd like us clean up the lifetimes a bit before landing, if that's ok with
you.

Definitely, and at any rate this won't build on Linux or Android right now so
I'll have to come up with at least a bandaid for that.

I think it might makes sense for the android view stuff to move here eventually.

What having this stuff distinct from shell buys us right now is tighter control
over shell's DEPS (and causes us to begin thinking about service DEPS). Insofar
as we think this is a good architecture for Mojo, it saves us cleanup later
also.

[abarth-chromium, 2013-10-30 06:34:44]

LGTM

Like a lot of the code we've written so far, we're going to need to evolve how
it fits together as we learn more, but this looks like a good starting point. 
It would be nice to integrate with the Android view sooner rather than later
because that will help us better understand where this code is headed.

[Ben Goodger (Google), 2013-10-30 23:03:07]

OK, I cleaned up the windows ownership story a bit more and fixed the android
build. Note that everything is still leaked (along with AppContainer).
AppContainer should probably delete itself after the app runs (or fails to).

I think this is ready to checkpoint now. Once I land this I'll figure out how to
make the android side actually work in the same way as it does on Windows.

[Ben Goodger (Google), 2013-10-31 16:39:22]

Adam, still OK with this now? Sorry for not being clear earlier, it was a
Work-in-Progress.

-Ben


On Wed, Oct 30, 2013 at 4:03 PM, <ben@chromium.org> wrote:

> OK, I cleaned up the windows ownership story a bit more and fixed the
> android
> build. Note that everything is still leaked (along with AppContainer).
> AppContainer should probably delete itself after the app runs (or fails
> to).
>
> I think this is ready to checkpoint now. Once I land this I'll figure out
> how to
> make the android side actually work in the same way as it does on Windows.
>
>
https://codereview.chromium.**org/51373002/<https://codereview.chromium.org/5...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2013-10-31 16:46:23]

Yes, LGTM

[Ben Goodger (Google), 2013-10-31 21:06:34]

Committed patchset #13 manually as r232210 (presubmit successful).