cc: Split ThreadProxy into ProxyMain and ProxyImpl

cc/test/layer_tree_test.cc

 // Copyright 2011 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 "cc/test/layer_tree_test.h"
 
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 12 matching lines @@
 #include "cc/test/fake_layer_tree_host_client.h"
 #include "cc/test/fake_output_surface.h"
 #include "cc/test/test_context_provider.h"
 #include "cc/test/test_gpu_memory_buffer_manager.h"
 #include "cc/test/test_shared_bitmap_manager.h"
 #include "cc/test/test_task_graph_runner.h"
 #include "cc/trees/layer_tree_host_client.h"
 #include "cc/trees/layer_tree_host_impl.h"
 #include "cc/trees/layer_tree_host_single_thread_client.h"
 #include "cc/trees/layer_tree_impl.h"
+#include "cc/trees/proxy_impl.h"
+#include "cc/trees/proxy_main.h"
 #include "cc/trees/single_thread_proxy.h"
-#include "cc/trees/thread_proxy.h"
+#include "cc/trees/threaded_channel.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
 namespace cc {
 
 void CreateVirtualViewportLayers(Layer* root_layer,
                                  scoped_refptr<Layer> outer_scroll_layer,
                                  const gfx::Size& inner_bounds,
                                  const gfx::Size& outer_bounds,
                                  LayerTreeHost* host,
@@ skipping 60 matching lines @@
 }
 
 void TestHooks::CreateResourceAndTileTaskWorkerPool(
     LayerTreeHostImpl* host_impl,
     scoped_ptr<TileTaskWorkerPool>* tile_task_worker_pool,
     scoped_ptr<ResourcePool>* resource_pool) {
   host_impl->LayerTreeHostImpl::CreateResourceAndTileTaskWorkerPool(
       tile_task_worker_pool, resource_pool);
 }
 
-// Adapts ThreadProxy for test. Injects test hooks for testing.
-class ThreadProxyForTest : public ThreadProxy {
+// Creates a ProxyImpl that notifies the supplied |test_hooks| of various
+// actions.
+class ProxyImplForTest : public ProxyImpl {

[vmpstr, 2015/12/04 00:25:40]

Maybe just TestProxyImpl?

[Khushal, 2015/12/04 22:45:22]

I kept it consistent with the current naming scheme for
SingleThreadProxyForTest. If we are renaming this, then how about XForTesting
instead for all classes here, to match the LayerTreeHostForTesting, etc. Looks
like generally inherited classes for tests in cc code are suffixed with
ForTesting.

[vmpstr, 2015/12/07 22:14:42]

Ok, let's keep it as it is for now. There's way to make FakeBlah, TestBlah, and
BlahForTesting to really decide and change in this CL.

  public:
-  static scoped_ptr<Proxy> Create(
+  static scoped_ptr<ProxyImpl> Create(
       TestHooks* test_hooks,
-      LayerTreeHost* host,
+      ChannelImpl* channel_impl,
+      LayerTreeHost* layer_tree_host,
       TaskRunnerProvider* task_runner_provider,
       scoped_ptr<BeginFrameSource> external_begin_frame_source) {
-    return make_scoped_ptr(
-        new ThreadProxyForTest(test_hooks, host, task_runner_provider,
-                               std::move(external_begin_frame_source)));
+    return make_scoped_ptr(new ProxyImplForTest(
+        test_hooks, channel_impl, layer_tree_host, task_runner_provider,
+        std::move(external_begin_frame_source)));
   }
 
-  ~ThreadProxyForTest() override {}
-
  private:
   TestHooks* test_hooks_;
 
-  void SetNeedsUpdateLayers() override {
-    ThreadProxy::SetNeedsUpdateLayers();
-    test_hooks_->DidSetNeedsUpdateLayers();
-  }
-
   void ScheduledActionSendBeginMainFrame(const BeginFrameArgs& args) override {
     test_hooks_->ScheduledActionWillSendBeginMainFrame();
-    ThreadProxy::ScheduledActionSendBeginMainFrame(args);
+    ProxyImpl::ScheduledActionSendBeginMainFrame(args);
     test_hooks_->ScheduledActionSendBeginMainFrame();
   }
 
   DrawResult ScheduledActionDrawAndSwapIfPossible() override {
-    DrawResult result = ThreadProxy::ScheduledActionDrawAndSwapIfPossible();
+    DrawResult result = ProxyImpl::ScheduledActionDrawAndSwapIfPossible();
     test_hooks_->ScheduledActionDrawAndSwapIfPossible();
     return result;
   }
 
   void ScheduledActionAnimate() override {
-    ThreadProxy::ScheduledActionAnimate();
+    ProxyImpl::ScheduledActionAnimate();
     test_hooks_->ScheduledActionAnimate();
   }
 
   void ScheduledActionCommit() override {
-    ThreadProxy::ScheduledActionCommit();
+    ProxyImpl::ScheduledActionCommit();
     test_hooks_->ScheduledActionCommit();
   }
 
   void ScheduledActionBeginOutputSurfaceCreation() override {
-    ThreadProxy::ScheduledActionBeginOutputSurfaceCreation();
+    ProxyImpl::ScheduledActionBeginOutputSurfaceCreation();
     test_hooks_->ScheduledActionBeginOutputSurfaceCreation();
   }
 
   void ScheduledActionPrepareTiles() override {
-    ThreadProxy::ScheduledActionPrepareTiles();
+    ProxyImpl::ScheduledActionPrepareTiles();
     test_hooks_->ScheduledActionPrepareTiles();
   }
 
   void ScheduledActionInvalidateOutputSurface() override {
-    ThreadProxy::ScheduledActionInvalidateOutputSurface();
+    ProxyImpl::ScheduledActionInvalidateOutputSurface();
     test_hooks_->ScheduledActionInvalidateOutputSurface();
   }
 
   void SendBeginMainFrameNotExpectedSoon() override {
-    ThreadProxy::SendBeginMainFrameNotExpectedSoon();
+    ProxyImpl::SendBeginMainFrameNotExpectedSoon();
     test_hooks_->SendBeginMainFrameNotExpectedSoon();
   }
 
   void DidActivateSyncTree() override {
-    ThreadProxy::DidActivateSyncTree();
+    ProxyImpl::DidActivateSyncTree();
     test_hooks_->DidActivateSyncTree();
   }
 
   void SetThrottleFrameProductionOnImpl(bool throttle) override {
     test_hooks_->SetThrottleFrameProductionOnImpl(throttle);
-    ThreadProxy::SetThrottleFrameProductionOnImpl(throttle);
+    ProxyImpl::SetThrottleFrameProductionOnImpl(throttle);
   }
 
   void InitializeOutputSurfaceOnImpl(OutputSurface* output_surface) override {
     test_hooks_->InitializeOutputSurfaceOnImpl(output_surface);
-    ThreadProxy::InitializeOutputSurfaceOnImpl(output_surface);
+    ProxyImpl::InitializeOutputSurfaceOnImpl(output_surface);
   }
 
   void MainThreadHasStoppedFlingingOnImpl() override {
     test_hooks_->MainThreadHasStoppedFlingingOnImpl();
-    ThreadProxy::MainThreadHasStoppedFlingingOnImpl();
+    ProxyImpl::MainThreadHasStoppedFlingingOnImpl();
   }
 
   void SetInputThrottledUntilCommitOnImpl(bool is_throttled) override {
     test_hooks_->SetInputThrottledUntilCommitOnImpl(is_throttled);
-    ThreadProxy::SetInputThrottledUntilCommitOnImpl(is_throttled);
+    ProxyImpl::SetInputThrottledUntilCommitOnImpl(is_throttled);
   }
 
   void UpdateTopControlsStateOnImpl(TopControlsState constraints,
                                     TopControlsState current,
                                     bool animate) override {
     test_hooks_->UpdateTopControlsStateOnImpl(constraints, current, animate);
-    ThreadProxy::UpdateTopControlsStateOnImpl(constraints, current, animate);
+    ProxyImpl::UpdateTopControlsStateOnImpl(constraints, current, animate);
   }
 
   void SetDeferCommitsOnImpl(bool defer_commits) const override {
     test_hooks_->SetDeferCommitsOnImpl(defer_commits);
-    ThreadProxy::SetDeferCommitsOnImpl(defer_commits);
+    ProxyImpl::SetDeferCommitsOnImpl(defer_commits);
   }
 
   void BeginMainFrameAbortedOnImpl(
       CommitEarlyOutReason reason,
       base::TimeTicks main_thread_start_time) override {
     test_hooks_->BeginMainFrameAbortedOnImpl(reason);
-    ThreadProxy::BeginMainFrameAbortedOnImpl(reason, main_thread_start_time);
+    ProxyImpl::BeginMainFrameAbortedOnImpl(reason, main_thread_start_time);
   }
 
   void SetNeedsRedrawOnImpl(const gfx::Rect& damage_rect) override {
     test_hooks_->SetNeedsRedrawOnImpl(damage_rect);
-    ThreadProxy::SetNeedsRedrawOnImpl(damage_rect);
+    ProxyImpl::SetNeedsRedrawOnImpl(damage_rect);
   };
 
   void SetNeedsCommitOnImpl() override {
     test_hooks_->SetNeedsCommitOnImpl();
-    ThreadProxy::SetNeedsCommitOnImpl();
+    ProxyImpl::SetNeedsCommitOnImpl();
   }
 
   void FinishAllRenderingOnImpl(CompletionEvent* completion) override {
     test_hooks_->FinishAllRenderingOnImpl();
-    ThreadProxy::FinishAllRenderingOnImpl(completion);
+    ProxyImpl::FinishAllRenderingOnImpl(completion);
   };
 
   void SetVisibleOnImpl(bool visible) override {
     test_hooks_->SetVisibleOnImpl(visible);
-    ThreadProxy::SetVisibleOnImpl(visible);
+    ProxyImpl::SetVisibleOnImpl(visible);
   }
 
   void ReleaseOutputSurfaceOnImpl(CompletionEvent* completion) override {
     test_hooks_->ReleaseOutputSurfaceOnImpl();
-    ThreadProxy::ReleaseOutputSurfaceOnImpl(completion);
+    ProxyImpl::ReleaseOutputSurfaceOnImpl(completion);
   }
 
   void FinishGLOnImpl(CompletionEvent* completion) override {
     test_hooks_->FinishGLOnImpl();
-    ThreadProxy::FinishGLOnImpl(completion);
+    ProxyImpl::FinishGLOnImpl(completion);
   }
 
   void StartCommitOnImpl(CompletionEvent* completion,
                          LayerTreeHost* layer_tree_host,
                          base::TimeTicks main_thread_start_time,
                          bool hold_commit_for_activation) override {
     test_hooks_->StartCommitOnImpl();
-    ThreadProxy::StartCommitOnImpl(completion, layer_tree_host,
-                                   main_thread_start_time,
-                                   hold_commit_for_activation);
+    ProxyImpl::StartCommitOnImpl(completion, layer_tree_host,
+                                 main_thread_start_time,
+                                 hold_commit_for_activation);
   }
 
-  void InitializeImplOnImpl(CompletionEvent* completion,
-                            LayerTreeHost* layer_tree_host) override {
-    ThreadProxy::InitializeImplOnImpl(completion, layer_tree_host);
-    test_hooks_->InitializeImplOnImpl();
+  ProxyImplForTest(TestHooks* test_hooks,

[vmpstr, 2015/12/04 00:25:40]

Can you move this to the top of the private block

[Khushal, 2015/12/04 22:45:22]

Done.

+                   ChannelImpl* channel_impl,
+                   LayerTreeHost* layer_tree_host,
+                   TaskRunnerProvider* task_runner_provider,
+                   scoped_ptr<BeginFrameSource> external_begin_frame_source)
+      : ProxyImpl(channel_impl,
+                  layer_tree_host,
+                  task_runner_provider,
+                  std::move(external_begin_frame_source)),
+        test_hooks_(test_hooks) {}
+};
+
+// ThreadedChannel that notifies |test_hooks| of internal actions by ProxyImpl.
+class ThreadedChannelForTest : public ThreadedChannel {
+ public:
+  static scoped_ptr<ThreadedChannel> Create(
+      TestHooks* test_hooks,
+      ProxyMain* proxy_main,
+      TaskRunnerProvider* task_runner_provider) {
+    return make_scoped_ptr(new ThreadedChannelForTest(test_hooks, proxy_main,
+                                                      task_runner_provider));
   }
 
-  void LayerTreeHostClosedOnImpl(CompletionEvent* completion) override {
-    test_hooks_->WillCloseLayerTreeHostOnImpl();
-    ThreadProxy::LayerTreeHostClosedOnImpl(completion);
+ private:
+  TestHooks* test_hooks_;
+
+  scoped_ptr<ProxyImpl> CreateProxyImpl(
+      ChannelImpl* channel_impl,
+      LayerTreeHost* layer_tree_host,
+      TaskRunnerProvider* task_runner_provider,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source) override {
+    return ProxyImplForTest::Create(test_hooks_, channel_impl, layer_tree_host,
+                                    task_runner_provider,
+                                    std::move(external_begin_frame_source));
+  }
+
+  ThreadedChannelForTest(TestHooks* test_hooks,
+                         ProxyMain* proxy_main,
+                         TaskRunnerProvider* task_runner_provider)
+      : ThreadedChannel(proxy_main, task_runner_provider),
+        test_hooks_(test_hooks) {}
+};
+
+// Creates a ProxyMain that notifies the supplied |test_hooks| of various
+// actions.
+class ProxyMainForTest : public ProxyMain {
+ public:
+  static scoped_ptr<ProxyMain> Create(
+      TestHooks* test_hooks,
+      LayerTreeHost* host,
+      TaskRunnerProvider* task_runner_provider,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source) {
+    return make_scoped_ptr(
+        new ProxyMainForTest(test_hooks, host, task_runner_provider,
+                             std::move(external_begin_frame_source)));
+  }
+
+  ~ProxyMainForTest() override {}
+
+ private:
+  TestHooks* test_hooks_;

[vmpstr, 2015/12/04 00:25:40]

Can you put functions before variables

[Khushal, 2015/12/04 22:45:22]

Done.

+
+  void SetNeedsUpdateLayers() override {
+    ProxyMain::SetNeedsUpdateLayers();
+    test_hooks_->DidSetNeedsUpdateLayers();
   }
 
   void DidCompleteSwapBuffers() override {
     test_hooks_->ReceivedDidCompleteSwapBuffers();
-    ThreadProxy::DidCompleteSwapBuffers();
+    ProxyMain::DidCompleteSwapBuffers();
   }
 
   void SetRendererCapabilitiesMainCopy(
       const RendererCapabilities& capabilities) override {
     test_hooks_->ReceivedSetRendererCapabilitiesMainCopy(capabilities);
-    ThreadProxy::SetRendererCapabilitiesMainCopy(capabilities);
+    ProxyMain::SetRendererCapabilitiesMainCopy(capabilities);
   }
 
   void BeginMainFrameNotExpectedSoon() override {
     test_hooks_->ReceivedBeginMainFrameNotExpectedSoon();
-    ThreadProxy::BeginMainFrameNotExpectedSoon();
+    ProxyMain::BeginMainFrameNotExpectedSoon();
   }
 
   void DidCommitAndDrawFrame() override {
     test_hooks_->ReceivedDidCommitAndDrawFrame();
-    ThreadProxy::DidCommitAndDrawFrame();
+    ProxyMain::DidCommitAndDrawFrame();
   }
 
   void SetAnimationEvents(scoped_ptr<AnimationEventsVector> events) override {
     test_hooks_->ReceivedSetAnimationEvents();
-    ThreadProxy::SetAnimationEvents(std::move(events));
+    ProxyMain::SetAnimationEvents(std::move(events));
   }
 
   void DidLoseOutputSurface() override {
     test_hooks_->ReceivedDidLoseOutputSurface();
-    ThreadProxy::DidLoseOutputSurface();
+    ProxyMain::DidLoseOutputSurface();
   }
 
   void RequestNewOutputSurface() override {
     test_hooks_->ReceivedRequestNewOutputSurface();
-    ThreadProxy::RequestNewOutputSurface();
+    ProxyMain::RequestNewOutputSurface();
   }
 
   void DidInitializeOutputSurface(
       bool success,
       const RendererCapabilities& capabilities) override {
     test_hooks_->ReceivedDidInitializeOutputSurface(success, capabilities);
-    ThreadProxy::DidInitializeOutputSurface(success, capabilities);
+    ProxyMain::DidInitializeOutputSurface(success, capabilities);
   }
 
   void DidCompletePageScaleAnimation() override {
     test_hooks_->ReceivedDidCompletePageScaleAnimation();
-    ThreadProxy::DidCompletePageScaleAnimation();
+    ProxyMain::DidCompletePageScaleAnimation();
   }
 
   void PostFrameTimingEventsOnMain(
       scoped_ptr<FrameTimingTracker::CompositeTimingSet> composite_events,
       scoped_ptr<FrameTimingTracker::MainFrameTimingSet> main_frame_events)
       override {
     test_hooks_->ReceivedPostFrameTimingEventsOnMain();
-    ThreadProxy::PostFrameTimingEventsOnMain(std::move(composite_events),
-                                             std::move(main_frame_events));
+    ProxyMain::PostFrameTimingEventsOnMain(std::move(composite_events),
+                                           std::move(main_frame_events));
   }
 
   void BeginMainFrame(scoped_ptr<BeginMainFrameAndCommitState>
                           begin_main_frame_state) override {
     test_hooks_->ReceivedBeginMainFrame();
-    ThreadProxy::BeginMainFrame(std::move(begin_main_frame_state));
+    ProxyMain::BeginMainFrame(std::move(begin_main_frame_state));
   };
 
-  ThreadProxyForTest(TestHooks* test_hooks,
-                     LayerTreeHost* host,
-                     TaskRunnerProvider* task_runner_provider,
-                     scoped_ptr<BeginFrameSource> external_begin_frame_source)
-      : ThreadProxy(host,
-                    task_runner_provider,
-                    std::move(external_begin_frame_source)),
+  ProxyMainForTest(TestHooks* test_hooks,
+                   LayerTreeHost* host,
+                   TaskRunnerProvider* task_runner_provider,
+                   scoped_ptr<BeginFrameSource> external_begin_frame_source)
+      : ProxyMain(host,
+                  task_runner_provider,
+                  std::move(external_begin_frame_source)),
         test_hooks_(test_hooks) {}
 };
 
-// Adapts ThreadProxy for test. Injects test hooks for testing.
+// Creates a SingleThreadProxy that notifies the supplied |test_hooks| of
+// various actions.
 class SingleThreadProxyForTest : public SingleThreadProxy {
  public:
   static scoped_ptr<Proxy> Create(
       TestHooks* test_hooks,
       LayerTreeHost* host,
       LayerTreeHostSingleThreadClient* client,
       TaskRunnerProvider* task_runner_provider,
       scoped_ptr<BeginFrameSource> external_begin_frame_source) {
     return make_scoped_ptr(new SingleThreadProxyForTest(
         test_hooks, host, client, task_runner_provider,
@@ skipping 342 matching lines @@
     params.shared_bitmap_manager = shared_bitmap_manager;
     params.gpu_memory_buffer_manager = gpu_memory_buffer_manager;
     params.task_graph_runner = task_graph_runner;
     params.settings = &settings;
     scoped_ptr<LayerTreeHostForTesting> layer_tree_host(
         new LayerTreeHostForTesting(test_hooks, &params));
     scoped_ptr<TaskRunnerProvider> task_runner_provider =
         TaskRunnerProvider::Create(main_task_runner, impl_task_runner);
     scoped_ptr<Proxy> proxy;
     if (impl_task_runner.get()) {
-      proxy = ThreadProxyForTest::Create(
+      scoped_ptr<ProxyMain> proxy_main = ProxyMainForTest::Create(
           test_hooks, layer_tree_host.get(), task_runner_provider.get(),
           std::move(external_begin_frame_source));
+      scoped_ptr<ThreadedChannel> threaded_channel =
+          ThreadedChannelForTest::Create(test_hooks, proxy_main.get(),
+                                         task_runner_provider.get());
+      proxy_main->SetChannel(std::move(threaded_channel));

[vmpstr, 2015/12/04 00:25:40]

Can you inline the creation in the function? It doesn't really matter, but I
feel like it would look better:

proxy_main->SetChannel(ThreadedChannelForTest::Create(test_hooks,
proxy_main.get(), task_runner_provider.get()));

[Khushal, 2015/12/04 22:45:22]

Done.

+      proxy = std::move(proxy_main);
     } else {
       proxy = SingleThreadProxyForTest::Create(
           test_hooks, layer_tree_host.get(), client, task_runner_provider.get(),
           std::move(external_begin_frame_source));
     }
     layer_tree_host->InitializeForTesting(std::move(task_runner_provider),
                                           std::move(proxy));
     return layer_tree_host;
   }
 
@@ skipping 430 matching lines @@
   // We check for a null task_runner_provider here as we sometimes ask for the
   // layer tree host when the task_runner_provider does not exist, often for
   // checking settings after a test has completed. For example,
   // LTHPixelResourceTest::RunPixelResourceTest. See elsewhere in this file for
   // other examples.
   DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread() ||
          task_runner_provider()->IsMainThreadBlocked());
   return layer_tree_host_.get();
 }
 
+ProxyMain* LayerTreeTest::GetProxyMain() const {
+  DCHECK(HasImplThread());
+  return static_cast<ProxyMain*>(proxy());
+}
+
+ProxyImpl* LayerTreeTest::GetProxyImpl() const {

[vmpstr, 2015/12/04 00:25:40]

Should this return ProxyImplForTest, which can also expose whatever
functionality is needed for tests (such as some getters that are used in this
patch).

[Khushal, 2015/12/04 22:45:22]

Sure. It seemed to me that the test classes in this file were meant to be an
internal detail of the LayerTreeTest, which is why they are not declared in the
header either.

If we are exposing ProxyImplForTest to all the LayerTreeTests, should we do this
for ProxyMainForTest as well?

[vmpstr, 2015/12/07 22:14:42]

If that's helpful, we can do that. It's usually much better to expose an
internal test-only class in test-only code than to write test-only functions in
non-test-code. Especially if the test-only class is only exposing extra getters,
nothing more.

[Khushal, 2015/12/08 02:19:35]

Done.

+  DCHECK(HasImplThread());
+  ThreadedChannel* threaded_channel =
+      static_cast<ThreadedChannel*>(GetProxyMain()->channel_main());
+  ProxyImpl* proxy_impl = threaded_channel->GetProxyImplForTesting();
+
+  // We check for null ProxyImpl since ProxyImpl exists in the ThreadedChannel
+  // only after it is initialized.
+  DCHECK(proxy_impl);
+  return proxy_impl;
+}
+
 }  // namespace cc