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..0c51569c363f14a5a2b47bc9646b7ef00b907906 100644
--- a/mojo/application_manager/application_manager.cc
+++ b/mojo/application_manager/application_manager.cc
@@ -50,55 +50,6 @@ 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()) {}
-
- 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,
-                              URLResponsePtr url_response) override {
-    if (manager_) {
-      manager_->LoadWithContentHandler(requested_url_,
-                                       resolved_url_,
-                                       requestor_url_,
-                                       content_handler_url,
-                                       url_response.Pass(),
-                                       service_provider_.Pass());
-    }
-  }
-
-  base::WeakPtr<ApplicationManager> manager_;
-  GURL requested_url_;
-  GURL resolved_url_;
-  GURL requestor_url_;
-  ServiceProviderPtr service_provider_;
-};
-
 class ApplicationManager::ShellImpl : public Shell, public ErrorHandler {
  public:
   ShellImpl(ScopedMessagePipeHandle handle,
@@ -250,24 +201,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/20 18:26:08]

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.

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

 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.

+    url_to_shell_impl_[resolved_url] = shell;
+    shell->client()->Initialize(GetArgsForURL(requested_url));
+
+    loader->Load(this, resolved_url, pipe.handle1.Pass(),
+                 base::Bind(&ApplicationManager::LoadWithContentHandler,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
-
-  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 +251,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 +265,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,