Verify FrameSinkIds are registered.

Add a DCHECK to enforce that FrameSinkIds are registered before adding
them to hierarchy. This is already the case for all usage, except one
test which I fixed, but this will enforce it doesn't get broken.

Also remove ImageTransportFactory initialization from
OffscreenCanvasProviderImplTests. It's not actually used.

Bug: 657959
Change-Id: Ie197b655ecb1b1eabb5da56ca92fd590e2dfa450
Reviewed-on: https://chromium-review.googlesource.com/738284Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512508}

components/viz/host/host_frame_sink_manager.cc

@@ -133,10 +133,12 @@ void HostFrameSinkManager::RegisterFrameSinkHierarchy(
                                                   child_frame_sink_id);
 
   FrameSinkData& child_data = frame_sink_data_map_[child_frame_sink_id];
+  DCHECK(child_data.IsFrameSinkRegistered());
   DCHECK(!base::ContainsValue(child_data.parents, parent_frame_sink_id));
   child_data.parents.push_back(parent_frame_sink_id);
 
   FrameSinkData& parent_data = frame_sink_data_map_[parent_frame_sink_id];
+  DCHECK(parent_data.IsFrameSinkRegistered());
   DCHECK(!base::ContainsValue(parent_data.children, child_frame_sink_id));
   parent_data.children.push_back(child_frame_sink_id);
 }

components/viz/host/host_frame_sink_manager.h

@@ -101,7 +101,8 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
                                  mojom::CompositorFrameSinkRequest request,
                                  mojom::CompositorFrameSinkClientPtr client);
 
-  // Registers frame sink hierarchy. A frame sink can have multiple parents.
+  // Registers frame sink hierarchy. Both parent and child FrameSinkIds must be
+  // registered before calling. A frame sink can have multiple parents.
   void RegisterFrameSinkHierarchy(const FrameSinkId& parent_frame_sink_id,
                                   const FrameSinkId& child_frame_sink_id);
 

components/viz/host/host_frame_sink_manager_unittest.cc

@@ -16,6 +16,7 @@
 #include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
 #include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
 #include "components/viz/service/surfaces/surface_manager.h"
+#include "components/viz/test/fake_host_frame_sink_client.h"
 #include "components/viz/test/mock_compositor_frame_sink_client.h"
 #include "services/viz/privileged/interfaces/compositing/frame_sink_manager.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -46,19 +47,6 @@ SurfaceInfo MakeSurfaceInfo(const SurfaceId& surface_id) {
   return SurfaceInfo(surface_id, 1.f, gfx::Size(1, 1));
 }
 
-// A fake (do-nothing) implementation of HostFrameSinkClient.
-class FakeHostFrameSinkClient : public HostFrameSinkClient {
- public:
-  FakeHostFrameSinkClient() = default;
-  ~FakeHostFrameSinkClient() override = default;
-
-  // HostFrameSinkClient implementation.
-  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FakeHostFrameSinkClient);
-};
-
 // A mock implementation of mojom::FrameSinkManager.
 class MockFrameSinkManagerImpl : public FrameSinkManagerImpl {
  public:

components/viz/test/BUILD.gn

@@ -19,6 +19,8 @@ viz_static_library("test_support") {
     "fake_delay_based_time_source.h",
     "fake_external_begin_frame_source.cc",
     "fake_external_begin_frame_source.h",
+    "fake_host_frame_sink_client.cc",
+    "fake_host_frame_sink_client.h",
     "fake_surface_observer.cc",
     "fake_surface_observer.h",
     "mock_compositor_frame_sink_client.cc",
@@ -41,6 +43,7 @@ viz_static_library("test_support") {
     "//base",
     "//base/test:test_support",
     "//cc",
+    "//components/viz/host",
     "//components/viz/service",
     "//services/viz/privileged/interfaces/compositing",
     "//testing/gmock",

components/viz/test/fake_host_frame_sink_client.cc

+// 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 "components/viz/test/fake_host_frame_sink_client.h"
+
+namespace viz {
+
+FakeHostFrameSinkClient::FakeHostFrameSinkClient() = default;
+
+FakeHostFrameSinkClient::~FakeHostFrameSinkClient() = default;
+
+}  // namespace viz

components/viz/test/fake_host_frame_sink_client.h

+// 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.
+
+#ifndef COMPONENTS_VIZ_TEST_FAKE_HOST_FRAME_SINK_CLIENT_H_
+#define COMPONENTS_VIZ_TEST_FAKE_HOST_FRAME_SINK_CLIENT_H_
+
+#include "base/macros.h"
+#include "components/viz/common/surfaces/surface_info.h"
+#include "components/viz/host/host_frame_sink_client.h"
+
+namespace viz {
+
+// HostFrameSinkClient implementation that does nothing.
+class FakeHostFrameSinkClient : public HostFrameSinkClient {
+ public:
+  FakeHostFrameSinkClient();
+  ~FakeHostFrameSinkClient() override;
+
+  // HostFrameSinkClient implementation.
+  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakeHostFrameSinkClient);
+};
+
+}  // namespace viz
+
+#endif  // COMPONENTS_VIZ_TEST_FAKE_HOST_FRAME_SINK_CLIENT_H_

content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc

@@ -12,9 +12,11 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "components/viz/common/quads/compositor_frame.h"
+#include "components/viz/host/host_frame_sink_manager.h"
+#include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
+#include "components/viz/test/fake_host_frame_sink_client.h"
 #include "components/viz/test/mock_compositor_frame_sink_client.h"
 #include "content/browser/compositor/surface_utils.h"
-#include "content/browser/compositor/test/no_transport_image_transport_factory.h"
 #include "content/browser/renderer_host/offscreen_canvas_surface_impl.h"
 #include "services/viz/public/interfaces/compositing/compositor_frame_sink.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -120,10 +122,6 @@ class OffscreenCanvasProviderImplTest : public testing::Test {
 
  protected:
   void SetUp() override {
-#if !defined(OS_ANDROID)
-    ImageTransportFactory::SetFactory(
-        std::make_unique<NoTransportImageTransportFactory>());
-#endif
     host_frame_sink_manager_ = std::make_unique<viz::HostFrameSinkManager>();
 
     // The FrameSinkManagerImpl implementation is in-process here for tests.
@@ -133,20 +131,21 @@ class OffscreenCanvasProviderImplTest : public testing::Test {
 
     provider_ = std::make_unique<OffscreenCanvasProviderImpl>(
         host_frame_sink_manager_.get(), kRendererClientId);
+
+    host_frame_sink_manager_->RegisterFrameSinkId(kFrameSinkParent,
+                                                  &host_frame_sink_client_);
   }
   void TearDown() override {
     provider_.reset();
     frame_sink_manager_.reset();
     host_frame_sink_manager_.reset();
-#if !defined(OS_ANDROID)
-    ImageTransportFactory::Terminate();
-#endif
   }
 
  private:
   // A MessageLoop is required for mojo bindings which are used to
   // connect to graphics services.
   base::MessageLoop message_loop_;
+  viz::FakeHostFrameSinkClient host_frame_sink_client_;
   std::unique_ptr<viz::HostFrameSinkManager> host_frame_sink_manager_;
   std::unique_ptr<viz::FrameSinkManagerImpl> frame_sink_manager_;
   std::unique_ptr<OffscreenCanvasProviderImpl> provider_;