[service-manager] Clean up Connector API

Updates the Connector mojom and C++ APIs with clarifying naming and
type usage.

Connector has accumulated lots of cruft over the years and its API
conveys a number of muddled concepts which have lead to general
confusion, misuse, and abuse.

This attempts to sort all that out. Namely:

  - All use of Identity in the Connector API has been migrated to
    ServiceFilter where appropriate.
  - StartService (first version) has been renamed to WarmService to
    convey the correct intent. Bugs filed and TODOs dropped in places
    where this is being used in an undesirable fashion.
  - StartService (second version) has been renamed to
    RegisterServiceInstance to convey more accurately what it does.
    It also requires a complete Identity to be provided by the trusted
    caller.
  - QueryService takes only a service name instead of an Identity, since
    it only needs to take a service name. It also makes no service
    connection so it no longer returns a ConnectResult.
  - FilterInterfaces is marked as deprecated on the mojom interface,
    because we want to obliterate it.
  - Test-only methods have been properly suffixed and made public,
    eliminating the need for the weird TestApi wrapper thing. A
    follow-up CL will delete TestApi and migrate its users.
  - Connector methods which have async replies have optional callbacks
    now instead of the Connector weirdly having a single catch-all
    (and dubiously named) "StartServiceCallback" to handle all replies.

Bug: 902590
Change-Id: I5a986a5c114da914fd624dac7e6fd021cf139958
Reviewed-on: https://chromium-review.googlesource.com/c/1329845
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#607750}

android_webview/renderer/aw_content_renderer_client.cc

@@ -300,7 +300,8 @@ void AwContentRendererClient::GetInterface(
   // TODO(crbug.com/806394): Use a WebView-specific service for SpellCheckHost
   // and SafeBrowsing, instead of |content_browser|.
   RenderThread::Get()->GetConnector()->BindInterface(
-      service_manager::Identity(content::mojom::kBrowserServiceName),
+      service_manager::ServiceFilter::ByName(
+          content::mojom::kBrowserServiceName),
       interface_name, std::move(interface_pipe));
 }
 

ash/accelerators/debug_commands.cc

@@ -115,7 +115,9 @@ void HandlePrintWindowHierarchy() {
 }
 
 void HandleShowQuickLaunch() {
-  Shell::Get()->connector()->StartService(quick_launch::mojom::kServiceName);
+  // TODO(https://crbug.com/904148): This should not use |WarmService()|.
+  Shell::Get()->connector()->WarmService(service_manager::ServiceFilter::ByName(
+      quick_launch::mojom::kServiceName));
 }
 
 gfx::ImageSkia CreateWallpaperImage(SkColor fill, SkColor rect) {

ash/app_launch_unittest.cc

@@ -42,11 +42,16 @@ TEST_F(AppLaunchTest, TestQuickLaunch) {
   if (features::IsSingleProcessMash())
     return;
 
-  connector()->StartService(mojom::kServiceName);
-  connector()->StartService(quick_launch::mojom::kServiceName);
+  // TODO(https://crbug.com/904148): These should not use |WarmService()|.
+  connector()->WarmService(
+      service_manager::ServiceFilter::ByName(mojom::kServiceName));
+  connector()->WarmService(service_manager::ServiceFilter::ByName(
+      quick_launch::mojom::kServiceName));
 
   ws::mojom::WindowServerTestPtr test_interface;
-  connector()->BindInterface(ws::mojom::kServiceName, &test_interface);
+  connector()->BindInterface(
+      service_manager::ServiceFilter::ByName(ws::mojom::kServiceName),
+      mojo::MakeRequest(&test_interface));
 
   base::RunLoop run_loop;
   bool success = false;

ash/ash_service_unittest.cc

@@ -105,8 +105,6 @@ TEST_F(AshServiceTest, OpenWindow) {
 
   WindowTreeClientDelegate window_tree_delegate;
 
-  connector()->StartService(mojom::kServiceName);
-
   // Connect to mus and create a new top level window. The request goes to
   // |ash|, but is async.
   std::unique_ptr<aura::WindowTreeClient> client =

ash/assistant/assistant_controller.cc

@@ -251,8 +251,8 @@ void AssistantController::GetNavigableContentsFactory(
   }
 
   Shell::Get()->connector()->BindInterface(
-      service_manager::Identity(content::mojom::kServiceName,
-                                service_instance_group),
+      service_manager::ServiceFilter::ByNameInGroup(
+          content::mojom::kServiceName, *service_instance_group),
       std::move(request));
 }
 

ash/keyboard/ash_keyboard_controller_unittest.cc

@@ -182,7 +182,8 @@ class AshKeyboardControllerTest : public AshTestBase {
 
     service_manager::Connector::TestApi test_api(connector_.get());
     test_api.OverrideBinderForTesting(
-        service_manager::Identity("test"), mojom::KeyboardController::Name_,
+        service_manager::ServiceFilter::ByName("test"),
+        mojom::KeyboardController::Name_,
         base::BindRepeating(
             &AshKeyboardControllerTest::AddKeyboardControllerBinding,
             base::Unretained(this)));

ash/multi_device_setup/multi_device_notification_presenter.cc

@@ -234,9 +234,9 @@ void MultiDeviceNotificationPresenter::ObserveMultiDeviceSetupIfPossible() {
     return;
 
   connector_->BindInterface(
-      service_manager::Identity(
+      service_manager::ServiceFilter::ByNameInGroup(
           chromeos::multidevice_setup::mojom::kServiceName,
-          service_instance_group),
+          *service_instance_group),
       &multidevice_setup_ptr_);
 
   // Add this object as the delegate of the MultiDeviceSetup Service.

ash/multi_device_setup/multi_device_notification_presenter_unittest.cc

@@ -136,7 +136,7 @@ class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase {
         std::make_unique<chromeos::multidevice_setup::FakeMultiDeviceSetup>();
     service_manager::Connector::TestApi test_api(connector_.get());
     test_api.OverrideBinderForTesting(
-        service_manager::Identity(
+        service_manager::ServiceFilter::ByNameInGroup(
             chromeos::multidevice_setup::mojom::kServiceName,
             kTestServiceInstanceGroup),
         chromeos::multidevice_setup::mojom::MultiDeviceSetup::Name_,

ash/shell.cc

@@ -1254,9 +1254,11 @@ void Shell::Init(
   // |connector_| is null in unit tests.
   if (connector_ &&
       base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowTaps)) {
-    // The show taps feature is a separate mojo app.
+    // The show taps feature is a separate service.
     // TODO(jamescook): Make this work in ash_shell_with_content.
-    connector_->StartService(tap_visualizer::mojom::kServiceName);
+    // TODO(https://crbug.com/904148): This should not use |WarmService()|.
+    connector_->WarmService(service_manager::ServiceFilter::ByName(
+        tap_visualizer::mojom::kServiceName));
   }
 
   if (!::features::IsMultiProcessMash()) {

ash/shell/content/client/shell_browser_main_parts.cc

@@ -85,14 +85,16 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
   chromeos::PowerPolicyController::Initialize(
       chromeos::DBusThreadManager::Get()->GetPowerManagerClient());
 
+  service_manager::Connector* const connector =
+      content::ServiceManagerConnection::GetForProcess()->GetConnector();
+
   ui::MaterialDesignController::Initialize();
   ash::ShellInitParams init_params;
   init_params.delegate = std::make_unique<ash::shell::ShellDelegateImpl>();
   init_params.context_factory = content::GetContextFactory();
   init_params.context_factory_private = content::GetContextFactoryPrivate();
   init_params.gpu_interface_provider = content::CreateGpuInterfaceProvider();
-  init_params.connector =
-      content::ServiceManagerConnection::GetForProcess()->GetConnector();
+  init_params.connector = connector;
   ash::Shell::CreateInstance(std::move(init_params));
 
   // Initialize session controller client and create fake user sessions. The
@@ -111,19 +113,17 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
 
   ash::Shell::GetPrimaryRootWindow()->GetHost()->Show();
 
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->StartService(test_ime_driver::mojom::kServiceName);
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->StartService(quick_launch::mojom::kServiceName);
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->StartService(tap_visualizer::mojom::kServiceName);
+  // TODO(https://crbug.com/904148): These should not use |WarmService()|.
+  connector->WarmService(service_manager::ServiceFilter::ByName(
+      test_ime_driver::mojom::kServiceName));
+  connector->WarmService(service_manager::ServiceFilter::ByName(
+      quick_launch::mojom::kServiceName));
+  connector->WarmService(service_manager::ServiceFilter::ByName(
+      tap_visualizer::mojom::kServiceName));
   shortcut_viewer::mojom::ShortcutViewerPtr shortcut_viewer;
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->BindInterface(shortcut_viewer::mojom::kServiceName, &shortcut_viewer);
+  connector->BindInterface(service_manager::ServiceFilter::ByName(
+                               shortcut_viewer::mojom::kServiceName),
+                           mojo::MakeRequest(&shortcut_viewer));
   shortcut_viewer->Toggle(base::TimeTicks::Now());
   ash::Shell::Get()->InitWaylandServer(nullptr);
 }

ash/system/flag_warning/flag_warning_tray.cc

@@ -59,7 +59,10 @@ void FlagWarningTray::ButtonPressed(views::Button* sender,
   DCHECK_EQ(button_, sender);
 
   // Open the quick launch mojo mini-app to demonstrate that mini-apps work.
-  Shell::Get()->connector()->StartService(quick_launch::mojom::kServiceName);
+  //
+  // TODO(https://crbug.com/904148): This should not use |WarmService()|.
+  Shell::Get()->connector()->WarmService(service_manager::ServiceFilter::ByName(
+      quick_launch::mojom::kServiceName));
 }
 
 void FlagWarningTray::GetAccessibleNodeData(ui::AXNodeData* node_data) {

chrome/browser/chrome_browser_main.cc

@@ -1873,8 +1873,10 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   if (parsed_command_line().HasSwitch(switches::kLaunchSimpleBrowserSwitch) ||
       parsed_command_line().HasSwitch(
           switches::kLaunchInProcessSimpleBrowserSwitch)) {
-    content::BrowserContext::GetConnectorFor(profile_)->StartService(
-        service_manager::Identity(simple_browser::mojom::kServiceName));
+    // TODO(https://crbug.com/904148): This should not use |WarmService()|.
+    content::BrowserContext::GetConnectorFor(profile_)->WarmService(
+        service_manager::ServiceFilter::ByName(
+            simple_browser::mojom::kServiceName));
   }
 #endif
 

chrome/browser/chrome_content_browser_client.cc

@@ -1337,10 +1337,11 @@ void ChromeContentBrowserClient::RenderProcessWillLaunch(
   *service_request = mojo::MakeRequest(&service);
   service_manager::mojom::PIDReceiverPtr pid_receiver;
   service_manager::Identity renderer_identity = host->GetChildIdentity();
-  ChromeService::GetInstance()->connector()->StartService(
+  ChromeService::GetInstance()->connector()->RegisterServiceInstance(
       service_manager::Identity(chrome::mojom::kRendererServiceName,
                                 renderer_identity.instance_group(),
-                                renderer_identity.instance_id()),
+                                renderer_identity.instance_id(),
+                                base::Token::CreateRandom()),
       std::move(service), mojo::MakeRequest(&pid_receiver));
 }
 

chrome/browser/chrome_service.cc

@@ -135,12 +135,13 @@ class ChromeService::ExtraParts : public ChromeBrowserMainExtraParts {
   void ServiceManagerConnectionStarted(
       content::ServiceManagerConnection* connection) override {
     // Initializing the connector asynchronously configures the Connector on the
-    // IO thread. This needs to be done before StartService() is called or
+    // IO thread. This needs to be done before WarmService() is called or
     // ChromeService::BindConnector() can race with ChromeService::OnStart().
     ChromeService::GetInstance()->InitConnector();
 
-    connection->GetConnector()->StartService(
-        service_manager::Identity(chrome::mojom::kServiceName));
+    // TODO(https://crbug.com/904148): This should not use |WarmService()|.
+    connection->GetConnector()->WarmService(
+        service_manager::ServiceFilter::ByName(chrome::mojom::kServiceName));
   }
 
   DISALLOW_COPY_AND_ASSIGN(ExtraParts);

chrome/browser/chrome_site_per_process_browsertest.cc

@@ -1021,12 +1021,9 @@ class TestBrowserClientForSpellCheck : public ChromeContentBrowserClient {
   void BindSpellCheckHostRequest(
       spellcheck::mojom::SpellCheckHostRequest request,
       const service_manager::BindSourceInfo& source_info) {
-    service_manager::Identity renderer_identity(
-        content::mojom::kRendererServiceName,
-        source_info.identity.instance_group(),
-        source_info.identity.instance_id());
     content::RenderProcessHost* host =
-        content::RenderProcessHost::FromRendererIdentity(renderer_identity);
+        content::RenderProcessHost::FromRendererInstanceId(
+            *source_info.identity.instance_id());
     auto spell_check_host = std::make_unique<MockSpellCheckHost>(host);
     spell_check_host->SpellCheckHostRequest(std::move(request));
     spell_check_hosts_.push_back(std::move(spell_check_host));

chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc

@@ -69,7 +69,7 @@ class DisplayInfoProviderChromeosTest : public ash::AshTestBase {
     connector_ = service_manager::Connector::Create(&request);
     service_manager::Connector::TestApi test_api(connector_.get());
     test_api.OverrideBinderForTesting(
-        service_manager::Identity(ash::mojom::kServiceName),
+        service_manager::ServiceFilter::ByName(ash::mojom::kServiceName),
         ash::mojom::CrosDisplayConfigController::Name_,
         base::BindRepeating(&DisplayInfoProviderChromeosTest::
                                 AddCrosDisplayConfigControllerBinding,

chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc

@@ -53,12 +53,9 @@ void SpellCheckContentBrowserClient::RunUntilBind() {
 void SpellCheckContentBrowserClient::BindSpellCheckPanelHostRequest(
     spellcheck::mojom::SpellCheckPanelHostRequest request,
     const service_manager::BindSourceInfo& source_info) {
-  service_manager::Identity renderer_identity(
-      content::mojom::kRendererServiceName,
-      source_info.identity.instance_group(),
-      source_info.identity.instance_id());
   content::RenderProcessHost* render_process_host =
-      content::RenderProcessHost::FromRendererIdentity(renderer_identity);
+      content::RenderProcessHost::FromRendererInstanceId(
+          *source_info.identity.instance_id());
   auto spell_check_panel_host =
       std::make_unique<SpellCheckMockPanelHost>(render_process_host);
   spell_check_panel_host->SpellCheckPanelHostRequest(std::move(request));

chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc

@@ -153,10 +153,10 @@ void ChromeBrowserMainExtraPartsAsh::ServiceManagerConnectionStarted(
   if (features::IsUsingWindowService()) {
     // Start up the window service and the ash system UI service.
     // NOTE: ash::Shell is still created below for SingleProcessMash.
-    connection->GetConnector()->StartService(
-        service_manager::Identity(ws::mojom::kServiceName));
-    connection->GetConnector()->StartService(
-        service_manager::Identity(ash::mojom::kServiceName));
+    connection->GetConnector()->WarmService(
+        service_manager::ServiceFilter::ByName(ws::mojom::kServiceName));
+    connection->GetConnector()->WarmService(
+        service_manager::ServiceFilter::ByName(ash::mojom::kServiceName));
 
     views::MusClient::InitParams params;
     params.connector = connection->GetConnector();

content/browser/browser_child_process_host_impl.cc

@@ -52,6 +52,7 @@
 #include "content/public/common/service_manager_connection.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "services/service_manager/embedder/switches.h"
+#include "services/service_manager/public/cpp/constants.h"
 
 #if defined(OS_MACOSX)
 #include "content/browser/mach_broker_mac.h"
@@ -169,13 +170,12 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
 
   if (!service_name.empty()) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
-    service_manager::Identity child_identity(service_name,
-                                             base::nullopt /* instance_group */,
-                                             base::Token::CreateRandom());
-    child_connection_.reset(
-        new ChildConnection(child_identity, &mojo_invitation_,
-                            ServiceManagerContext::GetConnectorForIOThread(),
-                            base::ThreadTaskRunnerHandle::Get()));
+    child_connection_ = std::make_unique<ChildConnection>(
+        service_manager::Identity(
+            service_name, service_manager::kSystemInstanceGroup,
+            base::Token::CreateRandom(), base::Token::CreateRandom()),
+        &mojo_invitation_, ServiceManagerContext::GetConnectorForIOThread(),
+        base::ThreadTaskRunnerHandle::Get());
     data_.metrics_name = service_name;
   }
 

content/browser/browser_context.cc

@@ -582,12 +582,13 @@ void BrowserContext::Initialize(
     auto service_request = mojo::MakeRequest(&service);
 
     service_manager::mojom::PIDReceiverPtr pid_receiver;
-    service_manager::Identity identity(mojom::kBrowserServiceName, new_group);
-    service_manager_connection->GetConnector()->StartService(
+    service_manager::Identity identity(mojom::kBrowserServiceName, new_group,
+                                       base::Token{},
+                                       base::Token::CreateRandom());
+    service_manager_connection->GetConnector()->RegisterServiceInstance(
         identity, std::move(service), mojo::MakeRequest(&pid_receiver));
     pid_receiver->SetPID(base::GetCurrentProcId());
 
-    service_manager_connection->GetConnector()->StartService(identity);
     BrowserContextServiceManagerConnectionHolder* connection_holder =
         new BrowserContextServiceManagerConnectionHolder(
             std::move(service_request));

content/browser/browser_main_loop.cc

@@ -1674,8 +1674,9 @@ void BrowserMainLoop::InitializeAudio() {
           if (connection) {
             // The browser is not shutting down: |connection| would be null
             // otherwise.
-            connection->GetConnector()->StartService(
-                audio::mojom::kServiceName);
+            connection->GetConnector()->WarmService(
+                service_manager::ServiceFilter::ByName(
+                    audio::mojom::kServiceName));
           }
         }));
   }

content/browser/renderer_host/media/media_devices_manager.cc

@@ -311,19 +311,20 @@ class MediaDevicesManager::AudioServiceDeviceListener
   void TryConnectToService(service_manager::Connector* connector) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     // Check if the service manager is managing the audio service.
+    //
+    // TODO: Is this necessary? We should know at build how this will respond.
     connector->QueryService(
-        service_manager::Identity(audio::mojom::kServiceName),
+        audio::mojom::kServiceName,
         base::BindOnce(&AudioServiceDeviceListener::ServiceQueried,
                        weak_factory_.GetWeakPtr(), connector));
   }
 
   void ServiceQueried(service_manager::Connector* connector,
-                      service_manager::mojom::ConnectResult connect_result,
-                      const std::string& ignore) {
+                      service_manager::mojom::ServiceInfoPtr info) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     // Do not connect if the service manager is not managing the audio service.
-    if (connect_result != service_manager::mojom::ConnectResult::SUCCEEDED) {
-      LOG(WARNING) << "Audio service not available: " << connect_result;
+    if (!info) {
+      LOG(WARNING) << "Audio service not available.";
       return;
     }
     DoConnectToService(connector);

content/browser/renderer_host/render_process_host_impl.cc

@@ -1870,12 +1870,12 @@ void RenderProcessHostImpl::InitializeChannelProxy() {
 
   // Establish a ServiceManager connection for the new render service instance.
   mojo_invitation_ = {};
-  service_manager::Identity child_identity(
-      mojom::kRendererServiceName,
-      BrowserContext::GetServiceInstanceGroupFor(GetBrowserContext()),
-      base::Token::CreateRandom());
   child_connection_ = std::make_unique<ChildConnection>(
-      child_identity, &mojo_invitation_, connector, io_task_runner);
+      service_manager::Identity(
+          mojom::kRendererServiceName,
+          BrowserContext::GetServiceInstanceGroupFor(GetBrowserContext()),
+          base::Token::CreateRandom(), base::Token::CreateRandom()),
+      &mojo_invitation_, connector, io_task_runner);
 
   // Send an interface request to bootstrap the IPC::Channel. Note that this
   // request will happily sit on the pipe until the process is launched and
@@ -3805,12 +3805,12 @@ RenderProcessHost* RenderProcessHost::FromID(int render_process_id) {
 }
 
 // static
-RenderProcessHost* RenderProcessHost::FromRendererIdentity(
-    const service_manager::Identity& identity) {
+RenderProcessHost* RenderProcessHost::FromRendererInstanceId(
+    const base::Token& instance_id) {
   for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator());
        !i.IsAtEnd(); i.Advance()) {
     RenderProcessHost* process = i.GetCurrentValue();
-    if (process->GetChildIdentity().Matches(identity))
+    if (process->GetChildIdentity().instance_id() == instance_id)
       return process;
   }
   return nullptr;