Move pdf_compositor off ServiceContext/ServiceTest

Migrates pdf_compositor from ServiceContext to ServiceBinding and makes
its tests regular unit tests instead of using the deprecated ServiceTest
framework.

TBR=thestig@chromium.org

Bug: 891780,906239
Change-Id: Ia2485320e71352b3263108544fbea06c6b8ca6ef
Reviewed-on: https://chromium-review.googlesource.com/c/1343506
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#610340}

Author: krockot

chrome/utility/chrome_content_utility_client.cc

@@ -204,13 +204,6 @@ void ChromeContentUtilityClient::RegisterServices(
   services->emplace(device::mojom::kVrIsolatedServiceName, service_info);
 #endif
 
-#if BUILDFLAG(ENABLE_PRINTING)
-  service_manager::EmbeddedServiceInfo pdf_compositor_info;
-  pdf_compositor_info.factory = base::BindRepeating(
-      &printing::CreatePdfCompositorService, GetUserAgent());
-  services->emplace(printing::mojom::kServiceName, pdf_compositor_info);
-#endif
-
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW) || \
     (BUILDFLAG(ENABLE_PRINTING) && defined(OS_WIN))
   service_manager::EmbeddedServiceInfo printing_info;
@@ -328,6 +321,13 @@ ChromeContentUtilityClient::HandleServiceRequest(
     return std::make_unique<patch::PatchService>(std::move(request));
 #endif
 
+#if BUILDFLAG(ENABLE_PRINTING)
+  if (service_name == printing::mojom::kServiceName) {
+    return printing::CreatePdfCompositorService(GetUserAgent(),
+                                                std::move(request));
+  }
+#endif
+
 #if defined(OS_CHROMEOS)
   if (service_name == chromeos::ime::mojom::kServiceName)
     return std::make_unique<chromeos::ime::ImeService>(std::move(request));

components/services/pdf_compositor/BUILD.gn

@@ -53,22 +53,18 @@ if (enable_basic_printing) {
       "//testing/gmock/include",
       "//third_party/skia/include/core",
     ]
+
     deps = [
       ":pdf_compositor",
+      "//base",
       "//base/test:test_support",
       "//cc/paint:paint",
       "//components/crash/core/common:crash_key",
       "//components/services/pdf_compositor/public/interfaces",
-      "//services/service_manager/public/cpp:service_test_support",
+      "//services/service_manager/public/cpp/test:test_support",
       "//skia",
       "//testing/gmock",
       "//testing/gtest",
     ]
   }
-
-  service_manifest("pdf_compositor_service_unittest_manifest") {
-    name = "pdf_compositor_service_unittest"
-    source = "pdf_compositor_service_unittest_manifest.json"
-    packaged_services = [ ":pdf_compositor_manifest" ]
-  }
 }

components/services/pdf_compositor/OWNERS

@@ -2,6 +2,3 @@ file://printing/OWNERS
 
 per-file pdf_compositor_manifest.json=set noparent
 per-file pdf_compositor_manifest.json=file://ipc/SECURITY_OWNERS
-
-per-file pdf_compositor_service_unittest_manifest.json=set noparent
-per-file pdf_compositor_service_unittest_manifest.json=file://ipc/SECURITY_OWNERS

components/services/pdf_compositor/pdf_compositor_service.cc

@@ -32,20 +32,24 @@
 
 namespace {
 
-void OnPdfCompositorRequest(
-    const std::string& creator,
-    service_manager::ServiceContextRefFactory* ref_factory,
-    printing::mojom::PdfCompositorRequest request) {
+void OnPdfCompositorRequest(const std::string& creator,
+                            service_manager::ServiceKeepalive* keepalive,
+                            printing::mojom::PdfCompositorRequest request) {
   mojo::MakeStrongBinding(std::make_unique<printing::PdfCompositorImpl>(
-                              creator, ref_factory->CreateRef()),
+                              creator, keepalive->CreateRef()),
                           std::move(request));
 }
+
 }  // namespace
 
 namespace printing {
 
-PdfCompositorService::PdfCompositorService(const std::string& creator)
-    : creator_(creator.empty() ? "Chromium" : creator), weak_factory_(this) {}
+PdfCompositorService::PdfCompositorService(
+    const std::string& creator,
+    service_manager::mojom::ServiceRequest request)
+    : creator_(creator.empty() ? "Chromium" : creator),
+      binding_(this, std::move(request)),
+      keepalive_(&binding_, base::TimeDelta{}) {}
 
 PdfCompositorService::~PdfCompositorService() {
 #if defined(OS_WIN)
@@ -55,27 +59,35 @@ PdfCompositorService::~PdfCompositorService() {
 
 // static
 std::unique_ptr<service_manager::Service> PdfCompositorService::Create(
-    const std::string& creator) {
+    const std::string& creator,
+    service_manager::mojom::ServiceRequest request) {
 #if defined(OS_WIN)
   // Initialize direct write font proxy so skia can use it.
   content::InitializeDWriteFontProxy();
 #endif
-  return std::make_unique<printing::PdfCompositorService>(creator);
+  return std::make_unique<printing::PdfCompositorService>(creator,
+                                                          std::move(request));
 }
 
-void PdfCompositorService::PrepareToStart() {
+void PdfCompositorService::OnStart() {
+  registry_.AddInterface(
+      base::BindRepeating(&OnPdfCompositorRequest, creator_, &keepalive_));
+
+  if (skip_initialization_for_testing_)
+    return;
+
   // Set up discardable memory manager.
   discardable_memory::mojom::DiscardableSharedMemoryManagerPtr manager_ptr;
   if (features::IsMultiProcessMash()) {
 #if defined(USE_AURA)
-    context()->connector()->BindInterface(ws::mojom::kServiceName,
-                                          &manager_ptr);
+    binding_.GetConnector()->BindInterface(ws::mojom::kServiceName,
+                                           &manager_ptr);
 #else
     NOTREACHED();
 #endif
   } else {
-    context()->connector()->BindInterface(content::mojom::kBrowserServiceName,
-                                          &manager_ptr);
+    binding_.GetConnector()->BindInterface(content::mojom::kBrowserServiceName,
+                                           &manager_ptr);
   }
   discardable_shared_memory_manager_ = std::make_unique<
       discardable_memory::ClientDiscardableSharedMemoryManager>(
@@ -99,19 +111,10 @@ void PdfCompositorService::PrepareToStart() {
   // Initialize a connection to FontLoaderMac service so blink platform's web
   // sandbox support can communicate with it to load font.
   content::UtilityThread::Get()->InitializeFontLoaderMac(
-      context()->connector());
+      binding_.GetConnector());
 #endif
 }
 
-void PdfCompositorService::OnStart() {
-  PrepareToStart();
-
-  ref_factory_ = std::make_unique<service_manager::ServiceContextRefFactory>(
-      context()->CreateQuitClosure());
-  registry_.AddInterface(
-      base::Bind(&OnPdfCompositorRequest, creator_, ref_factory_.get()));
-}
-
 void PdfCompositorService::OnBindInterface(
     const service_manager::BindSourceInfo& source_info,
     const std::string& interface_name,

components/services/pdf_compositor/pdf_compositor_service.h

@@ -14,38 +14,45 @@
 #include "components/services/pdf_compositor/public/interfaces/pdf_compositor.mojom.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_context_ref.h"
+#include "services/service_manager/public/cpp/service_binding.h"
+#include "services/service_manager/public/cpp/service_keepalive.h"
 
 namespace printing {
 
 class PdfCompositorService : public service_manager::Service {
  public:
-  explicit PdfCompositorService(const std::string& creator);
+  PdfCompositorService(const std::string& creator,
+                       service_manager::mojom::ServiceRequest request);
   ~PdfCompositorService() override;
 
   // Factory function for use as an embedded service.
   static std::unique_ptr<service_manager::Service> Create(
-      const std::string& creator);
+      const std::string& creator,
+      service_manager::mojom::ServiceRequest request);
 
   // service_manager::Service:
   void OnStart() override;
   void OnBindInterface(const service_manager::BindSourceInfo& source_info,
                        const std::string& interface_name,
                        mojo::ScopedMessagePipeHandle interface_pipe) override;
 
-  virtual void PrepareToStart();
+  void set_skip_initialization_for_testing(bool skip) {
+    skip_initialization_for_testing_ = skip;
+  }
 
  private:
   // The creator of this service.
   // Currently contains the service creator's user agent string if given,
   // otherwise just use string "Chromium".
   const std::string creator_;
 
+  service_manager::ServiceBinding binding_;
+  service_manager::ServiceKeepalive keepalive_;
+  bool skip_initialization_for_testing_ = false;
   std::unique_ptr<discardable_memory::ClientDiscardableSharedMemoryManager>
       discardable_shared_memory_manager_;
-  std::unique_ptr<service_manager::ServiceContextRefFactory> ref_factory_;
   service_manager::BinderRegistry registry_;
-  base::WeakPtrFactory<PdfCompositorService> weak_factory_;
+  base::WeakPtrFactory<PdfCompositorService> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(PdfCompositorService);
 };

components/services/pdf_compositor/pdf_compositor_service_unittest.cc

@@ -8,86 +8,40 @@
 #include "base/callback.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/test_discardable_memory_allocator.h"
 #include "cc/paint/paint_flags.h"
 #include "cc/paint/skia_paint_canvas.h"
 #include "components/services/pdf_compositor/pdf_compositor_service.h"
 #include "components/services/pdf_compositor/public/cpp/pdf_service_mojo_types.h"
 #include "components/services/pdf_compositor/public/interfaces/pdf_compositor.mojom.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_context.h"
-#include "services/service_manager/public/cpp/service_test.h"
-#include "services/service_manager/public/mojom/service_factory.mojom.h"
+#include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkStream.h"
 #include "third_party/skia/src/utils/SkMultiPictureDocument.h"
 
 namespace printing {
 
-// In order to test PdfCompositorService, this class overrides PrepareToStart()
-// to do nothing. So the test discardable memory allocator set up by
-// PdfCompositorServiceTest will be used. Also checks for the service setup are
-// skipped since we don't have those setups in unit tests.
-class PdfCompositorTestService : public printing::PdfCompositorService {
+class PdfCompositorServiceTest : public testing::Test {
  public:
-  explicit PdfCompositorTestService(const std::string& creator)
-      : PdfCompositorService(creator) {}
-  ~PdfCompositorTestService() override {}
-
-  // PdfCompositorService:
-  void PrepareToStart() override {}
-};
-
-class PdfServiceTestClient : public service_manager::test::ServiceTestClient,
-                             public service_manager::mojom::ServiceFactory {
- public:
-  explicit PdfServiceTestClient(service_manager::test::ServiceTest* test)
-      : service_manager::test::ServiceTestClient(test) {
-    registry_.AddInterface<service_manager::mojom::ServiceFactory>(
-        base::Bind(&PdfServiceTestClient::Create, base::Unretained(this)));
-  }
-  ~PdfServiceTestClient() override {}
-
-  // service_manager::Service
-  void OnBindInterface(const service_manager::BindSourceInfo& source_info,
-                       const std::string& interface_name,
-                       mojo::ScopedMessagePipeHandle interface_pipe) override {
-    registry_.BindInterface(interface_name, std::move(interface_pipe));
+  PdfCompositorServiceTest()
+      : connector_(test_connector_factory_.CreateConnector()),
+        service_(
+            "pdf_compositor_service_unittest",
+            test_connector_factory_.RegisterInstance(mojom::kServiceName)) {
+    // We don't want the service instance setting up its own discardable memory
+    // allocator, which it normally does. Instead it will use the one provided
+    // by our fixture in |SetUp()| below.
+    service_.set_skip_initialization_for_testing(true);
   }
 
-  // service_manager::mojom::ServiceFactory
-  void CreateService(
-      service_manager::mojom::ServiceRequest request,
-      const std::string& name,
-      service_manager::mojom::PIDReceiverPtr pid_receiver) override {
-    if (!name.compare(mojom::kServiceName)) {
-      service_context_ = std::make_unique<service_manager::ServiceContext>(
-          std::make_unique<PdfCompositorTestService>("pdf_compositor_unittest"),
-          std::move(request));
-    }
-  }
-
-  void Create(service_manager::mojom::ServiceFactoryRequest request) {
-    service_factory_bindings_.AddBinding(this, std::move(request));
-  }
-
- private:
-  service_manager::BinderRegistry registry_;
-  mojo::BindingSet<service_manager::mojom::ServiceFactory>
-      service_factory_bindings_;
-  std::unique_ptr<service_manager::ServiceContext> service_context_;
-};
-
-class PdfCompositorServiceTest : public service_manager::test::ServiceTest {
- public:
-  PdfCompositorServiceTest() : ServiceTest("pdf_compositor_service_unittest") {}
-  ~PdfCompositorServiceTest() override {}
+  ~PdfCompositorServiceTest() override = default;
 
   MOCK_METHOD1(CallbackOnCompositeSuccess,
                void(const base::ReadOnlySharedMemoryRegion&));
   MOCK_METHOD1(CallbackOnCompositeStatus, void(mojom::PdfCompositor::Status));
+
   void OnCompositeToPdfCallback(mojom::PdfCompositor::Status status,
                                 base::ReadOnlySharedMemoryRegion region) {
     if (status == mojom::PdfCompositor::Status::SUCCESS)
@@ -100,11 +54,11 @@ class PdfCompositorServiceTest : public service_manager::test::ServiceTest {
   MOCK_METHOD0(ConnectionClosed, void());
 
  protected:
-  // service_manager::test::ServiceTest:
+  service_manager::Connector* connector() { return connector_.get(); }
+
   void SetUp() override {
     base::DiscardableMemoryAllocator::SetInstance(
         &discardable_memory_allocator_);
-    ServiceTest::SetUp();
 
     ASSERT_FALSE(compositor_);
     connector()->BindInterface(mojom::kServiceName, &compositor_);
@@ -114,15 +68,10 @@ class PdfCompositorServiceTest : public service_manager::test::ServiceTest {
   }
 
   void TearDown() override {
-    // Clean up
     compositor_.reset();
     base::DiscardableMemoryAllocator::SetInstance(nullptr);
   }
 
-  std::unique_ptr<service_manager::Service> CreateService() override {
-    return std::make_unique<PdfServiceTestClient>(this);
-  }
-
   base::ReadOnlySharedMemoryRegion CreateMSKP() {
     SkDynamicMemoryWStream stream;
     sk_sp<SkDocument> doc = SkMakeMultiPictureDocument(&stream);
@@ -156,11 +105,16 @@ class PdfCompositorServiceTest : public service_manager::test::ServiceTest {
     run_loop_->Run();
   }
 
+  base::test::ScopedTaskEnvironment task_environment_;
   std::unique_ptr<base::RunLoop> run_loop_;
   mojom::PdfCompositorPtr compositor_;
   base::TestDiscardableMemoryAllocator discardable_memory_allocator_;
 
  private:
+  service_manager::TestConnectorFactory test_connector_factory_;
+  std::unique_ptr<service_manager::Connector> connector_;
+  PdfCompositorService service_;
+
   DISALLOW_COPY_AND_ASSIGN(PdfCompositorServiceTest);
 };
 
@@ -183,8 +137,8 @@ TEST_F(PdfCompositorServiceTest, InvokeCallbackOnSuccess) {
   CallCompositorWithSuccess(std::move(compositor_));
 }
 
-// Test coexistence of multiple service instances.
-TEST_F(PdfCompositorServiceTest, MultipleServiceInstances) {
+// Test coexistence of multiple PdfCompositor interface bindings.
+TEST_F(PdfCompositorServiceTest, MultipleCompositors) {
   // One service can bind multiple interfaces.
   mojom::PdfCompositorPtr another_compositor;
   ASSERT_FALSE(another_compositor);
@@ -197,9 +151,9 @@ TEST_F(PdfCompositorServiceTest, MultipleServiceInstances) {
   CallCompositorWithSuccess(std::move(another_compositor));
 }
 
-// Test data structures and content of multiple service instances
+// Test data structures and content of multiple PdfCompositor interface bindings
 // are independent from each other.
-TEST_F(PdfCompositorServiceTest, IndependentServiceInstances) {
+TEST_F(PdfCompositorServiceTest, IndependentCompositors) {
   // Create a new connection 2.
   mojom::PdfCompositorPtr compositor2;
   ASSERT_FALSE(compositor2);