sync: Per-type update application

This change moves the update application functionality from the
ApplyUpdatesAndResolveConflictsCommand into the
SyncDirectoryUpdateHandler class.  This change will allow us to
implement update application differently for different types.

Because update application happens on the model threads, the
ApplyUpdatesAndResolveConflictsCommand had to be aware of
ModelSafeRoutingInfo, ModelSafeWorkers, and other concepts intended to
hide threading details.  The new code takes a different approach.  It
hides the threading details specific to each type inside its
SyncDirectoryUpateHandler by initializing it with a scoped_refptr to its
associated ModelSafeWorker.

The ApplyUpdatesAndResolveConflictsCommand was the last SyncerCommand.
With its removal, we can also remove the definitions of SyncerCommand,
ModelChangingSyncerCommand and SyncerCommandTest.

BUG=278484

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

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

6 files changed, 42 insertions, 106 deletions

diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index f6fbdb5..314a232 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -13,9 +13,7 @@
 namespace syncer {
 namespace sessions {
 
-StatusController::StatusController()
-    : group_restriction_in_effect_(false),
-      group_restriction_(GROUP_PASSIVE) {
+StatusController::StatusController() {
 }
 
 StatusController::~StatusController() {}
@@ -129,20 +127,14 @@ int StatusController::num_encryption_conflicts() const {
 }
 
 int StatusController::num_hierarchy_conflicts() const {
-  DCHECK(!group_restriction_in_effect_)
-      << "num_hierarchy_conflicts applies to all ModelSafeGroups";
   return model_neutral_.num_hierarchy_conflicts;
 }
 
 int StatusController::num_server_conflicts() const {
-  DCHECK(!group_restriction_in_effect_)
-      << "num_server_conflicts applies to all ModelSafeGroups";
   return model_neutral_.num_server_conflicts;
 }
 
 int StatusController::TotalNumConflictingItems() const {
-  DCHECK(!group_restriction_in_effect_)
-      << "TotalNumConflictingItems applies to all ModelSafeGroups";
   int sum = 0;
   sum += num_encryption_conflicts();
   sum += num_hierarchy_conflicts();
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 75b8b82..0e2c6f2 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -5,25 +5,14 @@
 // StatusController handles all counter and status related number crunching and
 // state tracking on behalf of a SyncSession.
 //
-// The most important feature of StatusController is the
-// ScopedModelSafeGroupRestriction.  Some of its functions expose per-thread
-// state, and can be called only when the restriction is in effect.  For
-// example, if GROUP_UI is set then the value returned from
-// commit_id_projection() will be useful for iterating over the commit IDs of
-// items that live on the UI thread.
+// This object may be accessed from many different threads.  It will be accessed
+// most often from the syncer thread.  However, when update application is in
+// progress it may also be accessed from the worker threads.  This is safe
+// because only one of them will run at a time, and the syncer thread will be
+// blocked until update application completes.
 //
-// Other parts of its state are global, and do not require the restriction.
-//
-// NOTE: There is no concurrent access protection provided by this class. It
-// assumes one single thread is accessing this class for each unique
-// ModelSafeGroup, and also only one single thread (in practice, the
-// SyncerThread) responsible for all "shared" access when no restriction is in
-// place. Thus, every bit of data is to be accessed mutually exclusively with
-// respect to threads.
-//
-// StatusController can also track if changes occur to certain parts of state
-// so that various parts of the sync engine can avoid broadcasting
-// notifications if no changes occurred.
+// This object contains only global state.  None of its members are per model
+// type counters.
 
 #ifndef SYNC_SESSIONS_STATUS_CONTROLLER_H_
 #define SYNC_SESSIONS_STATUS_CONTROLLER_H_
@@ -96,10 +85,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
   // download: in that case, this also returns false.
   bool ServerSaysNothingMoreToDownload() const;
 
-  ModelSafeGroup group_restriction() const {
-    return group_restriction_;
-  }
-
   base::Time sync_start_time() const {
     // The time at which we sent the first GetUpdates command for this sync.
     return sync_start_time_;
@@ -142,40 +127,13 @@ class SYNC_EXPORT_PRIVATE StatusController {
   void UpdateStartTime();
 
  private:
-  friend class ScopedModelSafeGroupRestriction;
-
   ModelNeutralState model_neutral_;
 
-  // Used to fail read/write operations on state that don't obey the current
-  // active ModelSafeWorker contract.
-  bool group_restriction_in_effect_;
-  ModelSafeGroup group_restriction_;
-
   base::Time sync_start_time_;
 
   DISALLOW_COPY_AND_ASSIGN(StatusController);
 };
 
-// A utility to restrict access to only those parts of the given
-// StatusController that pertain to the specified ModelSafeGroup.
-class ScopedModelSafeGroupRestriction {
- public:
-  ScopedModelSafeGroupRestriction(StatusController* to_restrict,
-                                  ModelSafeGroup restriction)
-      : status_(to_restrict) {
-    DCHECK(!status_->group_restriction_in_effect_);
-    status_->group_restriction_ = restriction;
-    status_->group_restriction_in_effect_ = true;
-  }
-  ~ScopedModelSafeGroupRestriction() {
-    DCHECK(status_->group_restriction_in_effect_);
-    status_->group_restriction_in_effect_ = false;
-  }
- private:
-  StatusController* status_;
-  DISALLOW_COPY_AND_ASSIGN(ScopedModelSafeGroupRestriction);
-};
-
 }  // namespace sessions
 }  // namespace syncer
 
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index e6b59e8..32e7b64 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -52,14 +52,5 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) {
   EXPECT_EQ(6, status.TotalNumConflictingItems());
 }
 
-// Basic test that non group-restricted state accessors don't cause violations.
-TEST_F(StatusControllerTest, Unrestricted) {
-  StatusController status;
-  status.model_neutral_state();
-  status.download_updates_succeeded();
-  status.ServerSaysNothingMoreToDownload();
-  status.group_restriction();
-}
-
 }  // namespace sessions
 }  // namespace syncer
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index af846d4..f576720 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -4,12 +4,8 @@
 
 // A class representing an attempt to synchronize the local syncable data
 // store with a sync server. A SyncSession instance is passed as a stateful
-// bundle to and from various SyncerCommands with the goal of converging the
-// client view of data with that of the server. The commands twiddle with
-// session status in response to events and hiccups along the way, set and
-// query session progress with regards to conflict resolution and applying
-// server updates, and access the SyncSessionContext for the current session
-// via SyncSession instances.
+// bundle throughout the sync cycle.  The SyncSession is not reused across
+// sync cycles; each cycle starts with a new one.
 
 #ifndef SYNC_SESSIONS_SYNC_SESSION_H_
 #define SYNC_SESSIONS_SYNC_SESSION_H_
diff --git a/sync/sessions/sync_session_context.cc b/sync/sessions/sync_session_context.cc
index d44591d..aa5dfa5 100644
--- a/sync/sessions/sync_session_context.cc
+++ b/sync/sessions/sync_session_context.cc
@@ -35,8 +35,10 @@ SyncSessionContext::SyncSessionContext(
       server_enabled_pre_commit_update_avoidance_(false),
       client_enabled_pre_commit_update_avoidance_(
           client_enabled_pre_commit_update_avoidance) {
-  for (size_t i = 0u; i < workers.size(); ++i)
-    workers_.push_back(workers[i]);
+  for (size_t i = 0u; i < workers.size(); ++i) {
+    workers_.insert(
+        std::make_pair(workers[i]->GetModelSafeGroup(), workers[i]));
+  }
 
   std::vector<SyncEngineEventListener*>::const_iterator it;
   for (it = listeners.begin(); it != listeners.end(); ++it)
@@ -48,24 +50,28 @@ SyncSessionContext::~SyncSessionContext() {
 
 void SyncSessionContext::set_routing_info(
     const ModelSafeRoutingInfo& routing_info) {
-  routing_info_ = routing_info;
+  enabled_types_ = GetRoutingInfoTypes(routing_info);
 
   // TODO(rlarocque): This is not a good long-term solution.  We must find a
   // better way to initialize the set of CommitContributors and UpdateHandlers.
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
-
+  STLDeleteValues<UpdateHandlerMap>(&update_handler_map_);
   STLDeleteValues<CommitContributorMap>(&commit_contributor_map_);
-  for (ModelTypeSet::Iterator it = enabled_types.First(); it.Good(); it.Inc()) {
-    SyncDirectoryCommitContributor* contributor =
-        new SyncDirectoryCommitContributor(directory(), it.Get());
-    commit_contributor_map_.insert(std::make_pair(it.Get(), contributor));
-  }
+  for (ModelSafeRoutingInfo::const_iterator routing_iter = routing_info.begin();
+       routing_iter != routing_info.end(); ++routing_iter) {
+    ModelType type = routing_iter->first;
+    ModelSafeGroup group = routing_iter->second;
+    std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> >::iterator
+        worker_it = workers_.find(group);
+    DCHECK(worker_it != workers_.end());
+    scoped_refptr<ModelSafeWorker> worker = worker_it->second;
 
-  STLDeleteValues<UpdateHandlerMap>(&update_handler_map_);
-  for (ModelTypeSet::Iterator it = enabled_types.First(); it.Good(); it.Inc()) {
     SyncDirectoryUpdateHandler* handler =
-        new SyncDirectoryUpdateHandler(directory(), it.Get());
-    update_handler_map_.insert(std::make_pair(it.Get(), handler));
+        new SyncDirectoryUpdateHandler(directory(), type, worker);
+    update_handler_map_.insert(std::make_pair(type, handler));
+
+    SyncDirectoryCommitContributor* contributor =
+        new SyncDirectoryCommitContributor(directory(), type);
+    commit_contributor_map_.insert(std::make_pair(type, contributor));
   }
 }
 
diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h
index d195943..5995ab1 100644
--- a/sync/sessions/sync_session_context.h
+++ b/sync/sessions/sync_session_context.h
@@ -3,15 +3,12 @@
 // found in the LICENSE file.
 
 // SyncSessionContext encapsulates the contextual information and engine
-// components specific to a SyncSession. A context is accessible via
-// a SyncSession so that session SyncerCommands and parts of the engine have
-// a convenient way to access other parts. In this way it can be thought of as
-// the surrounding environment for the SyncSession. The components of this
-// environment are either valid or not valid for the entire context lifetime,
-// or they are valid for explicitly scoped periods of time by using Scoped
-// installation utilities found below. This means that the context assumes no
-// ownership whatsoever of any object that was not created by the context
-// itself.
+// components specific to a SyncSession.  Unlike the SyncSession, the context
+// can be reused across several sync cycles.
+//
+// The context does not take ownership of its pointer members.  It's up to
+// the surrounding classes to ensure those members remain valid while the
+// context is in use.
 //
 // It can only be used from the SyncerThread.
 
@@ -70,8 +67,8 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext {
     return directory_;
   }
 
-  const ModelSafeRoutingInfo& routing_info() const {
-    return routing_info_;
+  ModelTypeSet enabled_types() const {
+    return enabled_types_;
   }
 
   void set_routing_info(const ModelSafeRoutingInfo& routing_info);
@@ -84,10 +81,6 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext {
     return &commit_contributor_map_;
   }
 
-  const std::vector<scoped_refptr<ModelSafeWorker> >& workers() const {
-    return workers_;
-  }
-
   ExtensionsActivity* extensions_activity() {
     return extensions_activity_.get();
   }
@@ -159,9 +152,9 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext {
   ServerConnectionManager* const connection_manager_;
   syncable::Directory* const directory_;
 
-  // A cached copy of SyncBackendRegistrar's routing info.
-  // Must be updated manually when SBR's state is modified.
-  ModelSafeRoutingInfo routing_info_;
+  // The set of enabled types.  Derrived from the routing info set with
+  // set_routing_info().
+  ModelTypeSet enabled_types_;
 
   // A map of 'update handlers', one for each enabled type.
   // This must be kept in sync with the routing info.  Our temporary solution to
@@ -180,7 +173,7 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext {
   STLValueDeleter<CommitContributorMap> commit_contributor_deleter_;
 
   // The set of ModelSafeWorkers.  Used to execute tasks of various threads.
-  std::vector<scoped_refptr<ModelSafeWorker> > workers_;
+  std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> > workers_;
 
   // We use this to stuff extensions activity into CommitMessages so the server
   // can correlate commit traffic with extension-related bookmark mutations.