Make last_download_timestamp and initial_sync_ended per-datatype.

Persist them on a per-datatype basis.  Add a migration from the old
database scheme.

In DownloadUpdates, pick the datatype(s) with the lowest
last_download_timestamp; and fetch those. Keep running
DownloadUpdatesCommand until we've gotten a "no-timestamp" result when
requesting all datatypes.

BUG=33065,37359,37331,37369,37373
TEST=included unit tests.  Also, ran with Bookmark sync enabled, then added autofill the next time I started up, and observed that the incremental GetUpdates happened for autofill only, and that eventually the timestamps for bookmarks and autofill coalesced.

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

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

6 files changed, 105 insertions, 28 deletions

diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index eb59f94..6ac8971 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -19,6 +19,7 @@
 #include "chrome/browser/sync/engine/syncer_types.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/sessions/ordered_commit_set.h"
+#include "chrome/browser/sync/syncable/syncable.h"
 
 namespace syncable {
 class DirectoryManager;
@@ -235,7 +236,9 @@ struct AllModelTypeState {
   // Commits for all model types are bundled together into a single message.
   ClientToServerMessage commit_message;
   ClientToServerResponse commit_response;
-  // We GetUpdates for all desired types at once.
+  // We GetUpdates for some combination of types at once.
+  // requested_update_types stores the set of types which were requested.
+  syncable::MultiTypeTimeStamp updates_request_parameters;
   ClientToServerResponse updates_response;
   // Used to build the shared commit message.
   DirtyOnWrite<std::vector<int64> > unsynced_handles;
@@ -257,8 +260,8 @@ struct PerModelSafeGroupState {
 // Grouping of all state that applies to a single ModelType.
 struct PerModelTypeState {
   explicit PerModelTypeState(bool* dirty_flag)
-      : current_sync_timestamp(dirty_flag, 0) {}
-  DirtyOnWrite<int64> current_sync_timestamp;
+      : current_download_timestamp(dirty_flag, 0) {}
+  DirtyOnWrite<int64> current_download_timestamp;
 };
 
 }  // namespace sessions
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index cfd6fa8..cb15468 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -5,10 +5,14 @@
 #include "chrome/browser/sync/sessions/status_controller.h"
 
 #include "base/basictypes.h"
+#include "chrome/browser/sync/syncable/model_type.h"
 
 namespace browser_sync {
 namespace sessions {
 
+using syncable::FIRST_REAL_MODEL_TYPE;
+using syncable::MODEL_TYPE_COUNT;
+
 StatusController::StatusController(const ModelSafeRoutingInfo& routes)
     : shared_(&is_dirty_),
       per_model_group_deleter_(&per_model_group_),
@@ -81,11 +85,12 @@ void StatusController::set_num_consecutive_errors(int value) {
     shared_.error_counters.mutate()->consecutive_errors = value;
 }
 
-void StatusController::set_current_sync_timestamp(syncable::ModelType model,
-                                                  int64 current_timestamp) {
+void StatusController::set_current_download_timestamp(
+    syncable::ModelType model,
+    int64 current_timestamp) {
   PerModelTypeState* state = GetOrCreateModelTypeState(false, model);
-  if (current_timestamp > state->current_sync_timestamp.value())
-    *(state->current_sync_timestamp.mutate()) = current_timestamp;
+  if (current_timestamp > state->current_download_timestamp.value())
+    *(state->current_download_timestamp.mutate()) = current_timestamp;
 }
 
 void StatusController::set_num_server_changes_remaining(
@@ -184,8 +189,8 @@ int64 StatusController::ComputeMaxLocalTimestamp() const {
       per_model_type_.begin();
   int64 max_timestamp = 0;
   for (; it != per_model_type_.end(); ++it) {
-    if (it->second->current_sync_timestamp.value() > max_timestamp)
-      max_timestamp = it->second->current_sync_timestamp.value();
+    if (it->second->current_download_timestamp.value() > max_timestamp)
+      max_timestamp = it->second->current_download_timestamp.value();
   }
   return max_timestamp;
 }
@@ -214,5 +219,21 @@ int StatusController::TotalNumConflictingItems() const {
   return sum;
 }
 
+bool StatusController::ServerSaysNothingMoreToDownload() const {
+  if (!download_updates_succeeded())
+    return false;
+  // If we didn't request every enabled datatype, then we can't say for
+  // sure that there's nothing left to download.
+  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
+    if (!updates_request_parameters().data_types[i] &&
+        routing_info_.count(syncable::ModelTypeFromInt(i)) != 0) {
+      return false;
+    }
+  }
+  // The server indicates "you're up to date" by not sending a new
+  // timestamp.
+  return !updates_response().get_updates().has_new_timestamp();
+}
+
 }  // namespace sessions
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index ac7cbba..c7c89dd 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -82,6 +82,13 @@ class StatusController {
   ClientToServerResponse* mutable_commit_response() {
     return &shared_.commit_response;
   }
+  const syncable::MultiTypeTimeStamp& updates_request_parameters() const {
+    return shared_.updates_request_parameters;
+  }
+  void set_updates_request_parameters(
+      const syncable::MultiTypeTimeStamp& value) {
+    shared_.updates_request_parameters = value;
+  }
   const ClientToServerResponse& updates_response() const {
     return shared_.updates_response;
   }
@@ -166,14 +173,10 @@ class StatusController {
   // 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.
-  bool server_says_nothing_more_to_download() const {
-    if (!download_updates_succeeded())
-      return false;
-    // The server indicates "you're up to date" by not sending a new
-    // timestamp.
-    return !updates_response().get_updates().has_new_timestamp();
-  }
+  // 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;
 
   ModelSafeGroup group_restriction() const {
     return group_restriction_;
@@ -198,8 +201,8 @@ class StatusController {
   void set_num_consecutive_errors(int value);
   void increment_num_consecutive_errors();
   void increment_num_consecutive_errors_by(int value);
-  void set_current_sync_timestamp(syncable::ModelType model,
-                                  int64 current_timestamp);
+  void set_current_download_timestamp(syncable::ModelType model,
+                                      int64 current_timestamp);
   void set_num_server_changes_remaining(int64 changes_remaining);
   void set_over_quota(bool over_quota);
   void set_invalid_store(bool invalid_store);
diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc
index d1ee710..2f07039 100644
--- a/chrome/browser/sync/sessions/status_controller_unittest.cc
+++ b/chrome/browser/sync/sessions/status_controller_unittest.cc
@@ -49,7 +49,7 @@ TEST_F(StatusControllerTest, GetsDirty) {
 
   {
     ScopedModelSafeGroupRestriction r(&status, GROUP_UI);
-    status.set_current_sync_timestamp(syncable::BOOKMARKS, 100);
+    status.set_current_download_timestamp(syncable::BOOKMARKS, 100);
     EXPECT_TRUE(status.TestAndClearIsDirty());
   }
 
@@ -137,7 +137,7 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
 
   {
     ScopedModelSafeGroupRestriction r(&status, GROUP_UI);
-    status.set_current_sync_timestamp(syncable::BOOKMARKS, 12);
+    status.set_current_download_timestamp(syncable::BOOKMARKS, 12);
     EXPECT_EQ(12, status.ComputeMaxLocalTimestamp());
   }
 
@@ -239,7 +239,7 @@ TEST_F(StatusControllerTest, Unrestricted) {
   status.commit_ids();
   status.HasBookmarkCommitActivity();
   status.download_updates_succeeded();
-  status.server_says_nothing_more_to_download();
+  status.ServerSaysNothingMoreToDownload();
   status.group_restriction();
 }
 
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index 7e04d71..0c50d4d 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -27,13 +27,22 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
   if (!dir.good())
     LOG(ERROR) << "Scoped dir lookup failed!";
 
-  const bool is_share_usable = dir->initial_sync_ended();
+  // TODO(ncarter): Either move this to a function in the status controller,
+  // or else make the session snapshot have per-type initialsyncendedness.
+  bool is_share_useable = true;
+  for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) {
+    if (routing_info_.count(syncable::ModelTypeFromInt(i)) != 0 &&
+        !dir->initial_sync_ended_for_type(syncable::ModelTypeFromInt(i))) {
+      is_share_useable = false;
+    }
+  }
+
   return SyncSessionSnapshot(
       status_controller_->syncer_status(),
       status_controller_->error_counters(),
       status_controller_->num_server_changes_remaining(),
       status_controller_->ComputeMaxLocalTimestamp(),
-      is_share_usable,
+      is_share_useable,
       HasMoreToSync(),
       delegate_->IsSyncingCurrentlySilenced(),
       status_controller_->unsynced_handles().size(),
diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc
index 43f9769..4251f0d 100644
--- a/chrome/browser/sync/sessions/sync_session_unittest.cc
+++ b/chrome/browser/sync/sessions/sync_session_unittest.cc
@@ -12,6 +12,7 @@
 #include "chrome/test/sync/engine/test_directory_setter_upper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using syncable::MultiTypeTimeStamp;
 using syncable::WriteTransaction;
 
 namespace browser_sync {
@@ -54,6 +55,7 @@ class SyncSessionTest : public testing::Test,
   virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) {}
   virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {
     (*out)[syncable::BOOKMARKS] = GROUP_UI;
+    (*out)[syncable::AUTOFILL] = GROUP_UI;
   }
 
   StatusController* status() { return session_->status_controller(); }
@@ -62,6 +64,22 @@ class SyncSessionTest : public testing::Test,
     if (!controller_invocations_allowed_)
       FAIL() << msg;
   }
+
+  MultiTypeTimeStamp ParamsMeaningAllEnabledTypes() {
+    MultiTypeTimeStamp request_params;
+    request_params.timestamp = 2000;
+    request_params.data_types[syncable::BOOKMARKS] = true;
+    request_params.data_types[syncable::AUTOFILL] = true;
+    return request_params;
+  }
+
+  MultiTypeTimeStamp ParamsMeaningJustOneEnabledType() {
+    MultiTypeTimeStamp request_params;
+    request_params.timestamp = 5000;
+    request_params.data_types[syncable::AUTOFILL] = true;
+    return request_params;
+  }
+
   bool controller_invocations_allowed_;
   scoped_ptr<SyncSession> session_;
   scoped_ptr<SyncSessionContext> context_;
@@ -134,8 +152,10 @@ TEST_F(SyncSessionTest, MoreToSyncIfConflictSetsBuilt) {
 }
 
 TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
+  status()->set_updates_request_parameters(ParamsMeaningAllEnabledTypes());
+
   // When DownloadUpdatesCommand fails, these should be false.
-  EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+  EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
   EXPECT_FALSE(status()->download_updates_succeeded());
 
   // Download updates has its own loop in the syncer; it shouldn't factor
@@ -144,10 +164,12 @@ TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
 }
 
 TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestamp) {
+  status()->set_updates_request_parameters(ParamsMeaningAllEnabledTypes());
+
   // 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_FALSE(status()->ServerSaysNothingMoreToDownload());
   EXPECT_TRUE(status()->download_updates_succeeded());
 
   // Download updates has its own loop in the syncer; it shouldn't factor
@@ -156,10 +178,28 @@ TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestamp) {
 }
 
 TEST_F(SyncSessionTest, MoreToDownloadIfGotNoTimestamp) {
+  status()->set_updates_request_parameters(ParamsMeaningAllEnabledTypes());
+
   // 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()->ServerSaysNothingMoreToDownload());
+  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, MoreToDownloadIfGotNoTimestampForSubset) {
+  status()->set_updates_request_parameters(ParamsMeaningJustOneEnabledType());
+
+  // When the server returns a timestamp, that means we're up to date for that
+  // type.  But there may still be more to download if there are other
+  // datatypes that we didn't request on this go-round.
+  status()->mutable_updates_response()->mutable_get_updates()
+    ->clear_new_timestamp();
+  EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
   EXPECT_TRUE(status()->download_updates_succeeded());
 
   // Download updates has its own loop in the syncer; it shouldn't factor
@@ -168,12 +208,13 @@ TEST_F(SyncSessionTest, MoreToDownloadIfGotNoTimestamp) {
 }
 
 TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestampAndEntries) {
+  status()->set_updates_request_parameters(ParamsMeaningAllEnabledTypes());
   // 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_FALSE(status()->ServerSaysNothingMoreToDownload());
   EXPECT_TRUE(status()->download_updates_succeeded());
 
   // Download updates has its own loop in the syncer; it shouldn't factor