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

5 files changed, 76 insertions, 36 deletions

diff --git a/sync/sessions/session_state_unittest.cc b/sync/sessions/session_state_unittest.cc
index 92d4b6f..9d20437 100644
--- a/sync/sessions/session_state_unittest.cc
+++ b/sync/sessions/session_state_unittest.cc
@@ -90,7 +90,8 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                                source,
                                false,
                                0,
-                               base::Time::Now());
+                               base::Time::Now(),
+                               false);
   scoped_ptr<DictionaryValue> value(snapshot.ToValue());
   EXPECT_EQ(20u, value->size());
   ExpectDictIntegerValue(model_neutral.num_successful_commits,
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 9be78d8..5a2b572 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -77,7 +77,8 @@ SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate,
       delegate_(delegate),
       workers_(workers),
       routing_info_(routing_info),
-      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)) {
+      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)),
+      finished_(false) {
   status_controller_.reset(new StatusController(routing_info_));
   std::sort(workers_.begin(), workers_.end());
 }
@@ -114,31 +115,30 @@ void SyncSession::Coalesce(const SyncSession& session) {
   enabled_groups_ = ComputeEnabledGroups(routing_info_, workers_);
 }
 
-void SyncSession::RebaseRoutingInfoWithLatest(
-    const ModelSafeRoutingInfo& routing_info,
-    const std::vector<ModelSafeWorker*>& workers) {
+void SyncSession::RebaseRoutingInfoWithLatest(const SyncSession& session) {
   ModelSafeRoutingInfo temp_routing_info;
 
-  // Take the intersection and also set the routing info(it->second) from the
+  // Take the intersecion and also set the routing info(it->second) from the
   // passed in session.
   for (ModelSafeRoutingInfo::const_iterator it =
-       routing_info.begin(); it != routing_info.end();
+       session.routing_info_.begin(); it != session.routing_info_.end();
        ++it) {
     if (routing_info_.find(it->first) != routing_info_.end()) {
       temp_routing_info[it->first] = it->second;
     }
   }
+
+  // Now swap it.
   routing_info_.swap(temp_routing_info);
 
-  PurgeStaleStates(&source_.types, routing_info);
+  // Now update the payload map.
+  PurgeStaleStates(&source_.types, session.routing_info_);
 
   // Now update the workers.
   std::vector<ModelSafeWorker*> temp;
-  std::vector<ModelSafeWorker*> sorted_workers = workers;
-  std::sort(sorted_workers.begin(), sorted_workers.end());
   std::set_intersection(workers_.begin(), workers_.end(),
-                        sorted_workers.begin(), sorted_workers.end(),
-                        std::back_inserter(temp));
+                 session.workers_.begin(), session.workers_.end(),
+                 std::back_inserter(temp));
   workers_.swap(temp);
 
   // Now update enabled groups.
@@ -146,6 +146,7 @@ void SyncSession::RebaseRoutingInfoWithLatest(
 }
 
 void SyncSession::PrepareForAnotherSyncCycle() {
+  finished_ = false;
   source_.updates_source =
       sync_pb::GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION;
   status_controller_.reset(new StatusController(routing_info_));
@@ -182,7 +183,8 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
       source_,
       context_->notifications_enabled(),
       dir->GetEntriesCount(),
-      status_controller_->sync_start_time());
+      status_controller_->sync_start_time(),
+      !Succeeded());
 }
 
 void SyncSession::SendEventNotification(SyncEngineEvent::EventCause cause) {
@@ -217,11 +219,35 @@ std::set<ModelSafeGroup> SyncSession::GetEnabledGroupsWithConflicts() const {
   return enabled_groups_with_conflicts;
 }
 
-bool SyncSession::DidReachServer() const {
+namespace {
+
+// Returns false iff one of the command results had an error.
+bool HadErrors(const ModelNeutralState& state) {
+  const bool get_key_error = SyncerErrorIsError(state.last_get_key_result);
+  const bool download_updates_error =
+      SyncerErrorIsError(state.last_download_updates_result);
+  const bool commit_error = SyncerErrorIsError(state.commit_result);
+  return get_key_error || download_updates_error || commit_error;
+}
+}  // namespace
+
+bool SyncSession::Succeeded() const {
+  return finished_ && !HadErrors(status_controller_->model_neutral_state());
+}
+
+bool SyncSession::SuccessfullyReachedServer() const {
   const ModelNeutralState& state = status_controller_->model_neutral_state();
-  return state.last_get_key_result >= FIRST_SERVER_RETURN_VALUE ||
-      state.last_download_updates_result >= FIRST_SERVER_RETURN_VALUE ||
-      state.commit_result >= FIRST_SERVER_RETURN_VALUE;
+  bool reached_server = state.last_get_key_result == SYNCER_OK ||
+                        state.last_download_updates_result == SYNCER_OK;
+  // It's possible that we reached the server on one attempt, then had an error
+  // on the next (or didn't perform some of the server-communicating commands).
+  // We want to verify that, for all commands attempted, we successfully spoke
+  // with the server. Therefore, we verify no errors and at least one SYNCER_OK.
+  return reached_server && !HadErrors(state);
+}
+
+void SyncSession::SetFinished() {
+  finished_ = true;
 }
 
 }  // namespace sessions
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 4026e88..93d7f41 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -112,11 +112,25 @@ class SyncSession {
   // engine again.
   bool HasMoreToSync() const;
 
-  // Returns true if we reached the server. Note that "reaching the server"
-  // here means that from an HTTP perspective, we succeeded (HTTP 200).  The
-  // server **MAY** have returned a sync protocol error.
-  // See SERVER_RETURN_* in the SyncerError enum for values.
-  bool DidReachServer() const;
+  // Returns true if we completely ran the session without errors.
+  //
+  // There are many errors that could prevent a sync cycle from succeeding.
+  // These include invalid local state, inability to contact the server,
+  // inability to authenticate with the server, and server errors.  What they
+  // have in common is that the we either need to take some action and then
+  // retry the sync cycle or, in the case of transient errors, retry after some
+  // backoff timer has expired.  Most importantly, the SyncScheduler should not
+  // assume that the original action that triggered the sync cycle (ie. a nudge
+  // or a notification) has been properly serviced.
+  //
+  // This function also returns false if SyncShare has not been called on this
+  // session yet, or if ResetTransientState() has been called on this session
+  // since the last call to SyncShare.
+  bool Succeeded() const;
+
+  // Returns true if we reached the server successfully and the server did not
+  // return any error codes. Returns false if no connection was attempted.
+  bool SuccessfullyReachedServer() const;
 
   // Collects all state pertaining to how and why |s| originated and unions it
   // with corresponding state in |this|, leaving |s| unchanged.  Allows |this|
@@ -125,13 +139,11 @@ class SyncSession {
   // sessions.
   void Coalesce(const SyncSession& session);
 
-  // Compares the routing_info_, workers and payload map with those passed in.
-  // Purges types from the above 3 which are not present in latest. Useful
+  // Compares the routing_info_, workers and payload map with the passed in
+  // session. Purges types from the above 3 which are not in session. Useful
   // to update the sync session when the user has disabled some types from
   // syncing.
-  void RebaseRoutingInfoWithLatest(
-      const ModelSafeRoutingInfo& routing_info,
-      const std::vector<ModelSafeWorker*>& workers);
+  void RebaseRoutingInfoWithLatest(const SyncSession& session);
 
   // Should be called any time |this| is being re-used in a new call to
   // SyncShare (e.g., HasMoreToSync returned true).
@@ -165,6 +177,9 @@ class SyncSession {
   // Returns the set of enabled groups that have conflicts.
   std::set<ModelSafeGroup> GetEnabledGroupsWithConflicts() const;
 
+  // Mark the session has having finished all the sync steps it needed.
+  void SetFinished();
+
  private:
   // Extend the encapsulation boundary to utilities for internal member
   // assignments. This way, the scope of these actions is explicit, they can't
@@ -202,6 +217,10 @@ class SyncSession {
   // |routing_info_|.
   std::set<ModelSafeGroup> enabled_groups_;
 
+  // Whether this session has reached its last step or not. Gets reset on each
+  // new cycle (via PrepareForAnotherSyncCycle).
+  bool finished_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSession);
 };
 
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index e23f6f6..285f33a 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -347,7 +347,7 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestRemoveOneType) {
   EXPECT_EQ(expected_enabled_groups_one, one.GetEnabledGroups());
   EXPECT_EQ(expected_enabled_groups_two, two.GetEnabledGroups());
 
-  two.RebaseRoutingInfoWithLatest(one.routing_info(), one.workers());
+  two.RebaseRoutingInfoWithLatest(one);
 
   EXPECT_EQ(expected_enabled_groups_one, one.GetEnabledGroups());
   EXPECT_EQ(expected_enabled_groups_one, two.GetEnabledGroups());
@@ -417,7 +417,7 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestWithSameType) {
   EXPECT_EQ(expected_enabled_groups, first.GetEnabledGroups());
   EXPECT_EQ(expected_enabled_groups, second.GetEnabledGroups());
 
-  second.RebaseRoutingInfoWithLatest(first.routing_info(), first.workers());
+  second.RebaseRoutingInfoWithLatest(first);
 
   EXPECT_EQ(expected_enabled_groups, first.GetEnabledGroups());
   EXPECT_EQ(expected_enabled_groups, second.GetEnabledGroups());
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index cef5121..4214c3d 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -31,9 +31,6 @@ void SimulateDownloadUpdatesFailed(sessions::SyncSession* session,
 
 void SimulateCommitFailed(sessions::SyncSession* session,
                           SyncerStep begin, SyncerStep end) {
-  session->mutable_status_controller()->set_last_get_key_result(SYNCER_OK);
-  session->mutable_status_controller()->set_last_download_updates_result(
-      SYNCER_OK);
   session->mutable_status_controller()->set_commit_result(
       SERVER_RETURN_TRANSIENT_ERROR);
 }
@@ -50,6 +47,7 @@ void SimulateSuccess(sessions::SyncSession* session,
     ADD_FAILURE() << "Shouldn't have more to sync";
   }
   ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining());
+  session->SetFinished();
   switch(end) {
     case SYNCER_END:
       session->mutable_status_controller()->set_commit_result(SYNCER_OK);
@@ -68,20 +66,16 @@ void SimulateSuccess(sessions::SyncSession* session,
 
 void SimulateThrottledImpl(sessions::SyncSession* session,
     const base::TimeDelta& delta) {
-  session->mutable_status_controller()->set_last_download_updates_result(
-      SERVER_RETURN_THROTTLED);
   session->delegate()->OnSilencedUntil(base::TimeTicks::Now() + delta);
 }
 
 void SimulatePollIntervalUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_poll) {
-  SimulateSuccess(session, SYNCER_BEGIN, SYNCER_END);
   session->delegate()->OnReceivedLongPollIntervalUpdate(new_poll);
 }
 
 void SimulateSessionsCommitDelayUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_delay) {
-  SimulateSuccess(session, SYNCER_BEGIN, SYNCER_END);
   session->delegate()->OnReceivedSessionsCommitDelay(new_delay);
 }