ApplicationManager: Use callback to get notified on application shutdown.

shell/application_manager/application_manager.cc

Index: shell/application_manager/application_manager.cc
diff --git a/shell/application_manager/application_manager.cc b/shell/application_manager/application_manager.cc
index 11f875945dee83c6c999cfa50efee9d24e62ed28..140d0baf2242182b9bedd3a70992acd58dd9b570 100644
--- a/shell/application_manager/application_manager.cc
+++ b/shell/application_manager/application_manager.cc
@@ -31,10 +31,6 @@ bool has_created_instance = false;
 ApplicationManager::Delegate::~Delegate() {
 }
 
-void ApplicationManager::Delegate::OnApplicationError(const GURL& url) {
-  LOG(ERROR) << "Communication error with application: " << url.spec();
-}
-
 GURL ApplicationManager::Delegate::ResolveURL(const GURL& url) {
   return url;
 }
@@ -50,7 +46,8 @@ class ApplicationManager::ContentHandlerConnection : public ErrorHandler {
       : manager_(manager), content_handler_url_(content_handler_url) {
     ServiceProviderPtr services;
     manager->ConnectToApplication(content_handler_url, GURL(),
-                                  GetProxy(&services), nullptr);
+                                  GetProxy(&services), nullptr,
+                                  base::Closure());
     MessagePipe pipe;
     content_handler_.Bind(pipe.handle0.Pass());
     services->ConnectToService(ContentHandler::Name_, pipe.handle1.Pass());
@@ -108,7 +105,8 @@ void ApplicationManager::ConnectToApplication(
     const GURL& requested_url,
     const GURL& requestor_url,
     InterfaceRequest<ServiceProvider> services,
-    ServiceProviderPtr exposed_services) {
+    ServiceProviderPtr exposed_services,
+    const base::Closure& on_application_end) {
   DCHECK(requested_url.is_valid());
 
   // We check both the mapped and resolved urls for existing shell_impls because
@@ -126,28 +124,28 @@ void ApplicationManager::ConnectToApplication(
     return;
   }
 
-  if (ConnectToApplicationWithLoader(requested_url, mapped_url, requestor_url,
-                                     &services, &exposed_services,
+  if (ConnectToApplicationWithLoader(mapped_url, requestor_url, &services,
+                                     &exposed_services, on_application_end,
                                      GetLoaderForURL(mapped_url))) {
     return;
   }
 
-  if (ConnectToApplicationWithLoader(requested_url, resolved_url, requestor_url,
-                                     &services, &exposed_services,
+  if (ConnectToApplicationWithLoader(resolved_url, requestor_url, &services,
+                                     &exposed_services, on_application_end,
                                      GetLoaderForURL(resolved_url))) {
     return;
   }
 
-  if (ConnectToApplicationWithLoader(requested_url, resolved_url, requestor_url,
-                                     &services, &exposed_services,
+  if (ConnectToApplicationWithLoader(resolved_url, requestor_url, &services,
+                                     &exposed_services, on_application_end,
                                      default_loader_.get())) {
     return;
   }
 
-  auto callback = base::Bind(&ApplicationManager::HandleFetchCallback,
-                             weak_ptr_factory_.GetWeakPtr(), requested_url,
-                             requestor_url, base::Passed(services.Pass()),
-                             base::Passed(exposed_services.Pass()));
+  auto callback = base::Bind(
+      &ApplicationManager::HandleFetchCallback, weak_ptr_factory_.GetWeakPtr(),
+      requestor_url, base::Passed(services.Pass()),
+      base::Passed(exposed_services.Pass()), on_application_end);
 
   if (resolved_url.SchemeIsFile()) {
     new LocalFetcher(resolved_url, GetBaseURLAndQuery(resolved_url, nullptr),
@@ -178,35 +176,35 @@ bool ApplicationManager::ConnectToRunningApplication(
 }
 
 bool ApplicationManager::ConnectToApplicationWithLoader(
-    const GURL& requested_url,
     const GURL& resolved_url,
     const GURL& requestor_url,
     InterfaceRequest<ServiceProvider>* services,
     ServiceProviderPtr* exposed_services,
+    const base::Closure& on_application_end,
     ApplicationLoader* loader) {
   if (!loader)
     return false;
 
   loader->Load(resolved_url,
-               RegisterShell(requested_url, resolved_url, requestor_url,
-                             services->Pass(), exposed_services->Pass()));
+               RegisterShell(resolved_url, requestor_url, services->Pass(),
+                             exposed_services->Pass(), on_application_end));
   return true;
 }
 
 InterfaceRequest<Application> ApplicationManager::RegisterShell(
-    const GURL& original_url,
     const GURL& resolved_url,
     const GURL& requestor_url,
     InterfaceRequest<ServiceProvider> services,
-    ServiceProviderPtr exposed_services) {
+    ServiceProviderPtr exposed_services,
+    const base::Closure& on_application_end) {
   GURL app_url = GetBaseURLAndQuery(resolved_url, nullptr);
 
   ApplicationPtr application;
   InterfaceRequest<Application> application_request = GetProxy(&application);
   ShellImpl* shell =
-      new ShellImpl(application.Pass(), this, original_url, app_url);
+      new ShellImpl(application.Pass(), this, app_url, on_application_end);
   url_to_shell_impl_[app_url] = shell;
-  shell->InitializeApplication(GetArgsForURL(original_url));
+  shell->InitializeApplication(GetArgsForURL(app_url));

[qsr, 2015/03/09 16:50:21]

I cannot push this until this is fixed -> this changed the behavior from using
the requested url to using the app url.

Using the requested url seems bad to me (because the app won't receive the same
arguments depending on which URL is used the first time it is connected to), but
using the real app url is not very good either because it will now be depending
on all the mapping, and redirect, some that cannot be known in advance.

I'm proposing the following, that I will implement in another CL if that's fine
with you:

If an URL A is mapped to B, and that both A and B have arguments, we should
concatenate all arguments and pass it to the app whether it is called with URL A
or B.

Now, we still have an issue with http redirect. Unfortunately I do not have a
very good solution, and I propose we continue to send the arguments that can be
found by using the given URL, it means that if an URL A is mapped to B, it will
not get the same arguments whether it is called the first time with A, or
directly with B.

 We kind of need this, because when we will map URL to the cloud to get the last
version of the app, we will still want to pass argument using the canonical name
for those Apps.

 WDYT?

   ConnectToClient(shell, resolved_url, requestor_url, services.Pass(),
                   exposed_services.Pass());
   return application_request.Pass();
@@ -230,10 +228,10 @@ void ApplicationManager::ConnectToClient(
 }
 
 void ApplicationManager::HandleFetchCallback(
-    const GURL& requested_url,
     const GURL& requestor_url,
     InterfaceRequest<ServiceProvider> services,
     ServiceProviderPtr exposed_services,
+    const base::Closure& on_application_end,
     NativeRunner::CleanupBehavior cleanup_behavior,
     scoped_ptr<Fetcher> fetcher) {
   if (!fetcher) {
@@ -245,7 +243,7 @@ void ApplicationManager::HandleFetchCallback(
   if (!redirect_url.is_empty()) {
     // And around we go again... Whee!
     ConnectToApplication(redirect_url, requestor_url, services.Pass(),
-                         exposed_services.Pass());
+                         exposed_services.Pass(), on_application_end);
     return;
   }
 
@@ -261,8 +259,8 @@ void ApplicationManager::HandleFetchCallback(
   }
 
   InterfaceRequest<Application> request(
-      RegisterShell(requested_url, fetcher->GetURL(), requestor_url,
-                    services.Pass(), exposed_services.Pass()));
+      RegisterShell(fetcher->GetURL(), requestor_url, services.Pass(),
+                    exposed_services.Pass(), on_application_end));
 
   // If the response begins with a #!mojo <content-handler-url>, use it.
   GURL content_handler_url;
@@ -292,7 +290,7 @@ void ApplicationManager::HandleFetchCallback(
   if (url_to_native_options_.find(fetcher->GetURL()) !=
       url_to_native_options_.end()) {
     DVLOG(2) << "Applying stored native options to resolved URL "
-             << fetcher->GetURL() << " (requested URL " << requested_url << ")";
+             << fetcher->GetURL();
     options = url_to_native_options_[fetcher->GetURL()];
   }
 
@@ -337,7 +335,8 @@ void ApplicationManager::RegisterExternalApplication(
     LOG(WARNING) << "--args-for provided for external application " << url
                  << " <ignored>";
   }
-  ShellImpl* shell_impl = new ShellImpl(application.Pass(), this, url, url);
+  ShellImpl* shell_impl =
+      new ShellImpl(application.Pass(), this, url, base::Closure());
   url_to_shell_impl_[url] = shell_impl;
   shell_impl->InitializeApplication(Array<String>::From(args));
 }
@@ -415,13 +414,14 @@ ApplicationLoader* ApplicationManager::GetLoaderForURL(const GURL& url) {
 void ApplicationManager::OnShellImplError(ShellImpl* shell_impl) {
   // Called from ~ShellImpl, so we do not need to call Destroy here.
   const GURL url = shell_impl->url();
-  const GURL requested_url = shell_impl->requested_url();
+  base::Closure on_application_end = shell_impl->on_application_end();
   // 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);
-  delegate_->OnApplicationError(requested_url);
+  if (!on_application_end.is_null())
+    on_application_end.Run();
 }
 
 void ApplicationManager::OnContentHandlerError(
@@ -438,7 +438,8 @@ ScopedMessagePipeHandle ApplicationManager::ConnectToServiceByName(
     const GURL& application_url,
     const std::string& interface_name) {
   ServiceProviderPtr services;
-  ConnectToApplication(application_url, GURL(), GetProxy(&services), nullptr);
+  ConnectToApplication(application_url, GURL(), GetProxy(&services), nullptr,
+                       base::Closure());
   MessagePipe pipe;
   services->ConnectToService(interface_name, pipe.handle1.Pass());
   return pipe.handle0.Pass();