Revert 164565 - sync: make scheduling logic and job ownership more obvious.

Revert Reason - see bug 158313

- inlines (and removes) SaveJob, to perform only the relevant "saving" bits
where needed. As it turns out, most of it was only necessary for ShouldRunJob
(which I'd like to rename, as it's destructive), and it's a lot easier to read
what's happening and why. Note also removed TODO at SaveJob callsites. 

- inlines (and removes) InitOrCoalescePendingJob to perform only the relevant
bits at each callsite. 

- pulls SyncSessionJob into a class, to handle basic responsibilities that need
the job Purpose + session (like "Succeeded()" and "Finish") that were previously
strewn about and partially copy/pasted in the scheduler. 

- meticulously transfers ownership (removes linked_ptr usage!) of jobs instead
of making many implicit struct copies, as it resulted in non-obvious behavior.
To do this, ownership is given to the scheduling mechanism (the MessageLoop for
ScheduleSyncSessionJob, WaitInterval::timer for canary jobs), instead of jobs
sitting around without knowing whether they're scheduled or not. There are a
few cases where we can't actually "schedule" a job -- which wasn't even obvious
from the old code, and other interesting revelations, like fact that we were 
actually pre-empt nudge canary jobs and "unscheduling" them in certain cases.

- removes DoPendingJobIfPossible, since it is no longer needed for 
DoCanaryJob/Unthrottle cases, and make the behavior more explicit in the other
cases (mode-switch and SCM error change), see TakePendingJobForCurrentMode. 

- removes the ability to create jobs as canary, since this wasn't needed and
was adding complexity (see DoCanaryJob / GrantCanaryPrivilege). 

- removes retry_scheduled as it wasn't used anywhere 

- adds lots of comments.

Also discovered that THROTTLED intervals seem to never fire a canary job with 
today's code (broken), config jobs are immune to per-data-type throttling, and
the had_nudge allowance was defeated by extra IsBackingOff check in 
ScheduleNudgeImpl (see comment on review).

BUG=

Review URL: https://chromiumcodereview.appspot.com/10917234

TBR=tim@chromium.org
Review URL: https://codereview.chromium.org/11347027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@164780 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 13 insertions, 36 deletions

diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 7746aea..31f850a 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -42,7 +42,7 @@ using sync_pb::GetUpdatesCallerInfo;
 
 class MockSyncer : public Syncer {
  public:
-  MOCK_METHOD3(SyncShare, bool(sessions::SyncSession*, SyncerStep,
+  MOCK_METHOD3(SyncShare, void(sessions::SyncSession*, SyncerStep,
                                SyncerStep));
 };
 
@@ -110,7 +110,6 @@ class SyncSchedulerTest : public testing::Test {
     routing_info_[THEMES] = GROUP_UI;
     routing_info_[NIGORI] = GROUP_PASSIVE;
 
-    workers_.clear();
     workers_.push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
     workers_.push_back(make_scoped_refptr(new FakeModelWorker(GROUP_DB)));
     workers_.push_back(make_scoped_refptr(new FakeModelWorker(GROUP_PASSIVE)));
@@ -246,7 +245,6 @@ ACTION_P(RecordSyncShare, record) {
   RecordSyncShareImpl(arg0, record);
   if (MessageLoop::current()->is_running())
     QuitLoopNow();
-  return true;
 }
 
 ACTION_P2(RecordSyncShareMultiple, record, quit_after) {
@@ -256,18 +254,15 @@ ACTION_P2(RecordSyncShareMultiple, record, quit_after) {
       MessageLoop::current()->is_running()) {
     QuitLoopNow();
   }
-  return true;
 }
 
 ACTION(AddFailureAndQuitLoopNow) {
   ADD_FAILURE();
   QuitLoopNow();
-  return true;
 }
 
 ACTION(QuitLoopNowAction) {
   QuitLoopNow();
-  return true;
 }
 
 // Test nudge scheduling.
@@ -651,10 +646,8 @@ TEST_F(SyncSchedulerTest, PollIntervalUpdate) {
   TimeDelta poll2(TimeDelta::FromMilliseconds(30));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll1);
   EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(AtLeast(kMinNumSamples))
-      .WillOnce(DoAll(
-          WithArg<0>(
-              sessions::test_util::SimulatePollIntervalUpdate(poll2)),
-          Return(true)))
+      .WillOnce(WithArg<0>(
+          sessions::test_util::SimulatePollIntervalUpdate(poll2)))
       .WillRepeatedly(
           DoAll(Invoke(sessions::test_util::SimulateSuccess),
                 WithArg<0>(
@@ -707,9 +700,7 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
 
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          WithArg<0>(sessions::test_util::SimulateThrottled(throttle)),
-          Return(true)))
+      .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle)))
       .WillRepeatedly(AddFailureAndQuitLoopNow());
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ -738,9 +729,7 @@ TEST_F(SyncSchedulerTest, ThrottlingExpires) {
 
   ::testing::InSequence seq;
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          WithArg<0>(sessions::test_util::SimulateThrottled(throttle1)),
-          Return(true)))
+      .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle1)))
       .RetiresOnSaturation();
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
@@ -838,9 +827,7 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailCommitOnce) {
 // retry.  Expect that this clears the backoff state.
 TEST_F(BackoffTriggersSyncSchedulerTest, FailDownloadOnceThenSucceed) {
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          Invoke(sessions::test_util::SimulateDownloadUpdatesFailed),
-          Return(true)))
+      .WillOnce(Invoke(sessions::test_util::SimulateDownloadUpdatesFailed))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       QuitLoopNowAction()));
   EXPECT_FALSE(RunAndGetBackoff());
@@ -850,9 +837,7 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailDownloadOnceThenSucceed) {
 // that this clears the backoff state.
 TEST_F(BackoffTriggersSyncSchedulerTest, FailCommitOnceThenSucceed) {
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          Invoke(sessions::test_util::SimulateCommitFailed),
-          Return(true)))
+      .WillOnce(Invoke(sessions::test_util::SimulateCommitFailed))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       QuitLoopNowAction()));
   EXPECT_FALSE(RunAndGetBackoff());
@@ -862,9 +847,7 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailCommitOnceThenSucceed) {
 // Expect this will leave the scheduler in backoff.
 TEST_F(BackoffTriggersSyncSchedulerTest, FailDownloadTwice) {
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          Invoke(sessions::test_util::SimulateDownloadUpdatesFailed),
-          Return(true)))
+      .WillOnce(Invoke(sessions::test_util::SimulateDownloadUpdatesFailed))
       .WillRepeatedly(DoAll(
               Invoke(sessions::test_util::SimulateDownloadUpdatesFailed),
               QuitLoopNowAction()));
@@ -875,9 +858,7 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailDownloadTwice) {
 // updates. Expect this will leave the scheduler in backoff.
 TEST_F(BackoffTriggersSyncSchedulerTest, FailGetEncryptionKey) {
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(
-          Invoke(sessions::test_util::SimulateGetEncryptionKeyFailed),
-          Return(true)))
+      .WillOnce(Invoke(sessions::test_util::SimulateGetEncryptionKeyFailed))
       .WillRepeatedly(DoAll(
               Invoke(sessions::test_util::SimulateGetEncryptionKeyFailed),
               QuitLoopNowAction()));
@@ -1059,8 +1040,7 @@ TEST_F(SyncSchedulerTest, TransientPollFailure) {
 TEST_F(SyncSchedulerTest, SyncerSteps) {
   // Nudges.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
-                      Return(true)));
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   scheduler()->ScheduleNudgeAsync(
@@ -1074,8 +1054,7 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
 
   // Configuration.
   EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
-                      Return(true)));
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ModelTypeSet model_types(BOOKMARKS);
@@ -1116,10 +1095,8 @@ TEST_F(SyncSchedulerTest, StartWhenNotConnected) {
   connection()->SetServerNotReachable();
   connection()->UpdateConnectionStatus();
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-    .WillOnce(DoAll(Invoke(sessions::test_util::SimulateConnectionFailure),
-                    Return(true)))
-    .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
-                    QuitLoopNowAction()));
+    .WillOnce(Invoke(sessions::test_util::SimulateConnectionFailure))
+    .WillOnce(QuitLoopNowAction());
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   scheduler()->ScheduleNudgeAsync(