sync: Introduce ModelTypeRegistry and helpers

sync/engine/syncer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/syncer.h"
 
 #include "base/debug/trace_event.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 96 matching lines @@
                  kCreateMobileBookmarksFolder,
                  request_types));
   return HandleCycleEnd(session, sync_pb::GetUpdatesCallerInfo::PERIODIC);
 }
 
 void Syncer::ApplyUpdates(SyncSession* session) {
   TRACE_EVENT0("sync", "ApplyUpdates");
 
   ApplyControlDataUpdates(session->context()->directory());
 
-  UpdateHandlerMap* handler_map = session->context()->update_handler_map();
-  for (UpdateHandlerMap::iterator it = handler_map->begin();
-       it != handler_map->end(); ++it) {
-    it->second->ApplyUpdates(session->mutable_status_controller());
-  }
+  session->context()->updater_list()->ApplyUpdatesForAllTypes(

[tim (not reviewing), 2014/01/08 19:33:42]

:) I came to the conclusion that UpdaterList doesn't belong on the context
separately from seing this code (just from reasoning about what it's doing, see
my GetUpdatesProcessor comment), but this line made me smile; it drives home
that Syncer is a better spot, that the all the things "UpdaterList" expose with
the exception of RegisterType are really top level actions from the Syncer's
perspective.

Did you consider just keeping the UpdateHandlerMap in the ModelTypeRegistry, and
pulling the RegisterType functionality out of UpdaterList/GetUpdatesProcessor
and into MTR or SyncSessionContext, and having the rest of
UpdaterList/GetUpdatesProcessor operations just live in here or as free
functions acting on the UpdateHandlerMap?

[rlarocque, 2014/01/09 00:31:18]

That would make sense, but I think it makes more sense to wait until the
functions in download.cc have been refactored before dealing with this issue.  

There's a lot of state in SyncSessionContext that should be in Syncer instead. 
I'm working on fixing it, but we need to take this one step at a time.  Right
now, the SyncSessionContxt is basically the class that holds all the member
variables that the Syncer *should* own.  We should stick with that convention
for now.  
That's sounds pretty close to what patch set 1 was.  You objected to it because
it combined the UpdaterList and CommitterList functions in one object.

Unless you meant that UpdaterList should be merged into MTR while CommitterList
is kept separate.  In that case, it would be similar to patch set 2.  I
refactored that one to address some of Nicolas' comments about duplicate code
and definitions.

I don't think it makes sense to move the UpdaterList operations into here.  The
point of the UpdaterList class is to keep the data (the current set of
UpdateHandlers) and operations on them (iterating over them to make them all act
as one) together.  If we were to split the iteration and the definition of the
list into two places, then that would be spreading tightly coupled logic into
two places.

+      session->mutable_status_controller());
 
   session->context()->set_hierarchy_conflict_detected(
       session->status_controller().num_hierarchy_conflicts() > 0);
 
   session->SendEventNotification(SyncEngineEvent::STATUS_CHANGED);
 }
 
 bool Syncer::DownloadAndApplyUpdates(
     ModelTypeSet request_types,
     SyncSession* session,
@@ skipping 23 matching lines @@
 
 SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types,
                                         sessions::SyncSession* session) {
   // The ExitRequested() check is unnecessary, since we should start getting
   // errors from the ServerConnectionManager if an exist has been requested.
   // However, it doesn't hurt to check it anyway.
   while (!ExitRequested()) {
     scoped_ptr<Commit> commit(
         Commit::Init(
             requested_types,
+            session->context()->enabled_types(),
             session->context()->max_commit_batch_size(),
             session->context()->account_name(),
             session->context()->directory()->cache_guid(),
-            session->context()->commit_contributor_map(),
+            session->context()->committer_list(),
             session->context()->extensions_activity()));
     if (!commit) {
       break;
     }
 
     SyncerError error = commit->PostAndProcessResponse(
         session,
         session->mutable_status_controller(),
         session->context()->extensions_activity());
     commit->CleanUp();
@@ skipping 15 matching lines @@
     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source) {
   if (!ExitRequested()) {
     session->SendSyncCycleEndEventNotification(source);
     return true;
   } else {
     return false;
   }
 }
 
 }  // namespace syncer