cc: Split ThreadProxy into ProxyMain and ProxyImpl

cc/trees/proxy_impl.h

Index: cc/trees/proxy_impl.h
diff --git a/cc/trees/proxy_impl.h b/cc/trees/proxy_impl.h
index b2f4cd1da0f1cc7a29974509427453f08bdd9a5c..c1b3fe4bf5c074c9207f92c318c06b9dc1d85369 100644
--- a/cc/trees/proxy_impl.h
+++ b/cc/trees/proxy_impl.h
@@ -5,56 +5,189 @@
 #ifndef CC_TREES_PROXY_IMPL_H_
 #define CC_TREES_PROXY_IMPL_H_
 
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "cc/base/cc_export.h"
+#include "base/time/time.h"
+#include "cc/animation/animation_events.h"
 #include "cc/base/completion_event.h"
+#include "cc/base/delayed_unique_notifier.h"
 #include "cc/input/top_controls_state.h"
-#include "cc/output/output_surface.h"
 #include "cc/scheduler/commit_earlyout_reason.h"
-#include "cc/trees/proxy_common.h"
+#include "cc/scheduler/scheduler.h"
+#include "cc/trees/channel_impl.h"
+#include "cc/trees/layer_tree_host_impl.h"
 
 namespace cc {
-// TODO(khushalsagar): The impl side of ThreadProxy. It is currently defined as
-// an interface with the implementation provided by ThreadProxy and will be
-// made an independent class.
-// The methods added to this interface should only use the CompositorThreadOnly
-// variables from ThreadProxy.
-// See crbug/527200
-class CC_EXPORT ProxyImpl {
+
+class CC_EXPORT ProxyImpl : public NON_EXPORTED_BASE(LayerTreeHostImplClient),
+                            public NON_EXPORTED_BASE(SchedulerClient) {
+ public:
+  static scoped_ptr<ProxyImpl> Create(

[Wez, 2015/11/10 01:41:50]

Clarify ownership/lifetime requirements on the objects passed as bare pointers?

[Khushal, 2015/11/11 04:17:47]

Done.

+      ChannelImpl* channel_impl,
+      LayerTreeHost* layer_tree_host,
+      TaskRunnerProvider* task_runner_provider,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source);
+
+  ~ProxyImpl() override;
+
+  // Accessed on the impl thread when the main thread is blocked for a commit.

[Wez, 2015/11/10 01:41:51]

This comment is on a struct, not a member; it's not clear how the comment
applies to it, nor how the struct itself is intended to be used?

[Khushal, 2015/11/11 04:17:47]

I'm trying to follow a similar pattern to what we had with ThreadProxy's
MainThreadOnly and CompositorThreadOnly structs.

Modified the comment to make its purpose clearer. The class which uses the
struct ensures that it follows the guidelines of this comment by using an
accessor to limit access to the struct only when the main thread is blocked for
a commit.

+  struct BlockedMainCommitOnly {
+    BlockedMainCommitOnly();
+    ~BlockedMainCommitOnly();
+    LayerTreeHost* layer_tree_host;
+  };
+
+  // LayerTreeHostImplClient implementation
+  void UpdateRendererCapabilitiesOnImplThread() override;
+  void DidLoseOutputSurfaceOnImplThread() override;
+  void CommitVSyncParameters(base::TimeTicks timebase,
+                             base::TimeDelta interval) override;
+  void SetEstimatedParentDrawTime(base::TimeDelta draw_time) override;
+  void SetMaxSwapsPendingOnImplThread(int max) override;
+  void DidSwapBuffersOnImplThread() override;
+  void DidSwapBuffersCompleteOnImplThread() override;
+  void OnResourcelessSoftareDrawStateChanged(bool resourceless_draw) override;
+  void OnCanDrawStateChanged(bool can_draw) override;
+  void NotifyReadyToActivate() override;
+  void NotifyReadyToDraw() override;
+  // Please call these 3 functions through

[Wez, 2015/11/10 01:41:51]

nit: It's OK to mark the implementation of an interface that a class implements,
but which isn't really intended to be public, as private or protected (even
though the class inheritance must be public), to effectively document that
callers shouldn't be using it - would that be appropriate here?

[Khushal, 2015/11/11 04:17:47]

I think that might be suitable for both the LayerTreeHostImplClient and
SchedulerClient implementation, since neither of them are intended to be public.
For now, I've kept this consistent with the original implementation in
ThreadProxy. I'd like it if someone from the cc team could also review it.

[Wez, 2015/11/12 22:30:18]

SGTM

+  // LayerTreeHostImpl's SetNeedsRedraw(), SetNeedsRedrawRect() and
+  // SetNeedsAnimate().
+  void SetNeedsRedrawOnImplThread() override;
+  void SetNeedsRedrawRectOnImplThread(const gfx::Rect& dirty_rect) override;
+  void SetNeedsAnimateOnImplThread() override;
+  void SetNeedsPrepareTilesOnImplThread() override;
+  void SetNeedsCommitOnImplThread() override;
+  void SetVideoNeedsBeginFrames(bool needs_begin_frames) override;
+  void PostAnimationEventsToMainThreadOnImplThread(
+      scoped_ptr<AnimationEventsVector> queue) override;
+  bool IsInsideDraw() override;
+  void RenewTreePriority() override;
+  void PostDelayedAnimationTaskOnImplThread(const base::Closure& task,
+                                            base::TimeDelta delay) override;
+  void DidActivateSyncTree() override;
+  void WillPrepareTiles() override;
+  void DidPrepareTiles() override;
+  void DidCompletePageScaleAnimationOnImplThread() override;
+  void OnDrawForOutputSurface() override;
+  // This should only be called by LayerTreeHostImpl::PostFrameTimingEvents.

[Wez, 2015/11/10 01:41:51]

nit: Is that restriction specific to this implementation? If not then document
it on the interface?

[Khushal, 2015/11/11 04:17:47]

Kept this consistent with the original implementation in ThreadProxy. I am not
sure about this.

+  void PostFrameTimingEventsOnImplThread(
+      scoped_ptr<FrameTimingTracker::CompositeTimingSet> composite_events,
+      scoped_ptr<FrameTimingTracker::MainFrameTimingSet> main_frame_events)
+      override;
+
+  // SchedulerClient implementation
+  void WillBeginImplFrame(const BeginFrameArgs& args) override;
+  void DidFinishImplFrame() override;
+  void ScheduledActionSendBeginMainFrame() override;
+  DrawResult ScheduledActionDrawAndSwapIfPossible() override;
+  DrawResult ScheduledActionDrawAndSwapForced() override;
+  void ScheduledActionAnimate() override;
+  void ScheduledActionCommit() override;
+  void ScheduledActionActivateSyncTree() override;
+  void ScheduledActionBeginOutputSurfaceCreation() override;
+  void ScheduledActionPrepareTiles() override;
+  void ScheduledActionInvalidateOutputSurface() override;
+  void SendBeginFramesToChildren(const BeginFrameArgs& args) override;
+  void SendBeginMainFrameNotExpectedSoon() override;
+
+  bool HasCommitCompletionEvent() const;
+  bool next_commit_waits_for_activation() const {
+    return next_commit_waits_for_activation_;
+  }
+
+ protected:
+  ProxyImpl(ChannelImpl* channel_impl,
+            LayerTreeHost* layer_tree_host,
+            TaskRunnerProvider* task_runner_provider,
+            scoped_ptr<BeginFrameSource> external_begin_frame_source);
+
  private:
   friend class ThreadedChannel;
+  friend class ProxyImplForTest;
 
   // Callback for impl side commands received from the channel.

[Wez, 2015/11/10 01:41:50]

nit: Callbacks?

[Khushal, 2015/11/11 04:17:47]

Done.

-  virtual void SetThrottleFrameProductionOnImpl(bool throttle) = 0;
+  // virtual for testing.
+  virtual void SetThrottleFrameProductionOnImpl(bool throttle);
   virtual void UpdateTopControlsStateOnImpl(TopControlsState constraints,
                                             TopControlsState current,
-                                            bool animate) = 0;
-  virtual void InitializeOutputSurfaceOnImpl(OutputSurface* output_surface) = 0;
-  virtual void MainThreadHasStoppedFlingingOnImpl() = 0;
-  virtual void SetInputThrottledUntilCommitOnImpl(bool is_throttled) = 0;
-  virtual void SetDeferCommitsOnImpl(bool defer_commits) const = 0;
-  virtual void SetNeedsRedrawOnImpl(const gfx::Rect& damage_rect) = 0;
-  virtual void SetNeedsCommitOnImpl() = 0;
-  virtual void BeginMainFrameAbortedOnImpl(CommitEarlyOutReason reason) = 0;
-  virtual void FinishAllRenderingOnImpl(CompletionEvent* completion) = 0;
-  virtual void SetVisibleOnImpl(bool visible) = 0;
-  virtual void ReleaseOutputSurfaceOnImpl(CompletionEvent* completion) = 0;
-  virtual void FinishGLOnImpl(CompletionEvent* completion) = 0;
+                                            bool animate);
+  virtual void InitializeOutputSurfaceOnImpl(OutputSurface* output_surface);
+  virtual void MainThreadHasStoppedFlingingOnImpl();
+  virtual void SetInputThrottledUntilCommitOnImpl(bool is_throttled);
+  virtual void SetDeferCommitsOnImpl(bool defer_commits) const;
+  virtual void SetNeedsRedrawOnImpl(const gfx::Rect& damage_rect);
+  virtual void SetNeedsCommitOnImpl();
+  virtual void BeginMainFrameAbortedOnImpl(CommitEarlyOutReason reason);
+  virtual void FinishAllRenderingOnImpl(CompletionEvent* completion);
+  virtual void SetVisibleOnImpl(bool visible);
+  virtual void ReleaseOutputSurfaceOnImpl(CompletionEvent* completion);
+  virtual void FinishGLOnImpl(CompletionEvent* completion);
   virtual void MainFrameWillHappenOnImplForTesting(
       CompletionEvent* completion,
-      bool* main_frame_will_happen) = 0;
+      bool* main_frame_will_happen);
   virtual void StartCommitOnImpl(CompletionEvent* completion,
                                  LayerTreeHost* layer_tree_host,
-                                 bool hold_commit_for_activation) = 0;
-  virtual void InitializeImplOnImpl(CompletionEvent* completion,
-                                    LayerTreeHost* layer_tree_host) = 0;
-  virtual void LayerTreeHostClosedOnImpl(CompletionEvent* completion) = 0;
+                                 bool hold_commit_for_activation);
+  // Must be called before deleting ProxyImpl.

[Wez, 2015/11/10 01:41:51]

By whom, given that this is private?

[Khushal, 2015/11/11 04:17:47]

This was for the channel classes that will be added as friend classes to
ProxyMain and ProxyImpl.

The function existed in ThreadProxy to destroy the state for the impl side on
the impl thread. Since ProxyImpl represents this state, I think its suitable to
do this in the dtor itself.

+  virtual void LayerTreeHostClosedOnImpl();
 
-  // TODO(khushalsagar): Rename as GetWeakPtr() once ThreadProxy is split.
-  virtual base::WeakPtr<ProxyImpl> GetImplWeakPtr() = 0;
+  base::WeakPtr<ProxyImpl> GetWeakPtr();
 
- protected:
-  virtual ~ProxyImpl() {}
+  DrawResult DrawSwapInternal(bool forced_draw);
+
+  bool IsImplThread() const;
+  bool IsMainThreadBlocked() const;
+
+  const int layer_tree_host_id_;
+
+  scoped_ptr<Scheduler> scheduler_;

[Wez, 2015/11/10 01:41:51]

nit: There doesn't seem to be any obvious logic to the order of the members - in
the absence of an obvious set of logically-related sets of members, one helpful
convention is to put the construction-time parameters together, first, and then
the internal bits come after those.

[Khushal, 2015/11/11 04:17:47]

I've kept this consistent with the implementation of the CompositorThreadOnly
struct from ThreadProxy since that struct represented the data members for this
class.

+
+  // Set when the main thread is waiting on a pending tree activation.
+  bool next_commit_waits_for_activation_;
+
+  // Set when the main thread is waiting on a commit to complete or on a
+  // pending tree activation.
+  CompletionEvent* commit_completion_event_;
+
+  // Set when the next draw should post DidCommitAndDrawFrame to the main
+  // thread.
+  bool next_frame_is_newly_committed_frame_;
+
+  bool inside_draw_;
+  bool input_throttled_until_commit_;
+
+  bool stopped_;
+
+  // Whether a commit has been completed since the last time animations were
+  // ticked. If this happens, we need to animate again.
+  bool did_commit_after_animating_;
+
+  TaskRunnerProvider* task_runner_provider_;
+
+  DelayedUniqueNotifier smoothness_priority_expiration_notifier_;
+
+  scoped_ptr<BeginFrameSource> external_begin_frame_source_;
+
+  RenderingStatsInstrumentation* rendering_stats_instrumentation_;
+
+  // Values used to keep track of frame durations. Used only in frame timing.
+  BeginFrameArgs last_begin_main_frame_args_;
+  BeginFrameArgs last_processed_begin_main_frame_args_;
+
+  scoped_ptr<LayerTreeHostImpl> layer_tree_host_impl_;
+
+  ChannelImpl* channel_impl_;
+
+  // Use accessors instead of this variable directly.
+  BlockedMainCommitOnly main_thread_blocked_commit_vars_unsafe_;
+  BlockedMainCommitOnly& blocked_main_commit();
+
+  base::WeakPtr<ProxyImpl> impl_thread_weak_ptr_;

[Wez, 2015/11/10 01:41:51]

nit: Is this actually a WeakPtr to a thread..?

[Khushal, 2015/11/11 04:17:47]

In the thread proxy implementation, we had 2 pointers and the thread suggested
the task runner each was bound to.
Since now ProxyMain and ProxyImpl live entirely on the main or impl thread, I
guess it doesn't make sense to maintain a pointer for this. Removed it.

+  base::WeakPtrFactory<ProxyImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProxyImpl);
 };
 
 }  // namespace cc