Channel all low lever scheduling calls through single function

sync/engine/sync_scheduler_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/auto_reset.h"
@@ skipping 219 matching lines @@
   Mode old_mode = mode_;
   mode_ = mode;
   AdjustPolling(UPDATE_INTERVAL);  // Will kick start poll timer if needed.
 
   if (old_mode != mode_ &&
       mode_ == NORMAL_MODE &&
       nudge_tracker_.IsSyncRequired() &&
       CanRunNudgeJobNow(NORMAL_PRIORITY)) {
     // We just got back to normal mode.  Let's try to run the work that was
     // queued up while we were configuring.
-    DoNudgeSyncSessionJob(NORMAL_PRIORITY);
+    TryJob(NORMAL_PRIORITY);
   }
 }
 
 ModelTypeSet SyncSchedulerImpl::GetEnabledAndUnthrottledTypes() {
   ModelTypeSet enabled_types =
       GetRoutingInfoTypes(session_context_->routing_info());
   ModelTypeSet throttled_types =
       nudge_tracker_.GetThrottledTypes();
   return Difference(enabled_types, throttled_types);
 }
@@ skipping 41 matching lines @@
 
   ModelSafeRoutingInfo restricted_routes;
   BuildModelSafeParams(params.types_to_download,
                        params.routing_info,
                        &restricted_routes);
   session_context_->set_routing_info(restricted_routes);
 
   // Only reconfigure if we have types to download.
   if (!params.types_to_download.Empty()) {
     pending_configure_params_.reset(new ConfigurationParams(params));
-    DoConfigurationSyncSessionJob(NORMAL_PRIORITY);
+    TryJob(NORMAL_PRIORITY);
   } else {
     SDVLOG(2) << "No change in routing info, calling ready task directly.";
     params.ready_task.Run();
   }
 }
 
 bool SyncSchedulerImpl::CanRunJobNow(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   if (wait_interval_ && wait_interval_->mode == WaitInterval::THROTTLED) {
     SDVLOG(1) << "Unable to run a job because we're throttled.";
@@ skipping 252 matching lines @@
     return;
   }
 
   SDVLOG(2) << "Polling with routes "
            << ModelSafeRoutingInfoToString(session_context_->routing_info());
   scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
   syncer_->PollSyncShare(
       GetEnabledAndUnthrottledTypes(),
       session.get());
 
-  AdjustPolling(UPDATE_INTERVAL);
+  AdjustPolling(FORCE_RESET);
 
   if (IsCurrentlyThrottled()) {
     SDVLOG(2) << "Poll request got us throttled.";
     // The OnSilencedUntil() call set up the WaitInterval for us.  All we need
     // to do is start the timer.
     RestartWaiting();
   }
 }
 
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(ModelTypeSet types) {
   DCHECK(CalledOnValidThread());
   base::TimeTicks now = TimeTicks::Now();
   // Update timing information for how often datatypes are triggering nudges.
   for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) {
     base::TimeTicks previous = last_local_nudges_by_model_type_[iter.Get()];
     last_local_nudges_by_model_type_[iter.Get()] = now;
     if (previous.is_null())
       continue;
 
 #define PER_DATA_TYPE_MACRO(type_str) \
     SYNC_FREQ_HISTOGRAM("Sync.Freq" type_str, now - previous);
     SYNC_DATA_TYPE_HISTOGRAM(iter.Get());
 #undef PER_DATA_TYPE_MACRO
   }
 }
 
+TimeDelta SyncSchedulerImpl::GetPollInterval() {
+  return (!session_context_->notifications_enabled() ||
+          !session_context_->ShouldFetchUpdatesBeforeCommit()) ?
+      syncer_short_poll_interval_seconds_ :
+      syncer_long_poll_interval_seconds_;
+}
+
 void SyncSchedulerImpl::AdjustPolling(PollAdjustType type) {
   DCHECK(CalledOnValidThread());
 
-  TimeDelta poll  = (!session_context_->notifications_enabled() ||
-                     !session_context_->ShouldFetchUpdatesBeforeCommit()) ?
-      syncer_short_poll_interval_seconds_ :
-      syncer_long_poll_interval_seconds_;
+  TimeDelta poll = GetPollInterval();
   bool rate_changed = !poll_timer_.IsRunning() ||
                        poll != poll_timer_.GetCurrentDelay();
 
-  if (type == FORCE_RESET && !rate_changed)
-    poll_timer_.Reset();
+  if (type == FORCE_RESET) {
+    last_poll_reset_ = base::TimeTicks::Now();
+    if (!rate_changed)
+      poll_timer_.Reset();
+  }
 
   if (!rate_changed)
     return;
 
   // Adjust poll rate.
   poll_timer_.Stop();
   poll_timer_.Start(FROM_HERE, poll, this,
                     &SyncSchedulerImpl::PollTimerCallback);
 }
 
@@ skipping 29 matching lines @@
   poll_timer_.Stop();
   pending_wakeup_timer_.Stop();
   pending_configure_params_.reset();
   if (started_)
     started_ = false;
 }
 
 // This is the only place where we invoke DoSyncSessionJob with canary
 // privileges.  Everyone else should use NORMAL_PRIORITY.
 void SyncSchedulerImpl::TryCanaryJob() {
-  DCHECK(CalledOnValidThread());
-
-  if (mode_ == CONFIGURATION_MODE && pending_configure_params_) {
-    SDVLOG(2) << "Found pending configure job; will run as canary";
-    DoConfigurationSyncSessionJob(CANARY_PRIORITY);
-  } else if (mode_ == NORMAL_MODE && nudge_tracker_.IsSyncRequired() &&
-             CanRunNudgeJobNow(CANARY_PRIORITY)) {
-    SDVLOG(2) << "Found pending nudge job; will run as canary";
-    DoNudgeSyncSessionJob(CANARY_PRIORITY);
-  } else if (mode_ == NORMAL_MODE && CanRunJobNow(CANARY_PRIORITY) &&
-             do_poll_after_credentials_updated_) {
-    // Retry poll if poll timer recently fired and ProfileSyncService received
-    // fresh access token.
-    DoPollSyncSessionJob();
-  } else {
-    SDVLOG(2) << "Found no work to do; will not run a canary";
-  }
+  TryJob(CANARY_PRIORITY);
   // Don't run poll job till the next time poll timer fires.
   do_poll_after_credentials_updated_ = false;
 }
 
+void SyncSchedulerImpl::TryJob(JobPriority priority) {

[tim (not reviewing), 2013/11/17 00:36:47]

I like it. I've always been a fan of having a consolidation point like this,
even just to wrap my head around.

We could be more consistent in terms of naming with the Do*SyncSessionJob
functions though, e.g TrySyncSessionJob

[pavely, 2013/11/18 18:39:55]

Done.

+  DCHECK(CalledOnValidThread());
+  if (mode_ == CONFIGURATION_MODE) {
+    if (pending_configure_params_) {
+      SDVLOG(2) << "Found pending configure job";
+      DoConfigurationSyncSessionJob(priority);
+    }
+  } else {
+    DCHECK(mode_ == NORMAL_MODE);
+    if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(priority)) {
+      SDVLOG(2) << "Found pending nudge job";
+      DoNudgeSyncSessionJob(priority);
+    } else if (((base::TimeTicks::Now() - last_poll_reset_)
+               >= GetPollInterval()) || do_poll_after_credentials_updated_) {

[tim (not reviewing), 2013/11/17 00:36:47]

Whoa, this is hard to read.  Consider pulling Now() - last_poll_reset_ into a
variable.

I'm not sure if it's stipulated anywhere, but in general in chromium the >=
would go on the previous line.

Also, the indent is off.  The GetPollInterval() line should go 4 spaces from the
left of the previous line.

Maybe if you put the "do_poll_after_credentials_updated_ ||" first, the rest
won't look so messy.

[pavely, 2013/11/18 18:39:55]

I reordered conditions, decided not to extract subtraction into variable because
this way it will be calculated and potentially not used.

+      DoPollSyncSessionJob();
+    }
+  }
+}
+
 void SyncSchedulerImpl::PollTimerCallback() {
   DCHECK(CalledOnValidThread());
   if (no_scheduling_allowed_) {
     // The no_scheduling_allowed_ flag is set by a function-scoped AutoReset in
     // functions that are called only on the sync thread.  This function is also
     // called only on the sync thread, and only when it is posted by an expiring
     // timer.  If we find that no_scheduling_allowed_ is set here, then
     // something is very wrong.  Maybe someone mistakenly called us directly, or
     // mishandled the book-keeping for no_scheduling_allowed_.
     NOTREACHED() << "Illegal to schedule job while session in progress.";
     return;
   }
 
-  DoPollSyncSessionJob();
+  TryJob(NORMAL_PRIORITY);
   // Poll timer fires infrequently. Usually by this time access token is already
   // expired and poll job will fail with auth error. Set flag to retry poll once
   // ProfileSyncService gets new access token, TryCanaryJob will be called in
   // this case.
   if (HttpResponse::SYNC_AUTH_ERROR ==
       session_context_->connection_manager()->server_status()) {
     do_poll_after_credentials_updated_ = true;
   }
 }
 
@@ skipping 24 matching lines @@
     type_unthrottle_timer_.Start(
         FROM_HERE,
         time_until_next_unthrottle,
         base::Bind(&SyncSchedulerImpl::TypeUnthrottle,
                    weak_ptr_factory_.GetWeakPtr(),
                    unthrottle_time + time_until_next_unthrottle));
   }
 
   // Maybe this is a good time to run a nudge job.  Let's try it.
   if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(NORMAL_PRIORITY))
-    DoNudgeSyncSessionJob(NORMAL_PRIORITY);
+    TryJob(NORMAL_PRIORITY);
 }
 
 void SyncSchedulerImpl::PerformDelayedNudge() {
   // Circumstances may have changed since we scheduled this delayed nudge.
   // We must check to see if it's OK to run the job before we do so.
   if (CanRunNudgeJobNow(NORMAL_PRIORITY))
-    DoNudgeSyncSessionJob(NORMAL_PRIORITY);
+    TryJob(NORMAL_PRIORITY);
 
   // We're not responsible for setting up any retries here.  The functions that
   // first put us into a state that prevents successful sync cycles (eg. global
   // throttling, type throttling, network errors, transient errors) will also
   // setup the appropriate retry logic (eg. retry after timeout, exponential
   // backoff, retry when the network changes).
 }
 
 void SyncSchedulerImpl::ExponentialBackoffRetry() {
   TryCanaryJob();
@@ skipping 126 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer