ApplicationManager: Use callback to get notified on application shutdown.
Until now, the Delegate had a OnApplicationError that was called
whenever an application ended. Because it was used to keep track of when
a given started application ended, it needed to send back the URL that
was used to start an application. This was cumbersome as mapping and
rediect change the url during the loading process.
Instead this change allows a caller fo ApplicationManager to register a
closure that will be called when the application ends, if the call
started a new application.
R=davemoore@chromium.org
Review URL: https://codereview.chromium.org/983113002
diff --git a/shell/application_manager/application_manager.cc b/shell/application_manager/application_manager.cc
index c4ad8f4..5227096 100644
--- a/shell/application_manager/application_manager.cc
+++ b/shell/application_manager/application_manager.cc
@@ -44,10 +44,6 @@
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;
}
@@ -63,7 +59,8 @@
: 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());
@@ -121,10 +118,11 @@
const GURL& requested_url,
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
- ServiceProviderPtr exposed_services) {
- ConnectToApplicationWithParameters(requested_url, requestor_url,
- services.Pass(), exposed_services.Pass(),
- std::vector<std::string>());
+ ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end) {
+ ConnectToApplicationWithParameters(
+ requested_url, requestor_url, services.Pass(), exposed_services.Pass(),
+ on_application_end, std::vector<std::string>());
}
void ApplicationManager::ConnectToApplicationWithParameters(
@@ -132,6 +130,7 @@
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& pre_redirect_parameters) {
DCHECK(requested_url.is_valid());
@@ -154,28 +153,29 @@
std::vector<std::string> parameters =
Concatenate(pre_redirect_parameters, GetArgsForURL(resolved_url));
- if (ConnectToApplicationWithLoader(requested_url, mapped_url, requestor_url,
- &services, &exposed_services, parameters,
- GetLoaderForURL(mapped_url))) {
+ if (ConnectToApplicationWithLoader(mapped_url, requestor_url, &services,
+ &exposed_services, on_application_end,
+ parameters, GetLoaderForURL(mapped_url))) {
return;
}
- if (ConnectToApplicationWithLoader(requested_url, resolved_url, requestor_url,
- &services, &exposed_services, parameters,
- GetLoaderForURL(resolved_url))) {
+ if (ConnectToApplicationWithLoader(
+ resolved_url, requestor_url, &services, &exposed_services,
+ on_application_end, parameters, GetLoaderForURL(resolved_url))) {
return;
}
- if (ConnectToApplicationWithLoader(requested_url, resolved_url, requestor_url,
- &services, &exposed_services, parameters,
- default_loader_.get())) {
+ if (ConnectToApplicationWithLoader(resolved_url, requestor_url, &services,
+ &exposed_services, on_application_end,
+ parameters, 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()), parameters);
+ auto callback = base::Bind(
+ &ApplicationManager::HandleFetchCallback, weak_ptr_factory_.GetWeakPtr(),
+ requestor_url, base::Passed(services.Pass()),
+ base::Passed(exposed_services.Pass()), on_application_end,
+ parameters);
if (resolved_url.SchemeIsFile()) {
new LocalFetcher(resolved_url, GetBaseURLAndQuery(resolved_url, nullptr),
@@ -212,11 +212,11 @@
}
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,
const std::vector<std::string>& parameters,
ApplicationLoader* loader) {
if (!loader)
@@ -224,24 +224,24 @@
loader->Load(
resolved_url,
- RegisterShell(requested_url, resolved_url, requestor_url,
- services->Pass(), exposed_services->Pass(), parameters));
+ RegisterShell(resolved_url, requestor_url, services->Pass(),
+ exposed_services->Pass(), on_application_end, parameters));
return true;
}
InterfaceRequest<Application> ApplicationManager::RegisterShell(
- const GURL& original_url,
const GURL& resolved_url,
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& parameters) {
Identity app_identity(resolved_url);
ApplicationPtr application;
InterfaceRequest<Application> application_request = GetProxy(&application);
ShellImpl* shell =
- new ShellImpl(application.Pass(), this, original_url, app_identity);
+ new ShellImpl(application.Pass(), this, app_identity, on_application_end);
identity_to_shell_impl_[app_identity] = shell;
shell->InitializeApplication(Array<String>::From(parameters));
ConnectToClient(shell, resolved_url, requestor_url, services.Pass(),
@@ -267,10 +267,10 @@
}
void ApplicationManager::HandleFetchCallback(
- const GURL& requested_url,
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& parameters,
NativeRunner::CleanupBehavior cleanup_behavior,
scoped_ptr<Fetcher> fetcher) {
@@ -284,7 +284,7 @@
// And around we go again... Whee!
ConnectToApplicationWithParameters(redirect_url, requestor_url,
services.Pass(), exposed_services.Pass(),
- parameters);
+ on_application_end, parameters);
return;
}
@@ -300,8 +300,8 @@
}
InterfaceRequest<Application> request(
- RegisterShell(requested_url, fetcher->GetURL(), requestor_url,
- services.Pass(), exposed_services.Pass(), parameters));
+ RegisterShell(fetcher->GetURL(), requestor_url, services.Pass(),
+ exposed_services.Pass(), on_application_end, parameters));
// If the response begins with a #!mojo <content-handler-url>, use it.
GURL content_handler_url;
@@ -332,7 +332,7 @@
if (url_to_native_options_.find(base_resolved_url) !=
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_[base_resolved_url];
}
@@ -379,7 +379,7 @@
}
Identity identity(url);
ShellImpl* shell_impl =
- new ShellImpl(application.Pass(), this, url, identity);
+ new ShellImpl(application.Pass(), this, identity, base::Closure());
identity_to_shell_impl_[identity] = shell_impl;
shell_impl->InitializeApplication(Array<String>::From(args));
}
@@ -469,13 +469,14 @@
void ApplicationManager::OnShellImplError(ShellImpl* shell_impl) {
// Called from ~ShellImpl, so we do not need to call Destroy here.
const Identity identity = shell_impl->identity();
- const GURL requested_url = shell_impl->requested_url();
+ base::Closure on_application_end = shell_impl->on_application_end();
// Remove the shell.
auto it = identity_to_shell_impl_.find(identity);
DCHECK(it != identity_to_shell_impl_.end());
delete it->second;
identity_to_shell_impl_.erase(it);
- delegate_->OnApplicationError(requested_url);
+ if (!on_application_end.is_null())
+ on_application_end.Run();
}
void ApplicationManager::OnContentHandlerError(
@@ -492,7 +493,8 @@
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();
diff --git a/shell/application_manager/application_manager.h b/shell/application_manager/application_manager.h
index aa77bbb..3bcbc0c 100644
--- a/shell/application_manager/application_manager.h
+++ b/shell/application_manager/application_manager.h
@@ -36,9 +36,6 @@
class Delegate {
public:
virtual ~Delegate();
- // Send when the Application holding the handle on the other end of the
- // Shell pipe goes away.
- virtual void OnApplicationError(const GURL& url);
virtual GURL ResolveURL(const GURL& url);
virtual GURL ResolveMappings(const GURL& url);
};
@@ -67,7 +64,8 @@
void ConnectToApplication(const GURL& application_url,
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
- ServiceProviderPtr exposed_services);
+ ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end);
template <typename Interface>
inline void ConnectToService(const GURL& application_url,
@@ -145,6 +143,7 @@
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& pre_redirect_parameters);
bool ConnectToRunningApplication(const GURL& resolved_url,
@@ -153,24 +152,21 @@
ServiceProviderPtr* exposed_services);
bool 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,
const std::vector<std::string>& parameters,
ApplicationLoader* loader);
InterfaceRequest<Application> RegisterShell(
- // The original URL requested by client, before any resolution or
- // redirects.
- // This is mostly useless and should be removed.
- const GURL& original_url,
// The URL after resolution and redirects, including the querystring.
const GURL& resolved_url,
const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& parameters);
ShellImpl* GetShellImpl(const GURL& url);
@@ -181,10 +177,10 @@
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services);
- void HandleFetchCallback(const GURL& requested_url,
- const GURL& requestor_url,
+ void HandleFetchCallback(const GURL& requestor_url,
InterfaceRequest<ServiceProvider> services,
ServiceProviderPtr exposed_services,
+ const base::Closure& on_application_end,
const std::vector<std::string>& parameters,
NativeRunner::CleanupBehavior cleanup_behavior,
scoped_ptr<Fetcher> fetcher);
diff --git a/shell/application_manager/application_manager_unittest.cc b/shell/application_manager/application_manager_unittest.cc
index 19edcff..2e7c279 100644
--- a/shell/application_manager/application_manager_unittest.cc
+++ b/shell/application_manager/application_manager_unittest.cc
@@ -33,6 +33,11 @@
int num_loader_deletes;
};
+void QuitClosure(bool* value) {
+ *value = true;
+ base::MessageLoop::current()->QuitWhenIdle();
+}
+
class QuitMessageLoopErrorHandler : public ErrorHandler {
public:
QuitMessageLoopErrorHandler() {}
@@ -137,6 +142,13 @@
DISALLOW_COPY_AND_ASSIGN(TestApplicationLoader);
};
+class ClosingApplicationLoader : public ApplicationLoader {
+ private:
+ // ApplicationLoader implementation.
+ void Load(const GURL& url,
+ InterfaceRequest<Application> application_request) override {}
+};
+
class TesterContext {
public:
explicit TesterContext(base::MessageLoop* loop)
@@ -403,8 +415,6 @@
return mapped_url;
}
- void OnApplicationError(const GURL& url) override {}
-
private:
std::map<GURL, GURL> mappings_;
};
@@ -801,6 +811,19 @@
EXPECT_EQ(1, scheme_loader->num_loads());
}
+TEST_F(ApplicationManagerTest, TestEndApplicationClosure) {
+ ClosingApplicationLoader* loader = new ClosingApplicationLoader();
+ application_manager_->SetLoaderForScheme(
+ scoped_ptr<ApplicationLoader>(loader), "test");
+
+ bool called = false;
+ application_manager_->ConnectToApplication(
+ GURL("test:test"), GURL(), nullptr, nullptr,
+ base::Bind(&QuitClosure, base::Unretained(&called)));
+ loop_.Run();
+ EXPECT_TRUE(called);
+}
+
} // namespace
} // namespace shell
} // namespace mojo
diff --git a/shell/application_manager/shell_impl.cc b/shell/application_manager/shell_impl.cc
index d519cba..76c54da 100644
--- a/shell/application_manager/shell_impl.cc
+++ b/shell/application_manager/shell_impl.cc
@@ -13,11 +13,11 @@
ShellImpl::ShellImpl(ApplicationPtr application,
ApplicationManager* manager,
- const GURL& requested_url,
- const Identity& identity)
+ const Identity& identity,
+ const base::Closure& on_application_end)
: manager_(manager),
- requested_url_(requested_url),
identity_(identity),
+ on_application_end_(on_application_end),
application_(application.Pass()),
binding_(this) {
binding_.set_error_handler(this);
@@ -50,7 +50,7 @@
return;
}
manager_->ConnectToApplication(app_gurl, identity_.url, services.Pass(),
- exposed_services.Pass());
+ exposed_services.Pass(), base::Closure());
}
void ShellImpl::OnConnectionError() {
diff --git a/shell/application_manager/shell_impl.h b/shell/application_manager/shell_impl.h
index f3fcefe..4dbef20 100644
--- a/shell/application_manager/shell_impl.h
+++ b/shell/application_manager/shell_impl.h
@@ -5,6 +5,7 @@
#ifndef SHELL_APPLICATION_MANAGER_SHELL_IMPL_H_
#define SHELL_APPLICATION_MANAGER_SHELL_IMPL_H_
+#include "base/callback.h"
#include "mojo/public/cpp/bindings/binding.h"
#include "mojo/public/cpp/bindings/error_handler.h"
#include "mojo/public/interfaces/application/application.mojom.h"
@@ -21,9 +22,8 @@
public:
ShellImpl(ApplicationPtr application,
ApplicationManager* manager,
- // The original URL that was first requested, before any resolution.
- const GURL& original_url,
- const Identity& resolved_identity);
+ const Identity& resolved_identity,
+ const base::Closure& on_application_end);
~ShellImpl() override;
@@ -36,7 +36,7 @@
Application* application() { return application_.get(); }
const Identity& identity() const { return identity_; }
- const GURL& requested_url() const { return requested_url_; }
+ base::Closure on_application_end() const { return on_application_end_; }
private:
// Shell implementation:
@@ -48,8 +48,8 @@
void OnConnectionError() override;
ApplicationManager* const manager_;
- const GURL requested_url_;
const Identity identity_;
+ base::Closure on_application_end_;
ApplicationPtr application_;
Binding<Shell> binding_;
diff --git a/shell/context.cc b/shell/context.cc
index 91bfa12..271480d 100644
--- a/shell/context.cc
+++ b/shell/context.cc
@@ -289,7 +289,7 @@
new TracingServiceProvider(GetProxy(&tracing_service_provider_ptr));
application_manager_.ConnectToApplication(
GURL("mojo:tracing"), GURL(""), nullptr,
- tracing_service_provider_ptr.Pass());
+ tracing_service_provider_ptr.Pass(), base::Closure());
if (listener_)
listener_->WaitForListening();
@@ -305,17 +305,6 @@
base::MessageLoop::current()->Run();
}
-void Context::OnApplicationError(const GURL& url) {
- if (app_urls_.find(url) != app_urls_.end()) {
- app_urls_.erase(url);
- if (app_urls_.empty() && base::MessageLoop::current()->is_running()) {
- DCHECK_EQ(base::MessageLoop::current()->task_runner(),
- task_runners_->shell_runner());
- base::MessageLoop::current()->Quit();
- }
- }
-}
-
GURL Context::ResolveURL(const GURL& url) {
return url_resolver_.ResolveMojoURL(url);
}
@@ -335,16 +324,20 @@
ServiceProviderPtr exposed_services;
app_urls_.insert(url);
- application_manager_.ConnectToApplication(url, GURL(), GetProxy(&services),
- exposed_services.Pass());
+ application_manager_.ConnectToApplication(
+ url, GURL(), GetProxy(&services), exposed_services.Pass(),
+ base::Bind(&Context::OnApplicationEnd, base::Unretained(this), url));
}
-ScopedMessagePipeHandle Context::ConnectToServiceByName(
- const GURL& application_url,
- const std::string& service_name) {
- app_urls_.insert(application_url);
- return application_manager_.ConnectToServiceByName(application_url,
- service_name).Pass();
+void Context::OnApplicationEnd(const GURL& url) {
+ if (app_urls_.find(url) != app_urls_.end()) {
+ app_urls_.erase(url);
+ if (app_urls_.empty() && base::MessageLoop::current()->is_running()) {
+ DCHECK_EQ(base::MessageLoop::current()->task_runner(),
+ task_runners_->shell_runner());
+ base::MessageLoop::current()->Quit();
+ }
+ }
}
} // namespace shell
diff --git a/shell/context.h b/shell/context.h
index 645764d..beb1e78 100644
--- a/shell/context.h
+++ b/shell/context.h
@@ -55,9 +55,6 @@
void Shutdown();
void Run(const GURL& url);
- ScopedMessagePipeHandle ConnectToServiceByName(
- const GURL& application_url,
- const std::string& service_name);
TaskRunners* task_runners() { return task_runners_.get(); }
ApplicationManager* application_manager() { return &application_manager_; }
@@ -67,13 +64,14 @@
class NativeViewportApplicationLoader;
// ApplicationManager::Delegate overrides.
- void OnApplicationError(const GURL& url) override;
GURL ResolveURL(const GURL& url) override;
GURL ResolveMappings(const GURL& url) override;
// ProcessDelegate implementation.
void OnShutdownComplete() override;
+ void OnApplicationEnd(const GURL& url);
+
std::set<GURL> app_urls_;
scoped_ptr<TaskRunners> task_runners_;
scoped_ptr<ExternalApplicationListener> listener_;
diff --git a/shell/native_runner_unittest.cc b/shell/native_runner_unittest.cc
index 7faee04..5676405 100644
--- a/shell/native_runner_unittest.cc
+++ b/shell/native_runner_unittest.cc
@@ -78,8 +78,6 @@
private:
// ApplicationManager::Delegate
- void OnApplicationError(const GURL& url) override {}
-
GURL ResolveURL(const GURL& url) override { return url; }
GURL ResolveMappings(const GURL& url) override { return url; }
@@ -92,8 +90,8 @@
GURL url(FilePathToFileURL(temp_dir.path().Append(nonexistent_file)));
InterfaceRequest<ServiceProvider> services;
ServiceProviderPtr service_provider;
- application_manager_.ConnectToApplication(url, GURL(), services.Pass(),
- service_provider.Pass());
+ application_manager_.ConnectToApplication(
+ url, GURL(), services.Pass(), service_provider.Pass(), base::Closure());
EXPECT_FALSE(state_.runner_was_created);
EXPECT_FALSE(state_.runner_was_started);
EXPECT_FALSE(state_.runner_was_destroyed);
diff --git a/shell/shell_test_base.cc b/shell/shell_test_base.cc
index 20e6b35..6546d75 100644
--- a/shell/shell_test_base.cc
+++ b/shell/shell_test_base.cc
@@ -4,6 +4,7 @@
#include "shell/shell_test_base.h"
+#include "base/bind.h"
#include "base/command_line.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
@@ -17,6 +18,17 @@
namespace shell {
namespace test {
+namespace {
+
+void QuitIfRunning() {
+ if (base::MessageLoop::current() &&
+ base::MessageLoop::current()->is_running()) {
+ base::MessageLoop::current()->QuitWhenIdle();
+ }
+}
+
+} // namespace
+
ShellTestBase::ShellTestBase() {
}
@@ -35,8 +47,13 @@
ScopedMessagePipeHandle ShellTestBase::ConnectToService(
const GURL& application_url,
const std::string& service_name) {
- return shell_context_.ConnectToServiceByName(application_url, service_name)
- .Pass();
+ ServiceProviderPtr services;
+ shell_context_.application_manager()->ConnectToApplication(
+ application_url, GURL(), GetProxy(&services), nullptr,
+ base::Bind(&QuitIfRunning));
+ MessagePipe pipe;
+ services->ConnectToService(service_name, pipe.handle1.Pass());
+ return pipe.handle0.Pass();
}
#if !defined(OS_ANDROID)
