Refactor Blink's ServiceConnector and add ability to mock in layout tests

content/renderer/mojo/blink_connector_impl.cc

Index: content/renderer/mojo/blink_connector_impl.cc
diff --git a/content/renderer/mojo/blink_connector_impl.cc b/content/renderer/mojo/blink_connector_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9e9c4594403fdaa5aa326485041de9915afd3511
--- /dev/null
+++ b/content/renderer/mojo/blink_connector_impl.cc
@@ -0,0 +1,91 @@
+// Copyright 2017 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 "content/renderer/mojo/blink_connector_impl.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "services/service_manager/public/cpp/identity.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
+
+namespace content {
+
+BlinkConnectorImpl::BlinkConnectorImpl(
+    std::unique_ptr<service_manager::Connector> connector)
+    : connector_(std::move(connector)),
+      main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_ptr_factory_(this) {}
+
+BlinkConnectorImpl::~BlinkConnectorImpl() = default;
+
+void BlinkConnectorImpl::getInterface(const char* service_name,
+                                      const char* interface_name,
+                                      mojo::ScopedMessagePipeHandle handle) {
+  // |connector_| is null in some testing contexts.
+  if (!connector_.get())

[dcheng, 2017/02/01 07:45:16]

Nit: no .get(). Also... it would be best to just fix the tests so we don't have
to null check like this.

+    return;
+
+  if (!main_thread_task_runner_->BelongsToCurrentThread()) {
+    main_thread_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&BlinkConnectorImpl::getInterface, GetWeakPtr(),

[esprehn, 2017/02/01 06:02:23]

Woah, this round trips through main to connect to a service? Why would a worker
thread want to do that?

[dcheng, 2017/02/01 07:45:16]

FWIW, this already happens today in
https://cs.chromium.org/chromium/src/content/renderer/mojo/blink_interface_pr....
for that class, it's because we hold a weak pointer to the interface provider.

(It's not clear to me that service_manager::InterfaceProvider is thread-safe
either)

[Ken Rockot(use gerrit already), 2017/02/01 19:16:00]

Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.

Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl, so we could have a thread-local
Connector if we wanted.

That is more complexity for this CL though and I think it's reasonable to land
that change later. This is certainly not currently on a critical path.

[Ken Rockot(use gerrit already), 2017/02/01 19:16:00]

Correct, we already tolerate this behavior. It's not ideal, but doesn't seem
like a deal-breaker.

Unlike InterfaceProvider, we could solve this for Connector today by taking
advantage of the Clone() interface, which essentially allows you to bind
arbitrary new ConnectorPtrs to the same impl.

That is more complexity for this CL though and I think it's reasonable to land
that change later.

+                   service_name, interface_name, base::Passed(&handle)));
+    return;
+  }
+
+  // Tests might have overridden this interface with a local implementation.
+  InterfaceBinderMap* overrides_for_service =
+      GetOverridesForService(service_name);
+  if (overrides_for_service) {
+    auto it = overrides_for_service->find(std::string(interface_name));

[esprehn, 2017/02/01 06:02:23]

this should implicitly construct, no need to write std::string here.

+    if (it != overrides_for_service->end()) {
+      it->second.Run(std::move(handle));
+      return;
+    }
+  }
+
+  service_manager::Identity service_identity(
+      service_name, service_manager::mojom::kInheritUserID);
+  connector_->BindInterface(service_identity, interface_name,
+                            std::move(handle));
+}
+
+void BlinkConnectorImpl::AddOverrideForTesting(
+    const std::string& service_name,
+    const std::string& interface_name,
+    const base::Callback<void(mojo::ScopedMessagePipeHandle)>& binder) {
+  if (service_binders_.find(service_name) == service_binders_.end())
+    service_binders_[service_name] = base::MakeUnique<InterfaceBinderMap>();
+
+  service_binders_[service_name]->insert(
+      std::pair<std::string, Binder>(interface_name, binder));
+}
+
+void BlinkConnectorImpl::ClearOverridesForTesting() {
+  service_binders_.clear();
+}
+
+base::WeakPtr<BlinkConnectorImpl> BlinkConnectorImpl::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+BlinkConnectorImpl::InterfaceBinderMap*
+BlinkConnectorImpl::GetOverridesForService(const char* service_name) {
+  // Short-circuit out in the case where there are no overrides (always true in
+  // production).
+  if (service_binders_.empty())
+    return nullptr;
+
+  auto it = service_binders_.find(std::string(service_name));
+
+  if (it != service_binders_.end())
+    return it->second.get();
+
+  return nullptr;
+}
+
+}  // namespace content