sync: Loop committing items without downloading updates

Since its inception sync has required all commits to be preceded by a
GetUpdates request. This was done to try to ensure we detect and resolve
conflicts on the client, rather than having two conflicting commits
collide at the server. It could never work perfectly, but it was likely
to work in most cases and the server would bounce the commit if it
didn't.

Now we have a new server that doesn't always give us the latest version
of a node when we ask for it, especially when the request was not
solicited by the server (ie. not triggered by notification). The update
before commit is now much less likely to detect conflicts. Even worse,
the useless update requests can be surprisingly expensive in certain
scenarios.

This change allows us to avoid fetching updates between 'batches' of
commits.  This should improve performance in the case where we have lots
of items to commit (ie. first time sync) and reduce load on the server.

This CL has some far-reaching effects. This is in part because I decided
to refactor the commit code, but major changes would have been required
with or without the refactor. Highlights include:
- The commit-related SyncerCommands are now paramaterized with local
  variables, allowing us to remove many members from the SyncSession
  classes.
- Several syncer states have been collapsed into one COMMIT state
  which redirects to the new BuildAndPostCommits() function.
- The PostCommitMessageCommand had been reduced to one line, which
  has now been inlined into the BuildAndPostCommits() function.
- The unsynced_handles counter has been removed for now. Following this
  change, it would have always been zero unless an error was encountered
  during the commit. The functions that reference it expect different
  behaviour and would have been broken by this change.
- The code to put extensions activity data back into the
  ExtensionsActivityMonitor in case of failure has been moved around to
  avoid double-counting if we have to post many commit messages.
- A CONFLICT response from the server is now treated as a transient
  error.  If we had continued to treat it as a success we might risk
  going into an infinite loop. See comment in code for details.
- The "purposeless anachronism" conflicting_new_folder_ids_ has been
  removed.

Review URL: https://chromiumcodereview.appspot.com/10210009

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

12 files changed, 71 insertions, 184 deletions

diff --git a/sync/sessions/ordered_commit_set.cc b/sync/sessions/ordered_commit_set.cc
index 51a354e..c3fc5d0 100644
--- a/sync/sessions/ordered_commit_set.cc
+++ b/sync/sessions/ordered_commit_set.cc
@@ -31,8 +31,15 @@ void OrderedCommitSet::AddCommitItem(const int64 metahandle,
   }
 }
 
+const OrderedCommitSet::Projection& OrderedCommitSet::GetCommitIdProjection(
+    browser_sync::ModelSafeGroup group) const {
+  Projections::const_iterator i = projections_.find(group);
+  DCHECK(i != projections_.end());
+  return i->second;
+}
+
 void OrderedCommitSet::Append(const OrderedCommitSet& other) {
-  for (int i = 0; i < other.Size(); ++i) {
+  for (size_t i = 0; i < other.Size(); ++i) {
     CommitItem item = other.GetCommitItemAt(i);
     AddCommitItem(item.meta, item.id, item.group);
   }
@@ -71,8 +78,19 @@ void OrderedCommitSet::Truncate(size_t max_size) {
   }
 }
 
+void OrderedCommitSet::Clear() {
+  inserted_metahandles_.clear();
+  commit_ids_.clear();
+  metahandle_order_.clear();
+  for (Projections::iterator it = projections_.begin();
+       it != projections_.end(); ++it) {
+    it->second.clear();
+  }
+  types_.clear();
+}
+
 OrderedCommitSet::CommitItem OrderedCommitSet::GetCommitItemAt(
-    const int position) const {
+    const size_t position) const {
   DCHECK(position < Size());
   CommitItem return_item = {metahandle_order_[position],
       commit_ids_[position],
diff --git a/sync/sessions/ordered_commit_set.h b/sync/sessions/ordered_commit_set.h
index 8551c07..f8e6938 100644
--- a/sync/sessions/ordered_commit_set.h
+++ b/sync/sessions/ordered_commit_set.h
@@ -64,14 +64,17 @@ class OrderedCommitSet {
   // belonging to |group|.  This is useful when you need to process a commit
   // response one ModelSafeGroup at a time. See GetCommitIdAt for how the
   // indices contained in the returned Projection can be used.
-  const Projection& GetCommitIdProjection(browser_sync::ModelSafeGroup group) {
-    return projections_[group];
-  }
+  const Projection& GetCommitIdProjection(
+      browser_sync::ModelSafeGroup group) const;
 
-  int Size() const {
+  size_t Size() const {
     return commit_ids_.size();
   }
 
+  bool Empty() const {
+    return Size() == 0;
+  }
+
   // Returns true iff any of the commit ids added to this set have model type
   // BOOKMARKS.
   bool HasBookmarkCommitId() const;
@@ -80,6 +83,9 @@ class OrderedCommitSet {
   void AppendReverse(const OrderedCommitSet& other);
   void Truncate(size_t max_size);
 
+  // Removes all entries from this set.
+  void Clear();
+
   void operator=(const OrderedCommitSet& other);
  private:
   // A set of CommitIdProjections associated with particular ModelSafeGroups.
@@ -92,7 +98,7 @@ class OrderedCommitSet {
     syncable::ModelType group;
   };
 
-  CommitItem GetCommitItemAt(const int position) const;
+  CommitItem GetCommitItemAt(const size_t position) const;
 
   // These lists are different views of the same items; e.g they are
   // isomorphic.
diff --git a/sync/sessions/ordered_commit_set_unittest.cc b/sync/sessions/ordered_commit_set_unittest.cc
index fee37bf..6310c9d 100644
--- a/sync/sessions/ordered_commit_set_unittest.cc
+++ b/sync/sessions/ordered_commit_set_unittest.cc
@@ -115,6 +115,18 @@ TEST_F(OrderedCommitSetTest, HasBookmarkCommitId) {
   EXPECT_FALSE(commit_set.HasBookmarkCommitId());
 }
 
+TEST_F(OrderedCommitSetTest, AddAndRemoveEntries) {
+  OrderedCommitSet commit_set(routes_);
+
+  ASSERT_TRUE(commit_set.Empty());
+
+  commit_set.AddCommitItem(0, ids_.NewLocalId(), syncable::AUTOFILL);
+  ASSERT_EQ(static_cast<size_t>(1), commit_set.Size());
+
+  commit_set.Clear();
+  ASSERT_TRUE(commit_set.Empty());
+}
+
 }  // namespace sessions
 }  // namespace browser_sync
 
diff --git a/sync/sessions/session_state.cc b/sync/sessions/session_state.cc
index d0d23fa..ba9931f 100644
--- a/sync/sessions/session_state.cc
+++ b/sync/sessions/session_state.cc
@@ -86,12 +86,10 @@ SyncSessionSnapshot::SyncSessionSnapshot()
       is_share_usable_(false),
       has_more_to_sync_(false),
       is_silenced_(false),
-      unsynced_count_(0),
       num_encryption_conflicts_(0),
       num_hierarchy_conflicts_(0),
       num_simple_conflicts_(0),
       num_server_conflicts_(0),
-      did_commit_items_(false),
       notifications_enabled_(false),
       num_entries_(0),
       retry_scheduled_(false) {
@@ -106,12 +104,10 @@ SyncSessionSnapshot::SyncSessionSnapshot(
     const syncable::ModelTypePayloadMap& download_progress_markers,
     bool more_to_sync,
     bool is_silenced,
-    int64 unsynced_count,
     int num_encryption_conflicts,
     int num_hierarchy_conflicts,
     int num_simple_conflicts,
     int num_server_conflicts,
-    bool did_commit_items,
     const SyncSourceInfo& source,
     bool notifications_enabled,
     size_t num_entries,
@@ -125,12 +121,10 @@ SyncSessionSnapshot::SyncSessionSnapshot(
       download_progress_markers_(download_progress_markers),
       has_more_to_sync_(more_to_sync),
       is_silenced_(is_silenced),
-      unsynced_count_(unsynced_count),
       num_encryption_conflicts_(num_encryption_conflicts),
       num_hierarchy_conflicts_(num_hierarchy_conflicts),
       num_simple_conflicts_(num_simple_conflicts),
       num_server_conflicts_(num_server_conflicts),
-      did_commit_items_(did_commit_items),
       source_(source),
       notifications_enabled_(notifications_enabled),
       num_entries_(num_entries),
@@ -154,8 +148,6 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const {
   value->SetBoolean("hasMoreToSync", has_more_to_sync_);
   value->SetBoolean("isSilenced", is_silenced_);
   // We don't care too much if we lose precision here, also.
-  value->SetInteger("unsyncedCount",
-                    static_cast<int>(unsynced_count_));
   value->SetInteger("numEncryptionConflicts",
                     num_encryption_conflicts_);
   value->SetInteger("numHierarchyConflicts",
@@ -164,7 +156,6 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const {
                     num_simple_conflicts_);
   value->SetInteger("numServerConflicts",
                     num_server_conflicts_);
-  value->SetBoolean("didCommitItems", did_commit_items_);
   value->SetInteger("numEntries", num_entries_);
   value->Set("source", source_.ToValue());
   value->SetBoolean("notificationsEnabled", notifications_enabled_);
@@ -213,10 +204,6 @@ bool SyncSessionSnapshot::is_silenced() const {
   return is_silenced_;
 }
 
-int64 SyncSessionSnapshot::unsynced_count() const {
-  return unsynced_count_;
-}
-
 int SyncSessionSnapshot::num_encryption_conflicts() const {
   return num_encryption_conflicts_;
 }
@@ -233,10 +220,6 @@ int SyncSessionSnapshot::num_server_conflicts() const {
   return num_server_conflicts_;
 }
 
-bool SyncSessionSnapshot::did_commit_items() const {
-  return did_commit_items_;
-}
-
 SyncSourceInfo SyncSessionSnapshot::source() const {
   return source_;
 }
@@ -381,11 +364,9 @@ bool UpdateProgress::HasConflictingUpdates() const {
 }
 
 AllModelTypeState::AllModelTypeState(bool* dirty_flag)
-    : unsynced_handles(dirty_flag),
-      syncer_status(dirty_flag),
+    : syncer_status(dirty_flag),
       error(dirty_flag),
-      num_server_changes_remaining(dirty_flag, 0),
-      commit_set(ModelSafeRoutingInfo()) {
+      num_server_changes_remaining(dirty_flag, 0) {
 }
 
 AllModelTypeState::~AllModelTypeState() {}
diff --git a/sync/sessions/session_state.h b/sync/sessions/session_state.h
index 3f37cd0..c2a3edc 100644
--- a/sync/sessions/session_state.h
+++ b/sync/sessions/session_state.h
@@ -23,7 +23,6 @@
 #include "sync/engine/syncer_types.h"
 #include "sync/engine/syncproto.h"
 #include "sync/protocol/sync_protocol_error.h"
-#include "sync/sessions/ordered_commit_set.h"
 #include "sync/syncable/model_type.h"
 #include "sync/syncable/model_type_payload_map.h"
 #include "sync/syncable/syncable.h"
@@ -114,12 +113,10 @@ class SyncSessionSnapshot {
       const syncable::ModelTypePayloadMap& download_progress_markers,
       bool more_to_sync,
       bool is_silenced,
-      int64 unsynced_count,
       int num_encryption_conflicts,
       int num_hierarchy_conflicts,
       int num_simple_conflicts,
       int num_server_conflicts,
-      bool did_commit_items,
       const SyncSourceInfo& source,
       bool notifications_enabled,
       size_t num_entries,
@@ -140,7 +137,6 @@ class SyncSessionSnapshot {
   syncable::ModelTypePayloadMap download_progress_markers() const;
   bool has_more_to_sync() const;
   bool is_silenced() const;
-  int64 unsynced_count() const;
   int num_encryption_conflicts() const;
   int num_hierarchy_conflicts() const;
   int num_simple_conflicts() const;
@@ -161,12 +157,10 @@ class SyncSessionSnapshot {
   syncable::ModelTypePayloadMap download_progress_markers_;
   bool has_more_to_sync_;
   bool is_silenced_;
-  int64 unsynced_count_;
   int num_encryption_conflicts_;
   int num_hierarchy_conflicts_;
   int num_simple_conflicts_;
   int num_server_conflicts_;
-  bool did_commit_items_;
   SyncSourceInfo source_;
   bool notifications_enabled_;
   size_t num_entries_;
@@ -321,20 +315,15 @@ struct AllModelTypeState {
   explicit AllModelTypeState(bool* dirty_flag);
   ~AllModelTypeState();
 
-  // Commits for all model types are bundled together into a single message.
-  ClientToServerMessage commit_message;
-  ClientToServerResponse commit_response;
   // We GetUpdates for some combination of types at once.
   // requested_update_types stores the set of types which were requested.
   syncable::ModelTypeSet updates_request_types;
   ClientToServerResponse updates_response;
   // Used to build the shared commit message.
-  DirtyOnWrite<std::vector<int64> > unsynced_handles;
   DirtyOnWrite<SyncerStatus> syncer_status;
   DirtyOnWrite<ErrorCounters> error;
   SyncCycleControlParameters control_params;
   DirtyOnWrite<int64> num_server_changes_remaining;
-  OrderedCommitSet commit_set;
 };
 
 // Grouping of all state that applies to a single ModelSafeGroup.
diff --git a/sync/sessions/session_state_unittest.cc b/sync/sessions/session_state_unittest.cc
index 011f832..7881de0 100644
--- a/sync/sessions/session_state_unittest.cc
+++ b/sync/sessions/session_state_unittest.cc
@@ -96,12 +96,10 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
 
   const bool kHasMoreToSync = false;
   const bool kIsSilenced = true;
-  const int kUnsyncedCount = 1053;
   const int kNumEncryptionConflicts = 1054;
   const int kNumHierarchyConflicts = 1055;
   const int kNumSimpleConflicts = 1056;
   const int kNumServerConflicts = 1057;
-  const bool kDidCommitItems = true;
 
   SyncSourceInfo source;
   scoped_ptr<DictionaryValue> expected_source_value(source.ToValue());
@@ -114,19 +112,17 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                                download_progress_markers,
                                kHasMoreToSync,
                                kIsSilenced,
-                               kUnsyncedCount,
                                kNumEncryptionConflicts,
                                kNumHierarchyConflicts,
                                kNumSimpleConflicts,
                                kNumServerConflicts,
-                               kDidCommitItems,
                                source,
                                false,
                                0,
                                base::Time::Now(),
                                false);
   scoped_ptr<DictionaryValue> value(snapshot.ToValue());
-  EXPECT_EQ(16u, value->size());
+  EXPECT_EQ(14u, value->size());
   ExpectDictDictionaryValue(*expected_syncer_status_value, *value,
                             "syncerStatus");
   ExpectDictIntegerValue(kNumServerChangesRemaining, *value,
@@ -138,7 +134,6 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                             *value, "downloadProgressMarkers");
   ExpectDictBooleanValue(kHasMoreToSync, *value, "hasMoreToSync");
   ExpectDictBooleanValue(kIsSilenced, *value, "isSilenced");
-  ExpectDictIntegerValue(kUnsyncedCount, *value, "unsyncedCount");
   ExpectDictIntegerValue(kNumEncryptionConflicts, *value,
                          "numEncryptionConflicts");
   ExpectDictIntegerValue(kNumHierarchyConflicts, *value,
@@ -147,8 +142,6 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                          "numSimpleConflicts");
   ExpectDictIntegerValue(kNumServerConflicts, *value,
                          "numServerConflicts");
-  ExpectDictBooleanValue(kDidCommitItems, *value,
-                         "didCommitItems");
   ExpectDictDictionaryValue(*expected_source_value, *value, "source");
   ExpectDictBooleanValue(false, *value, "notificationsEnabled");
 }
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 19fdbaf..a88d93c 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -142,13 +142,6 @@ void StatusController::set_num_successful_bookmark_commits(int value) {
     shared_.syncer_status.mutate()->num_successful_bookmark_commits = value;
 }
 
-void StatusController::set_unsynced_handles(
-    const std::vector<int64>& unsynced_handles) {
-  if (!operator==(unsynced_handles, shared_.unsynced_handles.value())) {
-    *(shared_.unsynced_handles.mutate()) = unsynced_handles;
-  }
-}
-
 void StatusController::increment_num_successful_bookmark_commits() {
   set_num_successful_bookmark_commits(
       shared_.syncer_status.value().num_successful_bookmark_commits + 1);
@@ -180,14 +173,17 @@ void StatusController::set_last_post_commit_result(const SyncerError result) {
   shared_.error.mutate()->last_post_commit_result = result;
 }
 
+SyncerError StatusController::last_post_commit_result() const {
+  return shared_.error.value().last_post_commit_result;
+}
+
 void StatusController::set_last_process_commit_response_result(
     const SyncerError result) {
   shared_.error.mutate()->last_process_commit_response_result = result;
 }
 
-void StatusController::set_commit_set(const OrderedCommitSet& commit_set) {
-  DCHECK(!group_restriction_in_effect_);
-  shared_.commit_set = commit_set;
+SyncerError StatusController::last_process_commit_response_result() const {
+  return shared_.error.value().last_process_commit_response_result;
 }
 
 void StatusController::update_conflicts_resolved(bool resolved) {
@@ -196,9 +192,6 @@ void StatusController::update_conflicts_resolved(bool resolved) {
 void StatusController::reset_conflicts_resolved() {
   shared_.control_params.conflicts_resolved = false;
 }
-void StatusController::set_items_committed() {
-  shared_.control_params.items_committed = true;
-}
 
 // Returns the number of updates received from the sync server.
 int64 StatusController::CountUpdates() const {
@@ -210,12 +203,6 @@ int64 StatusController::CountUpdates() const {
   }
 }
 
-bool StatusController::CurrentCommitIdProjectionHasIndex(size_t index) {
-  OrderedCommitSet::Projection proj =
-      shared_.commit_set.GetCommitIdProjection(group_restriction_);
-  return std::binary_search(proj.begin(), proj.end(), index);
-}
-
 bool StatusController::HasConflictingUpdates() const {
   DCHECK(!group_restriction_in_effect_)
       << "HasConflictingUpdates applies to all ModelSafeGroups";
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 7e2f258..d91efec 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -71,18 +71,6 @@ class StatusController {
       ModelSafeGroup group);
 
   // ClientToServer messages.
-  const ClientToServerMessage& commit_message() {
-    return shared_.commit_message;
-  }
-  ClientToServerMessage* mutable_commit_message() {
-    return &shared_.commit_message;
-  }
-  const ClientToServerResponse& commit_response() const {
-    return shared_.commit_response;
-  }
-  ClientToServerResponse* mutable_commit_response() {
-    return &shared_.commit_response;
-  }
   const syncable::ModelTypeSet updates_request_types() const {
     return shared_.updates_request_types;
   }
@@ -109,41 +97,17 @@ class StatusController {
     return shared_.num_server_changes_remaining.value();
   }
 
-  // Commit path data.
-  const std::vector<syncable::Id>& commit_ids() const {
-    DCHECK(!group_restriction_in_effect_) << "Group restriction in effect!";
-    return shared_.commit_set.GetAllCommitIds();
-  }
-  const OrderedCommitSet::Projection& commit_id_projection() {
+  const OrderedCommitSet::Projection& commit_id_projection(
+      const sessions::OrderedCommitSet &commit_set) {
     DCHECK(group_restriction_in_effect_)
         << "No group restriction for projection.";
-    return shared_.commit_set.GetCommitIdProjection(group_restriction_);
-  }
-  const syncable::Id& GetCommitIdAt(size_t index) {
-    DCHECK(CurrentCommitIdProjectionHasIndex(index));
-    return shared_.commit_set.GetCommitIdAt(index);
-  }
-  syncable::ModelType GetCommitModelTypeAt(size_t index) {
-    DCHECK(CurrentCommitIdProjectionHasIndex(index));
-    return shared_.commit_set.GetModelTypeAt(index);
-  }
-  syncable::ModelType GetUnrestrictedCommitModelTypeAt(size_t index) const {
-    DCHECK(!group_restriction_in_effect_) << "Group restriction in effect!";
-    return shared_.commit_set.GetModelTypeAt(index);
-  }
-  const std::vector<int64>& unsynced_handles() const {
-    DCHECK(!group_restriction_in_effect_)
-        << "unsynced_handles is unrestricted.";
-    return shared_.unsynced_handles.value();
+    return commit_set.GetCommitIdProjection(group_restriction_);
   }
 
   // Control parameters for sync cycles.
   bool conflicts_resolved() const {
     return shared_.control_params.conflicts_resolved;
   }
-  bool did_commit_items() const {
-    return shared_.control_params.items_committed;
-  }
 
   // If a GetUpdates for any data type resulted in downloading an update that
   // is in conflict, this method returns true.
@@ -165,13 +129,6 @@ class StatusController {
   // Returns the number of updates received from the sync server.
   int64 CountUpdates() const;
 
-  // Returns true iff any of the commit ids added during this session are
-  // bookmark related, and the bookmark group restriction is in effect.
-  bool HasBookmarkCommitActivity() const {
-    return ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS) &&
-        shared_.commit_set.HasBookmarkCommitId();
-  }
-
   // Returns true if the last download_updates_command received a valid
   // server response.
   bool download_updates_succeeded() const {
@@ -196,17 +153,13 @@ class StatusController {
     return sync_start_time_;
   }
 
-  // Check whether a particular model is included by the active group
-  // restriction.
-  bool ActiveGroupRestrictionIncludesModel(syncable::ModelType model) const {
-    if (!group_restriction_in_effect_)
-      return true;
-    ModelSafeRoutingInfo::const_iterator it = routing_info_.find(model);
-    if (it == routing_info_.end())
-      return false;
-    return group_restriction() == it->second;
+  bool HasBookmarkCommitActivity() const {
+    return ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS);
   }
 
+  SyncerError last_post_commit_result() const;
+  SyncerError last_process_commit_response_result() const;
+
   // A toolbelt full of methods for updating counters and flags.
   void set_num_server_changes_remaining(int64 changes_remaining);
   void set_invalid_store(bool invalid_store);
@@ -217,7 +170,6 @@ class StatusController {
   void increment_num_tombstone_updates_downloaded_by(int value);
   void increment_num_reflected_updates_downloaded_by(int value);
   void set_types_needing_local_migration(syncable::ModelTypeSet types);
-  void set_unsynced_handles(const std::vector<int64>& unsynced_handles);
   void increment_num_local_overwrites();
   void increment_num_server_overwrites();
   void set_sync_protocol_error(const SyncProtocolError& error);
@@ -225,10 +177,8 @@ class StatusController {
   void set_last_post_commit_result(const SyncerError result);
   void set_last_process_commit_response_result(const SyncerError result);
 
-  void set_commit_set(const OrderedCommitSet& commit_set);
   void update_conflicts_resolved(bool resolved);
   void reset_conflicts_resolved();
-  void set_items_committed();
 
   void UpdateStartTime();
 
@@ -239,9 +189,16 @@ class StatusController {
  private:
   friend class ScopedModelSafeGroupRestriction;
 
-  // Returns true iff the commit id projection for |group_restriction_|
-  // references position |index| into the full set of commit ids in play.
-  bool CurrentCommitIdProjectionHasIndex(size_t index);
+  // Check whether a particular model is included by the active group
+  // restriction.
+  bool ActiveGroupRestrictionIncludesModel(syncable::ModelType model) const {
+    if (!group_restriction_in_effect_)
+      return true;
+    ModelSafeRoutingInfo::const_iterator it = routing_info_.find(model);
+    if (it == routing_info_.end())
+      return false;
+    return group_restriction() == it->second;
+  }
 
   // Returns the state, if it exists, or NULL otherwise.
   const PerModelSafeGroupState* GetModelSafeGroupState(
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index 59d3919..9f38e80 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -39,29 +39,12 @@ TEST_F(StatusControllerTest, GetsDirty) {
         AddSimpleConflictingItemById(syncable::Id());
   }
   EXPECT_TRUE(status.TestAndClearIsDirty());
-
-  std::vector<int64> v;
-  v.push_back(1);
-  status.set_unsynced_handles(v);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  std::vector<int64> v2;
-  v2.push_back(1);
-  status.set_unsynced_handles(v2);
-  EXPECT_FALSE(status.TestAndClearIsDirty());  // Test for deep comparison.
 }
 
 TEST_F(StatusControllerTest, StaysClean) {
   StatusController status(routes_);
   status.update_conflicts_resolved(true);
   EXPECT_FALSE(status.TestAndClearIsDirty());
-
-  status.set_items_committed();
-  EXPECT_FALSE(status.TestAndClearIsDirty());
-
-  OrderedCommitSet commits(routes_);
-  commits.AddCommitItem(0, syncable::Id(), syncable::BOOKMARKS);
-  status.set_commit_set(commits);
-  EXPECT_FALSE(status.TestAndClearIsDirty());
 }
 
 // This test is useful, as simple as it sounds, due to the copy-paste prone
@@ -93,11 +76,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   for (int i = 0; i < 14; i++)
     status.increment_num_successful_commits();
   EXPECT_EQ(14, status.syncer_status().num_successful_commits);
-
-  std::vector<int64> v;
-  v.push_back(16);
-  status.set_unsynced_handles(v);
-  EXPECT_EQ(16, v[0]);
 }
 
 TEST_F(StatusControllerTest, HasConflictingUpdates) {
@@ -179,16 +157,9 @@ TEST_F(StatusControllerTest, Unrestricted) {
   const UpdateProgress* progress =
       status.GetUnrestrictedUpdateProgress(GROUP_UI);
   EXPECT_FALSE(progress);
-  status.mutable_commit_message();
-  status.commit_response();
-  status.mutable_commit_response();
-  status.updates_response();
-  status.mutable_updates_response();
   status.error();
   status.syncer_status();
   status.num_server_changes_remaining();
-  status.commit_ids();
-  status.HasBookmarkCommitActivity();
   status.download_updates_succeeded();
   status.ServerSaysNothingMoreToDownload();
   status.group_restriction();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 4c58215..85ac8cc 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -159,12 +159,10 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
       download_progress_markers,
       HasMoreToSync(),
       delegate_->IsSyncingCurrentlySilenced(),
-      status_controller_->unsynced_handles().size(),
       status_controller_->TotalNumEncryptionConflictingItems(),
       status_controller_->TotalNumHierarchyConflictingItems(),
       status_controller_->TotalNumSimpleConflictingItems(),
       status_controller_->TotalNumServerConflictingItems(),
-      status_controller_->did_commit_items(),
       source_,
       context_->notifications_enabled(),
       dir->GetEntriesCount(),
@@ -182,11 +180,7 @@ void SyncSession::SendEventNotification(SyncEngineEvent::EventCause cause) {
 
 bool SyncSession::HasMoreToSync() const {
   const StatusController* status = status_controller_.get();
-  return ((status->commit_ids().size() < status->unsynced_handles().size()) &&
-      status->syncer_status().num_successful_commits > 0) ||
-      status->conflicts_resolved();
-      // Or, we have conflicting updates, but we're making progress on
-      // resolving them...
+  return status->conflicts_resolved();
 }
 
 const std::set<ModelSafeGroup>& SyncSession::GetEnabledGroups() const {
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index d5ce46a..cf6b293 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -192,26 +192,6 @@ TEST_F(SyncSessionTest, SetWriteTransaction) {
   }
 }
 
-TEST_F(SyncSessionTest, MoreToSyncIfUnsyncedGreaterThanCommitted) {
-  // If any forward progress was made during the session, and the number of
-  // unsynced handles still exceeds the number of commit ids we added, there is
-  // more to sync. For example, this occurs if we had more commit ids
-  // than could fit in a single commit batch.
-  EXPECT_FALSE(session_->HasMoreToSync());
-  OrderedCommitSet commit_set(routes_);
-  commit_set.AddCommitItem(0, syncable::Id(), syncable::BOOKMARKS);
-  status()->set_commit_set(commit_set);
-  EXPECT_FALSE(session_->HasMoreToSync());
-
-  std::vector<int64> unsynced_handles;
-  unsynced_handles.push_back(1);
-  unsynced_handles.push_back(2);
-  status()->set_unsynced_handles(unsynced_handles);
-  EXPECT_FALSE(session_->HasMoreToSync());
-  status()->increment_num_successful_commits();
-  EXPECT_TRUE(session_->HasMoreToSync());
-}
-
 TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
   status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
 
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 387dbf6..0b1ad24 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -38,7 +38,6 @@ void SimulateSuccess(sessions::SyncSession* session,
     ADD_FAILURE() << "Shouldn't have more to sync";
   }
   ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining());
-  ASSERT_EQ(0U, session->status_controller().unsynced_handles().size());
 }
 
 void SimulateThrottledImpl(sessions::SyncSession* session,