cc: Split ThreadProxy into ProxyMain and ProxyImpl

cc/trees/threaded_channel.h

Index: cc/trees/threaded_channel.h
diff --git a/cc/trees/threaded_channel.h b/cc/trees/threaded_channel.h
index ca7fc04c0a7319f5aa6ce7bfe0bc2ca4b0c102e1..ed5f2c2c70bd9002ffcd19d9bafe3b322ada06df 100644
--- a/cc/trees/threaded_channel.h
+++ b/cc/trees/threaded_channel.h
@@ -7,13 +7,13 @@
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "cc/base/cc_export.h"
 #include "cc/trees/channel_impl.h"
 #include "cc/trees/channel_main.h"
 #include "cc/trees/proxy_common.h"
 #include "cc/trees/proxy_impl.h"
 #include "cc/trees/proxy_main.h"
-#include "cc/trees/thread_proxy.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -22,9 +22,9 @@ class SingleThreadTaskRunner;
 namespace cc {
 class ChannelImpl;
 class ChannelMain;
+class LayerTreeHost;
 class ProxyImpl;
 class ProxyMain;
-class ThreadProxy;
 
 // An implementation of ChannelMain and ChannelImpl that sends commands between
 // ProxyMain and ProxyImpl across thread boundaries.
@@ -39,8 +39,8 @@ class ThreadProxy;
 //        ProxyMain->Start()          |
 //               |              ThreadedChannel
 // ---------------------------------------------------------------------------
-//  ChannelMain::InitializeImpl ---PostTask---> ThreadedChannel::
-//                                                   InitializeImplOnImplThread
+//  ChannelMain::SynchronouslyInitializeImpl ---PostTask---> ThreadedChannel::
+//                                                    InitializeImplOnImpl
 //                                                          |
 //                                                   ProxyImpl::Create
 //                                                          |
@@ -56,20 +56,27 @@ class ThreadProxy;
 // ProxyMain->RequestNewOutputSurface()<----PostTask--------
 //              .
 //              .
-// ProxyMain->LayerTreeHostClosed
+// ProxyMain->Stop()
 //              |
 // ---------------------------------------------------------------------------
-// ChannelMain::SetLayerTreeClosedOnImpl---PostTask---> ProxyImpl->
-//                                                        SetLayerTreeClosed
+// ChannelMain::SynchronouslyCloseImpl ---PostTask---> ThreadedChannel::
+//                                                      CloseImplOnImpl
 // ----------------------------------------------------------------------------
+//
+// ThreadedChannel is created and destroyed on the main thread but can be
+// called from main or impl thread. It is safe for the Threadedchannel to be
+// called on the impl thread because:
+// 1) The only impl-threaded callers of ThreadedChannel are the ThreadedChannel
+// itself and ProxyImpl which is created and owned by the ThreadedChannel.
+// 2) ThreadedChannel blocks the main thread in
+// ThreadedChannel::SynchronouslyCloseImpl to wait for the impl-thread teardown
+// to complete, so there is no risk of any queued tasks calling it on the impl
+// thread after it has been deleted on the main thread.
 
 class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
  public:
   static scoped_ptr<ThreadedChannel> Create(
-      // TODO(khushalsagar): Make this ProxyMain* and write the initialization
-      // sequence. Currently ThreadProxy implements both so we pass the pointer
-      // and set ProxyImpl.
-      ThreadProxy* thread_proxy,
+      ProxyMain* proxy_main,
       TaskRunnerProvider* task_runner_provider);
 
   ~ThreadedChannel() override;
@@ -93,7 +100,6 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
   // Blocking calls to ProxyImpl
   void FinishAllRenderingOnImpl(CompletionEvent* completion) override;
   void ReleaseOutputSurfaceOnImpl(CompletionEvent* completion) override;
-  void FinishGLOnImpl(CompletionEvent* completion) override;
   void MainFrameWillHappenOnImplForTesting(
       CompletionEvent* completion,
       bool* main_frame_will_happen) override;
@@ -101,9 +107,10 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
                          LayerTreeHost* layer_tree_host,
                          base::TimeTicks main_thread_start_time,
                          bool hold_commit_for_activation) override;
-  void InitializeImplOnImpl(CompletionEvent* completion,
-                            LayerTreeHost* layer_tree_host) override;
-  void LayerTreeHostClosedOnImpl(CompletionEvent* completion) override;
+  void SynchronouslyInitializeImpl(
+      LayerTreeHost* layer_tree_host,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source) override;
+  void SynchronouslyCloseImpl() override;
 
   // ChannelImpl Implementation
   void DidCompleteSwapBuffers() override;
@@ -125,23 +132,75 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
   void BeginMainFrame(
       scoped_ptr<BeginMainFrameAndCommitState> begin_main_frame_state) override;
 
+  // Should be called on impl thread only.
+  ProxyImpl* GetProxyImplForTesting() const;
+
  protected:
-  ThreadedChannel(ThreadProxy* thread_proxy,
+  ThreadedChannel(ProxyMain* proxy_main,
                   TaskRunnerProvider* task_runner_provider);
 
+  // Virtual for testing.
+  virtual scoped_ptr<ProxyImpl> CreateProxyImpl(
+      ChannelImpl* channel_impl,
+      LayerTreeHost* layer_tree_host,
+      TaskRunnerProvider* task_runner_provider,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source);
+
  private:
-  base::SingleThreadTaskRunner* MainThreadTaskRunner() const;
-  base::SingleThreadTaskRunner* ImplThreadTaskRunner() const;
+  // The members of this struct should be accessed on the main thread only.
+  struct MainThreadOnly {
+    base::WeakPtrFactory<ProxyMain> proxy_main_weak_factory;
+    bool initialized;
+
+    explicit MainThreadOnly(ProxyMain* proxy_main);
+    ~MainThreadOnly();
+  };
+
+  // The members of this struct should be accessed on the impl thread only.
+  struct CompositorThreadOnly {
+    scoped_ptr<ProxyImpl> proxy_impl;
+    scoped_ptr<base::WeakPtrFactory<ProxyImpl>> proxy_impl_weak_factory;

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

Can you leave a comment here explaining that this is initialized after
construction (hence scoped ptr to weak ptr factor) and also explaining why the
weak ptr factory is here and not in proxy impl.

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

Done.

+
+    // Used on the impl thread to queue calls to ProxyMain to be run on the main
+    // thread. Since the weak pointer is invalidated after the impl-thread tear
+    // down in SynchronouslyCloseImpl, this ensures that any tasks posted to
+    // ProxyMain from the impl thread are abandoned after the impl side has been
+    // destroyed.
+    base::WeakPtr<ProxyMain> proxy_main_weak_ptr;
+
+    explicit CompositorThreadOnly(base::WeakPtr<ProxyMain> proxy_main_weak_ptr);

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

Move these to the top please

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

Done.

+    ~CompositorThreadOnly();
+  };
+
+  // Called on impl thread.
+  void InitializeImplOnImpl(
+      CompletionEvent* completion,
+      LayerTreeHost* layer_tree_host,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source);
+  void CloseImplOnImpl(CompletionEvent* completion);
 
-  ProxyMain* proxy_main_;
+  bool IsInitialized() const;
 
-  ProxyImpl* proxy_impl_;
+  base::SingleThreadTaskRunner* MainThreadTaskRunner() const;
+  base::SingleThreadTaskRunner* ImplThreadTaskRunner() const;
+  bool IsMainThread() const;
+  bool IsImplThread() const;
 
   TaskRunnerProvider* task_runner_provider_;
 
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  // Use accessors instead of this variable directly.
+  MainThreadOnly main_thread_only_vars_unsafe_;
+  MainThreadOnly& main();
+  const MainThreadOnly& main() const;

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

I understand the logical grouping, but I think we should still have functions
before variables. It makes it easier to find functions.

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

Done.

+
+  // Use accessors instead of this variable directly.
+  CompositorThreadOnly compositor_thread_vars_unsafe_;
+  CompositorThreadOnly& impl();
+  const CompositorThreadOnly& impl() const;
 
-  scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner_;
+  // Used on the main thread to safely queue calls to ProxyImpl to be run on the
+  // impl thread.
+  base::WeakPtr<ProxyImpl> proxy_impl_weak_ptr_;
 
   DISALLOW_COPY_AND_ASSIGN(ThreadedChannel);
 };