Refactoring the way begin frame sources inside scheduler work.

cc/scheduler/scheduler.h

Index: cc/scheduler/scheduler.h
diff --git a/cc/scheduler/scheduler.h b/cc/scheduler/scheduler.h
index 34717a04acd7469c273319765d72666d7167ee8d..a8eb2d93240165d1362650775e1f78067f24d763 100644
--- a/cc/scheduler/scheduler.h
+++ b/cc/scheduler/scheduler.h
@@ -16,6 +16,7 @@
 #include "cc/output/begin_frame_args.h"
 #include "cc/scheduler/delay_based_time_source.h"
 #include "cc/scheduler/draw_result.h"
+#include "cc/scheduler/frame_source.h"
 #include "cc/scheduler/scheduler_settings.h"
 #include "cc/scheduler/scheduler_state_machine.h"
 
@@ -30,7 +31,7 @@ namespace cc {
 
 class SchedulerClient {
  public:
-  virtual void SetNeedsBeginFrame(bool enable) = 0;
+  virtual BeginFrameSource* GetExternalBeginFrameSource() = 0;
   virtual void WillBeginImplFrame(const BeginFrameArgs& args) = 0;
   virtual void ScheduledActionSendBeginMainFrame() = 0;
   virtual DrawResult ScheduledActionDrawAndSwapIfPossible() = 0;
@@ -51,15 +52,17 @@ class SchedulerClient {
   virtual ~SchedulerClient() {}
 };
 
-class CC_EXPORT Scheduler {
+class CC_EXPORT Scheduler : public BeginFrameObserver {
  public:
   static scoped_ptr<Scheduler> Create(
       SchedulerClient* client,
       const SchedulerSettings& scheduler_settings,
       int layer_tree_host_id,
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
-    return make_scoped_ptr(new Scheduler(
+    scoped_ptr<Scheduler> scheduler = make_scoped_ptr(new Scheduler(
         client, scheduler_settings, layer_tree_host_id, task_runner));
+    scheduler->FinalizeSetup();
+    return scheduler.Pass();
   }
 
   virtual ~Scheduler();
@@ -123,70 +126,53 @@ class CC_EXPORT Scheduler {
 
   void NotifyBeginMainFrameStarted();
 
-  base::TimeTicks LastBeginImplFrameTime();
-
-  void BeginFrame(const BeginFrameArgs& args);
-
   scoped_refptr<base::debug::ConvertableToTraceFormat> AsValue() const;
-  void AsValueInto(base::debug::TracedValue* state) const;
+  virtual void AsValueInto(base::debug::TracedValue* value) const OVERRIDE;
 
   void SetContinuousPainting(bool continuous_painting) {
     state_machine_.SetContinuousPainting(continuous_painting);
   }
 
- protected:
-  class CC_EXPORT SyntheticBeginFrameSource : public TimeSourceClient {
-   public:
-    SyntheticBeginFrameSource(Scheduler* scheduler,
-                              scoped_refptr<DelayBasedTimeSource> time_source);
-    virtual ~SyntheticBeginFrameSource();
-
-    // Updates the phase and frequency of the timer.
-    void CommitVSyncParameters(base::TimeTicks timebase,
-                               base::TimeDelta interval);
-
-    // Activates future BeginFrames and, if activating, pushes the most
-    // recently missed BeginFrame to the back of a retroactive queue.
-    void SetNeedsBeginFrame(bool needs_begin_frame,
-                            std::deque<BeginFrameArgs>* begin_retro_frame_args);
-
-    bool IsActive() const;
-
-    // TimeSourceClient implementation of OnTimerTick triggers a BeginFrame.
-    virtual void OnTimerTick() OVERRIDE;
-
-    void AsValueInto(base::debug::TracedValue* dict) const;
+  // BeginFrameObserver
+  virtual void OnBeginFrame(const BeginFrameArgs& args) OVERRIDE;
+  virtual const BeginFrameArgs& LastBeginFrameArgs() const OVERRIDE;
+  base::TimeTicks LastBeginImplFrameTime();
 
-   private:
-    BeginFrameArgs CreateSyntheticBeginFrameArgs(base::TimeTicks frame_time);
+  ui::CompositorVSyncManager::Observer* vsync_observer_ = NULL;
+  scoped_ptr<BeginFrameSourceMultiplexer> frame_source_;
+  BeginFrameSource* primary_frame_source_ = NULL;
+  BeginFrameSource* background_frame_source_ = NULL;

[brianderson, 2014/09/18 00:27:26]

Should these be initialized in the constructor instead?
Also, why are they public rather than protected now?

[mithro-old, 2014/09/18 13:33:38]

After chatting with the C++ experts here, it turns out there is no actual
difference between doing it initializer list verses here.

The Chrome idiom is to use the constructor, so I changed it as you requested.
Opps! Fixed.

 
-    Scheduler* scheduler_;
-    scoped_refptr<DelayBasedTimeSource> time_source_;
-  };
+  // Storage for when using internal frame sources.
+  scoped_ptr<BeginFrameSource> primary_frame_source_internal_;
+  scoped_ptr<SyntheticBeginFrameSource> background_frame_source_internal_;
 
+ protected:
   Scheduler(SchedulerClient* client,
             const SchedulerSettings& scheduler_settings,
             int layer_tree_host_id,
             const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
 
+  // Two phase setup needed because C++ don't call the right virtual functions.

[brianderson, 2014/09/18 00:27:26]

Is there no way around this? FinalizeSetup seems very hacky. For testing
purposes, would it be okay to just undo what the Scheduler constructor does and
replace the frame sources? Or maybe add a new base constructor only used by
tests that skip initializing the frame sources? That way, the hackiness is only
exposed to tests.

[brianderson, 2014/09/18 00:27:26]

typo:
"because constructors" or "C++ doesn't"

[mithro-old, 2014/09/18 13:33:38]

Removed.

[mithro-old, 2014/09/18 13:33:38]

I had chat to the local C++ experts and came up with the following solution. Do
you think it's better?

[brianderson, 2014/09/18 21:54:19]

Neat. I like the SchedulerFrameSourcesConstructor solution.

[mithro-old, 2014/09/19 02:45:39]

Glad you approve. I personally don't, but then I don't like C++ very much :P

+  void FinalizeSetup();
+
+  // Overridable for testing.
   virtual base::TimeTicks Now() const;
+  virtual BeginFrameSource* GetPrimaryBeginFrameSource();
+  virtual BeginFrameSource* GetBackgroundBeginFrameSource();
 
   const SchedulerSettings settings_;
   SchedulerClient* client_;
   int layer_tree_host_id_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
-  base::TimeDelta vsync_interval_;
   base::TimeDelta estimated_parent_draw_time_;
 
-  bool last_set_needs_begin_frame_;
-  bool begin_unthrottled_frame_posted_;
   bool begin_retro_frame_posted_;
   std::deque<BeginFrameArgs> begin_retro_frame_args_;
+  BeginFrameArgs last_begin_frame_args_;
   BeginFrameArgs begin_impl_frame_args_;
 
-  scoped_ptr<SyntheticBeginFrameSource> synthetic_begin_frame_source_;
-
   base::Closure begin_retro_frame_closure_;
   base::Closure begin_unthrottled_frame_closure_;
 
@@ -201,8 +187,6 @@ class CC_EXPORT Scheduler {
   bool inside_process_scheduled_actions_;
   SchedulerStateMachine::Action inside_action_;
 
-  base::TimeDelta VSyncInterval() { return vsync_interval_; }
-
  private:
   base::TimeTicks AdjustedBeginImplFrameDeadline(
       const BeginFrameArgs& args,
@@ -210,17 +194,13 @@ class CC_EXPORT Scheduler {
   void ScheduleBeginImplFrameDeadline(base::TimeTicks deadline);
   void SetupNextBeginFrameIfNeeded();
   void PostBeginRetroFrameIfNeeded();
-  void SetupNextBeginFrameWhenVSyncThrottlingEnabled(bool needs_begin_frame);
-  void SetupNextBeginFrameWhenVSyncThrottlingDisabled(bool needs_begin_frame);
   void SetupPollingMechanisms(bool needs_begin_frame);
   void DrawAndSwapIfPossible();
   void ProcessScheduledActions();
   bool CanCommitAndActivateBeforeDeadline() const;
   void AdvanceCommitStateIfPossible();
   bool IsBeginMainFrameSentOrStarted() const;
-  void SetupSyntheticBeginFrames();
   void BeginRetroFrame();
-  void BeginUnthrottledFrame();
   void BeginImplFrame(const BeginFrameArgs& args);
   void OnBeginImplFrameDeadline();
   void PollForAnticipatedDrawTriggers();