mojo: Update content handler API

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 75ef7a76694b24773a65b38e644d83bd901d1ce9..7cf08eb6363607755ddb41713d1cb9158c7d7898 100644
--- a/mojo/application_manager/application_manager.cc
+++ b/mojo/application_manager/application_manager.cc
@@ -119,7 +119,7 @@ struct ApplicationManager::ContentHandlerConnection {
   ContentHandlerConnection(ApplicationManager* manager,
                            const GURL& content_handler_url) {
     ServiceProviderPtr service_provider;
-    BindToProxy(&service_provider_impl, &service_provider);
+    WeakBindToProxy(&service_provider_impl, &service_provider);

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

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.

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

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

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

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.

     manager->ConnectToApplication(
         content_handler_url, GURL(), service_provider.Pass());
     mojo::ConnectToService(service_provider_impl.client(), &content_handler);
@@ -161,6 +161,7 @@ ApplicationManager::~ApplicationManager() {
 
 void ApplicationManager::TerminateShellConnections() {
   STLDeleteValues(&url_to_shell_impl_);
+  STLDeleteElements(&content_shell_impls_);
 }
 
 // static
@@ -224,14 +225,10 @@ void ApplicationManager::RegisterLoadedApplication(
     shell_impl = iter->second;
   } else {
     MessagePipe pipe;
-    URLToArgsMap::const_iterator args_it = url_to_args_.find(url);
-    Array<String> args;
-    if (args_it != url_to_args_.end())
-      args = Array<String>::From(args_it->second);
     shell_impl = WeakBindToPipe(new ShellImpl(this, url), pipe.handle1.Pass());
     url_to_shell_impl_[url] = shell_impl;
     *shell_handle = pipe.handle0.Pass();
-    shell_impl->client()->Initialize(args.Pass());
+    shell_impl->client()->Initialize(GetArgsForURL(url));
   }
 
   ConnectToClient(shell_impl, url, requestor_url, service_provider.Pass());
@@ -253,10 +250,16 @@ void ApplicationManager::LoadWithContentHandler(
     url_to_content_handler_[content_handler_url] = connection;
   }
 
-  InterfaceRequest<ServiceProvider> spir;
-  spir.Bind(service_provider.PassMessagePipe());
-  connection->content_handler->OnConnect(
-      requestor_url.spec(), url_response.Pass(), spir.Pass());
+  ShellPtr shell_proxy;
+  ShellImpl* shell_impl =
+      WeakBindToProxy(new ShellImpl(this, content_url), &shell_proxy);
+  content_shell_impls_.insert(shell_impl);
+  shell_impl->client()->Initialize(GetArgsForURL(content_url));
+
+  connection->content_handler->StartApplication(shell_proxy.Pass(),
+                                                url_response.Pass());
+  ConnectToClient(
+      shell_impl, content_url, requestor_url, service_provider.Pass());
 }
 
 void ApplicationManager::SetLoaderForURL(scoped_ptr<ApplicationLoader> loader,
@@ -298,7 +301,21 @@ ApplicationLoader* ApplicationManager::GetLoaderForURL(const GURL& url) {
 
 void ApplicationManager::OnShellImplError(ShellImpl* shell_impl) {
   // Called from ~ShellImpl, so we do not need to call Destroy here.
+  auto content_shell_it = content_shell_impls_.find(shell_impl);
+  if (content_shell_it != content_shell_impls_.end()) {
+    delete (*content_shell_it);
+    content_shell_impls_.erase(content_shell_it);
+    return;
+  }
   const GURL url = shell_impl->url();
+  // If the shell is a connection to a content handler, remove the mapping/
+  URLToContentHandlerMap::iterator content_handler_it =

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

auto

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

Done.

+      url_to_content_handler_.find(url);

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

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.

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

What about instead cleaning up the content handlers with
 Done.

+  if (content_handler_it != url_to_content_handler_.end()) {
+    delete content_handler_it->second;
+    url_to_content_handler_.erase(content_handler_it);
+  }
+  // Remove the shell.
   URLToShellImplMap::iterator it = url_to_shell_impl_.find(url);
   DCHECK(it != url_to_shell_impl_.end());
   delete it->second;
@@ -322,4 +339,11 @@ ScopedMessagePipeHandle ApplicationManager::ConnectToServiceByName(
                                                         pipe.handle1.Pass());
   return pipe.handle0.Pass();
 }
+
+Array<String> ApplicationManager::GetArgsForURL(const GURL& url) {
+  URLToArgsMap::const_iterator args_it = url_to_args_.find(url);
+  if (args_it != url_to_args_.end())
+    return Array<String>::From(args_it->second);
+  return Array<String>();
+}
 }  // namespace mojo