Remove viz host dep on viz service

HostFrameSinkManager communicates with FrameSinkManagerImpl over IPC so
//components/viz/host should no longer need a dependency on
//components/viz/service. There still exists a way to connect the two
classes directly but it uses the same mojo interface just without a
message pipe.

Bug: 1035853
Change-Id: I2b570697744419b43b84999d290859bdd84ca7d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2021300Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735861}

components/viz/host/BUILD.gn

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import("//build/config/ui.gni")
 import("//components/viz/viz.gni")
 import("//testing/libfuzzer/fuzzer_test.gni")
 
@@ -48,10 +49,6 @@ viz_component("host") {
     "//services/viz/privileged/mojom",
     "//ui/base",
     "//ui/gfx",
-
-    # TODO(kylechar): This is temporary and will be removed when all host to
-    # service communication is over Mojo.
-    "//components/viz/service",
   ]
 
   public_deps = [
@@ -66,6 +63,10 @@ viz_component("host") {
   if (is_mac) {
     deps += [ "//ui/accelerated_widget_mac" ]
   }
+
+  if (use_ozone) {
+    deps += [ "//ui/ozone" ]
+  }
 }
 
 viz_source_set("unit_tests") {
@@ -89,11 +90,11 @@ viz_source_set("unit_tests") {
     "//services/viz/public/mojom",
     "//testing/gmock",
     "//testing/gtest",
-
-    # TODO(kylechar): This is temporary and will be removed when all host to
-    # service communication is over Mojo.
-    "//components/viz/service",
   ]
+
+  if (use_ozone) {
+    deps += [ "//ui/ozone" ]
+  }
 }
 
 fuzzer_test("hit_test_query_fuzzer") {

components/viz/host/DEPS

@@ -25,10 +25,6 @@ include_rules = [
 ]
 
 specific_include_rules = {
-  "host_frame_sink_manager*": [
-    "+components/viz/service/frame_sinks/compositor_frame_sink_support.h",
-    "+components/viz/service/frame_sinks/frame_sink_manager_impl.h",
-  ],
   "renderer_settings_creation\.cc": [
     "+components/viz/common/switches.h",
   ],

components/viz/host/host_frame_sink_manager.cc

@@ -12,10 +12,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "components/viz/common/surfaces/surface_info.h"
-#include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
-#include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
 #include "mojo/public/cpp/bindings/sync_call_restrictions.h"
-#include "services/viz/public/mojom/compositing/compositor_frame_sink.mojom.h"
 
 namespace viz {
 
@@ -24,18 +21,15 @@ HostFrameSinkManager::HostFrameSinkManager() = default;
 HostFrameSinkManager::~HostFrameSinkManager() = default;
 
 void HostFrameSinkManager::SetLocalManager(
-    FrameSinkManagerImpl* frame_sink_manager_impl) {
+    mojom::FrameSinkManager* frame_sink_manager) {
   DCHECK(!frame_sink_manager_remote_);
-  frame_sink_manager_impl_ = frame_sink_manager_impl;
-
-  frame_sink_manager_ = frame_sink_manager_impl;
+  frame_sink_manager_ = frame_sink_manager;
 }
 
 void HostFrameSinkManager::BindAndSetManager(
     mojo::PendingReceiver<mojom::FrameSinkManagerClient> receiver,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     mojo::PendingRemote<mojom::FrameSinkManager> remote) {
-  DCHECK(!frame_sink_manager_impl_);
   DCHECK(!receiver_.is_bound());
 
   receiver_.Bind(std::move(receiver), std::move(task_runner));
@@ -317,28 +311,6 @@ void HostFrameSinkManager::RemoveHitTestRegionObserver(
   observers_.RemoveObserver(observer);
 }
 
-std::unique_ptr<CompositorFrameSinkSupport>
-HostFrameSinkManager::CreateCompositorFrameSinkSupport(
-    mojom::CompositorFrameSinkClient* client,
-    const FrameSinkId& frame_sink_id,
-    bool is_root) {
-  DCHECK(frame_sink_manager_impl_);
-
-  FrameSinkData& data = frame_sink_data_map_[frame_sink_id];
-  DCHECK(data.IsFrameSinkRegistered());
-  DCHECK(!data.has_created_compositor_frame_sink);
-
-  auto support = std::make_unique<CompositorFrameSinkSupport>(
-      client, frame_sink_manager_impl_, frame_sink_id, is_root);
-
-  data.is_root = is_root;
-
-  if (is_root)
-    display_hit_test_query_[frame_sink_id] = std::make_unique<HitTestQuery>();
-
-  return support;
-}
-
 void HostFrameSinkManager::OnConnectionLost() {
   connection_was_lost_ = true;
 

components/viz/host/host_frame_sink_manager.h

@@ -35,8 +35,6 @@ class SingleThreadTaskRunner;
 
 namespace viz {
 
-class CompositorFrameSinkSupport;
-class FrameSinkManagerImpl;
 class SurfaceInfo;
 
 enum class ReportFirstSurfaceActivation { kYes, kNo };
@@ -58,7 +56,7 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   }
 
   // Sets a local FrameSinkManagerImpl instance and connects directly to it.
-  void SetLocalManager(FrameSinkManagerImpl* frame_sink_manager_impl);
+  void SetLocalManager(mojom::FrameSinkManager* frame_sink_manager);
 
   // Binds |this| as a FrameSinkManagerClient for |receiver| on |task_runner|.
   // On Mac |task_runner| will be the resize helper task runner. May only be
@@ -189,12 +187,6 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   void AddHitTestRegionObserver(HitTestRegionObserver* observer);
   void RemoveHitTestRegionObserver(HitTestRegionObserver* observer);
 
-  // TODO(crbug.com/1033683): Remove test usage and delete function.
-  std::unique_ptr<CompositorFrameSinkSupport> CreateCompositorFrameSinkSupport(
-      mojom::CompositorFrameSinkClient* client,
-      const FrameSinkId& frame_sink_id,
-      bool is_root);
-
   void SetHitTestAsyncQueriedDebugRegions(
       const FrameSinkId& root_frame_sink_id,
       const std::vector<FrameSinkId>& hit_test_async_queried_debug_queue);
@@ -268,23 +260,15 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
       const FrameSinkId& frame_sink_id,
       const std::vector<AggregatedHitTestRegion>& hit_test_data) override;
 
-  // This will point to |frame_sink_manager_remote_| if using Mojo or
-  // |frame_sink_manager_impl_| if directly connected. Use this to make function
+  // This will point to |frame_sink_manager_remote_| if using mojo or it may
+  // point directly at FrameSinkManagerImpl in tests. Use this to make function
   // calls.
   mojom::FrameSinkManager* frame_sink_manager_ = nullptr;
 
-  // Mojo connection to the FrameSinkManager. If this is bound then
-  // |frame_sink_manager_impl_| must be null.
+  // Connections to/from FrameSinkManagerImpl.
   mojo::Remote<mojom::FrameSinkManager> frame_sink_manager_remote_;
-
-  // Mojo connection back from the FrameSinkManager.
   mojo::Receiver<mojom::FrameSinkManagerClient> receiver_{this};
 
-  // A direct connection to FrameSinkManagerImpl. If this is set then
-  // |frame_sink_manager_remote_| must be unbound. For use in browser process
-  // only, viz process should not set this.
-  FrameSinkManagerImpl* frame_sink_manager_impl_ = nullptr;
-
   // Per CompositorFrameSink data.
   std::unordered_map<FrameSinkId, FrameSinkData, FrameSinkIdHash>
       frame_sink_data_map_;

components/viz/host/host_frame_sink_manager_unittest.cc

@@ -13,19 +13,18 @@
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "components/viz/common/surfaces/surface_id.h"
 #include "components/viz/common/surfaces/surface_info.h"
-#include "components/viz/service/display_embedder/server_shared_bitmap_manager.h"
-#include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
-#include "components/viz/service/surfaces/surface_manager.h"
 #include "components/viz/test/compositor_frame_helpers.h"
 #include "components/viz/test/fake_host_frame_sink_client.h"
 #include "components/viz/test/fake_surface_observer.h"
 #include "components/viz/test/mock_compositor_frame_sink_client.h"
 #include "components/viz/test/mock_display_client.h"
+#include "components/viz/test/test_frame_sink_manager.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/viz/privileged/mojom/compositing/frame_sink_manager.mojom.h"
+#include "services/viz/public/mojom/compositing/compositor_frame_sink.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -38,10 +37,6 @@ constexpr FrameSinkId kFrameSinkParent1(1, 1);
 constexpr FrameSinkId kFrameSinkParent2(2, 1);
 constexpr FrameSinkId kFrameSinkChild1(3, 1);
 
-ACTION_P(InvokeClosure, closure) {
-  closure.Run();
-}
-
 // Holds the four interface objects needed to create a RootCompositorFrameSink.
 struct RootCompositorFrameSinkData {
   mojom::RootCompositorFrameSinkParamsPtr BuildParams(
@@ -65,10 +60,9 @@ struct RootCompositorFrameSinkData {
 };
 
 // A mock implementation of mojom::FrameSinkManager.
-class MockFrameSinkManagerImpl : public FrameSinkManagerImpl {
+class MockFrameSinkManagerImpl : public TestFrameSinkManagerImpl {
  public:
-  explicit MockFrameSinkManagerImpl(SharedBitmapManager* shared_bitmap_manager)
-      : FrameSinkManagerImpl(shared_bitmap_manager) {}
+  MockFrameSinkManagerImpl() = default;
   ~MockFrameSinkManagerImpl() override = default;
 
   // mojom::FrameSinkManager:
@@ -78,15 +72,10 @@ class MockFrameSinkManagerImpl : public FrameSinkManagerImpl {
   MOCK_METHOD2(SetFrameSinkDebugLabel,
                void(const FrameSinkId& frame_sink_id,
                     const std::string& debug_label));
-  // Work around for gmock not supporting move-only types.
-  void CreateCompositorFrameSink(
-      const FrameSinkId& frame_sink_id,
-      mojo::PendingReceiver<mojom::CompositorFrameSink> receiver,
-      mojo::PendingRemote<mojom::CompositorFrameSinkClient> client) override {
-    MockCreateCompositorFrameSink(frame_sink_id);
-  }
-  MOCK_METHOD1(MockCreateCompositorFrameSink,
-               void(const FrameSinkId& frame_sink_id));
+  MOCK_METHOD3(CreateCompositorFrameSink,
+               void(const FrameSinkId&,
+                    mojo::PendingReceiver<mojom::CompositorFrameSink>,
+                    mojo::PendingRemote<mojom::CompositorFrameSinkClient>));
   void CreateRootCompositorFrameSink(
       mojom::RootCompositorFrameSinkParamsPtr params) override {
     MockCreateRootCompositorFrameSink(params->frame_sink_id);
@@ -105,9 +94,6 @@ class MockFrameSinkManagerImpl : public FrameSinkManagerImpl {
                void(const FrameSinkId& parent, const FrameSinkId& child));
   MOCK_METHOD2(UnregisterFrameSinkHierarchy,
                void(const FrameSinkId& parent, const FrameSinkId& child));
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockFrameSinkManagerImpl);
 };
 
 }  // namespace
@@ -151,8 +137,7 @@ class HostFrameSinkManagerTest : public testing::Test {
     DCHECK(!manager_impl_);
 
     manager_impl_ =
-        std::make_unique<testing::NiceMock<MockFrameSinkManagerImpl>>(
-            &shared_bitmap_manager_);
+        std::make_unique<testing::NiceMock<MockFrameSinkManagerImpl>>();
 
     mojo::PendingRemote<mojom::FrameSinkManager> frame_sink_manager;
     mojo::PendingReceiver<mojom::FrameSinkManager> frame_sink_manager_receiver =
@@ -166,13 +151,11 @@ class HostFrameSinkManagerTest : public testing::Test {
     host_manager_.BindAndSetManager(
         std::move(frame_sink_manager_client_receiver), nullptr,
         std::move(frame_sink_manager));
-    manager_impl_->BindAndSetClient(std::move(frame_sink_manager_receiver),
-                                    nullptr,
+    manager_impl_->BindReceiver(std::move(frame_sink_manager_receiver),
                                 std::move(frame_sink_manager_client));
   }
 
  protected:
-  ServerSharedBitmapManager shared_bitmap_manager_;
   HostFrameSinkManager host_manager_;
   std::unique_ptr<testing::NiceMock<MockFrameSinkManagerImpl>> manager_impl_;
 
@@ -194,7 +177,7 @@ TEST_F(HostFrameSinkManagerTest, CreateCompositorFrameSinks) {
 
   MockCompositorFrameSinkClient compositor_frame_sink_client;
   mojo::Remote<mojom::CompositorFrameSink> compositor_frame_sink;
-  EXPECT_CALL(impl(), MockCreateCompositorFrameSink(kFrameSinkChild1));
+  EXPECT_CALL(impl(), CreateCompositorFrameSink(kFrameSinkChild1, _, _));
   host().CreateCompositorFrameSink(
       kFrameSinkChild1, compositor_frame_sink.BindNewPipeAndPassReceiver(),
       compositor_frame_sink_client.BindInterfaceRemote());
@@ -373,7 +356,7 @@ TEST_F(HostFrameSinkManagerTest, RestartOnGpuCrash) {
 
   MockCompositorFrameSinkClient compositor_frame_sink_client;
   mojo::Remote<mojom::CompositorFrameSink> compositor_frame_sink;
-  EXPECT_CALL(impl(), MockCreateCompositorFrameSink(kFrameSinkChild1));
+  EXPECT_CALL(impl(), CreateCompositorFrameSink(kFrameSinkChild1, _, _));
   host().CreateCompositorFrameSink(
       kFrameSinkChild1, compositor_frame_sink.BindNewPipeAndPassReceiver(),
       compositor_frame_sink_client.BindInterfaceRemote());
@@ -481,7 +464,7 @@ TEST_F(HostFrameSinkManagerTest, ContextLossRecreateNonRoot) {
       compositor_frame_sink_client1.BindInterfaceRemote());
 
   // Verify CompositorFrameSink was created on other end of message pipe.
-  EXPECT_CALL(impl(), MockCreateCompositorFrameSink(kFrameSinkChild1));
+  EXPECT_CALL(impl(), CreateCompositorFrameSink(kFrameSinkChild1, _, _));
   FlushHostAndVerifyExpectations();
 
   // Create a new CompositorFrameSink and try to connect it with the same
@@ -494,7 +477,7 @@ TEST_F(HostFrameSinkManagerTest, ContextLossRecreateNonRoot) {
 
   // Verify CompositorFrameSink is destroyed and then recreated.
   EXPECT_CALL(impl(), MockDestroyCompositorFrameSink(kFrameSinkChild1));
-  EXPECT_CALL(impl(), MockCreateCompositorFrameSink(kFrameSinkChild1));
+  EXPECT_CALL(impl(), CreateCompositorFrameSink(kFrameSinkChild1, _, _));
   FlushHostAndVerifyExpectations();
 }