Add a new way of using ServiceProviderImpl without InterfaceFactory. Instead, just take a factory function. This has a number of advantages: * Classes don't have to implement InterfaceFactory<I> in order to "provide" services. * Classes can (easily) instantiate impls in different ways (or instantiate different impls for the same service). For now, the new method is called |AddServiceNew<I>()|, but I plan to get rid of |AddService<I>()| and rename |AddServiceNew| to |AddService|. Also, add a bunch of tests. R=vardhan@google.com Review URL: https://codereview.chromium.org/1973653002 .
diff --git a/mojo/public/cpp/application/application_connection.h b/mojo/public/cpp/application/application_connection.h index 3507974..8979d0c 100644 --- a/mojo/public/cpp/application/application_connection.h +++ b/mojo/public/cpp/application/application_connection.h
@@ -12,6 +12,7 @@ namespace mojo { +struct ConnectionContext; class ServiceConnector; // Represents a connection to another application. An instance of this class is @@ -34,6 +35,8 @@ // connection->AddService<Bar>(&my_foo_and_bar_factory_); // // The InterfaceFactory must outlive the ApplicationConnection. +// +// TODO(vtl): Don't get too attached to this class. I'm going to remove it. class ApplicationConnection { public: virtual ~ApplicationConnection(); @@ -47,6 +50,8 @@ Interface::Name_); } + virtual const ConnectionContext& GetConnectionContext() const = 0; + // Returns the URL that was used by the source application to establish a // connection to the destination application. //
diff --git a/mojo/public/cpp/application/lib/service_provider_impl.cc b/mojo/public/cpp/application/lib/service_provider_impl.cc index 5e5da65..e7978b6 100644 --- a/mojo/public/cpp/application/lib/service_provider_impl.cc +++ b/mojo/public/cpp/application/lib/service_provider_impl.cc
@@ -14,15 +14,15 @@ } ServiceProviderImpl::ServiceProviderImpl( - InterfaceRequest<ServiceProvider> request) - : binding_(this, request.Pass()), fallback_service_provider_(nullptr) { -} + InterfaceRequest<ServiceProvider> service_provider_request) + : binding_(this, service_provider_request.Pass()), + fallback_service_provider_(nullptr) {} -ServiceProviderImpl::~ServiceProviderImpl() { -} +ServiceProviderImpl::~ServiceProviderImpl() {} -void ServiceProviderImpl::Bind(InterfaceRequest<ServiceProvider> request) { - binding_.Bind(request.Pass()); +void ServiceProviderImpl::Bind( + InterfaceRequest<ServiceProvider> service_provider_request) { + binding_.Bind(service_provider_request.Pass()); } void ServiceProviderImpl::Close() {
diff --git a/mojo/public/cpp/application/lib/service_registry.cc b/mojo/public/cpp/application/lib/service_registry.cc index 6be0e10..3c34628 100644 --- a/mojo/public/cpp/application/lib/service_registry.cc +++ b/mojo/public/cpp/application/lib/service_registry.cc
@@ -34,6 +34,10 @@ service_connector_registry_.RemoveServiceConnectorForName(interface_name); } +const ConnectionContext& ServiceRegistry::GetConnectionContext() const { + return connection_context_; +} + const std::string& ServiceRegistry::GetConnectionURL() { return connection_context_.connection_url; }
diff --git a/mojo/public/cpp/application/lib/service_registry.h b/mojo/public/cpp/application/lib/service_registry.h index 50e5a43..1389d37 100644 --- a/mojo/public/cpp/application/lib/service_registry.h +++ b/mojo/public/cpp/application/lib/service_registry.h
@@ -29,6 +29,7 @@ // ApplicationConnection overrides. void SetServiceConnectorForName(ServiceConnector* service_connector, const std::string& interface_name) override; + const ConnectionContext& GetConnectionContext() const override; const std::string& GetConnectionURL() override; const std::string& GetRemoteApplicationURL() override;
diff --git a/mojo/public/cpp/application/service_provider_impl.h b/mojo/public/cpp/application/service_provider_impl.h index 1afa14b..b885924 100644 --- a/mojo/public/cpp/application/service_provider_impl.h +++ b/mojo/public/cpp/application/service_provider_impl.h
@@ -5,27 +5,57 @@ #ifndef MOJO_PUBLIC_APPLICATION_SERVICE_PROVIDER_IMPL_H_ #define MOJO_PUBLIC_APPLICATION_SERVICE_PROVIDER_IMPL_H_ +#include <functional> #include <string> +#include "mojo/public/cpp/application/application_connection.h" +#include "mojo/public/cpp/application/connection_context.h" #include "mojo/public/cpp/application/lib/interface_factory_connector.h" #include "mojo/public/cpp/application/lib/service_connector_registry.h" +#include "mojo/public/cpp/application/service_connector.h" #include "mojo/public/cpp/bindings/binding.h" #include "mojo/public/interfaces/application/service_provider.mojom.h" namespace mojo { -// Implements a registry that can be used to expose services to another app. +// An implementation of |ServiceProvider|, which can be customized appropriately +// (to select what services it provides). class ServiceProviderImpl : public ServiceProvider { public: + // A |InterfaceRequestHandler<Interface>| is simply a function that handles an + // interface request for |Interface|. If it determines (e.g., based on the + // given |ConnectionContext|) that the request should be "accepted", then it + // should "connect" ("take ownership of") request. Otherwise, it can simply + // drop |interface_request| (as implied by the interface). + template <typename Interface> + using InterfaceRequestHandler = + std::function<void(const ConnectionContext& connection_context, + InterfaceRequest<Interface> interface_request)>; + + // Constructs this service provider implementation in an unbound state. ServiceProviderImpl(); - explicit ServiceProviderImpl(InterfaceRequest<ServiceProvider> request); + + // Constructs this service provider implementation, binding it to the given + // interface request. + // TODO(vtl): This should take a |ConnectionContext|, to provide + // |InterfaceRequestHandler<I>|s. + explicit ServiceProviderImpl( + InterfaceRequest<ServiceProvider> service_provider_request); + ~ServiceProviderImpl() override; - void Bind(InterfaceRequest<ServiceProvider> request); - // Disconnect this service provider and put it in a state where it can be - // rebound to a new request. + // Binds this service provider implementation to the given interface request. + // This may only be called if this object is unbound. + // TODO(vtl): This should take a |ConnectionContext|, to provide + // |InterfaceRequestHandler<I>|s. + void Bind(InterfaceRequest<ServiceProvider> service_provider_request); + + // Disconnect this service provider implementation and put it in a state where + // it can be rebound to a new request (i.e., restores this object to an + // unbound state). This may be called even if this object is already unbound. void Close(); + // TODO(vtl): Remove this. template <typename Interface> void AddService(InterfaceFactory<Interface>* factory, const std::string& interface_name = Interface::Name_) { @@ -35,18 +65,70 @@ interface_name); } - // ServiceProviderImpl uses the fallback_service_provider_ whenever someone - // asks a service that doesn't exist in the service_connector_registry_. + // Adds a supported service with the given |name|, using the given + // |interface_request_handler| (see above for information about + // |InterfaceRequestHandler<Interface>|). |interface_request_handler| should + // remain valid for the lifetime of this object. // - // Note: ServiceProviderImpl does not take ownership of |fallback|. The caller - // must ensure that |fallback| outlives the ServiceProviderImpl. + // A typical usage may be: // - void set_fallback_service_provider(ServiceProvider* fallback) { - fallback_service_provider_ = fallback; + // service_provider_impl_->AddServiceNew<Foobar>( + // [](const ConnectionContext& connection_context, + // InterfaceRequest<FooBar> foobar_request) { + // // |FoobarImpl| owns itself. + // new FoobarImpl(std::move(foobar_request)); + // }); + // TODO(vtl): Remove the AddService() above and rename this to AddService(). + template <typename Interface> + void AddServiceNew( + InterfaceRequestHandler<Interface> interface_request_handler, + const std::string& name = Interface::Name_) { + service_connector_registry_.SetServiceConnectorForName( + std::unique_ptr<ServiceConnector>(new ServiceConnectorImpl<Interface>( + std::move(interface_request_handler))), + name); + } + + // This uses the provided |fallback_service_provider| for connection requests + // for services that are not known (haven't been added). (Set it to null to + // not have any fallback.) A fallback must outlive this object (or until it is + // "cleared" or replaced by a different fallback. + void set_fallback_service_provider( + ServiceProvider* fallback_service_provider) { + fallback_service_provider_ = fallback_service_provider; } private: - // Overridden from ServiceProvider: + // Objects of this class are used to adapt a generic (untyped) connection + // request (i.e., |ServiceConnector::ConnectToService()|) to the type-safe + // |InterfaceRequestHandler<Interface>|. + template <typename Interface> + class ServiceConnectorImpl : public ServiceConnector { + public: + explicit ServiceConnectorImpl( + InterfaceRequestHandler<Interface> interface_request_handler) + : interface_request_handler_(std::move(interface_request_handler)) {} + ~ServiceConnectorImpl() override {} + + void ConnectToService(ApplicationConnection* application_connection, + const std::string& interface_name, + ScopedMessagePipeHandle client_handle) override { + // TODO(vtl): This should be given a |const ConnectionContext&|, instead + // of an |ApplicationConnection*| -- which may be null! + interface_request_handler_( + application_connection + ? application_connection->GetConnectionContext() + : ConnectionContext(), + InterfaceRequest<Interface>(client_handle.Pass())); + } + + private: + const InterfaceRequestHandler<Interface> interface_request_handler_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(ServiceConnectorImpl); + }; + + // Overridden from |ServiceProvider|: void ConnectToService(const String& service_name, ScopedMessagePipeHandle client_handle) override;
diff --git a/mojo/public/cpp/application/tests/BUILD.gn b/mojo/public/cpp/application/tests/BUILD.gn index 7766a7d..284f54d 100644 --- a/mojo/public/cpp/application/tests/BUILD.gn +++ b/mojo/public/cpp/application/tests/BUILD.gn
@@ -8,6 +8,7 @@ testonly = true sources = [ + "service_provider_impl_unittest.cc", "service_registry_unittest.cc", ] @@ -18,6 +19,9 @@ mojo_sdk_deps = [ "mojo/public/cpp/application:standalone", "mojo/public/cpp/environment:standalone", + "mojo/public/cpp/system", "mojo/public/cpp/utility", + "mojo/public/interfaces/application", + "mojo/public/interfaces/bindings/tests:test_interfaces", ] }
diff --git a/mojo/public/cpp/application/tests/service_provider_impl_unittest.cc b/mojo/public/cpp/application/tests/service_provider_impl_unittest.cc new file mode 100644 index 0000000..e0a4f12 --- /dev/null +++ b/mojo/public/cpp/application/tests/service_provider_impl_unittest.cc
@@ -0,0 +1,257 @@ +// Copyright 2016 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/public/cpp/application/service_provider_impl.h" + +#include <utility> + +#include "mojo/public/cpp/application/connect.h" +#include "mojo/public/cpp/bindings/strong_binding.h" +#include "mojo/public/cpp/environment/environment.h" +#include "mojo/public/cpp/system/macros.h" +#include "mojo/public/cpp/utility/run_loop.h" +#include "mojo/public/interfaces/application/service_provider.mojom.h" +#include "mojo/public/interfaces/bindings/tests/ping_service.mojom.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace mojo { +namespace { + +class ServiceProviderImplTest : public testing::Test { + public: + ServiceProviderImplTest() {} + ~ServiceProviderImplTest() override { loop_.RunUntilIdle(); } + + RunLoop& loop() { return loop_; } + + protected: + void QuitLoop(bool ok) { + EXPECT_TRUE(ok); + loop_.Quit(); + } + + private: + Environment env_; + RunLoop loop_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(ServiceProviderImplTest); +}; + +class PingServiceImpl : public test::PingService { + public: + PingServiceImpl(InterfaceRequest<test::PingService> ping_service_request) + : strong_binding_(this, std::move(ping_service_request)) {} + ~PingServiceImpl() override {} + + // |test::PingService|: + void Ping(const PingCallback& callback) override { callback.Run(); } + + private: + StrongBinding<test::PingService> strong_binding_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(PingServiceImpl); +}; + +TEST_F(ServiceProviderImplTest, Basic) { + const char kPing1[] = "Ping1"; + const char kPing2[] = "Ping2"; + const char kPing3[] = "Ping3"; + + ServiceProviderPtr sp; + ServiceProviderImpl impl(GetProxy(&sp)); + + impl.AddServiceNew<test::PingService>( + [](const ConnectionContext& connection_context, + InterfaceRequest<test::PingService> ping_service_request) { + new PingServiceImpl(std::move(ping_service_request)); + }, + kPing1); + + impl.AddServiceNew<test::PingService>( + [](const ConnectionContext& connection_context, + InterfaceRequest<test::PingService> ping_service_request) { + new PingServiceImpl(std::move(ping_service_request)); + }, + kPing2); + + { + test::PingServicePtr ping1; + ConnectToService(sp.get(), GetProxy(&ping1), kPing1); + ping1.set_connection_error_handler([this] { QuitLoop(false); }); + ping1->Ping([this] { QuitLoop(true); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + { + test::PingServicePtr ping2; + ConnectToService(sp.get(), GetProxy(&ping2), kPing2); + ping2.set_connection_error_handler([this] { QuitLoop(false); }); + ping2->Ping([this] { QuitLoop(true); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + // "Ping3" isn't actually registered! + { + test::PingServicePtr ping3; + ConnectToService(sp.get(), GetProxy(&ping3), kPing3); + ping3.set_connection_error_handler([this] { QuitLoop(true); }); + ping3->Ping([this] { QuitLoop(false); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + sp.reset(); + loop().RunUntilIdle(); +} + +TEST_F(ServiceProviderImplTest, CloseAndRebind) { + const char kPing[] = "Ping"; + + ServiceProviderPtr sp1; + ServiceProviderImpl impl(GetProxy(&sp1)); + + impl.AddServiceNew<test::PingService>( + [](const ConnectionContext& connection_context, + InterfaceRequest<test::PingService> ping_service_request) { + new PingServiceImpl(std::move(ping_service_request)); + }, + kPing); + + { + test::PingServicePtr ping; + ConnectToService(sp1.get(), GetProxy(&ping), kPing); + ping.set_connection_error_handler([this] { QuitLoop(false); }); + ping->Ping([this] { QuitLoop(true); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + impl.Close(); + sp1.reset(); + loop().RunUntilIdle(); + + ServiceProviderPtr sp2; + impl.Bind(GetProxy(&sp2)); + + { + test::PingServicePtr ping; + ConnectToService(sp2.get(), GetProxy(&ping), kPing); + ping.set_connection_error_handler([this] { QuitLoop(false); }); + ping->Ping([this] { QuitLoop(true); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + // Can close multiple times. + impl.Close(); + impl.Close(); + sp2.reset(); + loop().RunUntilIdle(); +} + +TEST_F(ServiceProviderImplTest, Bind) { + const char kPing[] = "Ping"; + + ServiceProviderPtr sp; + ServiceProviderImpl impl; + + impl.Bind(GetProxy(&sp)); + + impl.AddServiceNew<test::PingService>( + [](const ConnectionContext& connection_context, + InterfaceRequest<test::PingService> request) { + new PingServiceImpl(std::move(request)); + }, + kPing); + + { + test::PingServicePtr ping; + ConnectToService(sp.get(), GetProxy(&ping), kPing); + ping.set_connection_error_handler([this] { QuitLoop(false); }); + ping->Ping([this] { QuitLoop(true); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + sp.reset(); + loop().RunUntilIdle(); +} + +class FauxServiceProvider : public ServiceProvider { + public: + explicit FauxServiceProvider( + std::function<void(const std::string& service_name)> + on_connect_to_service) + : on_connect_to_service_(on_connect_to_service) {} + ~FauxServiceProvider() override {} + + // |ServiceProvider|: + void ConnectToService(const String& service_name, + ScopedMessagePipeHandle client_handle) override { + on_connect_to_service_(service_name.get()); + } + + private: + std::function<void(const std::string& service_name)> on_connect_to_service_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(FauxServiceProvider); +}; + +TEST_F(ServiceProviderImplTest, FallbackServiceProvider) { + const char kWhatever[] = "Whatever"; + + ServiceProviderPtr sp; + ServiceProviderImpl impl(GetProxy(&sp)); + + { + test::PingServicePtr ping; + ConnectToService(sp.get(), GetProxy(&ping), kWhatever); + ping.set_connection_error_handler([this] { QuitLoop(true); }); + ping->Ping([this] { QuitLoop(false); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + bool was_run = false; + FauxServiceProvider fallback_sp( + [this, &kWhatever, &was_run](const std::string& service_name) { + EXPECT_EQ(kWhatever, service_name); + was_run = true; + }); + impl.set_fallback_service_provider(&fallback_sp); + + { + test::PingServicePtr ping; + ConnectToService(sp.get(), GetProxy(&ping), kWhatever); + ping.set_connection_error_handler([this] { QuitLoop(true); }); + EXPECT_FALSE(was_run); + ping->Ping([this] { QuitLoop(false); }); + loop().Run(); + EXPECT_TRUE(was_run); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + // Clear the fallback. + impl.set_fallback_service_provider(nullptr); + was_run = false; + + { + test::PingServicePtr ping; + ConnectToService(sp.get(), GetProxy(&ping), kWhatever); + ping.set_connection_error_handler([this] { QuitLoop(true); }); + ping->Ping([this] { QuitLoop(false); }); + loop().Run(); + } + loop().RunUntilIdle(); // Run stuff caused by destructors. + + sp.reset(); + loop().RunUntilIdle(); + + EXPECT_FALSE(was_run); +} + +} // namespace +} // namespace mojo
diff --git a/services/log/log_impl_unittest.cc b/services/log/log_impl_unittest.cc index 2511972..8500ce1 100644 --- a/services/log/log_impl_unittest.cc +++ b/services/log/log_impl_unittest.cc
@@ -11,6 +11,7 @@ #include "mojo/public/c/environment/logger.h" #include "mojo/public/cpp/application/application_impl.h" #include "mojo/public/cpp/application/application_test_base.h" +#include "mojo/public/cpp/application/connection_context.h" #include "mojo/public/cpp/system/time.h" #include "mojo/services/log/interfaces/entry.mojom.h" #include "mojo/services/log/interfaces/log.mojom.h" @@ -27,18 +28,28 @@ // We need to supply a ApplicationConnection to LogImpl::Create(). class TestApplicationConnection : public ApplicationConnection { public: + TestApplicationConnection() + : connection_context_(ConnectionContext::Type::INCOMING, + "mojo:log_impl_unittest", + "mojo:log") {} + + const ConnectionContext& GetConnectionContext() const override { + return connection_context_; + } + const std::string& GetConnectionURL() override { - static std::string kConnectionURL = "mojo:log"; - return kConnectionURL; + return connection_context_.connection_url; } const std::string& GetRemoteApplicationURL() override { - static std::string kRemoteApplicationURL = "mojo:log_impl_unittest"; - return kRemoteApplicationURL; + return connection_context_.remote_url; } void SetServiceConnectorForName(ServiceConnector* service_connector, const std::string& name) override {} + + private: + const ConnectionContext connection_context_; }; // Tests the Log service implementation by calling its AddEntry and verifying