Migrate video_capture away from ServiceContext

This changes the video_capture service to use ServiceBinding for its
Service interface binding, eliminating reliance on ServiceContext as
well as EmebddedServiceInfo etc for content integration.

Bug: 891780
Change-Id: Ic1b89aa5487e230b4fafef2fdddf4b0e8e307b3c
Reviewed-on: https://chromium-review.googlesource.com/c/1311797Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#604934}

chrome/browser/extensions/test_extension_system.cc

@@ -84,6 +84,7 @@ ExtensionService* TestExtensionSystem::CreateExtensionService(
   if (!connector_factory_) {
     connector_factory_ =
         std::make_unique<service_manager::TestConnectorFactory>();
+    connector_factory_->set_ignore_quit_requests(true);
     data_decoder_ = std::make_unique<data_decoder::DataDecoderService>(
         connector_factory_->RegisterInstance(
             data_decoder::mojom::kServiceName));

chrome/browser/extensions/zipfile_installer_unittest.cc

@@ -105,7 +105,9 @@ class ZipFileInstallerTest : public testing::Test {
         unzip_service_context_(unzip::UnzipService::CreateService(),
                                test_connector_factory_.RegisterInstance(
                                    unzip::mojom::kServiceName)),
-        connector_(test_connector_factory_.CreateConnector()) {}
+        connector_(test_connector_factory_.CreateConnector()) {
+    test_connector_factory_.set_ignore_quit_requests(true);
+  }
 
   void SetUp() override {
     extensions::LoadErrorReporter::Init(/*enable_noisy_errors=*/false);

content/browser/service_manager/service_manager_context.cc

@@ -371,6 +371,42 @@ bool AudioServiceOutOfProcess() {
          !GetContentClient()->browser()->OverridesAudioManager();
 }
 
+using InProcessServiceFactory =
+    base::RepeatingCallback<std::unique_ptr<service_manager::Service>(
+        service_manager::mojom::ServiceRequest request)>;
+
+void LaunchInProcessServiceOnSequence(
+    const InProcessServiceFactory& factory,
+    service_manager::mojom::ServiceRequest request) {
+  auto* raw_service_impl = factory.Run(std::move(request)).release();
+  raw_service_impl->set_termination_closure(base::BindOnce(
+      [](service_manager::Service* impl) { delete impl; }, raw_service_impl));
+}
+
+void LaunchInProcessService(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    const InProcessServiceFactory& factory,
+    service_manager::mojom::ServiceRequest request) {
+  task_runner->PostTask(
+      FROM_HERE, base::BindOnce(&LaunchInProcessServiceOnSequence, factory,
+                                std::move(request)));
+}
+
+void RegisterInProcessService(
+    ServiceManagerConnection* connection,
+    const std::string& service_name,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    const InProcessServiceFactory& factory) {
+  connection->AddServiceRequestHandler(
+      service_name,
+      base::BindRepeating(&LaunchInProcessService, task_runner, factory));
+}
+
+std::unique_ptr<service_manager::Service> CreateVideoCaptureService(
+    service_manager::mojom::ServiceRequest request) {
+  return std::make_unique<video_capture::ServiceImpl>(std::move(request));
+}
+
 }  // namespace
 
 // State which lives on the IO thread and drives the ServiceManager.
@@ -614,10 +650,8 @@ ServiceManagerContext::ServiceManagerContext(
   }
 
   if (features::IsVideoCaptureServiceEnabledForBrowserProcess()) {
-    service_manager::EmbeddedServiceInfo video_capture_info;
-    video_capture_info.factory =
-        base::BindRepeating(&video_capture::ServiceImpl::Create);
-    video_capture_info.task_runner =
+    RegisterInProcessService(
+        packaged_services_connection_.get(), video_capture::mojom::kServiceName,
 #if defined(OS_WIN)
         base::CreateCOMSTATaskRunnerWithTraits(
 #else
@@ -625,9 +659,8 @@ ServiceManagerContext::ServiceManagerContext(
 #endif
             base::TaskTraits({base::MayBlock(), base::WithBaseSyncPrimitives(),
                               base::TaskPriority::BEST_EFFORT}),
-            base::SingleThreadTaskRunnerThreadMode::DEDICATED);
-    packaged_services_connection_->AddEmbeddedService(
-        video_capture::mojom::kServiceName, video_capture_info);
+            base::SingleThreadTaskRunnerThreadMode::DEDICATED),
+        base::BindRepeating(&CreateVideoCaptureService));
   }
 
   {

content/utility/utility_service_factory.cc

@@ -133,12 +133,6 @@ void UtilityServiceFactory::CreateService(
 void UtilityServiceFactory::RegisterServices(ServiceMap* services) {
   GetContentClient()->utility()->RegisterServices(services);
 
-  service_manager::EmbeddedServiceInfo video_capture_info;
-  video_capture_info.factory =
-      base::BindRepeating(&video_capture::ServiceImpl::Create);
-  services->insert(
-      std::make_pair(video_capture::mojom::kServiceName, video_capture_info));
-
   GetContentClient()->utility()->RegisterAudioBinders(audio_registry_.get());
   service_manager::EmbeddedServiceInfo audio_info;
   audio_info.factory = base::BindRepeating(
@@ -186,6 +180,9 @@ bool UtilityServiceFactory::HandleServiceRequest(
     content::UtilityThread::Get()->EnsureBlinkInitialized();
     running_service_ =
         std::make_unique<data_decoder::DataDecoderService>(std::move(request));
+  } else if (name == video_capture::mojom::kServiceName) {
+    running_service_ =
+        std::make_unique<video_capture::ServiceImpl>(std::move(request));
   }
 
   if (running_service_) {

extensions/browser/sandboxed_unpacker_unittest.cc

@@ -141,7 +141,9 @@ class MockSandboxedUnpackerClient : public SandboxedUnpackerClient {
 class SandboxedUnpackerTest : public ExtensionsTest {
  public:
   SandboxedUnpackerTest()
-      : SandboxedUnpackerTest(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
+      : SandboxedUnpackerTest(content::TestBrowserThreadBundle::IO_MAINLOOP) {
+    test_connector_factory_.set_ignore_quit_requests(true);
+  }
 
   SandboxedUnpackerTest(content::TestBrowserThreadBundle::Options options)
       : ExtensionsTest(options) {}

services/service_manager/public/cpp/service_keepalive.cc

@@ -22,11 +22,9 @@ ServiceKeepalive::ServiceKeepalive(ServiceBinding* binding,
 }
 
 ServiceKeepalive::ServiceKeepalive(ServiceContext* context,
-                                   base::Optional<base::TimeDelta> idle_timeout,
-                                   TimeoutObserver* timeout_observer)
+                                   base::Optional<base::TimeDelta> idle_timeout)
     : context_(context),
       idle_timeout_(idle_timeout),
-      timeout_observer_(timeout_observer),
       ref_factory_(base::BindRepeating(&ServiceKeepalive::OnRefCountZero,
                                        base::Unretained(this))) {
   ref_factory_.SetRefAddedCallback(base::BindRepeating(
@@ -43,10 +41,20 @@ bool ServiceKeepalive::HasNoRefs() {
   return ref_factory_.HasNoRefs();
 }
 
+void ServiceKeepalive::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ServiceKeepalive::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 void ServiceKeepalive::OnRefAdded() {
-  if (idle_timer_ && timeout_observer_)
-    timeout_observer_->OnTimeoutCancelled();
-  idle_timer_.reset();
+  if (idle_timer_) {
+    idle_timer_.reset();
+    for (auto& observer : observers_)
+      observer.OnIdleTimeoutCancelled();
+  }
 }
 
 void ServiceKeepalive::OnRefCountZero() {
@@ -59,8 +67,9 @@ void ServiceKeepalive::OnRefCountZero() {
 }
 
 void ServiceKeepalive::OnTimerExpired() {
-  if (timeout_observer_)
-    timeout_observer_->OnTimeoutExpired();
+  for (auto& observer : observers_)
+    observer.OnIdleTimeout();
+
   if (context_)
     context_->CreateQuitClosure().Run();
   else

services/service_manager/public/cpp/service_keepalive.h

@@ -9,6 +9,8 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "base/optional.h"
 #include "base/timer/timer.h"
 #include "services/service_manager/public/cpp/service_context_ref.h"
@@ -33,11 +35,18 @@ class ServiceContext;
 // service references to different endpoints in your service.
 class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
  public:
-  class TimeoutObserver {
+  class Observer : public base::CheckedObserver {
    public:
-    virtual ~TimeoutObserver() {}
-    virtual void OnTimeoutExpired() = 0;
-    virtual void OnTimeoutCancelled() = 0;
+    ~Observer() override {}
+
+    // Invoked whenever the ServiceKeepalive detects that the service has been
+    // idle for at least the idle time delta specified (if any) upon
+    // construction of the ServiceKeepalive.
+    virtual void OnIdleTimeout() = 0;
+
+    // Invoked whenever the ServiceKeepalive detects new activity again after
+    // having been idle for any amount of time.
+    virtual void OnIdleTimeoutCancelled() = 0;
   };
 
   ServiceKeepalive(ServiceBinding* binding,
@@ -52,13 +61,15 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
   // DEPRECATED: Please consider switching from ServiceContext to ServiceBinding
   // and using the constructor above.
   ServiceKeepalive(ServiceContext* context,
-                   base::Optional<base::TimeDelta> idle_timeout,
-                   TimeoutObserver* timeout_observer = nullptr);
+                   base::Optional<base::TimeDelta> idle_timeout);
   ~ServiceKeepalive();
 
   std::unique_ptr<ServiceContextRef> CreateRef();
   bool HasNoRefs();
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  private:
   void OnRefAdded();
   void OnRefCountZero();
@@ -68,7 +79,7 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
   ServiceContext* const context_ = nullptr;
   const base::Optional<base::TimeDelta> idle_timeout_;
   base::Optional<base::OneShotTimer> idle_timer_;
-  TimeoutObserver* const timeout_observer_ = nullptr;
+  base::ObserverList<Observer> observers_;
   ServiceContextRefFactory ref_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceKeepalive);

services/service_manager/public/cpp/test/test_connector_factory.cc

@@ -189,23 +189,41 @@ class MultipleServiceConnector : public TestConnectorImplBase {
 
 class ProxiedServiceConnector : public mojom::Connector {
  public:
-  ProxiedServiceConnector(TestConnectorFactory::NameToServiceProxyMap* proxies,
-                          const std::string& test_user_id)
-      : proxies_(proxies), test_user_id_(test_user_id) {}
+  ProxiedServiceConnector(
+      TestConnectorFactory::NameToServiceProxyMap* proxies,
+      TestConnectorFactory::NameToServiceHandlerMap* handlers,
+      const std::string& test_user_id)
+      : proxies_(proxies), handlers_(handlers), test_user_id_(test_user_id) {}
 
   ~ProxiedServiceConnector() override = default;
 
  private:
+  mojom::Service* GetServiceProxy(const std::string& service_name) {
+    auto proxy_it = proxies_->find(service_name);
+    if (proxy_it == proxies_->end()) {
+      auto handler_it = handlers_->find(service_name);
+      if (handler_it == handlers_->end())
+        return nullptr;
+
+      mojom::ServicePtr proxy;
+      handler_it->second.Run(mojo::MakeRequest(&proxy));
+      auto result = proxies_->emplace(service_name, std::move(proxy));
+      DCHECK(result.second);
+      proxy_it = result.first;
+    }
+
+    return proxy_it->second.get();
+  }
+
   // mojom::Connector:
   void BindInterface(const Identity& target,
                      const std::string& interface_name,
                      mojo::ScopedMessagePipeHandle interface_pipe,
                      BindInterfaceCallback callback) override {
-    auto it = proxies_->find(target.name());
-    CHECK(it != proxies_->end())
+    auto* proxy = GetServiceProxy(target.name());
+    CHECK(proxy)
         << "TestConnectorFactory received a BindInterface request for an "
         << "unregistered service '" << target.name() << "'";
-    mojom::ServicePtr& proxy = it->second;
     proxy->OnBindInterface(
         BindSourceInfo(Identity("TestConnectorFactory", test_user_id_),
                        CapabilitySet()),
@@ -243,23 +261,19 @@ class ProxiedServiceConnector : public mojom::Connector {
   }
 
   TestConnectorFactory::NameToServiceProxyMap* const proxies_;
+  TestConnectorFactory::NameToServiceHandlerMap* const handlers_;
   const std::string test_user_id_;
   mojo::BindingSet<mojom::Connector> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ProxiedServiceConnector);
 };
 
-void OnStartResponse(mojom::Connector* connector,
-                     mojom::ConnectorRequest connector_request,
-                     mojom::ServiceControlAssociatedRequest control_request) {
-  connector->Clone(std::move(connector_request));
-}
-
 }  // namespace
 
 TestConnectorFactory::TestConnectorFactory() {
   std::string guid = base::GenerateGUID();
-  impl_ = std::make_unique<ProxiedServiceConnector>(&service_proxies_, guid);
+  impl_ = std::make_unique<ProxiedServiceConnector>(&service_proxies_,
+                                                    &service_handlers_, guid);
   test_user_id_ = guid;
 }
 
@@ -306,9 +320,34 @@ mojom::ServiceRequest TestConnectorFactory::RegisterInstance(
   mojom::ServicePtr proxy;
   mojom::ServiceRequest request = mojo::MakeRequest(&proxy);
   proxy->OnStart(Identity(service_name, test_user_id_),
-                 base::BindOnce(&OnStartResponse, impl_.get()));
+                 base::BindOnce(&TestConnectorFactory::OnStartResponseHandler,
+                                base::Unretained(this), service_name));
   service_proxies_[service_name] = std::move(proxy);
   return request;
 }
 
+void TestConnectorFactory::RegisterServiceHandler(
+    const std::string& service_name,
+    const ServiceHandler& handler) {
+  service_handlers_[service_name] = handler;
+}
+
+void TestConnectorFactory::OnStartResponseHandler(
+    const std::string& service_name,
+    mojom::ConnectorRequest connector_request,
+    mojom::ServiceControlAssociatedRequest control_request) {
+  impl_->Clone(std::move(connector_request));
+  service_control_bindings_.AddBinding(this, std::move(control_request),
+                                       service_name);
+}
+
+void TestConnectorFactory::RequestQuit() {
+  if (ignore_quit_requests_)
+    return;
+
+  service_proxies_.erase(service_control_bindings_.dispatch_context());
+  service_control_bindings_.RemoveBinding(
+      service_control_bindings_.dispatch_binding());
+}
+
 }  // namespace service_manager

services/service_manager/public/cpp/test/test_connector_factory.h

@@ -10,9 +10,11 @@
 #include <string>
 
 #include "base/macros.h"
+#include "mojo/public/cpp/bindings/associated_binding_set.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/service_manager/public/mojom/connector.mojom.h"
+#include "services/service_manager/public/mojom/service_control.mojom.h"
 
 namespace service_manager {
 
@@ -50,17 +52,23 @@ class Service;
 //   std::unique_ptr<service_manager::Connector> connector =
 //       connector_factory->CreateConnector();
 //   ...
-class TestConnectorFactory {
+class TestConnectorFactory : public mojom::ServiceControl {
  public:
   // Creates a simple TestConnectorFactory which can be used register unowned
   // Service instances and vend Connectors which can connect to them.
   TestConnectorFactory();
-  ~TestConnectorFactory();
+  ~TestConnectorFactory() override;
 
   // A mapping from service names to Service proxies for unowned Service
   // instances.
   using NameToServiceProxyMap = std::map<std::string, mojom::ServicePtr>;
 
+  // Used to hold a mapping from service names to callbacks which can
+  // instantiate an instance of those services.
+  using ServiceHandler =
+      base::RepeatingCallback<void(service_manager::mojom::ServiceRequest)>;
+  using NameToServiceHandlerMap = std::map<std::string, ServiceHandler>;
+
   // Used to hold a mapping from service names to owned Service instances.
   using NameToServiceMap = std::map<std::string, std::unique_ptr<Service>>;
 
@@ -90,12 +98,38 @@ class TestConnectorFactory {
   // simulated events from this object.
   mojom::ServiceRequest RegisterInstance(const std::string& service_name);
 
+  // Registers a callback to start an instance of a specific named service
+  // whenever it's needed by this TestConnectorFactory. This may be used instead
+  // of a one-time registration via |RegisterInstance()| in cases where a
+  // service-under-test may be stopped and restarted multiple times during a
+  // single test.
+  void RegisterServiceHandler(const std::string& service_name,
+                              const ServiceHandler& handler);
+
   const std::string& test_user_id() const { return test_user_id_; }
 
+  // Normally when a service instance registered via either |RegisterInstance()|
+  // or |RegisterServiceHandler()| requests termination from the Service
+  // Manager, TestConnectorFactory immediately severs the service instance's
+  // connection, typically triggering the service's shutdown path.
+  //
+  // If this is set to |true| (defaults to |false|), quit requests are ignored
+  // and each service instance will remain connected to the TestConnectorFactory
+  // until either it or the TestConnoctorFactory is destroyed.
+  void set_ignore_quit_requests(bool ignore) { ignore_quit_requests_ = ignore; }
+
  private:
   explicit TestConnectorFactory(std::unique_ptr<mojom::Connector> impl,
                                 std::string test_user_id);
 
+  void OnStartResponseHandler(
+      const std::string& service_name,
+      mojom::ConnectorRequest connector_request,
+      mojom::ServiceControlAssociatedRequest control_request);
+
+  // mojom::ServiceControl:
+  void RequestQuit() override;
+
   NameToServiceMap names_to_services_;
 
   std::unique_ptr<mojom::Connector> impl_;
@@ -106,6 +140,17 @@ class TestConnectorFactory {
   // proxies.
   NameToServiceProxyMap service_proxies_;
 
+  // Maps service names to handlers which can construct service instances.
+  NameToServiceHandlerMap service_handlers_;
+
+  // ServiceControl bindings which receive and process RequestQuit requests from
+  // connected service instances. The associated service name is used as
+  // context.
+  mojo::AssociatedBindingSet<mojom::ServiceControl, std::string>
+      service_control_bindings_;
+
+  bool ignore_quit_requests_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(TestConnectorFactory);
 };
 

services/video_capture/service_impl.cc

@@ -14,30 +14,32 @@
 
 namespace video_capture {
 
-ServiceImpl::ServiceImpl(base::Optional<base::TimeDelta> shutdown_delay)
-    : shutdown_delay_(shutdown_delay), weak_factory_(this) {}
+namespace {
 
-ServiceImpl::~ServiceImpl() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (destruction_cb_)
-    std::move(destruction_cb_).Run();
-}
-
-// static
-std::unique_ptr<service_manager::Service> ServiceImpl::Create() {
 #if defined(OS_ANDROID)
-  // On Android, we do not use automatic service shutdown, because when shutting
-  // down the service, we lose caching of the supported formats, and re-querying
-  // these can take several seconds on certain Android devices.
-  return std::make_unique<ServiceImpl>(base::Optional<base::TimeDelta>());
+// On Android, we do not use automatic service shutdown, because when shutting
+// down the service, we lose caching of the supported formats, and re-querying
+// these can take several seconds on certain Android devices.
+constexpr base::Optional<base::TimeDelta> kDefaultIdleTimeout;
 #else
-  return std::make_unique<ServiceImpl>(base::TimeDelta::FromSeconds(5));
+constexpr base::Optional<base::TimeDelta> kDefaultIdleTimeout =
+    base::TimeDelta::FromSeconds(5);
 #endif
+
+}  // namespace
+
+ServiceImpl::ServiceImpl(service_manager::mojom::ServiceRequest request)
+    : ServiceImpl(std::move(request), kDefaultIdleTimeout) {}
+
+ServiceImpl::ServiceImpl(service_manager::mojom::ServiceRequest request,
+                         base::Optional<base::TimeDelta> idle_timeout)
+    : binding_(this, std::move(request)), keepalive_(&binding_, idle_timeout) {
+  keepalive_.AddObserver(this);
 }
 
-void ServiceImpl::SetDestructionObserver(base::OnceClosure observer_cb) {
+ServiceImpl::~ServiceImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  destruction_cb_ = std::move(observer_cb);
+  keepalive_.RemoveObserver(this);
 }
 
 void ServiceImpl::SetFactoryProviderClientConnectedObserver(
@@ -60,7 +62,7 @@ void ServiceImpl::SetShutdownTimeoutCancelledObserver(
 
 bool ServiceImpl::HasNoContextRefs() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return ref_factory_->HasNoRefs();
+  return keepalive_.HasNoRefs();
 }
 
 void ServiceImpl::OnStart() {
@@ -69,21 +71,14 @@ void ServiceImpl::OnStart() {
   video_capture::uma::LogVideoCaptureServiceEvent(
       video_capture::uma::SERVICE_STARTED);
 
-  // Do not replace |ref_factory_| if one has already been set via
-  // SetServiceContextRefProviderForTesting().
-  if (!ref_factory_) {
-    ref_factory_ = std::make_unique<service_manager::ServiceKeepalive>(
-        context(), shutdown_delay_, this);
-  }
-
   registry_.AddInterface<mojom::DeviceFactoryProvider>(
       // Unretained |this| is safe because |registry_| is owned by |this|.
-      base::Bind(&ServiceImpl::OnDeviceFactoryProviderRequest,
-                 base::Unretained(this)));
+      base::BindRepeating(&ServiceImpl::OnDeviceFactoryProviderRequest,
+                          base::Unretained(this)));
   registry_.AddInterface<mojom::TestingControls>(
       // Unretained |this| is safe because |registry_| is owned by |this|.
-      base::Bind(&ServiceImpl::OnTestingControlsRequest,
-                 base::Unretained(this)));
+      base::BindRepeating(&ServiceImpl::OnTestingControlsRequest,
+                          base::Unretained(this)));
 
   // Unretained |this| is safe because |factory_provider_bindings_| is owned by
   // |this|.
@@ -104,12 +99,12 @@ bool ServiceImpl::OnServiceManagerConnectionLost() {
   return true;
 }
 
-void ServiceImpl::OnTimeoutExpired() {
+void ServiceImpl::OnIdleTimeout() {
   video_capture::uma::LogVideoCaptureServiceEvent(
       video_capture::uma::SERVICE_SHUTTING_DOWN_BECAUSE_NO_CLIENT);
 }
 
-void ServiceImpl::OnTimeoutCancelled() {
+void ServiceImpl::OnIdleTimeoutCancelled() {
   video_capture::uma::LogVideoCaptureServiceEvent(
       video_capture::uma::SERVICE_SHUTDOWN_TIMEOUT_CANCELED);
   if (shutdown_timeout_cancelled_cb_)
@@ -121,7 +116,7 @@ void ServiceImpl::OnDeviceFactoryProviderRequest(
   DCHECK(thread_checker_.CalledOnValidThread());
   LazyInitializeDeviceFactoryProvider();
   if (factory_provider_bindings_.empty())
-    device_factory_provider_->SetServiceRef(ref_factory_->CreateRef());
+    device_factory_provider_->SetServiceRef(keepalive_.CreateRef());
   factory_provider_bindings_.AddBinding(device_factory_provider_.get(),
                                         std::move(request));
 
@@ -134,7 +129,7 @@ void ServiceImpl::OnTestingControlsRequest(
     mojom::TestingControlsRequest request) {
   DCHECK(thread_checker_.CalledOnValidThread());
   mojo::MakeStrongBinding(
-      std::make_unique<TestingControlsImpl>(ref_factory_->CreateRef()),
+      std::make_unique<TestingControlsImpl>(keepalive_.CreateRef()),
       std::move(request));
 }
 

services/video_capture/service_impl.h

@@ -7,10 +7,13 @@
 
 #include <memory>
 
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/time.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "services/service_manager/public/cpp/service.h"
+#include "services/service_manager/public/cpp/service_binding.h"
 #include "services/service_manager/public/cpp/service_keepalive.h"
 #include "services/video_capture/device_factory_provider_impl.h"
 #include "services/video_capture/public/mojom/device_factory_provider.mojom.h"
@@ -23,16 +26,17 @@
 namespace video_capture {
 
 class ServiceImpl : public service_manager::Service,
-                    public service_manager::ServiceKeepalive::TimeoutObserver {
+                    public service_manager::ServiceKeepalive::Observer {
  public:
-  // If |shutdown_delay| is provided, the service will shut itself down as soon
-  // as no client was connect for the corresponding duration.
-  explicit ServiceImpl(base::Optional<base::TimeDelta> shutdown_delay);
-  ~ServiceImpl() override;
+  explicit ServiceImpl(service_manager::mojom::ServiceRequest request);
+
+  // Constructs a service instance which overrides the default idle timeout
+  // behavior.
+  ServiceImpl(service_manager::mojom::ServiceRequest request,
+              base::Optional<base::TimeDelta> idle_timeout);
 
-  static std::unique_ptr<service_manager::Service> Create();
+  ~ServiceImpl() override;
 
-  void SetDestructionObserver(base::OnceClosure observer_cb);
   void SetFactoryProviderClientConnectedObserver(
       base::RepeatingClosure observer_cb);
   void SetFactoryProviderClientDisconnectedObserver(
@@ -47,9 +51,9 @@ class ServiceImpl : public service_manager::Service,
                        mojo::ScopedMessagePipeHandle interface_pipe) override;
   bool OnServiceManagerConnectionLost() override;
 
-  // service_manager::ServiceKeepalive::TimeoutObserver implementation.
-  void OnTimeoutExpired() override;
-  void OnTimeoutCancelled() override;
+  // service_manager::ServiceKeepalive::Observer implementation.
+  void OnIdleTimeout() override;
+  void OnIdleTimeoutCancelled() override;
 
  private:
   void OnDeviceFactoryProviderRequest(
@@ -60,7 +64,9 @@ class ServiceImpl : public service_manager::Service,
   void LazyInitializeDeviceFactoryProvider();
   void OnProviderClientDisconnected();
 
-  const base::Optional<base::TimeDelta> shutdown_delay_;
+  service_manager::ServiceBinding binding_;
+  service_manager::ServiceKeepalive keepalive_;
+
 #if defined(OS_WIN)
   // COM must be initialized in order to access the video capture devices.
   base::win::ScopedCOMInitializer com_initializer_;
@@ -68,16 +74,13 @@ class ServiceImpl : public service_manager::Service,
   service_manager::BinderRegistry registry_;
   mojo::BindingSet<mojom::DeviceFactoryProvider> factory_provider_bindings_;
   std::unique_ptr<DeviceFactoryProviderImpl> device_factory_provider_;
-  std::unique_ptr<service_manager::ServiceKeepalive> ref_factory_;
 
   // Callbacks that can optionally be set by clients.
-  base::OnceClosure destruction_cb_;
   base::RepeatingClosure factory_provider_client_connected_cb_;
   base::RepeatingClosure factory_provider_client_disconnected_cb_;
   base::RepeatingClosure shutdown_timeout_cancelled_cb_;
 
   base::ThreadChecker thread_checker_;
-  base::WeakPtrFactory<ServiceImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceImpl);
 };

services/video_capture/service_main.cc

@@ -2,12 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "mojo/public/cpp/system/message_pipe.h"
 #include "services/service_manager/public/c/main.h"
-#include "services/service_manager/public/cpp/service_runner.h"
+#include "services/service_manager/public/mojom/service.mojom.h"
 #include "services/video_capture/service_impl.h"
 
 MojoResult ServiceMain(MojoHandle service_request_handle) {
-  return service_manager::ServiceRunner(
-             video_capture::ServiceImpl::Create().release())
-      .Run(service_request_handle);
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  video_capture::ServiceImpl service{service_manager::mojom::ServiceRequest(
+      mojo::MakeScopedHandle(mojo::MessagePipeHandle(service_request_handle)))};
+  service.set_termination_closure(run_loop.QuitClosure());
+  run_loop.Run();
+  return MOJO_RESULT_OK;
 }

services/video_capture/test/device_factory_provider_connectortest.cc

@@ -33,25 +33,20 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kUseFakeDeviceForMediaStream);
-    std::unique_ptr<ServiceImpl> service_impl = std::make_unique<ServiceImpl>(
+    service_impl_ = std::make_unique<ServiceImpl>(
+        connector_factory_.RegisterInstance(video_capture::mojom::kServiceName),
         DeviceFactoryProviderConnectorTestTraits::shutdown_delay());
-    service_impl->SetDestructionObserver(base::BindOnce(
-        [](base::RunLoop* service_destroyed_wait_loop) {
-          service_destroyed_wait_loop->Quit();
-        },
-        &service_destroyed_wait_loop_));
-    service_impl_ = service_impl.get();
-    connector_factory_ =
-        service_manager::TestConnectorFactory::CreateForUniqueService(
-            std::move(service_impl), true /*release_service_on_quit_request*/);
-    connector_ = connector_factory_->CreateConnector();
-    {
-      base::RunLoop wait_loop;
-      service_impl_->SetFactoryProviderClientConnectedObserver(
-          wait_loop.QuitClosure());
-      connector_->BindInterface(mojom::kServiceName, &factory_provider_);
-      wait_loop.Run();
-    }
+    service_impl_->set_termination_closure(
+        base::BindOnce(&DeviceFactoryProviderConnectorTest::OnServiceQuit,
+                       base::Unretained(this)));
+
+    connector_ = connector_factory_.CreateConnector();
+
+    base::RunLoop wait_loop;
+    service_impl_->SetFactoryProviderClientConnectedObserver(
+        wait_loop.QuitClosure());
+    connector_->BindInterface(mojom::kServiceName, &factory_provider_);
+    wait_loop.Run();
   }
 
   void TearDown() override {
@@ -65,15 +60,25 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-  ServiceImpl* service_impl_;
+  std::unique_ptr<ServiceImpl> service_impl_;
   mojom::DeviceFactoryProviderPtr factory_provider_;
   base::MockCallback<mojom::DeviceFactory::GetDeviceInfosCallback>
       device_info_receiver_;
   std::unique_ptr<service_manager::Connector> connector_;
   base::RunLoop service_destroyed_wait_loop_;
+  base::OnceClosure service_quit_callback_;
 
  private:
-  std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
+  void OnServiceQuit() {
+    service_impl_.reset();
+    service_destroyed_wait_loop_.Quit();
+    if (service_quit_callback_)
+      std::move(service_quit_callback_).Run();
+  }
+
+  service_manager::TestConnectorFactory connector_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeviceFactoryProviderConnectorTest);
 };
 
 // We need to set the shutdown delay to at least some epsilon > 0 in order
@@ -233,7 +238,7 @@ TEST_F(NoAutomaticShutdownDeviceFactoryProviderConnectorTest,
   }
 
   base::MockCallback<base::OnceClosure> service_impl_destructor_cb;
-  service_impl_->SetDestructionObserver(service_impl_destructor_cb.Get());
+  service_quit_callback_ = service_impl_destructor_cb.Get();
   EXPECT_CALL(service_impl_destructor_cb, Run()).Times(0);
 
   // Wait for an arbitrary short extra time after which we are convinced that
@@ -246,7 +251,7 @@ TEST_F(NoAutomaticShutdownDeviceFactoryProviderConnectorTest,
     wait_loop.Run();
   }
 
-  service_impl_->SetDestructionObserver(base::DoNothing());
+  service_quit_callback_.Reset();
 }
 
 }  // namespace video_capture