Remove canary member from SyncSessionJob

sync/engine/sync_scheduler_impl.cc

Index: sync/engine/sync_scheduler_impl.cc
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 33e3fd55028684a79633f9a64e9d4db6cd04ef60..35c5dd7ab6168251d07fd49d41058f1b46ecece7 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -280,7 +280,7 @@ void SyncSchedulerImpl::Start(Mode mode) {
     scoped_ptr<SyncSessionJob> pending(TakePendingJobForCurrentMode());
     if (pending.get()) {
       SDVLOG(2) << "Executing pending job. Good luck!";
-      DoSyncSessionJob(pending.Pass());
+      DoSyncSessionJob(pending.Pass(), false);
     }
   }
 }
@@ -350,7 +350,7 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
         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.
@@ -367,14 +367,15 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
 }
 
 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;
@@ -397,16 +398,17 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job) {
 
     // 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.
@@ -433,7 +435,7 @@ SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
   }
 
   if (wait_interval_.get())
-    return DecideWhileInWaitInterval(job);
+    return DecideWhileInWaitInterval(job, is_canary);
 
   if (mode_ == CONFIGURATION_MODE) {
     if (job.purpose() == SyncSessionJob::NUDGE)
@@ -495,7 +497,6 @@ SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
 }
 
 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";
@@ -622,7 +623,7 @@ void SyncSchedulerImpl::ScheduleNudgeImpl(
       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()
@@ -737,11 +738,13 @@ void SyncSchedulerImpl::ScheduleSyncSessionJob(
   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 ||
@@ -755,7 +758,7 @@ bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job) {
   }
 
   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()
@@ -947,11 +950,6 @@ void SyncSchedulerImpl::RestartWaiting(scoped_ptr<SyncSessionJob> 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 :
@@ -1016,12 +1014,6 @@ 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 ||
@@ -1033,7 +1025,7 @@ void SyncSchedulerImpl::DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary) {
     }
     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() {