sync: Merge GU retry logic into normal GU

Moves the implementation of the GU retry get updates into the normal GU
cycle.  This should have no impact on behvaior.  The point of this
refactoring is to eliminate an instance of the GetUpdateDelegate.  I
hope to build on that interface in the future, and removing one of its
four implementations should make that work 25% easier.

This CL should retain all the same quirks as the old retry
implemenation.  The timer management is the same.  It also sends up a
special RETRY value for GetUpdatesOrigin when the only reason for
performing an update is a scheduled retry, which was the behavior of the
old code.

This CL also refactors the NudgeTracker's mangement of the
updates_source_.  The old, stateful, implementation was getting out of
hand.  The new implementation should be easier to maintain, especially
as we start to support 'partially successful' sync cycles.

BUG=339984

Review URL: https://codereview.chromium.org/171813013

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

8 files changed, 28 insertions, 105 deletions

diff --git a/sync/engine/get_updates_delegate.cc b/sync/engine/get_updates_delegate.cc
index d0c7a17..4697485 100644
--- a/sync/engine/get_updates_delegate.cc
+++ b/sync/engine/get_updates_delegate.cc
@@ -46,12 +46,17 @@ void NormalGetUpdatesDelegate::HelpPopulateGuMessage(
     sync_pb::GetUpdatesMessage* get_updates) const {
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(
-      nudge_tracker_.updates_source());
+      nudge_tracker_.GetLegacySource());
 
   // Set the new and improved version of source, too.
   get_updates->set_get_updates_origin(sync_pb::SyncEnums::GU_TRIGGER);
   get_updates->set_is_retry(nudge_tracker_.IsRetryRequired());
 
+  // Special case: A GU performed for no other reason than retry will have its
+  // origin set to RETRY.
+  if (nudge_tracker_.GetLegacySource() == sync_pb::GetUpdatesCallerInfo::RETRY)
+    get_updates->set_get_updates_origin(sync_pb::SyncEnums::RETRY);
+
   // Fill in the notification hints.
   for (int i = 0; i < get_updates->from_progress_marker_size(); ++i) {
     sync_pb::DataTypeProgressMarker* progress_marker =
@@ -75,27 +80,6 @@ void NormalGetUpdatesDelegate::ApplyUpdates(
   NonPassiveApplyUpdates(status_controller, update_handler_map);
 }
 
-RetryGetUpdatesDelegate::RetryGetUpdatesDelegate() {}
-
-RetryGetUpdatesDelegate::~RetryGetUpdatesDelegate() {}
-
-void RetryGetUpdatesDelegate::HelpPopulateGuMessage(
-    sync_pb::GetUpdatesMessage* get_updates) const {
-  // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
-  get_updates->mutable_caller_info()->set_source(
-      sync_pb::GetUpdatesCallerInfo::RETRY);
-
-  // Set the new and improved version of source, too.
-  get_updates->set_get_updates_origin(sync_pb::SyncEnums::RETRY);
-  get_updates->set_is_retry(true);
-}
-
-void RetryGetUpdatesDelegate::ApplyUpdates(
-    sessions::StatusController* status_controller,
-    UpdateHandlerMap* update_handler_map) const {
-  NonPassiveApplyUpdates(status_controller, update_handler_map);
-}
-
 ConfigureGetUpdatesDelegate::ConfigureGetUpdatesDelegate(
     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source) : source_(source) {}
 
diff --git a/sync/engine/get_updates_delegate.h b/sync/engine/get_updates_delegate.h
index 825c061..9ebc6d8 100644
--- a/sync/engine/get_updates_delegate.h
+++ b/sync/engine/get_updates_delegate.h
@@ -54,23 +54,6 @@ class SYNC_EXPORT_PRIVATE NormalGetUpdatesDelegate : public GetUpdatesDelegate {
   const sessions::NudgeTracker& nudge_tracker_;
 };
 
-// Functionality specific to the retry GetUpdate request.
-class SYNC_EXPORT_PRIVATE RetryGetUpdatesDelegate : public GetUpdatesDelegate {
- public:
-  RetryGetUpdatesDelegate();
-  virtual ~RetryGetUpdatesDelegate();
-
-  virtual void HelpPopulateGuMessage(
-      sync_pb::GetUpdatesMessage* get_updates) const OVERRIDE;
-
-  virtual void ApplyUpdates(
-      sessions::StatusController* status,
-      UpdateHandlerMap* update_handler_map) const OVERRIDE;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(RetryGetUpdatesDelegate);
-};
-
 // Functionality specific to the configure GetUpdate request.
 class SYNC_EXPORT_PRIVATE ConfigureGetUpdatesDelegate
     : public GetUpdatesDelegate {
diff --git a/sync/engine/get_updates_processor_unittest.cc b/sync/engine/get_updates_processor_unittest.cc
index f8fd274..c494289 100644
--- a/sync/engine/get_updates_processor_unittest.cc
+++ b/sync/engine/get_updates_processor_unittest.cc
@@ -215,9 +215,9 @@ TEST_F(GetUpdatesProcessorTest, RetryTest) {
   nudge_tracker.SetSyncCycleStartTime(t1 + base::TimeDelta::FromSeconds(1));
 
   sync_pb::ClientToServerMessage message;
-  RetryGetUpdatesDelegate retry_delegate;
+  NormalGetUpdatesDelegate normal_delegate(nudge_tracker);
   scoped_ptr<GetUpdatesProcessor> processor(
-      BuildGetUpdatesProcessor(retry_delegate));
+      BuildGetUpdatesProcessor(normal_delegate));
   processor->PrepareGetUpdates(request_types(), &message);
 
   const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index ddd5baa..6ff250f 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -238,8 +238,7 @@ void SyncSchedulerImpl::Start(Mode mode) {
 
     // Update our current time before checking IsRetryRequired().
     nudge_tracker_.SetSyncCycleStartTime(base::TimeTicks::Now());
-    if ((nudge_tracker_.IsSyncRequired() || nudge_tracker_.IsRetryRequired()) &&
-        CanRunNudgeJobNow(NORMAL_PRIORITY)) {
+    if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(NORMAL_PRIORITY)) {
       TrySyncSessionJob();
     }
   }
@@ -572,25 +571,6 @@ void SyncSchedulerImpl::DoPollSyncSessionJob() {
   }
 }
 
-void SyncSchedulerImpl::DoRetrySyncSessionJob() {
-  DCHECK(CalledOnValidThread());
-  DCHECK_EQ(mode_, NORMAL_MODE);
-
-  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
-
-  SDVLOG(2) << "Retrying with types "
-            << ModelTypeSetToString(GetEnabledAndUnthrottledTypes());
-  scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
-  if (syncer_->RetrySyncShare(GetEnabledAndUnthrottledTypes(),
-                              session.get()) &&
-      !sessions::HasSyncerError(
-          session->status_controller().model_neutral_state())) {
-    nudge_tracker_.RecordSuccessfulSyncCycle();
-  } else {
-    HandleFailure(session->status_controller().model_neutral_state());
-  }
-}
-
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(ModelTypeSet types) {
   DCHECK(CalledOnValidThread());
   base::TimeTicks now = TimeTicks::Now();
@@ -704,8 +684,6 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() {
     if (nudge_tracker_.IsSyncRequired()) {
       SDVLOG(2) << "Found pending nudge job";
       DoNudgeSyncSessionJob(priority);
-    } else if (nudge_tracker_.IsRetryRequired()) {
-      DoRetrySyncSessionJob();
     } else if (do_poll_after_credentials_updated_ ||
         ((base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval())) {
       DoPollSyncSessionJob();
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 7cbf324f..75cad3e 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -161,9 +161,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // Invoke the Syncer to perform a poll job.
   void DoPollSyncSessionJob();
 
-  // Invoke the Syncer to perform a retry job.
-  void DoRetrySyncSessionJob();
-
   // Helper function to calculate poll interval.
   base::TimeDelta GetPollInterval();
 
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index b056717..45ebf19 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -52,7 +52,6 @@ class MockSyncer : public Syncer {
                     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource,
                     SyncSession*));
   MOCK_METHOD2(PollSyncShare, bool(ModelTypeSet, sessions::SyncSession*));
-  MOCK_METHOD2(RetrySyncShare, bool(ModelTypeSet, sessions::SyncSession*));
 };
 
 MockSyncer::MockSyncer()
@@ -552,7 +551,7 @@ TEST_F(SyncSchedulerTest, Polling) {
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
   EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                 RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   scheduler()->OnReceivedLongPollIntervalUpdate(poll_interval);
@@ -573,7 +572,7 @@ TEST_F(SyncSchedulerTest, PollNotificationsDisabled) {
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
   EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                 RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   scheduler()->OnReceivedShortPollIntervalUpdate(poll_interval);
@@ -601,7 +600,7 @@ TEST_F(SyncSchedulerTest, PollIntervalUpdate) {
               sessions::test_util::SimulatePollIntervalUpdate(poll2)),
           Return(true)))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                 WithArg<1>(
                     RecordSyncShareMultiple(&times, kMinNumSamples))));
 
@@ -693,7 +692,7 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromPoll) {
       .RetiresOnSaturation();
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                 RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   TimeTicks optimal_start = TimeTicks::Now() + poll + throttle1;
@@ -1126,7 +1125,7 @@ TEST_F(SyncSchedulerTest, BackoffRelief) {
   // Now let the Poll timer do its thing.
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
       .WillRepeatedly(DoAll(
-              Invoke(sessions::test_util::SimulatePollRetrySuccess),
+              Invoke(sessions::test_util::SimulatePollSuccess),
               RecordSyncShareMultiple(&times, kMinNumSamples)));
   RunLoop();
   Mock::VerifyAndClearExpectations(syncer());
@@ -1149,9 +1148,9 @@ TEST_F(SyncSchedulerTest, TransientPollFailure) {
   UseMockDelayProvider(); // Will cause test failure if backoff is initiated.
 
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollFailed),
                       RecordSyncShare(&times)))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                       RecordSyncShare(&times)));
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ -1285,7 +1284,7 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
   ::testing::InSequence seq;
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                 RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   connection()->SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
@@ -1298,7 +1297,7 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
   // but after poll finished with auth error from poll timer it should retry
   // poll once more
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
                       RecordSyncShare(&times)));
   scheduler()->OnCredentialsUpdated();
   connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
@@ -1314,9 +1313,9 @@ TEST_F(SyncSchedulerTest, SuccessfulRetry) {
   scheduler()->OnReceivedGuRetryDelay(delay);
   EXPECT_EQ(delay, GetRetryTimerDelay());
 
-  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+  EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_))
       .WillOnce(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulateNormalSuccess),
                 RecordSyncShare(&times)));
 
   // Run to wait for retrying.
@@ -1335,18 +1334,18 @@ TEST_F(SyncSchedulerTest, FailedRetry) {
   base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1);
   scheduler()->OnReceivedGuRetryDelay(delay);
 
-  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+  EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_))
       .WillOnce(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed),
+          DoAll(Invoke(sessions::test_util::SimulateDownloadUpdatesFailed),
                 QuitLoopNowAction()));
 
   // Run to wait for retrying.
   RunLoop();
 
   EXPECT_TRUE(scheduler()->IsBackingOff());
-  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+  EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_))
       .WillOnce(
-          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+          DoAll(Invoke(sessions::test_util::SimulateNormalSuccess),
                 QuitLoopNowAction()));
 
   // Run to wait for second retrying.
@@ -1381,8 +1380,8 @@ TEST_F(SyncSchedulerTest, ReceiveNewRetryDelay) {
   RunLoop();
   EXPECT_EQ(delay2, GetRetryTimerDelay());
 
-  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+  EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateNormalSuccess),
                       RecordSyncShare(&times)));
 
   // Run to wait for retrying.
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 240332c..c752510 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -70,7 +70,7 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types,
             session,
             &get_updates_processor,
             kCreateMobileBookmarksFolder)) {
-      return HandleCycleEnd(session, nudge_tracker.updates_source());
+      return HandleCycleEnd(session, nudge_tracker.GetLegacySource());
     }
   }
 
@@ -81,7 +81,7 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types,
       BuildAndPostCommits(request_types, session, &commit_processor);
   session->mutable_status_controller()->set_commit_result(commit_result);
 
-  return HandleCycleEnd(session, nudge_tracker.updates_source());
+  return HandleCycleEnd(session, nudge_tracker.GetLegacySource());
 }
 
 bool Syncer::ConfigureSyncShare(
@@ -118,22 +118,6 @@ bool Syncer::PollSyncShare(ModelTypeSet request_types,
   return HandleCycleEnd(session, sync_pb::GetUpdatesCallerInfo::PERIODIC);
 }
 
-bool Syncer::RetrySyncShare(ModelTypeSet request_types,
-                            SyncSession* session) {
-  VLOG(1) << "Retrying types " << ModelTypeSetToString(request_types);
-  HandleCycleBegin(session);
-  RetryGetUpdatesDelegate retry_delegate;
-  GetUpdatesProcessor get_updates_processor(
-      session->context()->model_type_registry()->update_handler_map(),
-      retry_delegate);
-  DownloadAndApplyUpdates(
-      request_types,
-      session,
-      &get_updates_processor,
-      kCreateMobileBookmarksFolder);
-  return HandleCycleEnd(session, sync_pb::GetUpdatesCallerInfo::RETRY);
-}
-
 bool Syncer::DownloadAndApplyUpdates(
     ModelTypeSet request_types,
     SyncSession* session,
diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h
index a1aeff8..02e66e3 100644
--- a/sync/engine/syncer.h
+++ b/sync/engine/syncer.h
@@ -67,8 +67,6 @@ class SYNC_EXPORT_PRIVATE Syncer {
   // in sync despite bugs or transient failures.
   virtual bool PollSyncShare(ModelTypeSet request_types,
                              sessions::SyncSession* session);
-  virtual bool RetrySyncShare(ModelTypeSet request_types,
-                              sessions::SyncSession* session);
 
  private:
   bool DownloadAndApplyUpdates(