sync: Refactor session tracking

This change refactors the related structs ErrorCounters, SyncerStatus, and 
SyncCycleControlParameters. Their values have all been merged into 
AllModelTypeState, which has been renamed to ModelNeutralState. All the 
functions which depend on this data have been updated to use the new struct. 

This change also removes the DirtyOnWrite template class, the is_dirty flag, 
and the SyncerCommand logic to send change events if it detects a change in the 
syncer's status. The changes are so frequent and predictable that it's easier 
to just send the snapshots manually after any major syncer steps. 

Finally, this change removes the 'invalid_store' status member, which was never 
set nor read outside of unit tests. 

BUG=132630
TEST=


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

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

8 files changed, 81 insertions, 252 deletions

diff --git a/sync/sessions/session_state.cc b/sync/sessions/session_state.cc
index 7562522..6a088d0 100644
--- a/sync/sessions/session_state.cc
+++ b/sync/sessions/session_state.cc
@@ -16,9 +16,9 @@ using std::vector;
 namespace csync {
 namespace sessions {
 
-ConflictProgress::ConflictProgress(bool* dirty_flag)
+ConflictProgress::ConflictProgress()
   : num_server_conflicting_items(0), num_hierarchy_conflicting_items(0),
-    num_encryption_conflicting_items(0), dirty_(dirty_flag) {
+    num_encryption_conflicting_items(0) {
 }
 
 ConflictProgress::~ConflictProgress() {
@@ -39,17 +39,12 @@ ConflictProgress::SimpleConflictingItemsEnd() const {
 
 void ConflictProgress::AddSimpleConflictingItemById(
     const syncable::Id& the_id) {
-  std::pair<std::set<syncable::Id>::iterator, bool> ret =
-      simple_conflicting_item_ids_.insert(the_id);
-  if (ret.second)
-    *dirty_ = true;
+  simple_conflicting_item_ids_.insert(the_id);
 }
 
 void ConflictProgress::EraseSimpleConflictingItemById(
     const syncable::Id& the_id) {
-  int items_erased = simple_conflicting_item_ids_.erase(the_id);
-  if (items_erased != 0)
-    *dirty_ = true;
+  simple_conflicting_item_ids_.erase(the_id);
 }
 
 void ConflictProgress::AddEncryptionConflictingItemById(
@@ -58,8 +53,8 @@ void ConflictProgress::AddEncryptionConflictingItemById(
       unresolvable_conflicting_item_ids_.insert(the_id);
   if (ret.second) {
     num_encryption_conflicting_items++;
-    *dirty_ = true;
   }
+  unresolvable_conflicting_item_ids_.insert(the_id);
 }
 
 void ConflictProgress::AddHierarchyConflictingItemById(
@@ -68,7 +63,6 @@ void ConflictProgress::AddHierarchyConflictingItemById(
       unresolvable_conflicting_item_ids_.insert(the_id);
   if (ret.second) {
     num_hierarchy_conflicting_items++;
-    *dirty_ = true;
   }
 }
 
@@ -78,7 +72,6 @@ void ConflictProgress::AddServerConflictingItemById(
       unresolvable_conflicting_item_ids_.insert(the_id);
   if (ret.second) {
     num_server_conflicting_items++;
-    *dirty_ = true;
   }
 }
 
@@ -139,16 +132,7 @@ bool UpdateProgress::HasConflictingUpdates() const {
   return false;
 }
 
-AllModelTypeState::AllModelTypeState(bool* dirty_flag)
-    : syncer_status(dirty_flag),
-      error(dirty_flag),
-      num_server_changes_remaining(dirty_flag, 0) {
-}
-
-AllModelTypeState::~AllModelTypeState() {}
-
-PerModelSafeGroupState::PerModelSafeGroupState(bool* dirty_flag)
-    : conflict_progress(dirty_flag) {
+PerModelSafeGroupState::PerModelSafeGroupState() {
 }
 
 PerModelSafeGroupState::~PerModelSafeGroupState() {
diff --git a/sync/sessions/session_state.h b/sync/sessions/session_state.h
index 5384f44..ab35ae8 100644
--- a/sync/sessions/session_state.h
+++ b/sync/sessions/session_state.h
@@ -16,22 +16,17 @@
 #include <set>
 #include <vector>
 
-#include "base/basictypes.h"
 #include "sync/engine/syncer_types.h"
 #include "sync/engine/syncproto.h"
-#include "sync/internal_api/public/sessions/error_counters.h"
-#include "sync/internal_api/public/sessions/syncer_status.h"
 #include "sync/syncable/syncable_id.h"
 
 namespace csync {
 namespace sessions {
 
-class UpdateProgress;
-
 // Tracks progress of conflicts and their resolutions.
 class ConflictProgress {
  public:
-  explicit ConflictProgress(bool* dirty_flag);
+  explicit ConflictProgress();
   ~ConflictProgress();
 
   bool HasSimpleConflictItem(const syncable::Id &id) const;
@@ -74,11 +69,6 @@ class ConflictProgress {
   size_t num_server_conflicting_items;
   size_t num_hierarchy_conflicting_items;
   size_t num_encryption_conflicting_items;
-
-  // Whether a conflicting item was added or removed since
-  // the last call to reset_progress_changed(), if any. In practice this
-  // points to StatusController::is_dirty_.
-  bool* dirty_;
 };
 
 typedef std::pair<VerifyResult, sync_pb::SyncEntity> VerifiedUpdate;
@@ -129,66 +119,9 @@ class UpdateProgress {
   std::vector<AppliedUpdate> applied_updates_;
 };
 
-struct SyncCycleControlParameters {
-  SyncCycleControlParameters() : conflicts_resolved(false),
-                                 items_committed(false),
-                                 debug_info_sent(false) {}
-  // Set to true by ResolveConflictsCommand if any forward progress was made.
-  bool conflicts_resolved;
-
-  // Set to true by PostCommitMessageCommand if any commits were successful.
-  bool items_committed;
-
-  // True indicates debug info has been sent once this session.
-  bool debug_info_sent;
-};
-
-// DirtyOnWrite wraps a value such that any write operation will update a
-// specified dirty bit, which can be used to determine if a notification should
-// be sent due to state change.
-template <typename T>
-class DirtyOnWrite {
- public:
-  explicit DirtyOnWrite(bool* dirty) : dirty_(dirty) {}
-  DirtyOnWrite(bool* dirty, const T& t) : t_(t), dirty_(dirty) {}
-  T* mutate() {
-    *dirty_ = true;
-    return &t_;
-  }
-  const T& value() const { return t_; }
- private:
-  T t_;
-  bool* dirty_;
-};
-
-// The next 3 structures declare how all the state involved in running a sync
-// cycle is divided between global scope (applies to all model types),
-// ModelSafeGroup scope (applies to all data types in a group), and single
-// model type scope.  Within this breakdown, each struct declares which bits
-// of state are dirty-on-write and should incur dirty bit updates if changed.
-
-// Grouping of all state that applies to all model types.  Note that some
-// components of the global grouping can internally implement finer grained
-// scope control (such as OrderedCommitSet), but the top level entity is still
-// a singleton with respect to model types.
-struct AllModelTypeState {
-  explicit AllModelTypeState(bool* dirty_flag);
-  ~AllModelTypeState();
-
-  // 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<SyncerStatus> syncer_status;
-  DirtyOnWrite<ErrorCounters> error;
-  SyncCycleControlParameters control_params;
-  DirtyOnWrite<int64> num_server_changes_remaining;
-};
-
 // Grouping of all state that applies to a single ModelSafeGroup.
 struct PerModelSafeGroupState {
-  explicit PerModelSafeGroupState(bool* dirty_flag);
+  explicit PerModelSafeGroupState();
   ~PerModelSafeGroupState();
 
   UpdateProgress update_progress;
diff --git a/sync/sessions/session_state_unittest.cc b/sync/sessions/session_state_unittest.cc
index 8e396c9..52d7e80 100644
--- a/sync/sessions/session_state_unittest.cc
+++ b/sync/sessions/session_state_unittest.cc
@@ -11,10 +11,8 @@
 #include "base/test/values_test_util.h"
 #include "base/time.h"
 #include "base/values.h"
-#include "sync/internal_api/public/sessions/error_counters.h"
 #include "sync/internal_api/public/sessions/sync_session_snapshot.h"
 #include "sync/internal_api/public/sessions/sync_source_info.h"
-#include "sync/internal_api/public/sessions/syncer_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace csync {
@@ -46,45 +44,17 @@ TEST_F(SessionStateTest, SyncSourceInfoToValue) {
   ExpectDictDictionaryValue(*expected_types_value, *value, "types");
 }
 
-TEST_F(SessionStateTest, SyncerStatusToValue) {
-  SyncerStatus status;
-  status.invalid_store = true;
-  status.num_successful_commits = 5;
-  status.num_successful_bookmark_commits = 10;
-  status.num_updates_downloaded_total = 100;
-  status.num_tombstone_updates_downloaded_total = 200;
-  status.num_reflected_updates_downloaded_total = 50;
-  status.num_local_overwrites = 15;
-  status.num_server_overwrites = 18;
-
-  scoped_ptr<DictionaryValue> value(status.ToValue());
-  EXPECT_EQ(8u, value->size());
-  ExpectDictBooleanValue(status.invalid_store, *value, "invalidStore");
-  ExpectDictIntegerValue(status.num_successful_commits,
-                         *value, "numSuccessfulCommits");
-  ExpectDictIntegerValue(status.num_successful_bookmark_commits,
-                         *value, "numSuccessfulBookmarkCommits");
-  ExpectDictIntegerValue(status.num_updates_downloaded_total,
-                         *value, "numUpdatesDownloadedTotal");
-  ExpectDictIntegerValue(status.num_tombstone_updates_downloaded_total,
-                         *value, "numTombstoneUpdatesDownloadedTotal");
-  ExpectDictIntegerValue(status.num_reflected_updates_downloaded_total,
-                         *value, "numReflectedUpdatesDownloadedTotal");
-  ExpectDictIntegerValue(status.num_local_overwrites,
-                         *value, "numLocalOverwrites");
-  ExpectDictIntegerValue(status.num_server_overwrites,
-                         *value, "numServerOverwrites");
-}
-
 TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
-  SyncerStatus syncer_status;
-  syncer_status.num_successful_commits = 500;
-  scoped_ptr<DictionaryValue> expected_syncer_status_value(
-      syncer_status.ToValue());
+  ModelNeutralState model_neutral;
+  model_neutral.num_server_changes_remaining = 105;
+  model_neutral.num_successful_commits = 5;
+  model_neutral.num_successful_bookmark_commits = 10;
+  model_neutral.num_updates_downloaded_total = 100;
+  model_neutral.num_tombstone_updates_downloaded_total = 200;
+  model_neutral.num_reflected_updates_downloaded_total = 50;
+  model_neutral.num_local_overwrites = 15;
+  model_neutral.num_server_overwrites = 18;
 
-  ErrorCounters errors;
-
-  const int kNumServerChangesRemaining = 105;
   const bool kIsShareUsable = true;
 
   const syncable::ModelTypeSet initial_sync_ended(
@@ -108,9 +78,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
   SyncSourceInfo source;
   scoped_ptr<DictionaryValue> expected_source_value(source.ToValue());
 
-  SyncSessionSnapshot snapshot(syncer_status,
-                               errors,
-                               kNumServerChangesRemaining,
+  SyncSessionSnapshot snapshot(model_neutral,
                                kIsShareUsable,
                                initial_sync_ended,
                                download_progress_markers,
@@ -126,11 +94,23 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                                base::Time::Now(),
                                false);
   scoped_ptr<DictionaryValue> value(snapshot.ToValue());
-  EXPECT_EQ(14u, value->size());
-  ExpectDictDictionaryValue(*expected_syncer_status_value, *value,
-                            "syncerStatus");
-  ExpectDictIntegerValue(kNumServerChangesRemaining, *value,
-                         "numServerChangesRemaining");
+  EXPECT_EQ(20u, value->size());
+  ExpectDictIntegerValue(model_neutral.num_successful_commits,
+                         *value, "numSuccessfulCommits");
+  ExpectDictIntegerValue(model_neutral.num_successful_bookmark_commits,
+                         *value, "numSuccessfulBookmarkCommits");
+  ExpectDictIntegerValue(model_neutral.num_updates_downloaded_total,
+                         *value, "numUpdatesDownloadedTotal");
+  ExpectDictIntegerValue(model_neutral.num_tombstone_updates_downloaded_total,
+                         *value, "numTombstoneUpdatesDownloadedTotal");
+  ExpectDictIntegerValue(model_neutral.num_reflected_updates_downloaded_total,
+                         *value, "numReflectedUpdatesDownloadedTotal");
+  ExpectDictIntegerValue(model_neutral.num_local_overwrites,
+                         *value, "numLocalOverwrites");
+  ExpectDictIntegerValue(model_neutral.num_server_overwrites,
+                         *value, "numServerOverwrites");
+  ExpectDictIntegerValue(model_neutral.num_server_changes_remaining,
+                         *value, "numServerChangesRemaining");
   ExpectDictBooleanValue(kIsShareUsable, *value, "isShareUsable");
   ExpectDictListValue(*expected_initial_sync_ended_value, *value,
                       "initialSyncEnded");
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 1339da8..cab189e 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -17,9 +17,7 @@ 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_),
-      is_dirty_(false),
+    : per_model_group_deleter_(&per_model_group_),
       group_restriction_in_effect_(false),
       group_restriction_(GROUP_PASSIVE),
       routing_info_(routes) {
@@ -27,12 +25,6 @@ StatusController::StatusController(const ModelSafeRoutingInfo& routes)
 
 StatusController::~StatusController() {}
 
-bool StatusController::TestAndClearIsDirty() {
-  bool is_dirty = is_dirty_;
-  is_dirty_ = false;
-  return is_dirty;
-}
-
 const UpdateProgress* StatusController::update_progress() const {
   const PerModelSafeGroupState* state =
       GetModelSafeGroupState(true, group_restriction_);
@@ -95,42 +87,34 @@ PerModelSafeGroupState* StatusController::GetOrCreateModelSafeGroupState(
   std::map<ModelSafeGroup, PerModelSafeGroupState*>::iterator it =
       per_model_group_.find(group);
   if (it == per_model_group_.end()) {
-    PerModelSafeGroupState* state = new PerModelSafeGroupState(&is_dirty_);
+    PerModelSafeGroupState* state = new PerModelSafeGroupState();
     it = per_model_group_.insert(std::make_pair(group, state)).first;
   }
   return it->second;
 }
 
 void StatusController::increment_num_updates_downloaded_by(int value) {
-  shared_.syncer_status.mutate()->num_updates_downloaded_total += value;
+  model_neutral_.num_updates_downloaded_total += value;
 }
 
 void StatusController::set_types_needing_local_migration(
     syncable::ModelTypeSet types) {
-  shared_.syncer_status.mutate()->types_needing_local_migration = types;
+  model_neutral_.types_needing_local_migration = types;
 }
 
 void StatusController::increment_num_tombstone_updates_downloaded_by(
     int value) {
-  shared_.syncer_status.mutate()->num_tombstone_updates_downloaded_total +=
-      value;
+  model_neutral_.num_tombstone_updates_downloaded_total += value;
 }
 
 void StatusController::increment_num_reflected_updates_downloaded_by(
     int value) {
-  shared_.syncer_status.mutate()->num_reflected_updates_downloaded_total +=
-      value;
+  model_neutral_.num_reflected_updates_downloaded_total += value;
 }
 
 void StatusController::set_num_server_changes_remaining(
     int64 changes_remaining) {
-  if (shared_.num_server_changes_remaining.value() != changes_remaining)
-    *(shared_.num_server_changes_remaining.mutate()) = changes_remaining;
-}
-
-void StatusController::set_invalid_store(bool invalid_store) {
-  if (shared_.syncer_status.value().invalid_store != invalid_store)
-    shared_.syncer_status.mutate()->invalid_store = invalid_store;
+  model_neutral_.num_server_changes_remaining = changes_remaining;
 }
 
 void StatusController::UpdateStartTime() {
@@ -138,51 +122,49 @@ void StatusController::UpdateStartTime() {
 }
 
 void StatusController::set_num_successful_bookmark_commits(int value) {
-  if (shared_.syncer_status.value().num_successful_bookmark_commits != value)
-    shared_.syncer_status.mutate()->num_successful_bookmark_commits = value;
+  model_neutral_.num_successful_bookmark_commits = value;
 }
 
 void StatusController::increment_num_successful_bookmark_commits() {
-  set_num_successful_bookmark_commits(
-      shared_.syncer_status.value().num_successful_bookmark_commits + 1);
+  model_neutral_.num_successful_bookmark_commits++;
 }
 
 void StatusController::increment_num_successful_commits() {
-  shared_.syncer_status.mutate()->num_successful_commits++;
+  model_neutral_.num_successful_commits++;
 }
 
 void StatusController::increment_num_local_overwrites() {
-  shared_.syncer_status.mutate()->num_local_overwrites++;
+  model_neutral_.num_local_overwrites++;
 }
 
 void StatusController::increment_num_server_overwrites() {
-  shared_.syncer_status.mutate()->num_server_overwrites++;
+  model_neutral_.num_server_overwrites++;
 }
 
 void StatusController::set_sync_protocol_error(
     const SyncProtocolError& error) {
-  shared_.error.mutate()->sync_protocol_error = error;
+  model_neutral_.sync_protocol_error = error;
 }
 
 void StatusController::set_last_download_updates_result(
     const SyncerError result) {
-  shared_.error.mutate()->last_download_updates_result = result;
+  model_neutral_.last_download_updates_result = result;
 }
 
 void StatusController::set_commit_result(const SyncerError result) {
-  shared_.error.mutate()->commit_result = result;
+  model_neutral_.commit_result = result;
 }
 
 void StatusController::update_conflicts_resolved(bool resolved) {
-  shared_.control_params.conflicts_resolved |= resolved;
+  model_neutral_.conflicts_resolved |= resolved;
 }
 void StatusController::reset_conflicts_resolved() {
-  shared_.control_params.conflicts_resolved = false;
+  model_neutral_.conflicts_resolved = false;
 }
 
 // Returns the number of updates received from the sync server.
 int64 StatusController::CountUpdates() const {
-  const ClientToServerResponse& updates = shared_.updates_response;
+  const ClientToServerResponse& updates = model_neutral_.updates_response;
   if (updates.has_get_updates()) {
     return updates.get_updates().entries().size();
   } else {
@@ -279,11 +261,11 @@ bool StatusController::ServerSaysNothingMoreToDownload() const {
 }
 
 void StatusController::set_debug_info_sent() {
-  shared_.control_params.debug_info_sent = true;
+  model_neutral_.debug_info_sent = true;
 }
 
 bool StatusController::debug_info_sent() const {
-  return shared_.control_params.debug_info_sent;
+  return model_neutral_.debug_info_sent;
 }
 
 }  // namespace sessions
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 7fc49f8..7837e90 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -39,6 +39,7 @@
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/time.h"
+#include "sync/internal_api/public/sessions/model_neutral_state.h"
 #include "sync/sessions/ordered_commit_set.h"
 #include "sync/sessions/session_state.h"
 
@@ -50,10 +51,6 @@ class StatusController {
   explicit StatusController(const ModelSafeRoutingInfo& routes);
   ~StatusController();
 
-  // Returns true if some portion of the session state has changed (is dirty)
-  // since it was created or was last reset.
-  bool TestAndClearIsDirty();
-
   // Progress counters.  All const methods may return NULL if the
   // progress structure doesn't exist, but all non-const methods
   // auto-create.
@@ -72,29 +69,21 @@ class StatusController {
 
   // ClientToServer messages.
   const syncable::ModelTypeSet updates_request_types() const {
-    return shared_.updates_request_types;
+    return model_neutral_.updates_request_types;
   }
   void set_updates_request_types(syncable::ModelTypeSet value) {
-    shared_.updates_request_types = value;
+    model_neutral_.updates_request_types = value;
   }
   const ClientToServerResponse& updates_response() const {
-    return shared_.updates_response;
+    return model_neutral_.updates_response;
   }
   ClientToServerResponse* mutable_updates_response() {
-    return &shared_.updates_response;
-  }
-
-  // Errors and SyncerStatus.
-  const ErrorCounters& error() const {
-    return shared_.error.value();
-  }
-  const SyncerStatus& syncer_status() const {
-    return shared_.syncer_status.value();
+    return &model_neutral_.updates_response;
   }
 
   // Changelog related state.
   int64 num_server_changes_remaining() const {
-    return shared_.num_server_changes_remaining.value();
+    return model_neutral_.num_server_changes_remaining;
   }
 
   const OrderedCommitSet::Projection& commit_id_projection(
@@ -106,7 +95,7 @@ class StatusController {
 
   // Control parameters for sync cycles.
   bool conflicts_resolved() const {
-    return shared_.control_params.conflicts_resolved;
+    return model_neutral_.conflicts_resolved;
   }
 
   // If a GetUpdates for any data type resulted in downloading an update that
@@ -132,7 +121,7 @@ class StatusController {
   // Returns true if the last download_updates_command received a valid
   // server response.
   bool download_updates_succeeded() const {
-    return shared_.error.value().last_download_updates_result
+    return model_neutral_.last_download_updates_result
         == SYNCER_OK;
   }
 
@@ -157,9 +146,12 @@ class StatusController {
     return ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS);
   }
 
+  const ModelNeutralState& model_neutral_state() const {
+    return model_neutral_;
+  }
+
   // 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);
   void set_num_successful_bookmark_commits(int value);
   void increment_num_successful_commits();
   void increment_num_successful_bookmark_commits();
@@ -204,16 +196,12 @@ class StatusController {
   PerModelSafeGroupState* GetOrCreateModelSafeGroupState(
       bool restrict, ModelSafeGroup group);
 
-  AllModelTypeState shared_;
+  ModelNeutralState model_neutral_;
   std::map<ModelSafeGroup, PerModelSafeGroupState*> per_model_group_;
 
   STLValueDeleter<std::map<ModelSafeGroup, PerModelSafeGroupState*> >
       per_model_group_deleter_;
 
-  // Set to true if any DirtyOnWrite pieces of state we maintain are changed.
-  // Reset to false by TestAndClearIsDirty.
-  bool is_dirty_;
-
   // Used to fail read/write operations on state that don't obey the current
   // active ModelSafeWorker contract.
   bool group_restriction_in_effect_;
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index 84d352f..9037c30 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -18,35 +18,6 @@ class StatusControllerTest : public testing::Test {
   ModelSafeRoutingInfo routes_;
 };
 
-TEST_F(StatusControllerTest, GetsDirty) {
-  StatusController status(routes_);
-  status.set_num_server_changes_remaining(30);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-
-  status.set_invalid_store(true);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.set_invalid_store(false);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-
-  status.increment_num_successful_commits();
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.increment_num_successful_commits();
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-
-  {
-    ScopedModelSafeGroupRestriction r(&status, GROUP_UI);
-    status.mutable_conflict_progress()->
-        AddSimpleConflictingItemById(syncable::Id());
-  }
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-}
-
-TEST_F(StatusControllerTest, StaysClean) {
-  StatusController status(routes_);
-  status.update_conflicts_resolved(true);
-  EXPECT_FALSE(status.TestAndClearIsDirty());
-}
-
 // This test is useful, as simple as it sounds, due to the copy-paste prone
 // nature of status_controller.cc (we have had bugs in the past where a set_foo
 // method was actually setting |bar_| instead!).
@@ -55,23 +26,20 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.set_num_server_changes_remaining(13);
   EXPECT_EQ(13, status.num_server_changes_remaining());
 
-  EXPECT_FALSE(status.syncer_status().invalid_store);
-  status.set_invalid_store(true);
-  EXPECT_TRUE(status.syncer_status().invalid_store);
-
   EXPECT_FALSE(status.conflicts_resolved());
   status.update_conflicts_resolved(true);
   EXPECT_TRUE(status.conflicts_resolved());
 
   status.set_last_download_updates_result(SYNCER_OK);
-  EXPECT_EQ(SYNCER_OK, status.error().last_download_updates_result);
+  EXPECT_EQ(SYNCER_OK,
+            status.model_neutral_state().last_download_updates_result);
 
   status.set_commit_result(SYNC_AUTH_ERROR);
-  EXPECT_EQ(SYNC_AUTH_ERROR, status.error().commit_result);
+  EXPECT_EQ(SYNC_AUTH_ERROR, status.model_neutral_state().commit_result);
 
   for (int i = 0; i < 14; i++)
     status.increment_num_successful_commits();
-  EXPECT_EQ(14, status.syncer_status().num_successful_commits);
+  EXPECT_EQ(14, status.model_neutral_state().num_successful_commits);
 }
 
 TEST_F(StatusControllerTest, HasConflictingUpdates) {
@@ -153,9 +121,7 @@ TEST_F(StatusControllerTest, Unrestricted) {
   const UpdateProgress* progress =
       status.GetUnrestrictedUpdateProgress(GROUP_UI);
   EXPECT_FALSE(progress);
-  status.error();
-  status.syncer_status();
-  status.num_server_changes_remaining();
+  status.model_neutral_state();
   status.download_updates_succeeded();
   status.ServerSaysNothingMoreToDownload();
   status.group_restriction();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 876c736..1ef6e40 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -153,9 +153,7 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
   }
 
   return SyncSessionSnapshot(
-      status_controller_->syncer_status(),
-      status_controller_->error(),
-      status_controller_->num_server_changes_remaining(),
+      status_controller_->model_neutral_state(),
       is_share_useable,
       initial_sync_ended,
       download_progress_markers,
@@ -232,27 +230,26 @@ bool IsError(SyncerError error) {
 }
 
 // Returns false iff one of the command results had an error.
-bool HadErrors(const ErrorCounters& error) {
+bool HadErrors(const ModelNeutralState& state) {
   const bool download_updates_error =
-      IsError(error.last_download_updates_result);
-  const bool commit_error = IsError(error.commit_result);
+      IsError(state.last_download_updates_result);
+  const bool commit_error = IsError(state.commit_result);
   return download_updates_error || commit_error;
 }
 }  // namespace
 
 bool SyncSession::Succeeded() const {
-  const ErrorCounters& error = status_controller_->error();
-  return finished_ && !HadErrors(error);
+  return finished_ && !HadErrors(status_controller_->model_neutral_state());
 }
 
 bool SyncSession::SuccessfullyReachedServer() const {
-  const ErrorCounters& error = status_controller_->error();
-  bool reached_server = error.last_download_updates_result == SYNCER_OK;
+  const ModelNeutralState& state = status_controller_->model_neutral_state();
+  bool reached_server = state.last_download_updates_result == SYNCER_OK;
   // It's possible that we reached the server on one attempt, then had an error
   // on the next (or didn't perform some of the server-communicating commands).
   // We want to verify that, for all commands attempted, we successfully spoke
   // with the server. Therefore, we verify no errors and at least one SYNCER_OK.
-  return reached_server && !HadErrors(error);
+  return reached_server && !HadErrors(state);
 }
 
 void SyncSession::SetFinished() {
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index f1d8031..c792c9d 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -261,7 +261,6 @@ TEST_F(SyncSessionTest, ResetTransientState) {
             session_->source().updates_source);
   EXPECT_FALSE(status()->conflicts_resolved());
   EXPECT_FALSE(session_->HasMoreToSync());
-  EXPECT_FALSE(status()->TestAndClearIsDirty());
 }
 
 TEST_F(SyncSessionTest, Coalesce) {