sync: Finish the SyncScheduler refactor

sync/engine/sync_scheduler_impl.h

Index: sync/engine/sync_scheduler_impl.h
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index dfdcc8040802cb756ffc28ce3dbfd09d10161691..baf1eb88f942edbaf88010739c73902374dd72d3 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -21,8 +21,8 @@
 #include "sync/base/sync_export.h"
 #include "sync/engine/net/server_connection_manager.h"
 #include "sync/engine/nudge_source.h"
+#include "sync/engine/nudge_tracker.h"
 #include "sync/engine/sync_scheduler.h"
-#include "sync/engine/sync_session_job.h"
 #include "sync/engine/syncer.h"
 #include "sync/internal_api/public/base/model_type_invalidation_map.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
@@ -34,6 +34,10 @@ namespace syncer {
 
 class BackoffDelayProvider;
 
+namespace sessions {
+struct ModelNeutralState;
+}  // namespace sessions

[tim (not reviewing), 2013/04/15 22:37:49]

nit - don't need comment.

[rlarocque, 2013/04/16 01:30:26]

Done.

+
 class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
     : public SyncScheduler,
       public base::NonThreadSafe {
@@ -83,15 +87,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
       const sessions::SyncSessionSnapshot& snapshot) OVERRIDE;
 
  private:
-  enum JobProcessDecision {
-    // Indicates we should continue with the current job.
-    CONTINUE,
-    // Indicates that we should save it to be processed later.
-    SAVE,
-    // Indicates we should drop this job.
-    DROP,
-  };
-
   enum JobPriority {
     // Non-canary jobs respect exponential backoff.
     NORMAL_PRIORITY,
@@ -103,6 +98,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   friend class SyncSchedulerWhiteboxTest;
   friend class SyncerTest;
 
+  FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, NoNudgesInConfigureMode);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest,
       DropNudgeWhileExponentialBackOff);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, SaveNudge);
@@ -145,57 +141,32 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
 
   static const char* GetModeString(Mode mode);
 
-  static const char* GetDecisionString(JobProcessDecision decision);
-
-  // Invoke the syncer to perform a non-POLL job.
-  bool DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
-                            JobPriority priority);
-
   // Invoke the syncer to perform a nudge job.
   void DoNudgeSyncSessionJob(JobPriority priority);
 
   // Invoke the syncer to perform a configuration job.
   bool DoConfigurationSyncSessionJob(JobPriority priority);
 
-  // Returns whether or not it's safe to run a poll job at this time.
-  bool ShouldPoll();
-
   // Invoke the Syncer to perform a poll job.
   void DoPollSyncSessionJob();
 
-  // Called after the Syncer has performed the sync represented by |job|, to
-  // reset our state.  |exited_prematurely| is true if the Syncer did not
-  // cycle from job.start_step() to job.end_step(), likely because the
-  // scheduler was forced to quit the job mid-way through.
-  bool FinishSyncSessionJob(SyncSessionJob* job,
-                            bool exited_prematurely,
-                            sessions::SyncSession* session);
-
-  // Helper to schedule retries of a failed configure or nudge job.
-  void ScheduleNextSync(scoped_ptr<SyncSessionJob> finished_job,
-                        sessions::SyncSession* session);
-
-  // Helper to configure polling intervals. Used by Start and ScheduleNextSync.
-  void AdjustPolling(const SyncSessionJob* old_job);
+  // Helper to configure polling intervals.
+  void AdjustPolling(bool force_reset);

[tim (not reviewing), 2013/04/15 22:37:49]

nit - enum vs bool parameter as the intent of the parameter isn't immediately
evident from the method name.

[rlarocque, 2013/04/16 01:30:26]

I'd prefer to not add another enum in that style.  Instead, I did what I should
have done in the first place.  I split this into two separate functions, and
factored out the common code.  

It would probably make even more sense to update the poll interval only when
Start() is called, or in OnReceived*PollIntervalUpdate(), or when we receive an
update to our notification status.  I'm not going to press my luck that far,
though.  I'd prefer to maintain the existing code structure.  

 
   // Helper to restart waiting with |wait_interval_|'s timer.
   void RestartWaiting();
 
-  // Helper to ScheduleNextSync in case of consecutive sync errors.
-  void HandleContinuationError(scoped_ptr<SyncSessionJob> old_job,
-                               sessions::SyncSession* session);
+  // Helper to adjust our wait interval when we expereince a transient failure.
+  void UpdateExponentialBackoff(
+      const sessions::ModelNeutralState& model_neutral_state);
 
-  // Decide whether we should CONTINUE, SAVE or DROP the job.
-  JobProcessDecision DecideOnJob(const SyncSessionJob& job,
-                                 JobPriority priority);
+  // Determines if we're allowed to contact the server right now.
+  bool CanRunJobNow(JobPriority priority);
 
-  // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
-  // backoff mode.
-  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job,
-                                               JobPriority priority);
+  // Determines if we're allowed to contact the server right now.
+  bool CanRunNudgeJobNow(JobPriority priority);
 
-  // 'Impl' here refers to real implementation of public functions, running on
-  // |thread_|.
+  // 'Impl' here refers to real implementation of public functions.
   void StopImpl(const base::Closure& callback);
 
   // If the scheduler's current state supports it, this will create a job based
@@ -230,9 +201,9 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // Creates a session for a poll and performs the sync.
   void PollTimerCallback();
 
-  // Called once the first time thread_ is started to broadcast an initial
-  // session snapshot containing data like initial_sync_ended.  Important when
-  // the client starts up and does not need to perform an initial sync.
+  // Called as we are started to broadcast an initial session snapshot
+  // containing data like initial_sync_ended.  Important when the client starts
+  // up and does not need to perform an initial sync.
   void SendInitialSnapshot();
 
   // This is used for histogramming and analysis of ScheduleNudge* APIs.
@@ -280,13 +251,15 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // The event that will wake us up.
   base::OneShotTimer<SyncSchedulerImpl> pending_wakeup_timer_;
 
-  // Pending configure job storage.  Note that
-  // (mode_ != CONFIGURATION_MODE) \implies !pending_configure_job_.
-  scoped_ptr<SyncSessionJob> pending_configure_job_;
+  // Storage for variables related to an in-progress configure request.  Note
+  // that (mode_ != CONFIGURATION_MODE) \implies !pending_configure_params_.
+  scoped_ptr<ConfigurationParams> pending_configure_params_;
+
+  // If we have a nudge pending to run soon, it will be listed here.
+  base::TimeTicks scheduled_nudge_time_;
 
-  // Pending nudge job storage.  These jobs can exist in CONFIGURATION_MODE, but
-  // they will be run only in NORMAL_MODE.
-  scoped_ptr<SyncSessionJob> pending_nudge_job_;
+  // Keeps track of work that the syncer needs to handle.
+  NudgeTracker nudge_tracker_;
 
   // Invoked to run through the sync cycle.
   scoped_ptr<Syncer> syncer_;