Make it clear what last_sync_timestamp actually tracks. Update

last_sync_timestamp from the new_timestamp only, never from per-entry
timestamps.  Use what the server sends us to know whether or not there
are more updates to fetch.  Eliminate some unnecessarily complicated
logic having to do with the # of updates returned -- that's always a red
herring; with server-side filtering, it is indeed possible for 0 updates
to be returned along with a new timestamp.

BUG=37373
TEST=manual testing of 2 browser sync; unit tests.

Review URL: http://codereview.chromium.org/1161006

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

1 files changed, 46 insertions, 19 deletions

diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc
index 85c19cd..43f9769 100644
--- a/chrome/browser/sync/sessions/sync_session_unittest.cc
+++ b/chrome/browser/sync/sessions/sync_session_unittest.cc
@@ -133,18 +133,55 @@ TEST_F(SyncSessionTest, MoreToSyncIfConflictSetsBuilt) {
   EXPECT_TRUE(session_->HasMoreToSync());
 }
 
-TEST_F(SyncSessionTest, MoreToSyncIfDidNotGetZeroUpdates) {
-  // We're not done getting updates until we get an empty response.
-  ClientToServerResponse response;
-  response.mutable_get_updates()->add_entries();
-  status()->mutable_updates_response()->CopyFrom(response);
-  EXPECT_TRUE(session_->HasMoreToSync());
-  status()->mutable_updates_response()->Clear();
+TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
+  // When DownloadUpdatesCommand fails, these should be false.
+  EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+  EXPECT_FALSE(status()->download_updates_succeeded());
+
+  // Download updates has its own loop in the syncer; it shouldn't factor
+  // into HasMoreToSync.
+  EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestamp) {
+  // When the server returns a timestamp, that means there's more to download.
+  status()->mutable_updates_response()->mutable_get_updates()
+      ->set_new_timestamp(1000000L);
+  EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+  EXPECT_TRUE(status()->download_updates_succeeded());
+
+  // Download updates has its own loop in the syncer; it shouldn't factor
+  // into HasMoreToSync.
+  EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+TEST_F(SyncSessionTest, MoreToDownloadIfGotNoTimestamp) {
+  // When the server returns a timestamp, that means we're up to date.
+  status()->mutable_updates_response()->mutable_get_updates()
+      ->clear_new_timestamp();
+  EXPECT_TRUE(status()->server_says_nothing_more_to_download());
+  EXPECT_TRUE(status()->download_updates_succeeded());
+
+  // Download updates has its own loop in the syncer; it shouldn't factor
+  // into HasMoreToSync.
   EXPECT_FALSE(session_->HasMoreToSync());
-  status()->mutable_updates_response()->CopyFrom(response);
-  EXPECT_TRUE(session_->HasMoreToSync());
 }
 
+TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestampAndEntries) {
+  // The actual entry count should not factor into the HasMoreToSync
+  // determination.
+  status()->mutable_updates_response()->mutable_get_updates()->add_entries();
+  status()->mutable_updates_response()->mutable_get_updates()
+      ->set_new_timestamp(1000000L);;
+  EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+  EXPECT_TRUE(status()->download_updates_succeeded());
+
+  // Download updates has its own loop in the syncer; it shouldn't factor
+  // into HasMoreToSync.
+  EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+
 TEST_F(SyncSessionTest, MoreToSyncIfConflictsResolved) {
   // Conflict resolution happens after get updates and commit,
   // so we need to loop back and get updates / commit again now
@@ -153,16 +190,6 @@ TEST_F(SyncSessionTest, MoreToSyncIfConflictsResolved) {
   EXPECT_TRUE(session_->HasMoreToSync());
 }
 
-TEST_F(SyncSessionTest, MoreToSyncIfTimestampDirty) {
-  // If there are more changes on the server that weren't processed during this
-  // GetUpdates request, the client should send another GetUpdates request and
-  // use new_timestamp as the from_timestamp value within GetUpdatesMessage.
-  status()->set_got_new_timestamp();
-  status()->update_conflicts_resolved(true);
-  EXPECT_TRUE(session_->HasMoreToSync());
-}
-
-
 }  // namespace
 }  // namespace sessions
 }  // namespace browser_sync