mojo: Update content handler API

mojo: Update content handler API

WIP

R=aa@chromium.org, abarth@chromium.org, hansmuller@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7c05714462a1f2c668dd3946d953cc36aa685bfc

[qsr, 2014-10-29 17:57:11]

qsr@chromium.org changed reviewers:
+ aa@chromium.org

[qsr, 2014-10-29 17:57:11]

Still in progress, as I didn't start looking into the js content handler.

But sky is working (all tests are passing).

[abarth-chromium, 2014-10-30 03:02:55]

The Sky part looks great.

[qsr, 2014-10-30 15:18:05]

qsr@chromium.org changed reviewers:
+ abarth@chromium.org, hansmuller@chromium.org, jam@chromium.org

[qsr, 2014-10-30 15:18:06]

CL is now ready. Everything compiles and all tests pass

Adding:
jam for pdf_viewer
abarth for sky
hans for js content handler

PS: Hans -> The js content handler could use mojo/application/content_handler.h
that would handle the threading, and also let each js app use its own connection
to the shell, but this is not compatible with the standalone js app. Is there
any reason you need the standalone app?

[hansmuller, 2014-10-30 15:38:59]

On 2014/10/30 15:18:06, qsr wrote:
> CL is now ready. Everything compiles and all tests pass
> 
> Adding:
> jam for pdf_viewer
> abarth for sky
> hans for js content handler
> 
> PS: Hans -> The js content handler could use
mojo/application/content_handler.h
> that would handle the threading, and also let each js app use its own
connection
> to the shell, but this is not compatible with the standalone js app. Is there
> any reason you need the standalone app?

We can let the standalone JS app die.

At the moment there aren't any tests for the JS content handler however please
try the sample apps here:
https://code.google.com/p/chromium/issues/detail?id=425684#c3

You should see output that implies that the two apps are communicating:

from.js: exampleServiceImpl.ping(10)
from.js: exampleServiceImpl.ping(20)
from.js: exampleServiceImpl.ping(30)
from.js: exampleServiceImpl.ping(40)
from.js: exampleServiceImpl.ping(0)
to.js: exampleClient.pong(11)
to.js: exampleClient.pong(21)
to.js: exampleClient.pong(31)
to.js: exampleClient.pong(41)
to.js: exampleServiceImpl.ping(100)
to.js: exampleServiceImpl.ping(200)
to.js: exampleServiceImpl.ping(300)
to.js: exampleServiceImpl.ping(400)

[qsr, 2014-10-30 15:48:23]

On 2014/10/30 15:38:59, hansmuller wrote:
> On 2014/10/30 15:18:06, qsr wrote:
> > CL is now ready. Everything compiles and all tests pass
> > 
> > Adding:
> > jam for pdf_viewer
> > abarth for sky
> > hans for js content handler
> > 
> > PS: Hans -> The js content handler could use
> mojo/application/content_handler.h
> > that would handle the threading, and also let each js app use its own
> connection
> > to the shell, but this is not compatible with the standalone js app. Is
there
> > any reason you need the standalone app?
> 
> We can let the standalone JS app die.
> 
> At the moment there aren't any tests for the JS content handler however please
> try the sample apps here:
> https://code.google.com/p/chromium/issues/detail?id=425684#c3
> 
> You should see output that implies that the two apps are communicating:
> 
> from.js: exampleServiceImpl.ping(10)
> from.js: exampleServiceImpl.ping(20)
> from.js: exampleServiceImpl.ping(30)
> from.js: exampleServiceImpl.ping(40)
> from.js: exampleServiceImpl.ping(0)
> to.js: exampleClient.pong(11)
> to.js: exampleClient.pong(21)
> to.js: exampleClient.pong(31)
> to.js: exampleClient.pong(41)
> to.js: exampleServiceImpl.ping(100)
> to.js: exampleServiceImpl.ping(200)
> to.js: exampleServiceImpl.ping(300)
> to.js: exampleServiceImpl.ping(400)

 Once I fix the address to the .mojom file and the port, it works perfectly
fine. I will update the code though and remove the standalone app. That would be
less code on the js side of things.

[qsr, 2014-10-30 17:45:59]

js content handler updated to use the new API.

[jam, 2014-10-30 20:04:50]

I defer to whoever is reviewing png_handler, since I just copied that code :)

Did you verify that png/pdf viewing works?

[abarth-chromium, 2014-10-31 02:23:49]

//sky LGTM

[Aaron Boodman, 2014-10-31 08:24:01]

Last comments inline.

Also, here is a concrete suggestion for what I was talking about earlier:

https://codereview.chromium.org/688693004/

This is a pretty minor difference, but I like it better because I feel it
composes better. Content handling doesn't take over your MojoMain() and
ApplicationImpl - instead it snaps into ConfigureIncomingConnection just the
same as other InterfaceFactory's do.

While it's true that it results in slightly more boilerplate in application
implementations, I feel it is a good tradeoff because it allows the developer to
put additional things in MojoMain, or use ApplicationImpl for app-global state
and setup as they would in other applications.

If this doesn't convince you, I'm OK with what you have, but thought I'd at
least flesh out the idea.

https://codereview.chromium.org/687273002/diff/140001/examples/content_handle...
File examples/content_handler_demo/content_handler_demo.cc (right):

https://codereview.chromium.org/687273002/diff/140001/examples/content_handle...
examples/content_handler_demo/content_handler_demo.cc:24: virtual void
Initialize(Array<String> args) override {}
Nit: add a blank line after this method to match the others.

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:187: ApplicationImpl* app_;
app_ needs to be initialized in ctor.

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:207: virtual
scoped_ptr<mojo::InterfaceImpl<mojo::Application>> CreateApplication(
I don't think you need mojo:: for any of these. You're in the mojo namespace
already. Same elsewhere in this CL.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
File mojo/application/content_handler.cc (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:53: scoped_ptr<ApplicationDelegate>
application_delegate_;
This is not used.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:92: // TODO(qsr): If the last active thread
exited, quit this application. Not
Ah, because we never remove the entry from url_to_content_handler_? Is that not
easy to add in OnShellImplError()?

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:103: class ContentHandlerApp : public
ApplicationDelegate {
I like the naming better, thanks.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
File mojo/application/content_handler.h (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
I would expect this class to be in mojo/public/cpp/application/, as it feels
like part of the public sdk.

Except that my understanding is that mojo/public/cpp/* is not supposed to rely
upon base. Is that why you've put this here?

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.h:8: #include "base/callback.h"
move this to cc file.

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:256:
content_handler_shells_.insert(shell_impl);
How about the name 'content_shell_impls'?

1. it's parallel to url_to_shell_impl_
2. helps reinforce that this is not the shell to the contenthandler, but to the
content

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:307:
content_handler_shells_.erase(it);
return early.

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:309: const GURL url =
shell_impl->url();
Can't you remove the entry from url_to_content_handlers_ here?

[qsr, 2014-10-31 12:10:44]

https://codereview.chromium.org/687273002/diff/140001/examples/content_handle...
File examples/content_handler_demo/content_handler_demo.cc (right):

https://codereview.chromium.org/687273002/diff/140001/examples/content_handle...
examples/content_handler_demo/content_handler_demo.cc:24: virtual void
Initialize(Array<String> args) override {}
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> Nit: add a blank line after this method to match the others.

Done.

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:187: ApplicationImpl* app_;
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> app_ needs to be initialized in ctor.

Done.

https://codereview.chromium.org/687273002/diff/140001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:207: virtual
scoped_ptr<mojo::InterfaceImpl<mojo::Application>> CreateApplication(
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> I don't think you need mojo:: for any of these. You're in the mojo namespace
> already. Same elsewhere in this CL.

 Well, sky isn't, and I mixed the two. Thanks.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
File mojo/application/content_handler.cc (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:53: scoped_ptr<ApplicationDelegate>
application_delegate_;
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> This is not used.

Done.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:92: // TODO(qsr): If the last active thread
exited, quit this application. Not
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> Ah, because we never remove the entry from url_to_content_handler_? Is that
not
> easy to add in OnShellImplError()?

 I tried, and the simplest thing resulted in crashes. Thanks to your remark on
application_manager, I found out the 2 issues, and the content handler now close
itself.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.cc:103: class ContentHandlerApp : public
ApplicationDelegate {
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> I like the naming better, thanks.

Acknowledged.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
File mojo/application/content_handler.h (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> I would expect this class to be in mojo/public/cpp/application/, as it feels
> like part of the public sdk.
> 
> Except that my understanding is that mojo/public/cpp/* is not supposed to rely
> upon base. Is that why you've put this here?

 That and because it depended on ApplicationImpl. With your proposal the latter
is not true anymore, but the base dependency is real (I need to create run
loops, callbacks and threads). At this time, I have no way to move this to the
public sdk.

https://codereview.chromium.org/687273002/diff/140001/mojo/application/conten...
mojo/application/content_handler.h:8: #include "base/callback.h"
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> move this to cc file.

Done.

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:256:
content_handler_shells_.insert(shell_impl);
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> How about the name 'content_shell_impls'?
> 
> 1. it's parallel to url_to_shell_impl_
> 2. helps reinforce that this is not the shell to the contenthandler, but to
the
> content

Good idea. Done.

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:307:
content_handler_shells_.erase(it);
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> return early.

Done.

https://codereview.chromium.org/687273002/diff/140001/mojo/application_manage...
mojo/application_manager/application_manager.cc:309: const GURL url =
shell_impl->url();
On 2014/10/31 08:24:01, Aaron Boodman wrote:
> Can't you remove the entry from url_to_content_handlers_ here?

More than can, I must. That was part of the reason I couldn't close the content
handler when it has not content anymore (the other part is on line 122 -> This
must be a weak bind as we keep the InterfacePtr.

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
File mojo/application/content_handler.h (right):

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
mojo/application/content_handler.h:18: class HandledApplicationHolder {
I don't really like this, but I'm a little bit out of idea, so any suggestion
welcome:

 What I need is that CreateApplication returns an object that will be owned by
the caller, but all the caller care about is the life cycle, it will not call
any method on it or anything. It just needs to keep it alive until the run loop
is quit. I didn't find any other way than forcing this class and having a
templated factory that take a class an create such a holder. But this is kind of
ugly.

[qsr, 2014-10-31 12:11:36]

> Also, here is a concrete suggestion for what I was talking about earlier:
> 
> https://codereview.chromium.org/688693004/
> 
> This is a pretty minor difference, but I like it better because I feel it
> composes better. Content handling doesn't take over your MojoMain() and
> ApplicationImpl - instead it snaps into ConfigureIncomingConnection just the
> same as other InterfaceFactory's do.
> 
> While it's true that it results in slightly more boilerplate in application
> implementations, I feel it is a good tradeoff because it allows the developer
to
> put additional things in MojoMain, or use ApplicationImpl for app-global state
> and setup as they would in other applications.
> 
> If this doesn't convince you, I'm OK with what you have, but thought I'd at
> least flesh out the idea.

 I like the idea and implemented it. Thanks.

[hansmuller, 2014-10-31 16:14:44]

//js LGTM with a few nits.

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/con...
File mojo/apps/js/content_handler_main.cc (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/con...
mojo/apps/js/content_handler_main.cc:32: virtual bool
ConfigureIncomingConnection(
Is the virtual keyword needed here? Likewise most other virtual method
overrides.

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
File mojo/apps/js/js_app.cc (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
mojo/apps/js/js_app.cc:65: shell_runner_.reset();
This method should probably use ApplicationImpl::Terminate(),
https://codereview.chromium.org/672313002/

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
File mojo/apps/js/js_app.h (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
mojo/apps/js/js_app.h:42: MessagePipeHandle RequestorMessagePipeHandle();
The comment for this method is no longer correct.

[qsr, 2014-10-31 16:31:36]

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/con...
File mojo/apps/js/content_handler_main.cc (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/con...
mojo/apps/js/content_handler_main.cc:32: virtual bool
ConfigureIncomingConnection(
On 2014/10/31 16:14:44, hansmuller wrote:
> Is the virtual keyword needed here? Likewise most other virtual method
> overrides.

Done.

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
File mojo/apps/js/js_app.cc (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
mojo/apps/js/js_app.cc:65: shell_runner_.reset();
On 2014/10/31 16:14:44, hansmuller wrote:
> This method should probably use ApplicationImpl::Terminate(),
> https://codereview.chromium.org/672313002/

 I don't think so. The application it does terminate is the JsApp. It is not an
ApplicationImpl, so it should not do anything related to ApplicationImpl.

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
File mojo/apps/js/js_app.h (right):

https://chromiumcodereview.appspot.com/687273002/diff/200001/mojo/apps/js/js_...
mojo/apps/js/js_app.h:42: MessagePipeHandle RequestorMessagePipeHandle();
On 2014/10/31 16:14:44, hansmuller wrote:
> The comment for this method is no longer correct.

Done.

[Aaron Boodman, 2014-10-31 16:40:46]

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
File mojo/application/content_handler.cc (right):

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
mojo/application/content_handler.cc:41: base::MessageLoop
loop(common::MessagePumpMojo::Create());
I thought you wanted to support content handlers that didn't use message loops.
Are you thinking that those would not use this infrastructure?

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
File mojo/application/content_handler.h (right):

https://codereview.chromium.org/687273002/diff/160001/mojo/application/conten...
mojo/application/content_handler.h:18: class HandledApplicationHolder {
On 2014/10/31 12:10:44, qsr wrote:
> I don't really like this, but I'm a little bit out of idea, so any suggestion
> welcome:
> 
>  What I need is that CreateApplication returns an object that will be owned by
> the caller, but all the caller care about is the life cycle, it will not call
> any method on it or anything. It just needs to keep it alive until the run
loop
> is quit. I didn't find any other way than forcing this class and having a
> templated factory that take a class an create such a holder. But this is kind
of
> ugly.

You could have apps be responsible for deleting themselves when they exit the
message loop. That's only thing I can think of. (well you could templatize a lot
more of the code in the cc file, but I think that would be worse)

Otherwise, I think you can remove make_handled_factory_holder. Doesn't this
work:

return scoped_ptr<HandledApplicationHolder>(new MyApp());

(assuming MyApp inherits HandledApplicationHolder).

Also, consider renaming HandledApplicationHolder to "Destructable" or some such,
since that's all it is really doing.

https://codereview.chromium.org/687273002/diff/200001/mojo/application/conten...
File mojo/application/content_handler_factory.h (right):

https://codereview.chromium.org/687273002/diff/200001/mojo/application/conten...
mojo/application/content_handler_factory.h:27: // content. The application will
be run on its own thread, and the returned
Nit: Clarify in this comment that this method is also called on the new thread.

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:122:
WeakBindToProxy(&service_provider_impl, &service_provider);
Alternately, what if we just don't hold onto the ServiceProviderImpl?

BindToProxy(new ServiceProviderImpl(), &service_provider)

We don't need the SPImpl for anything once we have the content handler, and
perhaps not retaining it in this structure simplifies understanding the code.

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:312:
URLToContentHandlerMap::iterator content_handler_it =
auto

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:313:
url_to_content_handler_.find(url);
Is it possible for both the above and this to be true?

Imagine a nested content handler. The inner shell will be in
content_shell_impls_. But there will also be a content handler for that URL.

What about instead cleaning up the content handlers with
InterfacePtr<ContentHandler>::set_error_handler(), instead of trying to use the
shell pipe as a signal.

[Aaron Boodman, 2014-10-31 16:42:04]

lgtm w/ above

[qsr, 2014-10-31 17:22:32]

https://codereview.chromium.org/687273002/diff/200001/mojo/application/conten...
File mojo/application/content_handler_factory.h (right):

https://codereview.chromium.org/687273002/diff/200001/mojo/application/conten...
mojo/application/content_handler_factory.h:27: // content. The application will
be run on its own thread, and the returned
On 2014/10/31 16:40:46, Aaron Boodman wrote:
> Nit: Clarify in this comment that this method is also called on the new
thread.
> 
> 

Done.

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:122:
WeakBindToProxy(&service_provider_impl, &service_provider);
On 2014/10/31 16:40:46, Aaron Boodman wrote:
> Alternately, what if we just don't hold onto the ServiceProviderImpl?
> 
> BindToProxy(new ServiceProviderImpl(), &service_provider)
> 
> We don't need the SPImpl for anything once we have the content handler, and
> perhaps not retaining it in this structure simplifies understanding the code.

 But then you can imagine the service provider surviving the shell if the other
side doesn't kill it. Which seems weird to me...

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:312:
URLToContentHandlerMap::iterator content_handler_it =
On 2014/10/31 16:40:46, Aaron Boodman wrote:
> auto

Done.

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:313:
url_to_content_handler_.find(url);
What about instead cleaning up the content handlers with
> InterfacePtr<ContentHandler>::set_error_handler(), instead of trying to use
the
> shell pipe as a signal.

 Done.

[qsr, 2014-10-31 17:23:14]

Committed patchset #14 (id:260001) manually as
7c05714462a1f2c668dd3946d953cc36aa685bfc.

[jamesr, 2014-10-31 17:44:43]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-10-31 17:44:44]

This landed with a commit message that just said "WIP".  Please write more
descriptive messages - it's hard to tell what this patch does from the
description.

[Aaron Boodman, 2014-10-31 18:43:18]

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/687273002/diff/200001/mojo/application_manage...
mojo/application_manager/application_manager.cc:122:
WeakBindToProxy(&service_provider_impl, &service_provider);
On 2014/10/31 17:22:32, qsr wrote:
> On 2014/10/31 16:40:46, Aaron Boodman wrote:
> > Alternately, what if we just don't hold onto the ServiceProviderImpl?
> > 
> > BindToProxy(new ServiceProviderImpl(), &service_provider)
> > 
> > We don't need the SPImpl for anything once we have the content handler, and
> > perhaps not retaining it in this structure simplifies understanding the
code.
> 
>  But then you can imagine the service provider surviving the shell if the
other
> side doesn't kill it. Which seems weird to me...

What is the problem with that? If the app somehow drops its ShellPtr, but
doesn't drop the ServiceProviderPtr, then it will be able to make requests to
it, but we're not providing anything over it.

The more I work with Mojo, the more idiomatic it feels to not try and tie
different pipe's lifetimes together... precisely because of what you are
pointing out: the other side can always drop their side of the various pipes in
unexpected order.