Make sure that Content Handled application can be connected multiple times.

Make sure that Content Handled application can be connected multiple times.

This CL updates ApplicationManager so that it always reconnect to an
application that is already launched.

To obtain this results, the following changes habe also been made:

1) The Loader interface has been simplified:
  - The pipe to the shell if given during the Load request, so that loader do
    not need to called RegisterLoadedApplication
2) PngViewer and PdfViewer allows multiple embedder to embed those.
3) For the second connection forward, Sky issue network request to get the
   content of the page to display.

R=aa@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/5170129e3c402b3ee8cecb914c934fd56019b6e9

[qsr, 2014-11-18 16:27:27]

qsr@chromium.org changed reviewers:
+ aa@chromium.org, davemoore@chromium.org, hansmuller@chromium.org

[qsr, 2014-11-18 16:27:27]

Aaron -> For review. I didn't do html_viewer yet, but it will work the same way
sky does.

Hans -> I didn't touch the javascript content handler, right now, a second
connection to an app will end the first application. The issue is that the API
for the content handler is weird, so I do not really know what you want to do
with it.

Dave -> You wanted to be CC'd on all application_manager changes.

[DaveMoore, 2014-11-18 17:00:46]

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_loader.h (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_loader.h:33: URLResponsePtr url_response) =
0;
I like this. It's a lot more like ApplicationLoader used to be before the
ContentHandler support was added.

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:57: : manager_(manager) {}
The changes simplified the code in application_manager nicely.

[Aaron Boodman, 2014-11-18 23:46:15]

This is great. I'm convinced now this is the right approach.

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
examples/pdf_viewer/pdf_viewer.cc:33: namespace {
It feels more natural to me that applications such as pdf viewer and html viewer
would re-fetch their content on subsequent connections.

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:126: ApplicationImpl::Terminate();
Out of scope for this CL, but I don't think this is the right time to shut down
the app.

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_loader.h (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_loader.h:27: class
MOJO_APPLICATION_MANAGER_EXPORT LoadCallbacks
We don't need LoadCallbacks anymore. We can just use an actual base::Callback,
right?

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/bac...
File mojo/application_manager/background_shell_application_loader.h (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/bac...
mojo/application_manager/background_shell_application_loader.h:38: class
BackgroundLoader;
This is dead now.

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
mojo/shell/dynamic_application_loader.cc:108:
AsPath(context_->task_runners()->blocking_pool(),
It will be great when we pull NativeContentHandler out of this, it will
dramatically simplify this guy.

Then maybe we can also separate DynamicApplicationLoader into totally separate
NetworkLoader and LocalLoader subclasses of ApplicationLoader.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
File sky/viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:15: class EmptyServiceProvider : public
mojo::InterfaceImpl<mojo::ServiceProvider> {
unused?

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:32: nb_views_(0) {
view_count_ ? or num_views_

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:35: mojo::GetProxy(&network_service_app_));
Just make network_service_app a local. You don't need to retain it. You can call
it service_provider, too.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:37:
network_service_app_->ConnectToService(mojo::NetworkService::Name_,
Use mojo/public/cpp/application/connect.h

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:50: mojo::URLLoaderPtr* loader = new
mojo::URLLoaderPtr;
What happens if this is a normal URLLoaderPtr, and you Pass() it to Start()?
Seems like that would be the lifetime you want, but you must have tried it and
it didn't work...?

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:70: void
OnResponseReceived(mojo::URLLoaderPtr* loader,
Doesn't loader leak?

You may have to keep the loader alive as long as the URLResponse is, which would
mean passing into DocumentView. I can't remember the rules there.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:81: mojo::ServiceProviderPtr
network_service_app_;
You don't need to keep this alive. Just make it a local where it is used.

[Aaron Boodman, 2014-11-19 01:47:24]

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:238: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
Argh, I just realized we can't make this simplification.

We don't know the identity of the application we're connecting to until after
the request. If http://a redirects to http://b, then it should be treated
identically to http://a.

This throws a wrench in the works ... that was part of the reason that making
the request for every connection was a good idea.

[qsr, 2014-11-19 13:42:38]

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
examples/pdf_viewer/pdf_viewer.cc:33: namespace {
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> It feels more natural to me that applications such as pdf viewer and html
viewer
> would re-fetch their content on subsequent connections.

 Can we delay those changes to a next CL?

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:126: ApplicationImpl::Terminate();
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> Out of scope for this CL, but I don't think this is the right time to shut
down
> the app.

 Can you elaborate? I can always change it in a next CL, but I'm not sure to
understand why, do you prefer to do this in OnViewManagerDisconnected? Never?

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_loader.h (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_loader.h:27: class
MOJO_APPLICATION_MANAGER_EXPORT LoadCallbacks
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> We don't need LoadCallbacks anymore. We can just use an actual base::Callback,
> right?

Done.

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:238: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
On 2014/11/19 01:47:24, Aaron Boodman wrote:
> Argh, I just realized we can't make this simplification.
> 
> We don't know the identity of the application we're connecting to until after
> the request. If http://a redirects to http://b, then it should be treated
> identically to http://a.
> 
> This throws a wrench in the works ... that was part of the reason that making
> the request for every connection was a good idea.

 I guess you mean it has to be treated identically to http://b ?

 So first of all, that's not what we are doing now. We already have the
resolved_url at this point, and nothing will ever change it, even if there is
redirects. I agree that if we do a network request and wait for it before doing
any connection, we could use the resolved_url instead.

 So I don't really know what we want there. On the good thing about resolving
the URL chain there is:
1) That can handle nicely URL as application state (for navigation, linking,
etc.): the application will redirect all URL it handles to the same one, and we
just need to pass the information about the initial URL to let the application
handle its state nicely.
2) We do not have to worry about running application of origin A in origin B.

 But on the other end, the API we have is cumbersome to use. We will be redoing
network request all the time. And we will need to have URLResponse for file://
urls, which mean streaming the data without it ever be used. Let me try to write
a CL for the other API, and we will see what to do with those.

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/bac...
File mojo/application_manager/background_shell_application_loader.h (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/bac...
mojo/application_manager/background_shell_application_loader.h:38: class
BackgroundLoader;
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> This is dead now.

Done.

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
mojo/shell/dynamic_application_loader.cc:108:
AsPath(context_->task_runners()->blocking_pool(),
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> It will be great when we pull NativeContentHandler out of this, it will
> dramatically simplify this guy.
> 
> Then maybe we can also separate DynamicApplicationLoader into totally separate
> NetworkLoader and LocalLoader subclasses of ApplicationLoader.

Acknowledged.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
File sky/viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:15: class EmptyServiceProvider : public
mojo::InterfaceImpl<mojo::ServiceProvider> {
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> unused?

Seems so.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:32: nb_views_(0) {
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> view_count_ ? or num_views_

Done.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:35: mojo::GetProxy(&network_service_app_));
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> Just make network_service_app a local. You don't need to retain it. You can
call
> it service_provider, too.

 I was not sure of what will happen when I close the service provider. Done.
Thanks.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:37:
network_service_app_->ConnectToService(mojo::NetworkService::Name_,
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> Use mojo/public/cpp/application/connect.h

Done.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:50: mojo::URLLoaderPtr* loader = new
mojo::URLLoaderPtr;
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> What happens if this is a normal URLLoaderPtr, and you Pass() it to Start()?
> Seems like that would be the lifetime you want, but you must have tried it and
> it didn't work...?

 The issue is we then do:

loader->Start(..., loader.Pass())

I'm not enough of a C++ expect to know the order of the -> and .Pass() calls.

As all I can find on order of execution is that order of discussion of
parameters is not specified, I'm not certain that I can have the -> be evaluated
before the .Pass(), and if it is not the case, then this will be a segfault.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:70: void
OnResponseReceived(mojo::URLLoaderPtr* loader,
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> Doesn't loader leak?

 Unless I'm misunderstanding base::Owned, I don't think it does. The loader is
owned by the callback that will die some time just after this call.

> You may have to keep the loader alive as long as the URLResponse is, which
would
> mean passing into DocumentView. I can't remember the rules there.

 I don't think I need to do this. At least all seems to be working :) (and I had
test failing without this change, so we indeed reach the case where the same sky
app is connected to multiple time).

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:81: mojo::ServiceProviderPtr
network_service_app_;
On 2014/11/18 23:46:15, Aaron Boodman wrote:
> You don't need to keep this alive. Just make it a local where it is used.

Done.

[Aaron Boodman, 2014-11-19 15:58:15]

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/pdf_viewer/pdf_view...
examples/pdf_viewer/pdf_viewer.cc:33: namespace {
On 2014/11/19 13:42:37, qsr wrote:
> On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > It feels more natural to me that applications such as pdf viewer and html
> viewer
> > would re-fetch their content on subsequent connections.
> 
>  Can we delay those changes to a next CL?

yes.

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:110: // TODO(aa): Need to figure out how
shutdown works.
Can you remove this comment? Or move it to OnViewDestroyed()?

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:126: ApplicationImpl::Terminate();
On 2014/11/19 13:42:37, qsr wrote:
> On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > Out of scope for this CL, but I don't think this is the right time to shut
> down
> > the app.
> 
>  Can you elaborate? I can always change it in a next CL, but I'm not sure to
> understand why, do you prefer to do this in OnViewManagerDisconnected? Never?

This depends on the order you receive OnEmbed() and OnViewDestroyed(). That
seems pretty brittle to rely upon.

I think if you wanted something like this you could listen to
OnViewManagerDisconnected(). Because once that happens you know that you cannot
receive any more messages from the view manager.

But in general, this doesn't seem like the right way to shut down mojo apps. How
do you know that some other service is not doing something? Or won't begin doing
something shortly?

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:238: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
On 2014/11/19 13:42:37, qsr wrote:
> On 2014/11/19 01:47:24, Aaron Boodman wrote:
> > Argh, I just realized we can't make this simplification.
> > 
> > We don't know the identity of the application we're connecting to until
after
> > the request. If http://a redirects to http://b, then it should be treated
> > identically to http://a.
> > 
> > This throws a wrench in the works ... that was part of the reason that
making
> > the request for every connection was a good idea.
> 
>  I guess you mean it has to be treated identically to http://b ?

Yes.

>  So first of all, that's not what we are doing now. We already have the
> resolved_url at this point, and nothing will ever change it, even if there is
> redirects.

Whoops, that sounds like a bug. It was at least in my head that we needed to do
this. I don't know if it was ever in code.

> I agree that if we do a network request and wait for it before doing
> any connection, we could use the resolved_url instead.
> 
>  So I don't really know what we want there. On the good thing about resolving
> the URL chain there is:
> 1) That can handle nicely URL as application state (for navigation, linking,
> etc.): the application will redirect all URL it handles to the same one, and
we
> just need to pass the information about the initial URL to let the application
> handle its state nicely.
> 2) We do not have to worry about running application of origin A in origin B.

Another thing is that if A redirects to B, and someone connects to both, we only
want to end up with one app running.

>  But on the other end, the API we have is cumbersome to use. We will be
redoing
> network request all the time. And we will need to have URLResponse for file://
> urls, which mean streaming the data without it ever be used. Let me try to
write
> a CL for the other API, and we will see what to do with those.

I thought we already converted file requests to URLResponse. In
DynamicApplicationLoader's NetworkLoader. Were you planning to remove that, or
am I misunderstanding?

I think we could avoid the "network requests all the time" by also keeping state
about the fact that A redirected to B. It feels weird to do this because it can
get out of sync with what the http stack thinks. But that is no different than
reconnecting to the app as long as it is running.

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/shell/dynamic_applicati...
mojo/shell/dynamic_application_loader.cc:108:
AsPath(context_->task_runners()->blocking_pool(),
On 2014/11/19 13:42:38, qsr wrote:
> On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > It will be great when we pull NativeContentHandler out of this, it will
> > dramatically simplify this guy.
> > 
> > Then maybe we can also separate DynamicApplicationLoader into totally
separate
> > NetworkLoader and LocalLoader subclasses of ApplicationLoader.
> 
> Acknowledged.

I wonder if we could do a MojoLoader too, that resolves the mojo URL, then
delegates to NetworkLoader and LocalLoader.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
File sky/viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:50: mojo::URLLoaderPtr* loader = new
mojo::URLLoaderPtr;
On 2014/11/19 13:42:38, qsr wrote:
> On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > What happens if this is a normal URLLoaderPtr, and you Pass() it to Start()?
> > Seems like that would be the lifetime you want, but you must have tried it
and
> > it didn't work...?
> 
>  The issue is we then do:
> 
> loader->Start(..., loader.Pass())
> 
> I'm not enough of a C++ expect to know the order of the -> and .Pass() calls.
> 
> As all I can find on order of execution is that order of discussion of
> parameters is not specified, I'm not certain that I can have the -> be
evaluated
> before the .Pass(), and if it is not the case, then this will be a segfault.

I pinged jyasskin, my go-to C++ expert. He says you're right -- the order is
undefined.

How about:

URLLoaderPtr loader;

< some explanatory comment >
auto loader_raw = loader.get();

loader_raw->Start(..., loader.Pass());

That way you avoid the scary-looking raw pointer at OnResponseReceived().

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:70: void
OnResponseReceived(mojo::URLLoaderPtr* loader,
On 2014/11/19 13:42:38, qsr wrote:
> On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > Doesn't loader leak?
> 
>  Unless I'm misunderstanding base::Owned, I don't think it does. The loader is
> owned by the callback that will die some time just after this call.

OK, just scary-looking. See above comment.

> > You may have to keep the loader alive as long as the URLResponse is, which
> would
> > mean passing into DocumentView. I can't remember the rules there.
> 
>  I don't think I need to do this. At least all seems to be working :) (and I
had
> test failing without this change, so we indeed reach the case where the same
sky
> app is connected to multiple time).

OK, it's probably that you need to keep URLResponse alive in order to use the
body pipe. Which you're doing.

[Aaron Boodman, 2014-11-19 16:09:58]

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/application_manager_unittest.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/application_manager_unittest.cc:119: LoadCallback
callbacks) override {
here and elsewhere, can you change the param name to 'callback' (singular)?

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/background_shell_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/background_shell_application_loader.cc:82:
loader_->Load(manager, url, shell_handle.Pass(), LoadCallback());
I thought you said in a previous review that there was some problem with calling
Run() on an empty callback.

I like this better than binding to an empty function, though. Did you convince
yourself it was OK?

[qsr, 2014-11-19 16:37:22]

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:110: // TODO(aa): Need to figure out how
shutdown works.
On 2014/11/19 15:58:15, Aaron Boodman wrote:
> Can you remove this comment? Or move it to OnViewDestroyed()?

Done.

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:126: ApplicationImpl::Terminate();
On 2014/11/19 15:58:15, Aaron Boodman wrote:
> On 2014/11/19 13:42:37, qsr wrote:
> > On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > > Out of scope for this CL, but I don't think this is the right time to shut
> > down
> > > the app.
> > 
> >  Can you elaborate? I can always change it in a next CL, but I'm not sure to
> > understand why, do you prefer to do this in OnViewManagerDisconnected?
Never?
> 
> This depends on the order you receive OnEmbed() and OnViewDestroyed(). That
> seems pretty brittle to rely upon.

 Hum... I really hope I can count on receiving OnEmbed before receiving
OnViewDestroyed for the same view, otherwise, this cannot work at all... And if
this work, then I do not understand why this is brittle. I'm clearly missing
something.

> I think if you wanted something like this you could listen to
> OnViewManagerDisconnected(). Because once that happens you know that you
cannot
> receive any more messages from the view manager.
> 
> But in general, this doesn't seem like the right way to shut down mojo apps.
How
> do you know that some other service is not doing something? Or won't begin
doing
> something shortly?

 Hum... Then I probably also need to keep track of connected app by keeping
track of ServiceProvider. Again, if you do not mind, I'll let that for another
CL?

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:238: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
On 2014/11/19 15:58:15, Aaron Boodman wrote:
> On 2014/11/19 13:42:37, qsr wrote:
> > On 2014/11/19 01:47:24, Aaron Boodman wrote:
> > > Argh, I just realized we can't make this simplification.
> > > 
> > > We don't know the identity of the application we're connecting to until
> after
> > > the request. If http://a redirects to http://b, then it should be treated
> > > identically to http://a.
> > > 
> > > This throws a wrench in the works ... that was part of the reason that
> making
> > > the request for every connection was a good idea.
> > 
> >  I guess you mean it has to be treated identically to http://b ?
> 
> Yes.
> 
> >  So first of all, that's not what we are doing now. We already have the
> > resolved_url at this point, and nothing will ever change it, even if there
is
> > redirects.
> 
> Whoops, that sounds like a bug. It was at least in my head that we needed to
do
> this. I don't know if it was ever in code.
> 
> > I agree that if we do a network request and wait for it before doing
> > any connection, we could use the resolved_url instead.
> > 
> >  So I don't really know what we want there. On the good thing about
resolving
> > the URL chain there is:
> > 1) That can handle nicely URL as application state (for navigation, linking,
> > etc.): the application will redirect all URL it handles to the same one, and
> we
> > just need to pass the information about the initial URL to let the
application
> > handle its state nicely.
> > 2) We do not have to worry about running application of origin A in origin
B.
> 
> Another thing is that if A redirects to B, and someone connects to both, we
only
> want to end up with one app running.
> 
> >  But on the other end, the API we have is cumbersome to use. We will be
> redoing
> > network request all the time. And we will need to have URLResponse for
file://
> > urls, which mean streaming the data without it ever be used. Let me try to
> write
> > a CL for the other API, and we will see what to do with those.
> 
> I thought we already converted file requests to URLResponse. In
> DynamicApplicationLoader's NetworkLoader. Were you planning to remove that, or
> am I misunderstanding?

 Only for content handled app. For usual app, we have this LocalLoader that
directly use the path to load it. So no need to stream any data.

> I think we could avoid the "network requests all the time" by also keeping
state
> about the fact that A redirected to B. It feels weird to do this because it
can
> get out of sync with what the http stack thinks. But that is no different than
> reconnecting to the app as long as it is running.

 I will have a CL for network request all the time + passing the request to the
application (once we always do the request, let the application know about it).
I think I found a way to have it not too horrible on the shell side. After this,
we will need to choose which API we want.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
File sky/viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:50: mojo::URLLoaderPtr* loader = new
mojo::URLLoaderPtr;
On 2014/11/19 15:58:15, Aaron Boodman wrote:
> On 2014/11/19 13:42:38, qsr wrote:
> > On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > > What happens if this is a normal URLLoaderPtr, and you Pass() it to
Start()?
> > > Seems like that would be the lifetime you want, but you must have tried it
> and
> > > it didn't work...?
> > 
> >  The issue is we then do:
> > 
> > loader->Start(..., loader.Pass())
> > 
> > I'm not enough of a C++ expect to know the order of the -> and .Pass()
calls.
> > 
> > As all I can find on order of execution is that order of discussion of
> > parameters is not specified, I'm not certain that I can have the -> be
> evaluated
> > before the .Pass(), and if it is not the case, then this will be a segfault.
> 
> I pinged jyasskin, my go-to C++ expert. He says you're right -- the order is
> undefined.
> 
> How about:
> 
> URLLoaderPtr loader;
> 
> < some explanatory comment >
> auto loader_raw = loader.get();
> 
> loader_raw->Start(..., loader.Pass());
> 
> That way you avoid the scary-looking raw pointer at OnResponseReceived(). 

Done.

https://codereview.chromium.org/741453002/diff/1/sky/viewer/content_handler_i...
sky/viewer/content_handler_impl.cc:70: void
OnResponseReceived(mojo::URLLoaderPtr* loader,
On 2014/11/19 15:58:15, Aaron Boodman wrote:
> On 2014/11/19 13:42:38, qsr wrote:
> > On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > > Doesn't loader leak?
> > 
> >  Unless I'm misunderstanding base::Owned, I don't think it does. The loader
is
> > owned by the callback that will die some time just after this call.
> 
> OK, just scary-looking. See above comment.
> 
> > > You may have to keep the loader alive as long as the URLResponse is, which
> > would
> > > mean passing into DocumentView. I can't remember the rules there.
> > 
> >  I don't think I need to do this. At least all seems to be working :) (and I
> had
> > test failing without this change, so we indeed reach the case where the same
> sky
> > app is connected to multiple time).
> 
> OK, it's probably that you need to keep URLResponse alive in order to use the
> body pipe. Which you're doing.

 Yes, that's for sure. The response owns the body. But I can clearly pass the
body and kill the response, and that has to work, because nothing can keep track
for struct on the other side.

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/application_manager_unittest.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/application_manager_unittest.cc:119: LoadCallback
callbacks) override {
On 2014/11/19 16:09:57, Aaron Boodman wrote:
> here and elsewhere, can you change the param name to 'callback' (singular)?

Done.

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/background_shell_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/background_shell_application_loader.cc:82:
loader_->Load(manager, url, shell_handle.Pass(), LoadCallback());
On 2014/11/19 16:09:58, Aaron Boodman wrote:
> I thought you said in a previous review that there was some problem with
calling
> Run() on an empty callback.
> 
> I like this better than binding to an empty function, though. Did you convince
> yourself it was OK?

 Nope, it is clearly not ok. But this code was using SimpleLoadCallbacks, which
was using NOT_REACHED on LoadWithContentHandler, so the code is expecting this
callback to never be called.

[Aaron Boodman, 2014-11-20 00:59:01]

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/741453002/diff/1/examples/png_viewer/png_view...
examples/png_viewer/png_viewer.cc:126: ApplicationImpl::Terminate();
On 2014/11/19 16:37:21, qsr wrote:
> On 2014/11/19 15:58:15, Aaron Boodman wrote:
> > On 2014/11/19 13:42:37, qsr wrote:
> > > On 2014/11/18 23:46:15, Aaron Boodman wrote:
> > > > Out of scope for this CL, but I don't think this is the right time to
shut
> > > down
> > > > the app.
> > > 
> > >  Can you elaborate? I can always change it in a next CL, but I'm not sure
to
> > > understand why, do you prefer to do this in OnViewManagerDisconnected?
> Never?
> > 
> > This depends on the order you receive OnEmbed() and OnViewDestroyed(). That
> > seems pretty brittle to rely upon.
> 
>  Hum... I really hope I can count on receiving OnEmbed before receiving
> OnViewDestroyed for the same view, otherwise, this cannot work at all... And
if
> this work, then I do not understand why this is brittle. I'm clearly missing
> something.

No what I mean is that, during navigation for example, you might receive:

Embed(v1)
Embed(v2)
Destroy(v1)
Destroy(v2)

Or you might receive:

Embed(v1)
Destroy(v1)
Embed(v2)
Destroy(v2)

Both are reasonable orders for the VM to dispatch these messages in, depending
on implementation details.

But in the second example, you will shut down the app before Embed(v2) happens.
I think it would be better if apps did not depend on the order of these
events...

> > I think if you wanted something like this you could listen to
> > OnViewManagerDisconnected(). Because once that happens you know that you
> cannot
> > receive any more messages from the view manager.
> > 
> > But in general, this doesn't seem like the right way to shut down mojo apps.
> How
> > do you know that some other service is not doing something? Or won't begin
> doing
> > something shortly?
> 
>  Hum... Then I probably also need to keep track of connected app by keeping
> track of ServiceProvider. Again, if you do not mind, I'll let that for another
> CL?

What I meant to convey is that shutdown is going to be a much larger problem
than this one CL or this one app. We should probably not write any shut down
code until we have a unified plan for how it's going to work for all apps. It's
just more code getting in the way.

Anyway this was already here before your CL, so you can just leave it.

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/1/mojo/application_manager/app...
mojo/application_manager/application_manager.cc:238: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
On 2014/11/19 16:37:21, qsr wrote:
> On 2014/11/19 15:58:15, Aaron Boodman wrote:
> > On 2014/11/19 13:42:37, qsr wrote:
> > > On 2014/11/19 01:47:24, Aaron Boodman wrote:
> > > > Argh, I just realized we can't make this simplification.
> > > > 
> > > > We don't know the identity of the application we're connecting to until
> > after
> > > > the request. If http://a redirects to http://b, then it should be
treated
> > > > identically to http://a.
> > > > 
> > > > This throws a wrench in the works ... that was part of the reason that
> > making
> > > > the request for every connection was a good idea.
> > > 
> > >  I guess you mean it has to be treated identically to http://b ?
> > 
> > Yes.
> > 
> > >  So first of all, that's not what we are doing now. We already have the
> > > resolved_url at this point, and nothing will ever change it, even if there
> is
> > > redirects.
> > 
> > Whoops, that sounds like a bug. It was at least in my head that we needed to
> do
> > this. I don't know if it was ever in code.
> > 
> > > I agree that if we do a network request and wait for it before doing
> > > any connection, we could use the resolved_url instead.
> > > 
> > >  So I don't really know what we want there. On the good thing about
> resolving
> > > the URL chain there is:
> > > 1) That can handle nicely URL as application state (for navigation,
linking,
> > > etc.): the application will redirect all URL it handles to the same one,
and
> > we
> > > just need to pass the information about the initial URL to let the
> application
> > > handle its state nicely.
> > > 2) We do not have to worry about running application of origin A in origin
> B.
> > 
> > Another thing is that if A redirects to B, and someone connects to both, we
> only
> > want to end up with one app running.
> > 
> > >  But on the other end, the API we have is cumbersome to use. We will be
> > redoing
> > > network request all the time. And we will need to have URLResponse for
> file://
> > > urls, which mean streaming the data without it ever be used. Let me try to
> > write
> > > a CL for the other API, and we will see what to do with those.
> > 
> > I thought we already converted file requests to URLResponse. In
> > DynamicApplicationLoader's NetworkLoader. Were you planning to remove that,
or
> > am I misunderstanding?
> 
>  Only for content handled app. For usual app, we have this LocalLoader that
> directly use the path to load it. So no need to stream any data.

OK, I don't follow exactly why this changes whether you use LocalLoader, but I
can assert that we need to use the post-redirect URL as the app identity one way
or other.

> > I think we could avoid the "network requests all the time" by also keeping
> state
> > about the fact that A redirected to B. It feels weird to do this because it
> can
> > get out of sync with what the http stack thinks. But that is no different
than
> > reconnecting to the app as long as it is running.
> 
>  I will have a CL for network request all the time + passing the request to
the
> application (once we always do the request, let the application know about
it).
> I think I found a way to have it not too horrible on the shell side. After
this,
> we will need to choose which API we want.

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/background_shell_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/background_shell_application_loader.cc:82:
loader_->Load(manager, url, shell_handle.Pass(), LoadCallback());
On 2014/11/19 16:37:22, qsr wrote:
> On 2014/11/19 16:09:58, Aaron Boodman wrote:
> > I thought you said in a previous review that there was some problem with
> calling
> > Run() on an empty callback.
> > 
> > I like this better than binding to an empty function, though. Did you
convince
> > yourself it was OK?
> 
>  Nope, it is clearly not ok. But this code was using SimpleLoadCallbacks,
which
> was using NOT_REACHED on LoadWithContentHandler, so the code is expecting this
> callback to never be called.

I see. In that case, we should probably make this more self-explanatory somehow.
Like passing a function that immediately NOTREACHED()'s.

[qsr, 2014-11-20 10:02:59]

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
File mojo/application_manager/background_shell_application_loader.cc (right):

https://codereview.chromium.org/741453002/diff/20001/mojo/application_manager...
mojo/application_manager/background_shell_application_loader.cc:82:
loader_->Load(manager, url, shell_handle.Pass(), LoadCallback());
On 2014/11/20 00:59:01, Aaron Boodman wrote:
> On 2014/11/19 16:37:22, qsr wrote:
> > On 2014/11/19 16:09:58, Aaron Boodman wrote:
> > > I thought you said in a previous review that there was some problem with
> > calling
> > > Run() on an empty callback.
> > > 
> > > I like this better than binding to an empty function, though. Did you
> convince
> > > yourself it was OK?
> > 
> >  Nope, it is clearly not ok. But this code was using SimpleLoadCallbacks,
> which
> > was using NOT_REACHED on LoadWithContentHandler, so the code is expecting
this
> > callback to never be called.
> 
> I see. In that case, we should probably make this more self-explanatory
somehow.
> Like passing a function that immediately NOTREACHED()'s.

Done.

[Aaron Boodman, 2014-11-20 18:26:08]

LGTM, but please delete the content_shell map thing.

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
mojo/application_manager/application_manager.cc:176:
STLDeleteElements(&content_shell_impls_);
content_shell_impls_ is no longer used and should be removed.

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
mojo/application_manager/application_manager.cc:217: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
This code is still incorrect. We can't register the ShellImpl until we know the
final URL.

The code for native apps is also wrong, in the same way, as you pointed out. I'm
not sure how to resolve this while still supporting the special loaders like
BackgroundThreadLoader. Argh. I wonder if we can find a way to get rid of those.

Anyway, it can be a separate change.

[Aaron Boodman, 2014-11-20 18:26:17]

set rather, whatever

[qsr, 2014-11-21 09:50:08]

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
mojo/application_manager/application_manager.cc:176:
STLDeleteElements(&content_shell_impls_);
On 2014/11/20 18:26:08, Aaron Boodman wrote:
> content_shell_impls_ is no longer used and should be removed.

Done.

https://codereview.chromium.org/741453002/diff/80001/mojo/application_manager...
mojo/application_manager/application_manager.cc:217: new
ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);
On 2014/11/20 18:26:08, Aaron Boodman wrote:
> This code is still incorrect. We can't register the ShellImpl until we know
the
> final URL.
> 
> The code for native apps is also wrong, in the same way, as you pointed out.
I'm
> not sure how to resolve this while still supporting the special loaders like
> BackgroundThreadLoader. Argh. I wonder if we can find a way to get rid of
those.
> 
> Anyway, it can be a separate change.

 Will look into this. By separating request and loading, we can solve this. We
can do the request without automatically follow url redirection, so that if we
are redirecting to a URL that already have a shell, we can reuse it.

[qsr, 2014-11-21 09:53:40]

Committed patchset #6 (id:100001) manually as
5170129e3c402b3ee8cecb914c934fd56019b6e9 (presubmit successful).

[eseidel, 2014-12-05 23:11:50]

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

[eseidel, 2014-12-05 23:11:50]

I believe this broke skydb reload.

sky/tools/skydb your_favorite_file.sky
(skydb) reload
// does nothing?
(skydb) reload
// properly reloads!
(skydb) reload
// does nothing?
(skydb) reload
// properly reloads!

It has to do with deleting the SkyApplication at a bad time.  I've hacked around
this in https://codereview.chromium.org/780413002/, but I'm sure there is a
better solution.