Split ThreadProxy methods to ProxyMain and ProxyImpl.

cc/trees/thread_proxy.cc

Index: cc/trees/thread_proxy.cc
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index 8f23e5c96b7abab620e3071ff2cc1ca4b4aa6f40..1743142fecb72ab67a7ea46be77e6888bd652918 100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -139,11 +139,7 @@ void ThreadProxy::FinishAllRendering() {
   // Make sure all GL drawing is finished on the impl thread.
   DebugScopedSetMainThreadBlocked main_thread_blocked(this);
   CompletionEvent completion;
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::FinishAllRenderingOnImplThread,
-                 impl_thread_weak_ptr_,
-                 &completion));
+  main().channel_main->FinishAllRenderingOnImpl(&completion);

[vmpstr, 2015/10/15 20:25:04]

It's a bit awkward for me that channel_main refers to the channel that
communicates with the impl side. Is "main" referring to where the channel is
located, but not where it's communicating to?

[Khushal, 2015/10/15 23:45:51]

Since the class is split into the main and impl components, each side needs an
interface to communicate with the other side. The current naming scheme is such
that ChannelMain is the interface and its implementation is located on the main
side. For Blimp, a RemoteChannelMain will implement this that lives on the
server.

We can rename it channel_main_to_impl if that makes it clearer?

[vmpstr, 2015/10/17 00:05:51]

I'm not sure, that's also very verbose. I guess we can keep it as is for now,
but if you think of something better that would be good :P

I'm almost tempted to say that main().channel->Foo(); is better than
main().channel_main->Foo(). But I think that's not verbose enough.

[Khushal, 2015/10/17 02:02:48]

main() wouldn't be there when ProxyMain comes. Then it would just be
channel_main_->Foo(). We can may be name the variable channel_ then?
Can't think of anything better right now. :(

   completion.Wait();

[vmpstr, 2015/10/15 20:25:04]

The typical thing to do is create a completion / post task / wait.

What's the story for when this isn't a post task? Should the parameter to the
function be something like a bool wait_until_done; and it can create the
completion in the channel? Or do you think it's better to keep this outside?

[Khushal, 2015/10/15 23:45:50]

Right now completion events make it explicit to methods on both side that the
main thread is blocked for this operation. And in case of the commit, since it
is done asynchronously on the impl side, it holds on to the event to decide when
to release the main thread. To hide the completion events completely inside the
channel, we'll have to move some of that logic inside the channel.

Would it be better if we had CompletionEvent as an interface instead.

CompletionEvent {
  Wait();
  Signal();
}
And the current completion event can be the implementation of this as
ThreadedCompletionEvent.

We pass the wait_until_done param to a function on ChannelMain to make a
synchronous call, which for ThreadedChannel will create this event and pass it
to ProxyImpl so the logic there remains same as now.

For Blimp we just make the network call. It shouldn't need to block the main
thread on the server since any state needed in the call at that point is
serialized and sent to the client.
The completion event implementation sent to ProxyImpl on the client will
probably just have a callback that clears any local state after an operation.
For instance, dropping the commit proto/recycling the local LTH object after the
impl side has committed the state. Is there anything I could be missing here?

[vmpstr, 2015/10/17 00:05:51]

Specifically what logic are you referring to? I mostly see

CompletionEvent event;
channel->Foo(&event);
event.Wait();

On the other end this maps to
Foo(CompletionEvent* event) {
 ...
 event->Signal();
}

If the logic is more complicated than that, we can keep it outside or whatever.
But if it's exactly this pattern over and over again, I would say that the
channel itself can do this, no?
All the more reason here to move the details of completion events into the
channel, right? If they remain outside, then who would signal the completion?
The channel, right?

[Khushal, 2015/10/17 02:02:48]

I like the idea of keeping it completely in the channel. Something like:
channel->Foo(bool wait_until_done);

And in the channel,
Foo(bool wait_until_done) {
  CompletionEvent event;
  TaskRunner->PostTask(FROM_HERE,
      base::Bind(&ThreadedChannel::DoBlockingFoo,
                 channel_weak_ptr_,
                 &completion);
  event.Wait();
}

DoBlockingFoo(CompletionEvent* event) {
  proxy_impl_->Foo();
  event->Signal();
}

With the commit, thread proxy holds on to the event to unblock it after the
commit is finished/after the sync tree is activated.
We could add callbacks for to the channel to move this logic into the channel?

channel_impl->DidActivateSyncTree() which would unblock the main thread?

 }
 
@@ -163,17 +159,11 @@ void ThreadProxy::SetVisible(bool visible) {
   DebugScopedSetMainThreadBlocked main_thread_blocked(this);
 
   CompletionEvent completion;
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::SetVisibleOnImplThread,
-                 impl_thread_weak_ptr_,
-                 &completion,
-                 visible));
+  main().channel_main->SetVisibleOnImpl(&completion, visible);
   completion.Wait();
 }
 
-void ThreadProxy::SetVisibleOnImplThread(CompletionEvent* completion,
-                                         bool visible) {
+void ThreadProxy::SetVisibleOnImpl(CompletionEvent* completion, bool visible) {
   TRACE_EVENT1("cc", "ThreadProxy::SetVisibleOnImplThread", "visible", visible);
   impl().layer_tree_host_impl->SetVisible(visible);
   impl().scheduler->SetVisible(visible);
@@ -204,9 +194,7 @@ void ThreadProxy::RequestNewOutputSurface() {
 }
 
 void ThreadProxy::SetOutputSurface(OutputSurface* output_surface) {
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&ThreadProxy::InitializeOutputSurfaceOnImplThread,
-                            impl_thread_weak_ptr_, output_surface));
+  main().channel_main->InitializeOutputSurfaceOnImpl(output_surface);
 }
 
 void ThreadProxy::ReleaseOutputSurface() {
@@ -215,9 +203,7 @@ void ThreadProxy::ReleaseOutputSurface() {
 
   DebugScopedSetMainThreadBlocked main_thread_blocked(this);
   CompletionEvent completion;
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&ThreadProxy::ReleaseOutputSurfaceOnImplThread,
-                            impl_thread_weak_ptr_, &completion));
+  main().channel_main->ReleaseOutputSurfaceOnImpl(&completion);
   completion.Wait();
 }
 
@@ -235,7 +221,7 @@ void ThreadProxy::DidInitializeOutputSurface(
   layer_tree_host()->DidInitializeOutputSurface();
 }
 
-void ThreadProxy::SetRendererCapabilitiesMainThreadCopy(
+void ThreadProxy::SetRendererCapabilitiesMainCopy(
     const RendererCapabilities& capabilities) {
   main().renderer_capabilities_main_thread_copy = capabilities;
 }
@@ -308,21 +294,16 @@ void ThreadProxy::SetNeedsCommit() {
 
 void ThreadProxy::UpdateRendererCapabilitiesOnImplThread() {
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::SetRendererCapabilitiesMainThreadCopy,
-                 main_thread_weak_ptr_,
-                 impl()
-                     .layer_tree_host_impl->GetRendererCapabilities()
-                     .MainThreadCapabilities()));
+  impl().channel_impl->SetRendererCapabilitiesMainCopy(
+      impl()
+          .layer_tree_host_impl->GetRendererCapabilities()
+          .MainThreadCapabilities());
 }
 
 void ThreadProxy::DidLoseOutputSurfaceOnImplThread() {
   TRACE_EVENT0("cc", "ThreadProxy::DidLoseOutputSurfaceOnImplThread");
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::DidLoseOutputSurface, main_thread_weak_ptr_));
+  impl().channel_impl->DidLoseOutputSurface();
   impl().scheduler->DidLoseOutputSurface();
 }
 
@@ -402,11 +383,7 @@ void ThreadProxy::PostAnimationEventsToMainThreadOnImplThread(
   TRACE_EVENT0("cc",
                "ThreadProxy::PostAnimationEventsToMainThreadOnImplThread");
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::SetAnimationEvents,
-                 main_thread_weak_ptr_,
-                 base::Passed(&events)));
+  impl().channel_impl->SetAnimationEvents(events.Pass());
 }
 
 bool ThreadProxy::IsInsideDraw() { return impl().inside_draw; }
@@ -438,14 +415,10 @@ void ThreadProxy::SetDeferCommits(bool defer_commits) {
   else
     TRACE_EVENT_ASYNC_END0("cc", "ThreadProxy::SetDeferCommits", this);
 
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::SetDeferCommitsOnImplThread,
-                 impl_thread_weak_ptr_,
-                 defer_commits));
+  main().channel_main->SetDeferCommitsOnImpl(defer_commits);
 }
 
-void ThreadProxy::SetDeferCommitsOnImplThread(bool defer_commits) const {
+void ThreadProxy::SetDeferCommitsOnImpl(bool defer_commits) {
   DCHECK(IsImplThread());
   impl().scheduler->SetDeferCommits(defer_commits);
 }
@@ -488,27 +461,20 @@ void ThreadProxy::SetNeedsRedrawRectOnImplThread(const gfx::Rect& damage_rect) {
 
 void ThreadProxy::MainThreadHasStoppedFlinging() {
   DCHECK(IsMainThread());
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::MainThreadHasStoppedFlingingOnImplThread,
-                 impl_thread_weak_ptr_));
+  main().channel_main->MainThreadHasStoppedFlingingOnImpl();
 }
 
-void ThreadProxy::MainThreadHasStoppedFlingingOnImplThread() {
+void ThreadProxy::MainThreadHasStoppedFlingingOnImpl() {
   DCHECK(IsImplThread());
   impl().layer_tree_host_impl->MainThreadHasStoppedFlinging();
 }
 
 void ThreadProxy::NotifyInputThrottledUntilCommit() {
   DCHECK(IsMainThread());
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::SetInputThrottledUntilCommitOnImplThread,
-                 impl_thread_weak_ptr_,
-                 true));
+  main().channel_main->SetInputThrottledUntilCommitOnImpl(true);
 }
 
-void ThreadProxy::SetInputThrottledUntilCommitOnImplThread(bool is_throttled) {
+void ThreadProxy::SetInputThrottledUntilCommitOnImpl(bool is_throttled) {
   DCHECK(IsImplThread());
   if (is_throttled == impl().input_throttled_until_commit)
     return;
@@ -584,13 +550,8 @@ void ThreadProxy::Stop() {
   // renderer before shutting it down.
   {
     DebugScopedSetMainThreadBlocked main_thread_blocked(this);
-
     CompletionEvent completion;
-    Proxy::ImplThreadTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&ThreadProxy::FinishGLOnImplThread,
-                   impl_thread_weak_ptr_,
-                   &completion));
+    main().channel_main->FinishGLOnImpl(&completion);
     completion.Wait();
   }
   {
@@ -614,7 +575,7 @@ bool ThreadProxy::SupportsImplScrolling() const {
   return true;
 }
 
-void ThreadProxy::FinishAllRenderingOnImplThread(CompletionEvent* completion) {
+void ThreadProxy::FinishAllRenderingOnImpl(CompletionEvent* completion) {
   TRACE_EVENT0("cc", "ThreadProxy::FinishAllRenderingOnImplThread");
   DCHECK(IsImplThread());
   impl().layer_tree_host_impl->FinishAllRendering();
@@ -650,9 +611,7 @@ void ThreadProxy::ScheduledActionSendBeginMainFrame() {
 }
 
 void ThreadProxy::SendBeginMainFrameNotExpectedSoon() {
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&ThreadProxy::BeginMainFrameNotExpectedSoon,
-                            main_thread_weak_ptr_));
+  impl().channel_impl->BeginMainFrameNotExpectedSoon();
 }
 
 void ThreadProxy::BeginMainFrame(
@@ -816,7 +775,7 @@ void ThreadProxy::BeginMainFrameAbortedOnImplThread(
   DCHECK(!impl().layer_tree_host_impl->pending_tree());
 
   if (CommitEarlyOutHandledCommit(reason)) {
-    SetInputThrottledUntilCommitOnImplThread(false);
+    SetInputThrottledUntilCommitOnImpl(false);
     impl().last_processed_begin_main_frame_args =
         impl().last_begin_main_frame_args;
   }
@@ -865,7 +824,7 @@ void ThreadProxy::ScheduledActionCommit() {
   // often a good bit of work to update the tree and prepare the new frame.
   impl().layer_tree_host_impl->CommitComplete();
 
-  SetInputThrottledUntilCommitOnImplThread(false);
+  SetInputThrottledUntilCommitOnImpl(false);
 
   impl().next_frame_is_newly_committed_frame = true;
 }
@@ -879,9 +838,7 @@ void ThreadProxy::ScheduledActionActivateSyncTree() {
 void ThreadProxy::ScheduledActionBeginOutputSurfaceCreation() {
   TRACE_EVENT0("cc", "ThreadProxy::ScheduledActionBeginOutputSurfaceCreation");
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::RequestNewOutputSurface, main_thread_weak_ptr_));
+  impl().channel_impl->RequestNewOutputSurface();
 }
 
 DrawResult ThreadProxy::DrawSwapInternal(bool forced_draw) {
@@ -937,9 +894,7 @@ DrawResult ThreadProxy::DrawSwapInternal(bool forced_draw) {
   // Tell the main thread that the the newly-commited frame was drawn.
   if (impl().next_frame_is_newly_committed_frame) {
     impl().next_frame_is_newly_committed_frame = false;
-    Proxy::MainThreadTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&ThreadProxy::DidCommitAndDrawFrame, main_thread_weak_ptr_));
+    impl().channel_impl->DidCommitAndDrawFrame();
   }
 
   DCHECK_NE(INVALID_RESULT, result);
@@ -1033,8 +988,7 @@ void ThreadProxy::InitializeImplOnImplThread(CompletionEvent* completion) {
   completion->Signal();
 }
 
-void ThreadProxy::InitializeOutputSurfaceOnImplThread(
-    OutputSurface* output_surface) {
+void ThreadProxy::InitializeOutputSurfaceOnImpl(OutputSurface* output_surface) {
   TRACE_EVENT0("cc", "ThreadProxy::InitializeOutputSurfaceOnImplThread");
   DCHECK(IsImplThread());
 
@@ -1046,19 +1000,13 @@ void ThreadProxy::InitializeOutputSurfaceOnImplThread(
         host_impl->GetRendererCapabilities().MainThreadCapabilities();
   }
 
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::DidInitializeOutputSurface,
-                 main_thread_weak_ptr_,
-                 success,
-                 capabilities));
+  impl().channel_impl->DidInitializeOutputSurface(success, capabilities);
 
   if (success)
     impl().scheduler->DidCreateAndInitializeOutputSurface();
 }
 
-void ThreadProxy::ReleaseOutputSurfaceOnImplThread(
-    CompletionEvent* completion) {
+void ThreadProxy::ReleaseOutputSurfaceOnImpl(CompletionEvent* completion) {
   DCHECK(IsImplThread());
 
   // Unlike DidLoseOutputSurfaceOnImplThread, we don't need to call
@@ -1068,7 +1016,7 @@ void ThreadProxy::ReleaseOutputSurfaceOnImplThread(
   completion->Signal();
 }
 
-void ThreadProxy::FinishGLOnImplThread(CompletionEvent* completion) {
+void ThreadProxy::FinishGLOnImpl(CompletionEvent* completion) {
   TRACE_EVENT0("cc", "ThreadProxy::FinishGLOnImplThread");
   DCHECK(IsImplThread());
   if (impl().layer_tree_host_impl->output_surface()) {
@@ -1103,16 +1051,12 @@ ThreadProxy::BeginMainFrameAndCommitState::~BeginMainFrameAndCommitState() {}
 
 bool ThreadProxy::MainFrameWillHappenForTesting() {
   DCHECK(IsMainThread());
-  CompletionEvent completion;
   bool main_frame_will_happen = false;
   {
     DebugScopedSetMainThreadBlocked main_thread_blocked(this);
-    Proxy::ImplThreadTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&ThreadProxy::MainFrameWillHappenOnImplThreadForTesting,
-                   impl_thread_weak_ptr_,
-                   &completion,
-                   &main_frame_will_happen));
+    CompletionEvent completion;
+    main().channel_main->MainFrameWillHappenOnImplForTesting(
+        &completion, &main_frame_will_happen);
     completion.Wait();
   }
   return main_frame_will_happen;
@@ -1122,7 +1066,7 @@ void ThreadProxy::SetChildrenNeedBeginFrames(bool children_need_begin_frames) {
   NOTREACHED() << "Only used by SingleThreadProxy";
 }
 
-void ThreadProxy::MainFrameWillHappenOnImplThreadForTesting(
+void ThreadProxy::MainFrameWillHappenOnImplForTesting(
     CompletionEvent* completion,
     bool* main_frame_will_happen) {
   DCHECK(IsImplThread());
@@ -1217,9 +1161,7 @@ void ThreadProxy::DidPrepareTiles() {
 
 void ThreadProxy::DidCompletePageScaleAnimationOnImplThread() {
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&ThreadProxy::DidCompletePageScaleAnimation,
-                            main_thread_weak_ptr_));
+  impl().channel_impl->DidCompletePageScaleAnimation();
 }
 
 void ThreadProxy::OnDrawForOutputSurface() {
@@ -1231,14 +1173,11 @@ void ThreadProxy::PostFrameTimingEventsOnImplThread(
     scoped_ptr<FrameTimingTracker::CompositeTimingSet> composite_events,
     scoped_ptr<FrameTimingTracker::MainFrameTimingSet> main_frame_events) {
   DCHECK(IsImplThread());
-  Proxy::MainThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::PostFrameTimingEvents, main_thread_weak_ptr_,
-                 base::Passed(composite_events.Pass()),
-                 base::Passed(main_frame_events.Pass())));
+  impl().channel_impl->PostFrameTimingEventsOnMain(composite_events.Pass(),
+                                                   main_frame_events.Pass());
 }
 
-void ThreadProxy::PostFrameTimingEvents(
+void ThreadProxy::PostFrameTimingEventsOnMain(
     scoped_ptr<FrameTimingTracker::CompositeTimingSet> composite_events,
     scoped_ptr<FrameTimingTracker::MainFrameTimingSet> main_frame_events) {
   DCHECK(IsMainThread());