Remove canary member from SyncSessionJob

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 262 matching lines @@
     // execute in the new mode.
     if (mode_ == NORMAL_MODE) {
       // It is illegal to switch to NORMAL_MODE if a previous CONFIGURATION job
       // has not yet completed.
       DCHECK(!wait_interval_.get() || !wait_interval_->pending_configure_job);
     }
 
     scoped_ptr<SyncSessionJob> pending(TakePendingJobForCurrentMode());
     if (pending.get()) {
       SDVLOG(2) << "Executing pending job. Good luck!";
-      DoSyncSessionJob(pending.Pass());
+      DoSyncSessionJob(pending.Pass(), false);
     }
   }
 }
 
 void SyncSchedulerImpl::SendInitialSnapshot() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   scoped_ptr<SyncSession> dummy(new SyncSession(
           session_context_, this, SyncSourceInfo()));
   SyncEngineEvent event(SyncEngineEvent::STATUS_CHANGED);
   event.snapshot = dummy->TakeSnapshot();
@@ skipping 49 matching lines @@
                        ModelSafeRoutingInfoToInvalidationMap(
                            restricted_routes,
                            std::string()))));
     scoped_ptr<SyncSessionJob> job(new SyncSessionJob(
         SyncSessionJob::CONFIGURATION,
         TimeTicks::Now(),
         session.Pass(),
         params,
         FROM_HERE));
     job->set_destruction_observer(weak_ptr_factory_.GetWeakPtr());
-    bool succeeded = DoSyncSessionJob(job.Pass());
+    bool succeeded = DoSyncSessionJob(job.Pass(), false);
 
     // If we failed, the job would have been saved as the pending configure
     // job and a wait interval would have been set.
     if (!succeeded) {
       DCHECK(wait_interval_.get() && wait_interval_->pending_configure_job);
       return false;
     }
   } else {
     SDVLOG(2) << "No change in routing info, calling ready task directly.";
     params.ready_task.Run();
   }
 
   return true;
 }
 
 SyncSchedulerImpl::JobProcessDecision
-SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job) {
+SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
+                                             bool is_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
             << (wait_interval_->had_nudge ? " (had nudge)" : "")
-            << (job.is_canary() ? " (canary)" : "");
+            << (is_canary ? " (canary)" : "");
 
   if (job.purpose() == SyncSessionJob::POLL)
     return DROP;
 
   // If we save a job while in a WaitInterval, there is a well-defined moment
   // in time in the future when it makes sense for that SAVE-worthy job to try
   // running again -- the end of the WaitInterval.
   DCHECK(job.purpose() == SyncSessionJob::NUDGE ||
          job.purpose() == SyncSessionJob::CONFIGURATION);
 
   // If throttled, there's a clock ticking to unthrottle. We want to get
   // on the same train.
   if (wait_interval_->mode == WaitInterval::THROTTLED)
     return SAVE;
 
   DCHECK_EQ(wait_interval_->mode, WaitInterval::EXPONENTIAL_BACKOFF);
   if (job.purpose() == SyncSessionJob::NUDGE) {
     if (mode_ == CONFIGURATION_MODE)
       return SAVE;
 
     // If we already had one nudge then just drop this nudge. We will retry
     // later when the timer runs out.
-    if (!job.is_canary())
+    if (!is_canary)
       return wait_interval_->had_nudge ? DROP : CONTINUE;
     else // We are here because timer ran out. So retry.
       return CONTINUE;
   }
-  return job.is_canary() ? CONTINUE : SAVE;
+  return is_canary ? CONTINUE : SAVE;
 }
 
 SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
-    const SyncSessionJob& job) {
+    const SyncSessionJob& job,
+    bool is_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   // See if our type is throttled.
   ModelTypeSet throttled_types =
       session_context_->throttled_data_type_tracker()->GetThrottledTypes();
   if (job.purpose() == SyncSessionJob::NUDGE &&
       job.session()->source().updates_source == GetUpdatesCallerInfo::LOCAL) {
     ModelTypeSet requested_types;
     for (ModelTypeInvalidationMap::const_iterator i =
          job.session()->source().types.begin();
          i != job.session()->source().types.end();
          ++i) {
       requested_types.Put(i->first);
     }
 
     // If all types are throttled, do not CONTINUE.  Today, we don't treat
     // a per-datatype "unthrottle" event as something that should force a
     // canary job. For this reason, there's no good time to reschedule this job
     // to run -- we'll lazily wait for an independent event to trigger a sync.
     // Note that there may already be such an event if we're in a WaitInterval,
     // so we can retry it then.
     if (!requested_types.Empty() && throttled_types.HasAll(requested_types))
       return SAVE;
   }
 
   if (wait_interval_.get())
-    return DecideWhileInWaitInterval(job);
+    return DecideWhileInWaitInterval(job, is_canary);
 
   if (mode_ == CONFIGURATION_MODE) {
     if (job.purpose() == SyncSessionJob::NUDGE)
       return SAVE;  // Running requires a mode switch.
     else if (job.purpose() == SyncSessionJob::CONFIGURATION)
       return CONTINUE;
     else
       return DROP;
   }
 
@@ skipping 41 matching lines @@
   if (!session_context_->connection_manager()->HasInvalidAuthToken())
     return CONTINUE;
 
   SDVLOG(2) << "No valid auth token. Using that to decide on job.";
   // Running the job would require updated auth, so we can't honour
   // job.scheduled_start().
   return job.purpose() == SyncSessionJob::NUDGE ? SAVE : DROP;
 }
 
 void SyncSchedulerImpl::HandleSaveJobDecision(scoped_ptr<SyncSessionJob> job) {
-  DCHECK_EQ(DecideOnJob(*job), SAVE);
   const bool is_nudge = job->purpose() == SyncSessionJob::NUDGE;
   if (is_nudge && pending_nudge_) {
     SDVLOG(2) << "Coalescing a pending nudge";
     // TODO(tim): This basically means we never use the more-careful coalescing
     // logic in ScheduleNudgeImpl that takes the min of the two nudge start
     // times, because we're calling this function first.  Pull this out
     // into a function to coalesce + set start times and reuse.
     pending_nudge_->mutable_session()->CoalesceSources(
         job->session()->source());
     return;
@@ skipping 106 matching lines @@
   SyncSourceInfo info(source, invalidation_map);
   UpdateNudgeTimeRecords(info);
 
   scoped_ptr<SyncSessionJob> job(new SyncSessionJob(
       SyncSessionJob::NUDGE,
       TimeTicks::Now() + delay,
       CreateSyncSession(info).Pass(),
       ConfigurationParams(),
       nudge_location));
   job->set_destruction_observer(weak_ptr_factory_.GetWeakPtr());
-  JobProcessDecision decision = DecideOnJob(*job);
+  JobProcessDecision decision = DecideOnJob(*job, false);
   SDVLOG(2) << "Should run "
             << SyncSessionJob::GetPurposeString(job->purpose())
             << " job " << job->session()
             << " in mode " << GetModeString(mode_)
             << ": " << GetDecisionString(decision);
   if (decision != CONTINUE) {
     // End of the line, though we may save the job for later.
     if (decision == SAVE) {
       HandleSaveJobDecision(job.Pass());
     } else {
@@ skipping 94 matching lines @@
   if (job->purpose() == SyncSessionJob::NUDGE) {
     SDVLOG_LOC(loc, 2) << "Resetting pending_nudge to ";
     DCHECK(!pending_nudge_ || pending_nudge_->session() ==
            job->session());
     set_pending_nudge(job.get());
   }
 
   PostDelayedTask(loc, "DoSyncSessionJob",
       base::Bind(base::IgnoreResult(&SyncSchedulerImpl::DoSyncSessionJob),
                                     weak_ptr_factory_.GetWeakPtr(),
-                                    base::Passed(&job)),
+                                    base::Passed(&job),
+                                    false),
       delay);
 }
 
-bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job) {
+bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
+                                         bool is_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (job->purpose() == SyncSessionJob::NUDGE) {
     if (pending_nudge_ == NULL ||
         pending_nudge_->session() != job->session()) {
       // |job| is abandoned.
       SDVLOG(2) << "Dropping a nudge in "
                 << "DoSyncSessionJob because another nudge was scheduled";
       return false;
     }
     pending_nudge_ = NULL;
   }
 
   base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
-  JobProcessDecision decision = DecideOnJob(*job);
+  JobProcessDecision decision = DecideOnJob(*job, is_canary);
   SDVLOG(2) << "Should run "
             << SyncSessionJob::GetPurposeString(job->purpose())
             << " job " << job->session()
             << " in mode " << GetModeString(mode_)
             << " with source " << job->session()->source().updates_source
             << ": " << GetDecisionString(decision);
   if (decision != CONTINUE) {
     if (decision == SAVE) {
       HandleSaveJobDecision(job.Pass());
     } else {
@@ skipping 171 matching lines @@
     wait_interval_->timer.Start(FROM_HERE, wait_interval_->length,
                                 base::Bind(&SyncSchedulerImpl::DoCanaryJob,
                                     weak_ptr_factory_.GetWeakPtr(),
                                     base::Passed(&job)));
   }
 }
 
 void SyncSchedulerImpl::HandleContinuationError(
     scoped_ptr<SyncSessionJob> old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (DCHECK_IS_ON()) {
-    if (IsBackingOff()) {
-      DCHECK(wait_interval_->timer.IsRunning() || old_job->is_canary());
-    }
-  }
 
   TimeDelta length = delay_provider_->GetDelay(
       IsBackingOff() ? wait_interval_->length :
           delay_provider_->GetInitialDelay(
               old_job->session()->status_controller().model_neutral_state()));
 
   SDVLOG(2) << "In handle continuation error with "
             << SyncSessionJob::GetPurposeString(old_job->purpose())
             << " job. The time delta(ms) is "
             << length.InMilliseconds();
@@ skipping 44 matching lines @@
     started_ = false;
   }
   if (!callback.is_null())
     callback.Run();
 }
 
 void SyncSchedulerImpl::DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "Do canary job";
 
-  // Only set canary privileges here, when we are about to run the job. This
-  // avoids confusion in managing canary bits during scheduling, when you
-  // consider that mode switches (e.g., to config) can "pre-empt" a NUDGE that
-  // was scheduled as canary, and send it to an "unscheduled" state.

[tim (not reviewing), 2013/03/02 01:03:39]

This comment is even more relevant than it was before, since at least before you
had to type the characters "GrantCanaryPrivilege", and the thinking was that
sounded scary enough to make you think twice (i.e canary is a privilege you have
to grant, versus just a bool setting). So we should keep the comment.  We should
also use an enum instead of a bool parameter to DoSyncSessionJob for the same
reason, and also because bool parameters are discouraged by the chromium style
guide.

[rlarocque, 2013/03/02 01:21:12]

OK, will fix.

-  to_be_canary->GrantCanaryPrivilege();
-
   if (to_be_canary->purpose() == SyncSessionJob::NUDGE) {
     // TODO(tim): Bug 158313.  Remove this check.
     if (pending_nudge_ == NULL ||
         pending_nudge_->session() != to_be_canary->session()) {
       // |job| is abandoned.
       SDVLOG(2) << "Dropping a nudge in "
                 << "DoSyncSessionJob because another nudge was scheduled";
       return;
     }
     DCHECK_EQ(pending_nudge_->session(), to_be_canary->session());
   }
-  DoSyncSessionJob(to_be_canary.Pass());
+  DoSyncSessionJob(to_be_canary.Pass(), true);
 }
 
 scoped_ptr<SyncSessionJob> SyncSchedulerImpl::TakePendingJobForCurrentMode() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   // If we find a scheduled pending_ job, abandon the old one and return a
   // a clone.  If unscheduled, just hand over ownership.
   scoped_ptr<SyncSessionJob> candidate;
   if (mode_ == CONFIGURATION_MODE && wait_interval_.get()
       && wait_interval_->pending_configure_job) {
     SDVLOG(2) << "Found pending configure job";
@@ skipping 151 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer