mojo: Update content handler API

mojo/application_manager/application_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/application_manager/application_manager.h"
 
 #include <stdio.h>
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
@@ skipping 101 matching lines @@
   ApplicationManager* const manager_;
   const GURL url_;
 
   DISALLOW_COPY_AND_ASSIGN(ShellImpl);
 };
 
 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);
   }
 
   StubServiceProvider service_provider_impl;
   ContentHandlerPtr content_handler;
 };
 
 // static
@@ skipping 21 matching lines @@
 
 ApplicationManager::~ApplicationManager() {
   STLDeleteValues(&url_to_content_handler_);
   TerminateShellConnections();
   STLDeleteValues(&url_to_loader_);
   STLDeleteValues(&scheme_to_loader_);
 }
 
 void ApplicationManager::TerminateShellConnections() {
   STLDeleteValues(&url_to_shell_impl_);
+  STLDeleteElements(&content_shell_impls_);
 }
 
 // static
 ApplicationManager* ApplicationManager::GetInstance() {
   static base::LazyInstance<ApplicationManager> instance =
       LAZY_INSTANCE_INITIALIZER;
   has_created_instance = true;
   return &instance.Get();
 }
 
@@ skipping 43 matching lines @@
     ScopedMessagePipeHandle* shell_handle) {
   ShellImpl* shell_impl = NULL;
   URLToShellImplMap::iterator iter = url_to_shell_impl_.find(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;
-    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());
 }
 
 void ApplicationManager::LoadWithContentHandler(
     const GURL& content_url,
     const GURL& requestor_url,
     const GURL& content_handler_url,
     URLResponsePtr url_response,
     ServiceProviderPtr service_provider) {
   ContentHandlerConnection* connection = NULL;
   URLToContentHandlerMap::iterator iter =
       url_to_content_handler_.find(content_handler_url);
   if (iter != url_to_content_handler_.end()) {
     connection = iter->second;
   } else {
     connection = new ContentHandlerConnection(this, content_handler_url);
     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,
                                          const GURL& url) {
   URLToLoaderMap::iterator it = url_to_loader_.find(url);
   if (it != url_to_loader_.end())
     delete it->second;
   url_to_loader_[url] = loader.release();
 }
 
@@ skipping 21 matching lines @@
     return url_it->second;
   SchemeToLoaderMap::const_iterator scheme_it =
       scheme_to_loader_.find(url.scheme());
   if (scheme_it != scheme_to_loader_.end())
     return scheme_it->second;
   return default_loader_.get();
 }
 
 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;
   url_to_shell_impl_.erase(it);
   ApplicationLoader* loader = GetLoaderForURL(url);
   if (loader)
     loader->OnApplicationError(this, url);
   if (delegate_)
     delegate_->OnApplicationError(url);
 }
 
 ScopedMessagePipeHandle ApplicationManager::ConnectToServiceByName(
     const GURL& application_url,
     const std::string& interface_name) {
   StubServiceProvider* stub_sp = new StubServiceProvider;
   ServiceProviderPtr spp;
   BindToProxy(stub_sp, &spp);
   ConnectToApplication(application_url, GURL(), spp.Pass());
   MessagePipe pipe;
   stub_sp->GetRemoteServiceProvider()->ConnectToService(interface_name,
                                                         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