sync: Introduce ModelTypeRegistry and helpers

sync/engine/updater_list.h

Index: sync/engine/updater_list.h
diff --git a/sync/engine/updater_list.h b/sync/engine/updater_list.h
new file mode 100644
index 0000000000000000000000000000000000000000..40f0b70e6f9a6fa0139d22127bc59c5d47c4f012
--- /dev/null
+++ b/sync/engine/updater_list.h
@@ -0,0 +1,82 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

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

2014

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SYNC_ENGINE_UPDATER_LIST_H_
+#define SYNC_ENGINE_UPDATER_LIST_H_
+
+#include <map>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/stl_util.h"
+#include "sync/base/sync_export.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/engine/model_safe_worker.h"
+
+namespace sync_pb {
+class GetUpdatesMessage;
+class GetUpdatesResponse;
+}  // namespace sync_pb
+
+namespace syncer {
+
+namespace sessions {
+class StatusController;
+}  // namespace sessions
+
+namespace syncable {
+class Directory;
+}  // namespace syncable
+
+class SyncDirectoryUpdateHandler;
+
+typedef std::vector<const sync_pb::SyncEntity*> SyncEntityList;
+typedef std::map<ModelType, SyncEntityList> TypeSyncEntityMap;
+
+// This class manages the set of per-type syncer objects.
+//
+// It owns these types and hides the details of iterating over all of them.
+// Most methods allow the caller to specify a subset of types on which the
+// operation is to be applied.  It is a logic error if the supplied set of types
+// contains a type which was not previously registered with the manager.
+class SYNC_EXPORT_PRIVATE UpdaterList {

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

I think UpdaterList is a poor name for this. This doesn't fit the "list"
paradigm much at all. It has no "list" state or add/remove methods. It's not
really an iterator, either, if you look at the methods it exposes. Nor is it
similar in function to CommitterList. And both "list" and "iterator" are common
enough terms that this class is, I think, easily contrary to everyone's
expectations. 

It was a little challenging to come up with a good name because this class is
both preparing GUs and processing GU responses, and applying updates, and then
there's "RegisterType". All but the latter are concerned with processing a
GetUpdates operation, though, so GetUpdatesProcessor seems like a better name.


Next up - When I got to syncer.cc, I had the impression UpdaterList
(GetUpdatesProcessor) in particular really belongs as a member of the Syncer vs
ModelTypeRegistry.  See my comment there, and consider perhaps not having an
UpdaterList* on ModelTypeRegistry, and just having an  UpdateHandlerMap, that
either the MTR or Context would perform the equivalent of RegisterType on
whenever SetEnabledDirectoryTypes is called, and have the rest of the
UpdaterList functionality live elsewhere (within the Syncer's reach).

(And yes, I think engine/ is where the resulting GetUpdatesProcessor code
belongs (class or otherwise) -- it doesn't interact with the Directory, it uses
the sessions DirectoryUpdateHandler, which is the thing actually hiding
Directory.)

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

In what ways does this class have a different function from CommitterList?
 
If you try to name this class based on how other classes interact with it, then
you're going to run into trouble.  The ModelTypeRegistry sees it as a plurality
of objects that it manages with RegisterType().  The Syncer sees it as one
unified thing that manages the GetUpdates procedure.  (Admittedly,
GetUpdatesProcessor is a good name for that facet of it).

I chose UpdaterList because if you look at this .h file on its own, and consider
the class' contents without any consideration of how other objects interact with
it, then it seems like a pretty good name.  Since naming it based on its
interactions would inevitably lead to conflict and confusion, this seemed like a
good strategy.

I'm willing to use the name GetUpdatesProcessor if you feel strongly about it,
but I think that's just naming the interface that the Syncer sees and misnaming
the other parts of it.
So engine/ is the place where we put both the syncable::Directory, and the
components that are not allowed to touch syncable::Directory?  The
GetUpdatesProcessor code will live engine/, but it has to go through sessions/
in order to access any of its folder-hierarchy siblings?

[rlarocque, 2014/01/09 05:10:39]

Actually, never mind that last bit.  That was a dumb question.

+ public:
+  UpdaterList();
+  ~UpdaterList();
+
+  // Registers an update handler directly.
+  void RegisterType(
+      ModelType type,
+      SyncDirectoryUpdateHandler* handler);
+
+  // Populates a GetUpdates request message with per-type information.
+  void PrepareGetUpdates(ModelTypeSet gu_types,
+                         sync_pb::GetUpdatesMessage* get_updates);
+
+  // Processes a GetUpdates responses for each type.
+  bool ProcessGetUpdatesResponse(
+      ModelTypeSet gu_types,
+      const sync_pb::GetUpdatesResponse& gu_response,
+      sessions::StatusController* status_controller);
+
+  // Applies pending updates for all sync types known to the manager.
+  void ApplyUpdatesForAllTypes(sessions::StatusController* status_controller);
+
+ private:
+  typedef std::map<ModelType, SyncDirectoryUpdateHandler*> UpdateHandlerMap;
+
+  // A map of 'update handlers', one for each enabled type.
+  // This must be kept in sync with the routing info.  Our temporary solution to
+  // that problem is to initialize this map in set_routing_info().
+  UpdateHandlerMap update_handler_map_;
+
+  // Deleter for the |update_handler_map_|.
+  STLValueDeleter<UpdateHandlerMap> update_handler_deleter_;
+
+  DISALLOW_COPY_AND_ASSIGN(UpdaterList);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_ENGINE_UPDATER_LIST_H_