Implement ServiceManager

Implement ServiceManager


BUG=None
R=abarth@chromium.org, abarth

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

[DaveMoore, 2013-12-09 23:52:57]

I know that the removal of Context from NativeViewport will break Android, but
I'll bring it back when I more tightly bind NativeViewport to the Shell.

[Ben Goodger (Google), 2013-12-10 01:04:21]

On 2013/12/09 23:52:57, DaveMoore wrote:
> I know that the removal of Context from NativeViewport will break Android, but
> I'll bring it back when I more tightly bind NativeViewport to the Shell.

Is there a way to make this change not remove these types, or follow up very
quickly with the fix, since the removal of the NVClient basically blocks my
input event work. Should I just not sync? Until when? :-)

[Ben Goodger (Google), 2013-12-10 01:05:45]

On 2013/12/10 01:04:21, Ben Goodger (Google) wrote:
> On 2013/12/09 23:52:57, DaveMoore wrote:
> > I know that the removal of Context from NativeViewport will break Android,
but
> > I'll bring it back when I more tightly bind NativeViewport to the Shell.
> 
> Is there a way to make this change not remove these types, or follow up very
> quickly with the fix, since the removal of the NVClient basically blocks my
> input event work. Should I just not sync? Until when? :-)

I guess I could try and fix gfx's dependency tree.

[abarth-chromium, 2013-12-10 01:06:03]

https://codereview.chromium.org/93793009/diff/20001/mojo/common/handle_watche...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:200: DCHECK(data.message_loop != NULL);
DCHECK_NE?

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

https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:42:
shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
mojo:mojo_native_viewport_service

We're going to be happier in the long term if we use standard URLs...  Maybe
something like:

mojo://services/native_viewport

?

https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:49: class NativeViewportClientImpl :
public mojo::NativeViewportClientStub {
Can this be a separate file?  It seems unnecessary to make this an inner private
class.

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_examples.gypi#n...
mojo/mojo_examples.gypi:15: 'shell_bindings',
mojo_shell_bindings ?

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_services.gypi#n...
mojo/mojo_services.gypi:59: 'services/native_viewport/android/mojo_viewport.h',
This looks like a bad merge.  This code doesn't exist anymore.

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:52: native_viewport_ =
services::NativeViewport::Create(this);
"services::" <-- not necessary.  We're already in the services namespace.

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:126:
scoped_ptr<services::NativeViewport> native_viewport_;
"services::" <-- not needed

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:66: base::AutoLock lock(lock_);
So, this lock is held as long as the app is running?

I don't understand the point of this lock.  It seems like all the functions that
take the lock happen in sequence.  DidCompleteLoad() can't be called until the
constructor completes and Run() can't be called until DidCompleteLoad calls
Start().

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:126: std::string lib("lib" + url.ExtractFileName()
+ ".so");
This won't work on Window or Mac.

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:129: base::AutoLock lock(service_map_lock_);
I don't understand this lock either.  Isn't this function always called on the
same thread?

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manager.h
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.h:23: virtual ~ServiceManager();
Please mark one-argument constructors explicit.  Why a virtual destructor?

[DaveMoore, 2013-12-10 01:07:26]

On 2013/12/10 01:04:21, Ben Goodger (Google) wrote:
> On 2013/12/09 23:52:57, DaveMoore wrote:
> > I know that the removal of Context from NativeViewport will break Android,
but
> > I'll bring it back when I more tightly bind NativeViewport to the Shell.
> 
> Is there a way to make this change not remove these types, or follow up very
> quickly with the fix, since the removal of the NVClient basically blocks my
> input event work. Should I just not sync? Until when? :-)

The NVClient isn't going away, it's just getting rolled into a nested class of
SampleApp. The Context is what I took away. Will that break you? Adam led me to
believe that Android was already broken so this wouldn't be doing any more
damage, and gave us a path to fixing it.

[abarth-chromium, 2013-12-10 01:08:00]

On 2013/12/10 01:07:26, DaveMoore wrote:
> The NVClient isn't going away, it's just getting rolled into a nested class of
> SampleApp. The Context is what I took away. Will that break you? Adam led me
to
> believe that Android was already broken so this wouldn't be doing any more
> damage, and gave us a path to fixing it.

Ben has been using a local patch to make it work.

[DaveMoore, 2013-12-10 03:00:23]

Fix review issues

[DaveMoore, 2013-12-10 03:00:45]

https://codereview.chromium.org/93793009/diff/20001/mojo/common/handle_watche...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:200: DCHECK(data.message_loop != NULL);
On 2013/12/10 01:06:03, abarth wrote:
> DCHECK_NE?

Done.

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

https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:42:
shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
Are you seeing that as mapping to libnative_viewport.so? Or
libnative_viewport_service.so?
On 2013/12/10 01:06:03, abarth wrote:
> mojo:mojo_native_viewport_service
> 
> We're going to be happier in the long term if we use standard URLs...  Maybe
> something like:
> 
> mojo://services/native_viewport
> 
> ?

https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:49: class NativeViewportClientImpl :
public mojo::NativeViewportClientStub {
I view it the other way; it's unnecessary for it to be a top level class as it's
only used in one place. Then we can skip the header too.

On 2013/12/10 01:06:03, abarth wrote:
> Can this be a separate file?  It seems unnecessary to make this an inner
private
> class.

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_examples.gypi#n...
mojo/mojo_examples.gypi:15: 'shell_bindings',
Seems redundant, but ok.
On 2013/12/10 01:06:03, abarth wrote:
> mojo_shell_bindings ?

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/mojo_services.gypi#n...
mojo/mojo_services.gypi:59: 'services/native_viewport/android/mojo_viewport.h',
On 2013/12/10 01:06:03, abarth wrote:
> This looks like a bad merge.  This code doesn't exist anymore.

Done.

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:52: native_viewport_ =
services::NativeViewport::Create(this);
Without that I get this error:
../../mojo/services/native_viewport/native_viewport_impl.cc:53:26: error: no
member named 'Create' in 'mojo::NativeViewport'; did you mean
'::mojo::services::NativeViewport::Create'?

On 2013/12/10 01:06:03, abarth wrote:
> "services::" <-- not necessary.  We're already in the services namespace.

https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:126:
scoped_ptr<services::NativeViewport> native_viewport_;
Same as above.
On 2013/12/10 01:06:03, abarth wrote:
> "services::" <-- not needed

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:66: base::AutoLock lock(lock_);
You are right. It was left in accidentally
On 2013/12/10 01:06:03, abarth wrote:
> So, this lock is held as long as the app is running?
> 
> I don't understand the point of this lock.  It seems like all the functions
that
> take the lock happen in sequence.  DidCompleteLoad() can't be called until the
> constructor completes and Run() can't be called until DidCompleteLoad calls
> Start().

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:126: std::string lib("lib" + url.ExtractFileName()
+ ".so");
You're right...it won't. I'll take a look at that while you look at my other
comments.
On 2013/12/10 01:06:03, abarth wrote:
> This won't work on Window or Mac.

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:129: base::AutoLock lock(service_map_lock_);
At one point I was thinking that each Service might get its own thread, or maybe
groups somehow, to reduce the risk of contention. Also at one point the
interaction with the Service object itself was more complicated, and it wasn't
allocated until later (on the thread running the app). I can remove this
locking. 
On 2013/12/10 01:06:03, abarth wrote:
> I don't understand this lock either.  Isn't this function always called on the
> same thread?

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manager.h
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/93793009/diff/20001/mojo/shell/service_manage...
mojo/shell/service_manager.h:23: virtual ~ServiceManager();
On 2013/12/10 01:06:03, abarth wrote:
> Please mark one-argument constructors explicit.  Why a virtual destructor?

Done.

[abarth-chromium, 2013-12-10 04:51:18]

On 2013/12/10 03:00:45, DaveMoore wrote:
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> File mojo/examples/sample_app/sample_app.cc (right):
> 
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> mojo/examples/sample_app/sample_app.cc:42:
> shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
> Are you seeing that as mapping to libnative_viewport.so? Or
> libnative_viewport_service.so?

Maybe libmojo_service_native_viewport.so?  We can also make the server give
these libs sensible URLs.

> On 2013/12/10 01:06:03, abarth wrote:
> > mojo:mojo_native_viewport_service
> > 
> > We're going to be happier in the long term if we use standard URLs...  Maybe
> > something like:
> > 
> > mojo://services/native_viewport
> > 
> > ?
> 
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> mojo/examples/sample_app/sample_app.cc:49: class NativeViewportClientImpl :
> public mojo::NativeViewportClientStub {
> I view it the other way; it's unnecessary for it to be a top level class as
it's
> only used in one place. Then we can skip the header too.

Ok.  We can always move things around if we want later.

>
https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
> mojo/services/native_viewport/native_viewport_impl.cc:52: native_viewport_ =
> services::NativeViewport::Create(this);
> Without that I get this error:
> ../../mojo/services/native_viewport/native_viewport_impl.cc:53:26: error: no
> member named 'Create' in 'mojo::NativeViewport'; did you mean
> '::mojo::services::NativeViewport::Create'?

Oh, you're right.  I forgot we haven't renamed this class yet.

[abarth-chromium, 2013-12-10 04:53:50]

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:32: base::AutoLock lock(lock_);
You're still planning to remove this lock, right?

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:125: std::string lib("lib" + url.ExtractFileName()
+ ".so");
We still have the problem of this not working on Windows or Mac.

[darin (slow to review), 2013-12-10 05:23:30]

On 2013/12/10 04:51:18, abarth wrote:
> On 2013/12/10 03:00:45, DaveMoore wrote:
> >
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> > File mojo/examples/sample_app/sample_app.cc (right):
> > 
> >
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> > mojo/examples/sample_app/sample_app.cc:42:
> > shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
> > Are you seeing that as mapping to libnative_viewport.so? Or
> > libnative_viewport_service.so?
> 
> Maybe libmojo_service_native_viewport.so?  We can also make the server give
> these libs sensible URLs.

I think we should certainly do that.

[Aside, we should eventually think about what it means for someone to try to
load a mojo:// URL using a normal URLLoader interface. In my dreams, the app
corresponding to the mojo:// URL would have the opportunity to provide responses
to such requests. Ditto for navigations. But, these seem like things that the
app would need to opt-in to providing, so we can just ignore them for now.]



> > On 2013/12/10 01:06:03, abarth wrote:
> > > mojo:mojo_native_viewport_service
> > > 
> > > We're going to be happier in the long term if we use standard URLs... 
Maybe
> > > something like:
> > > 
> > > mojo://services/native_viewport
> > > 
> > > ?
> > 
> >
>
https://codereview.chromium.org/93793009/diff/20001/mojo/examples/sample_app/...
> > mojo/examples/sample_app/sample_app.cc:49: class NativeViewportClientImpl :
> > public mojo::NativeViewportClientStub {
> > I view it the other way; it's unnecessary for it to be a top level class as
> it's
> > only used in one place. Then we can skip the header too.
> 
> Ok.  We can always move things around if we want later.
> 
> >
>
https://codereview.chromium.org/93793009/diff/20001/mojo/services/native_view...
> > mojo/services/native_viewport/native_viewport_impl.cc:52: native_viewport_ =
> > services::NativeViewport::Create(this);
> > Without that I get this error:
> > ../../mojo/services/native_viewport/native_viewport_impl.cc:53:26: error: no
> > member named 'Create' in 'mojo::NativeViewport'; did you mean
> > '::mojo::services::NativeViewport::Create'?
> 
> Oh, you're right.  I forgot we haven't renamed this class yet.

[darin (slow to review), 2013-12-10 06:12:35]

Exciting stuff!!! Some thoughts...

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

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:37: mojo::AllocationScope scope;
nit: you might consider moving |scope| closer to where it is needed.

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:38: mojo::ScopedMessagePipeHandle
app_handle, client_handle;
nit: app_handle -> service_handle ?

client_handle is the handle to the client, and XXX is the handle to the service?
XXX => service_handle?

We should establish a naming convention that we like and apply it consistently
:)

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:54: viewport_->Open();;
nit: extra ";"

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:37:
viewports_.push_back(new NativeViewportImpl(client_pipe.Pass()));
It looks like this vector only grows?

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:41: class
NativeViewportImpl : public NativeViewportStub,
It feels like in a build where NV service is statically linked into mojo_shell
that NativeViewportService would look different, or at least, it wouldn't need
to be a ShellClient implementation.

Perhaps NativeViewportImpl should not be a nested class then, and we should put
the two classes in separate files even? NativeViewportService just seems like
boilerplate code for how a NativeViewportImpl gets instantiated, but otherwise a
NVImpl doesn't really need to know about NVService.

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:129: extern "C"
NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain(
I'm confused about seeing a MojoMain for the NV service as I expected this to be
a service that is built into the mojo_shell. We can only have one MojoMain
symbol for the mojo_shell executable. Is the idea to eventually add some
#ifdef'd code here for statically linked builds?

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/loader.cc
File mojo/shell/loader.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/loader.cc#newc...
mojo/shell/loader.cc:54: // TODO(davemoore): Should we make this a command line
flag instead?
yes, a command line flag would be better.  that way we can simulate what happens
in production.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/run.cc
File mojo/shell/run.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/run.cc#newcode21
mojo/shell/run.cc:21: if (args.size() == 0) {
nit: "!args.empty()"

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/run.cc#newcode36
mojo/shell/run.cc:36: ScopedMessagePipeHandle handle;
nit: for readability, "handle" -> "no_handle" or "invalid_handle" perhaps?

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:47: virtual void Connect(const mojo::String& url,
nit: no need for the "mojo::" prefix inside the mojo namespace.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:49: manager_->Connect(GURL(url.To<std::string>()),
client_pipe.Pass());
note: we could write a TypeConverter specialization for String to GURL
conversion to simplify this code. Then you could just write:

  manager_->Connect(url, client_pipe.Pass());

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:101: mojo::RemotePtr<::shell::ShellClient>
shell_client_;
nit: ditto re: mojo::

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom
File mojo/shell/shell.mojom (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom#ne...
mojo/shell/shell.mojom:9: // Loads lib|app_name|.so relative to the parent
application.
nit: It might be nice to abstract this and just say that the string parameter is
a URL string. Then, the mapping mojo:// URLs to apps can be a detail of the
service manager.

If the URL is a http:// URL, then we would download the .so and load it :)

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom#ne...
mojo/shell/shell.mojom:15: void Connect(handle<message_pipe> client_pipe);
For apps that handle multiples URLs (or have multiple names), it might be nice
to see what URL / name they are being invoked as.

Also, I feel like "Connect" is not quite the right name for this method. This
method is an event to handle an incoming client connection. "AcceptConnection"
or "OnAcceptConnection"? I'm not sure if we want to standardize on using the
"On" prefix for events.

Perhaps there should also be a notification back to the guy who invoked
Shell::Connect() to tell them whether or not the connection was successful? Or,
should he just assume it was successful and then just monitor client_pipe to see
if it gets closed?

[abarth-chromium, 2013-12-10 06:13:45]

In https://codereview.chromium.org/107473002/, I needed to create an "apps"
directory in the output folder.  We could rename the dylibs when we copy them
into the folder.  We could even make a directory structure there.  :)

[beng (no_code_reviews), 2013-12-10 16:21:28]

Ah, didn't see the whole CL before I left last night. Disregard my comment.

-Ben


On Mon, Dec 9, 2013 at 5:07 PM, <davemoore@chromium.org> wrote:

> On 2013/12/10 01:04:21, Ben Goodger (Google) wrote:
>
>> On 2013/12/09 23:52:57, DaveMoore wrote:
>> > I know that the removal of Context from NativeViewport will break
>> Android,
>>
> but
>
>> > I'll bring it back when I more tightly bind NativeViewport to the Shell.
>>
>
>  Is there a way to make this change not remove these types, or follow up
>> very
>> quickly with the fix, since the removal of the NVClient basically blocks
>> my
>> input event work. Should I just not sync? Until when? :-)
>>
>
> The NVClient isn't going away, it's just getting rolled into a nested
> class of
> SampleApp. The Context is what I took away. Will that break you? Adam led
> me to
> believe that Android was already broken so this wouldn't be doing any more
> damage, and gave us a path to fixing it.
>
> https://codereview.chromium.org/93793009/
>

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

[Ben Goodger (Google), 2013-12-10 16:35:03]

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

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:42:
shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
not a direct comment on this CL but an observation... it'd be nice if this was
even simpler somehow, e.g. 2 lines of code instead of 4-5.

effectively the minimal pseudocode is:

NativeViewportClient client;
shell.Connect("mojo:native_viewport_service", client);

What we have instead still is decorated with a lot of mojo-ism.

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:29:
NativeViewportService(ScopedMessagePipeHandle shell_pipe)
indentation is wacky here

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:129: extern "C"
NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain(
Also what's the plan here for Android?

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:125: std::string lib("lib" + url.ExtractFileName()
+ ".so");
On 2013/12/10 04:53:51, abarth wrote:
> We still have the problem of this not working on Windows or Mac.

windows impl:

std::string lib(url.ExtractFileName() + ".dll");

[DaveMoore, 2013-12-11 18:51:21]

Resolve merge problems

[DaveMoore, 2013-12-11 18:56:43]

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

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:37: mojo::AllocationScope scope;
On 2013/12/10 06:12:36, darin wrote:
> nit: you might consider moving |scope| closer to where it is needed.

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:38: mojo::ScopedMessagePipeHandle
app_handle, client_handle;
I think I've made it consistent now. The handle is named by the thing it's a
handle to, basically the other side.
On 2013/12/10 06:12:36, darin wrote:
> nit: app_handle -> service_handle ?
> 
> client_handle is the handle to the client, and XXX is the handle to the
service?
> XXX => service_handle?
> 
> We should establish a naming convention that we like and apply it consistently
> :)

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:42:
shell_->Connect("mojo:mojo_native_viewport_service", app_handle.Pass());
Agreed. The goal is to have something in mojom that lets us define a
handle<client> that will result in an api that takes the interface directly.
Right now the wiring is exposed.

On 2013/12/10 16:35:03, Ben Goodger (Google) wrote:
> not a direct comment on this CL but an observation... it'd be nice if this was
> even simpler somehow, e.g. 2 lines of code instead of 4-5.
> 
> effectively the minimal pseudocode is:
> 
> NativeViewportClient client;
> shell.Connect("mojo:native_viewport_service", client);
> 
> What we have instead still is decorated with a lot of mojo-ism.

https://codereview.chromium.org/93793009/diff/40001/mojo/examples/sample_app/...
mojo/examples/sample_app/sample_app.cc:54: viewport_->Open();;
On 2013/12/10 06:12:36, darin wrote:
> nit: extra ";"

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
File mojo/services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:29:
NativeViewportService(ScopedMessagePipeHandle shell_pipe)
On 2013/12/10 16:35:03, Ben Goodger (Google) wrote:
> indentation is wacky here

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:37:
viewports_.push_back(new NativeViewportImpl(client_pipe.Pass()));
It does. We need to have it clean up. But we need some mechanism to make that
happen safely. Right now the error notifications aren't passed out of the
Connectors / RemotePtrs. Leaving a TODO behind.
On 2013/12/10 06:12:36, darin wrote:
> It looks like this vector only grows?

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:41: class
NativeViewportImpl : public NativeViewportStub,
I think it will be a ShellClient implementation, as it still talks to the Shell,
it just isn't loaded from a .so.

 Right now it does know about the service itself, because it keeps track of the
extant instances. it's possible we'll have some library or template support for
a Service that vends the connections generically.

On 2013/12/10 06:12:36, darin wrote:
> It feels like in a build where NV service is statically linked into mojo_shell
> that NativeViewportService would look different, or at least, it wouldn't need
> to be a ShellClient implementation.
> 
> Perhaps NativeViewportImpl should not be a nested class then, and we should
put
> the two classes in separate files even? NativeViewportService just seems like
> boilerplate code for how a NativeViewportImpl gets instantiated, but otherwise
a
> NVImpl doesn't really need to know about NVService.

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:129: extern "C"
NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain(
I think once we add support for statically linked services we'll need to deal
with this in general (although in this case we might decide that we never call a
main() because it always gets created with a message loop). Perhaps we should
make it name determined by the name of the module, or even a static class method
so we don't risk running into name collisions. Then it can exist w/out #defines
around it (although that's still an option).

On 2013/12/10 06:12:36, darin wrote:
> I'm confused about seeing a MojoMain for the NV service as I expected this to
be
> a service that is built into the mojo_shell. We can only have one MojoMain
> symbol for the mojo_shell executable. Is the idea to eventually add some
> #ifdef'd code here for statically linked builds?

https://codereview.chromium.org/93793009/diff/40001/mojo/services/native_view...
mojo/services/native_viewport/native_viewport_impl.cc:129: extern "C"
NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain(
We're going to create this with a messageloop  and whatever other context is
needed.
On 2013/12/10 16:35:03, Ben Goodger (Google) wrote:
> Also what's the plan here for Android?

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/loader.cc
File mojo/shell/loader.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/loader.cc#newc...
mojo/shell/loader.cc:54: // TODO(davemoore): Should we make this a command line
flag instead?
Removed...I'll add in separate cl.
On 2013/12/10 06:12:36, darin wrote:
> yes, a command line flag would be better.  that way we can simulate what
happens
> in production.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/run.cc
File mojo/shell/run.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/run.cc#newcode21
mojo/shell/run.cc:21: if (args.size() == 0) {
On 2013/12/10 06:12:36, darin wrote:
> nit: "!args.empty()"

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:32: base::AutoLock lock(lock_);
On 2013/12/10 04:53:51, abarth wrote:
> You're still planning to remove this lock, right?

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:47: virtual void Connect(const mojo::String& url,
On 2013/12/10 06:12:36, darin wrote:
> nit: no need for the "mojo::" prefix inside the mojo namespace.

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:101: mojo::RemotePtr<::shell::ShellClient>
shell_client_;
On 2013/12/10 06:12:36, darin wrote:
> nit: ditto re: mojo::

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/service_manage...
mojo/shell/service_manager.cc:125: std::string lib("lib" + url.ExtractFileName()
+ ".so");
Added windows impl, still need to figure out mac
On 2013/12/10 16:35:03, Ben Goodger (Google) wrote:
> On 2013/12/10 04:53:51, abarth wrote:
> > We still have the problem of this not working on Windows or Mac.
> 
> windows impl:
> 
> std::string lib(url.ExtractFileName() + ".dll");

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom
File mojo/shell/shell.mojom (right):

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom#ne...
mojo/shell/shell.mojom:9: // Loads lib|app_name|.so relative to the parent
application.
On 2013/12/10 06:12:36, darin wrote:
> nit: It might be nice to abstract this and just say that the string parameter
is
> a URL string. Then, the mapping mojo:// URLs to apps can be a detail of the
> service manager.
> 
> If the URL is a http:// URL, then we would download the .so and load it :)

Done.

https://codereview.chromium.org/93793009/diff/40001/mojo/shell/shell.mojom#ne...
mojo/shell/shell.mojom:15: void Connect(handle<message_pipe> client_pipe);
Changed to AcceptConnection();
On 2013/12/10 06:12:36, darin wrote:
> For apps that handle multiples URLs (or have multiple names), it might be nice
> to see what URL / name they are being invoked as.
> 
> Also, I feel like "Connect" is not quite the right name for this method. This
> method is an event to handle an incoming client connection. "AcceptConnection"
> or "OnAcceptConnection"? I'm not sure if we want to standardize on using the
> "On" prefix for events.
> 
> Perhaps there should also be a notification back to the guy who invoked
> Shell::Connect() to tell them whether or not the connection was successful?
Or,
> should he just assume it was successful and then just monitor client_pipe to
see
> if it gets closed?

[DaveMoore, 2013-12-15 21:39:35]

Support Android with SessionManager

[DaveMoore, 2013-12-15 22:02:54]

Cleanup

[DaveMoore, 2013-12-15 22:04:04]

Accidental inclusion of adb_run change

[DaveMoore, 2013-12-15 22:05:42]

I've added support for Android (although Android appears to be broken right now
independently so it's hard to be sure it works). One big change is that I've
made it so that service loaders can be registered for specific urls, allowing us
to have different behavior on different ports / builds.

[abarth-chromium, 2013-12-15 22:54:09]

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

https://codereview.chromium.org/93793009/diff/120001/mojo/examples/sample_app...
mojo/examples/sample_app/sample_app.cc:42:
shell_->Connect("mojo:mojo_native_viewport_service", client_handle.Pass());
We'll probably need to iterate on these strings in the future, but I think this
is ok for now.

https://codereview.chromium.org/93793009/diff/120001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/mojo_services.gypi#...
mojo/mojo_services.gypi:48: 'type': 'static_library',
I'm a bit confused.  You've marked this target as a static_library, but
native_viewport_service.cc has:

extern "C" NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain

That doesn't seem consistent.  How can we export something from a
static_library?

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:56: DynamicLoader(ServiceManager* manager) :
manager_(manager) {}
Please mark one-argument constructors explicit.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:61: public base::DelegateSimpleThread::Delegate {
I think we're going a bit crazy here with nested classes.  This class is now
nested three levels deep...

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:95: base::AutoLock lock(lock_);
Why is this lock needed?  This is the only line of code that takes this lock,
and this line of code is only ever executed once per instance.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:100: thread_.reset(new
base::DelegateSimpleThread(this, "app_thread"));
You don't create the other thread until here....

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.h:11: #include "base/synchronization/lock.h"
This header doesn't appear to be used.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/shell.mojom
File mojo/shell/shell.mojom (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/shell.mojom#n...
mojo/shell/shell.mojom:5: module shell {
So far we've been putting everything in the "mojo" module because these are core
mojo services.  That will save you some ugly "::shell" typing elsewhere.  It
also will avoid the problem of having both ::shell and ::mojo::shell namespaces.

[DaveMoore, 2013-12-16 17:39:07]

Address review concerns

[DaveMoore, 2013-12-16 17:40:21]

https://codereview.chromium.org/93793009/diff/120001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/mojo_services.gypi#...
mojo/mojo_services.gypi:48: 'type': 'static_library',
Changed to shared_library (A little confused as to why it worked when static).

On 2013/12/15 22:54:10, abarth wrote:
> I'm a bit confused.  You've marked this target as a static_library, but
> native_viewport_service.cc has:
> 
> extern "C" NATIVE_VIEWPORT_EXPORT MojoResult CDECL MojoMain
> 
> That doesn't seem consistent.  How can we export something from a
> static_library?

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:56: DynamicLoader(ServiceManager* manager) :
manager_(manager) {}
On 2013/12/15 22:54:10, abarth wrote:
> Please mark one-argument constructors explicit.

Done.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:61: public base::DelegateSimpleThread::Delegate {
It may be going too far, but it keeps the publicly visible api much cleaner. 
On 2013/12/15 22:54:10, abarth wrote:
> I think we're going a bit crazy here with nested classes.  This class is now
> nested three levels deep...

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:95: base::AutoLock lock(lock_);
Removed.
On 2013/12/15 22:54:10, abarth wrote:
> Why is this lock needed?  This is the only line of code that takes this lock,
> and this line of code is only ever executed once per instance.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:100: thread_.reset(new
base::DelegateSimpleThread(this, "app_thread"));
On 2013/12/15 22:54:10, abarth wrote:
> You don't create the other thread until here....

Done.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/service_manag...
mojo/shell/service_manager.h:11: #include "base/synchronization/lock.h"
On 2013/12/15 22:54:10, abarth wrote:
> This header doesn't appear to be used.

Done.

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/shell.mojom
File mojo/shell/shell.mojom (right):

https://codereview.chromium.org/93793009/diff/120001/mojo/shell/shell.mojom#n...
mojo/shell/shell.mojom:5: module shell {
Done, but I'd like to add the ability to put bindings in a nested namespace like
"mojo::shell".
On 2013/12/15 22:54:10, abarth wrote:
> So far we've been putting everything in the "mojo" module because these are
core
> mojo services.  That will save you some ugly "::shell" typing elsewhere.  It
> also will avoid the problem of having both ::shell and ::mojo::shell
namespaces.

[abarth-chromium, 2013-12-17 17:18:49]

LGTM

I'm not sure the dylib structure is exactly correct, but I think we're better
off landing this CL and iterating at this point.

https://codereview.chromium.org/93793009/diff/140001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/93793009/diff/140001/mojo/mojo_services.gypi#...
mojo/mojo_services.gypi:48: 'type': 'shared_library',
I'm not sure I quite understand how this works...  Maybe it works on the
component build?  In the static build, you'll have your own copy of base in this
dylib, which means you'll need to initialize all the statics again...

I feel like we just need to land this CL and iterate.  Sometime soon, we'll need
to sort out the static build.

https://codereview.chromium.org/93793009/diff/140001/mojo/shell/service_manag...
File mojo/shell/service_manager.cc (right):

https://codereview.chromium.org/93793009/diff/140001/mojo/shell/service_manag...
mojo/shell/service_manager.cc:79: #else
On Mac OS X, the suffix is dylib.

[DaveMoore, 2013-12-17 18:05:13]

Resolve merge problems

[DaveMoore, 2013-12-17 18:40:42]

Add mojo_bindings dep

[DaveMoore, 2013-12-17 18:55:56]

need mojo_common_lib

[DaveMoore, 2013-12-17 23:39:37]

Allow Android to be built

[DaveMoore, 2013-12-17 23:48:33]

Allow Android to be built

[DaveMoore, 2013-12-18 02:07:41]

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