cc: Split ThreadProxy into ProxyMain and ProxyImpl

cc/test/layer_tree_test.cc

Index: cc/test/layer_tree_test.cc
diff --git a/cc/test/layer_tree_test.cc b/cc/test/layer_tree_test.cc
index 0578128532d1db41387f2f40ddb888f7e75674dd..c0250aa99a624100a231773720a8c7d859860f35 100644
--- a/cc/test/layer_tree_test.cc
+++ b/cc/test/layer_tree_test.cc
@@ -30,8 +30,10 @@
 #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"
 
@@ -112,143 +114,138 @@ void TestHooks::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,
@@ -256,68 +253,124 @@ class ThreadProxyForTest : public ThreadProxy {
                          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);
+  }
+
+  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));
+  }
+
+ 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));
   }
 
-  void InitializeImplOnImpl(CompletionEvent* completion,
-                            LayerTreeHost* layer_tree_host) override {
-    ThreadProxy::InitializeImplOnImpl(completion, layer_tree_host);
-    test_hooks_->InitializeImplOnImpl();
+  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)));
   }
 
-  void LayerTreeHostClosedOnImpl(CompletionEvent* completion) override {
-    test_hooks_->WillCloseLayerTreeHostOnImpl();
-    ThreadProxy::LayerTreeHostClosedOnImpl(completion);
+  ~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(
@@ -325,27 +378,28 @@ class ThreadProxyForTest : public ThreadProxy {
       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(
@@ -708,9 +762,14 @@ class LayerTreeHostForTesting : public LayerTreeHost {
         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(),
@@ -1161,4 +1220,21 @@ LayerTreeHost* LayerTreeTest::layer_tree_host() {
   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