sync: Refactor GetUpdates loop

This is a follow-up to r238532.  I had wanted to implement this change
with that CL, but decided it would be easier to split them up into
independent changes.

This refactoring pulls the GetUpdates looping logic out of the
StatusController.  It was implemented that way in a time when the only
way to pass state between different parts of the sync cycle was through
a shared SyncSession and its members.  Now that this is no longer the
case, we can keep the "should continue looping" logic more or less local
to the code that performs  the looping.

The change allows us to remove the updates response message proto member
out of the StatusController.  That could save us a small amount of time,
since copying protos can be somewhat expensive.  More importantly, it
reduces complexity, since we no longer need to worry about the effects
of state stored in the StatusController.  The StatusController is now
used only to collect some counts for debugging.

This has already uncovered a bug.  There was a unit test that failed to
reset its SyncSession in the same manner that it would have been reset
in a non-test scenario.  It depended on the state that was "leaked" from
one sync cycle to the next via the unreset SyncSession in order to meet
its test expectations.

BUG=278484

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

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

4 files changed, 0 insertions, 84 deletions

diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 314a232..752b9ab 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -103,17 +103,6 @@ SyncerError StatusController::last_get_key_result() const {
   return model_neutral_.last_get_key_result;
 }
 
-// Returns the number of updates received from the sync server.
-int64 StatusController::CountUpdates() const {
-  const sync_pb::ClientToServerResponse& updates =
-      model_neutral_.updates_response;
-  if (updates.has_get_updates()) {
-    return updates.get_updates().entries().size();
-  } else {
-    return 0;
-  }
-}
-
 int StatusController::num_updates_applied() const {
   return model_neutral_.num_updates_applied;
 }
@@ -142,18 +131,5 @@ int StatusController::TotalNumConflictingItems() const {
   return sum;
 }
 
-bool StatusController::ServerSaysNothingMoreToDownload() const {
-  if (!download_updates_succeeded())
-    return false;
-
-  if (!updates_response().get_updates().has_changes_remaining()) {
-    NOTREACHED();  // Server should always send changes remaining.
-    return false;  // Avoid looping forever.
-  }
-  // Changes remaining is an estimate, but if it's estimated to be
-  // zero, that's firm and we don't have to ask again.
-  return updates_response().get_updates().changes_remaining() == 0;
-}
-
 }  // namespace sessions
 }  // namespace syncer
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 0e2c6f2..005f158 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -42,12 +42,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
   void set_commit_request_types(ModelTypeSet value) {
     model_neutral_.commit_request_types = value;
   }
-  const sync_pb::ClientToServerResponse& updates_response() const {
-    return model_neutral_.updates_response;
-  }
-  sync_pb::ClientToServerResponse* mutable_updates_response() {
-    return &model_neutral_.updates_response;
-  }
 
   // Changelog related state.
   int64 num_server_changes_remaining() const {
@@ -67,24 +61,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
 
   int num_server_overwrites() const;
 
-  // Returns the number of updates received from the sync server.
-  int64 CountUpdates() const;
-
-  // Returns true if the last download_updates_command received a valid
-  // server response.
-  bool download_updates_succeeded() const {
-    return model_neutral_.last_download_updates_result
-        == SYNCER_OK;
-  }
-
-  // Returns true if the last updates response indicated that we were fully
-  // up to date.  This is subtle: if it's false, it could either mean that
-  // the server said there WAS more to download, or it could mean that we
-  // were unable to reach the server.  If we didn't request every enabled
-  // datatype, then we can't say for sure that there's nothing left to
-  // download: in that case, this also returns false.
-  bool ServerSaysNothingMoreToDownload() const;
-
   base::Time sync_start_time() const {
     // The time at which we sent the first GetUpdates command for this sync.
     return sync_start_time_;
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index 32e7b64..c29bc5f 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -31,16 +31,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   EXPECT_EQ(14, status.model_neutral_state().num_successful_commits);
 }
 
-TEST_F(StatusControllerTest, CountUpdates) {
-  StatusController status;
-  EXPECT_EQ(0, status.CountUpdates());
-  sync_pb::ClientToServerResponse* response(status.mutable_updates_response());
-  sync_pb::SyncEntity* entity1 = response->mutable_get_updates()->add_entries();
-  sync_pb::SyncEntity* entity2 = response->mutable_get_updates()->add_entries();
-  ASSERT_TRUE(entity1 != NULL && entity2 != NULL);
-  EXPECT_EQ(2, status.CountUpdates());
-}
-
 // Test TotalNumConflictingItems
 TEST_F(StatusControllerTest, TotalNumConflictingItems) {
   StatusController status;
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index c93a1ed..e712552 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -145,32 +145,6 @@ class SyncSessionTest : public testing::Test,
   scoped_refptr<ExtensionsActivity> extensions_activity_;
 };
 
-TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
-  status()->set_last_download_updates_result(NETWORK_IO_ERROR);
-
-  // When DownloadUpdatesCommand fails, these should be false.
-  EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_FALSE(status()->download_updates_succeeded());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotChangesRemaining) {
-  // When the server returns changes_remaining, that means there's
-  // more to download.
-  status()->set_last_download_updates_result(SYNCER_OK);
-  status()->mutable_updates_response()->mutable_get_updates()
-     ->set_changes_remaining(1000L);
-  EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotNoChangesRemaining) {
-  status()->set_last_download_updates_result(SYNCER_OK);
-  status()->mutable_updates_response()->mutable_get_updates()
-      ->set_changes_remaining(0);
-  EXPECT_TRUE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-}
-
 }  // namespace
 }  // namespace sessions
 }  // namespace syncer