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

mojo/application_manager/application_manager.cc

Index: mojo/application_manager/application_manager.cc
diff --git a/mojo/application_manager/application_manager.cc b/mojo/application_manager/application_manager.cc
index e99c627cf74cfeca6e60b1230856e23d25152136..494128f8239779cd0a9073ba48a1e5042f070f51 100644
--- a/mojo/application_manager/application_manager.cc
+++ b/mojo/application_manager/application_manager.cc
@@ -53,50 +53,22 @@ GURL ApplicationManager::Delegate::ResolveURL(const GURL& url) {
 class ApplicationManager::LoadCallbacksImpl
     : public ApplicationLoader::LoadCallbacks {
  public:
-  LoadCallbacksImpl(base::WeakPtr<ApplicationManager> manager,
-                    const GURL& requested_url,
-                    const GURL& resolved_url,
-                    const GURL& requestor_url,
-                    ServiceProviderPtr service_provider)
-      : manager_(manager),
-        requested_url_(requested_url),
-        resolved_url_(resolved_url),
-        requestor_url_(requestor_url),
-        service_provider_(service_provider.Pass()) {}
+  LoadCallbacksImpl(base::WeakPtr<ApplicationManager> manager)
+      : manager_(manager) {}

[DaveMoore, 2014/11/18 17:00:45]

The changes simplified the code in application_manager nicely.

 
  private:
   ~LoadCallbacksImpl() override {}
 
-  // LoadCallbacks implementation
-  ScopedMessagePipeHandle RegisterApplication() override {
-    ScopedMessagePipeHandle shell_handle;
-    if (manager_) {
-      manager_->RegisterLoadedApplication(requested_url_,
-                                          resolved_url_,
-                                          requestor_url_,
-                                          service_provider_.Pass(),
-                                          &shell_handle);
-    }
-    return shell_handle.Pass();
-  }
-
   void LoadWithContentHandler(const GURL& content_handler_url,
+                              ScopedMessagePipeHandle shell_handle,
                               URLResponsePtr url_response) override {
     if (manager_) {
-      manager_->LoadWithContentHandler(requested_url_,
-                                       resolved_url_,
-                                       requestor_url_,
-                                       content_handler_url,
-                                       url_response.Pass(),
-                                       service_provider_.Pass());
+      manager_->LoadWithContentHandler(content_handler_url, shell_handle.Pass(),
+                                       url_response.Pass());
     }
   }
 
   base::WeakPtr<ApplicationManager> manager_;
-  GURL requested_url_;
-  GURL resolved_url_;
-  GURL requestor_url_;
-  ServiceProviderPtr service_provider_;
 };
 
 class ApplicationManager::ShellImpl : public Shell, public ErrorHandler {
@@ -250,24 +222,28 @@ void ApplicationManager::ConnectToApplication(
 }
 
 void ApplicationManager::ConnectToApplicationImpl(
-    const GURL& requested_url, const GURL& resolved_url,
-    const GURL& requestor_url, ServiceProviderPtr service_provider,
+    const GURL& requested_url,
+    const GURL& resolved_url,
+    const GURL& requestor_url,
+    ServiceProviderPtr service_provider,
     ApplicationLoader* loader) {
+  ShellImpl* shell = nullptr;
   URLToShellImplMap::const_iterator shell_it =
       url_to_shell_impl_.find(resolved_url);
   if (shell_it != url_to_shell_impl_.end()) {
-    ConnectToClient(shell_it->second, resolved_url, requestor_url,
-                    service_provider.Pass());
-    return;
+    shell = shell_it->second;
+  } else {
+    MessagePipe pipe;
+    shell =
+        new ShellImpl(pipe.handle0.Pass(), this, requested_url, resolved_url);

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

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:37]

 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.

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

Yes.
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.
Another thing is that if A redirects to B, and someone connects to both, we only
want to end up with one app running.
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.

[qsr, 2014/11/19 16:37:21]

 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 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.

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

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.

+    url_to_shell_impl_[resolved_url] = shell;
+    shell->client()->Initialize(GetArgsForURL(requested_url));
+
+    scoped_refptr<LoadCallbacksImpl> callbacks(
+        new LoadCallbacksImpl(weak_ptr_factory_.GetWeakPtr()));
+    loader->Load(this, resolved_url, pipe.handle1.Pass(), callbacks);
   }
-
-  scoped_refptr<LoadCallbacksImpl> callbacks(
-      new LoadCallbacksImpl(weak_ptr_factory_.GetWeakPtr(),
-                            requested_url,
-                            resolved_url,
-                            requestor_url,
-                            service_provider.Pass()));
-  loader->Load(this, resolved_url, callbacks);
+  ConnectToClient(shell, resolved_url, requestor_url, service_provider.Pass());
 }
 
 void ApplicationManager::ConnectToClient(ShellImpl* shell_impl,
@@ -296,39 +272,10 @@ void ApplicationManager::RegisterExternalApplication(
   shell_impl->client()->Initialize(args.Pass());
 }
 
-void ApplicationManager::RegisterLoadedApplication(
-    const GURL& requested_url,
-    const GURL& resolved_url,
-    const GURL& requestor_url,
-    ServiceProviderPtr service_provider,
-    ScopedMessagePipeHandle* shell_handle) {
-  ShellImpl* shell_impl = NULL;
-  URLToShellImplMap::iterator iter = url_to_shell_impl_.find(resolved_url);
-  if (iter != url_to_shell_impl_.end()) {
-    // This can happen because services are loaded asynchronously. So if we get
-    // two requests for the same service close to each other, we might get here
-    // and find that we already have it.
-    shell_impl = iter->second;
-  } else {
-    MessagePipe pipe;
-    shell_impl =
-        new ShellImpl(pipe.handle1.Pass(), this, requested_url, resolved_url);
-    url_to_shell_impl_[resolved_url] = shell_impl;
-    *shell_handle = pipe.handle0.Pass();
-    shell_impl->client()->Initialize(GetArgsForURL(requested_url));
-  }
-
-  ConnectToClient(shell_impl, resolved_url, requestor_url,
-                  service_provider.Pass());
-}
-
 void ApplicationManager::LoadWithContentHandler(
-    const GURL& requested_url,
-    const GURL& resolved_url,
-    const GURL& requestor_url,
     const GURL& content_handler_url,
-    URLResponsePtr url_response,
-    ServiceProviderPtr service_provider) {
+    ScopedMessagePipeHandle shell_handle,
+    URLResponsePtr url_response) {
   ContentHandlerConnection* connection = NULL;
   URLToContentHandlerMap::iterator iter =
       url_to_content_handler_.find(content_handler_url);
@@ -339,16 +286,8 @@ void ApplicationManager::LoadWithContentHandler(
     url_to_content_handler_[content_handler_url] = connection;
   }
 
-  ShellPtr shell_proxy;
-  ShellImpl* shell_impl =
-      new ShellImpl(&shell_proxy, this, requested_url, resolved_url);
-  content_shell_impls_.insert(shell_impl);
-  shell_impl->client()->Initialize(GetArgsForURL(requested_url));
-
-  connection->content_handler()->StartApplication(shell_proxy.Pass(),
-                                                  url_response.Pass());
-  ConnectToClient(
-      shell_impl, resolved_url, requestor_url, service_provider.Pass());
+  connection->content_handler()->StartApplication(
+      MakeProxy<Shell>(shell_handle.Pass()), url_response.Pass());
 }
 
 void ApplicationManager::SetLoaderForURL(scoped_ptr<ApplicationLoader> loader,