sync: Introduce ModelTypeRegistry and helpers

sync/engine/model_type_registry.h

+// Copyright (c) 2013 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.
+
+#ifndef SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+#define SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+
+#include <map>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.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 syncer {
+
+class CommitterList;
+class UpdaterList;
+
+namespace syncable {
+class Directory;
+}  // namespace syncable
+
+class SYNC_EXPORT_PRIVATE ModelTypeRegistry {
+ public:
+  ModelTypeRegistry(
+      const std::vector<ModelSafeWorker*>& workers,
+      syncable::Directory* directory);
+  ~ModelTypeRegistry();
+
+  // Sets the set of enabled types.

[Nicolas Zea, 2014/01/07 23:11:40]

It seems dangerous that this will destroy the updaterlist and committerlist
which may have been returned by the getters, possibly resulting in dangling
pointers. 

That might be an argument to not be recreating them. Alternatively maybe weak
pointers, or returning const refs might be viable?

[rlarocque, 2014/01/08 01:37:24]

True, not re-creating them and returning const refs would help a bit.  But if
you're willing to consider scenarios where some objects take a permanent pointer
to the UpdaterList, then you should probably start worrying about making sure
they relinquish those pointers before ModelTypeRegistry is deleted, too.  The
const refs won't help us there.

The safest way to avoid the kinds of errors you describe would be to not expose
the UpdaterList and CommitterList directly to the functions that need it.  The
functions they're interested in would be defined on the the ModelTypeRegistry
(eg. ModelTypeRegistry::GetCommitContributions()).  That would bring us back to
the original design of this CL, which Tim rejected because it combined too many
seemingly unrelated functions into one object.

I'm not too worried about lifetime errors in the patch as is.  We just need to
be disciplined about not holding on to pointers to the UpdaterList and
CommitterList.  I think that's not too hard of a rule to follow, and we'd be
likely to break some tests in an obvious way if that rule were violated.

+  void SetEnabledDirectoryTypes(const ModelSafeRoutingInfo& routing_info);
+
+  // Simple getters.
+  UpdaterList* updater_list();
+  CommitterList* committer_list();
+
+ private:
+  // Classes to manage the types hooked up to receive and commit sync data.
+  scoped_ptr<UpdaterList> updater_list_;
+  scoped_ptr<CommitterList> committer_list_;
+
+  // The known ModelSafeWorkers.
+  std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> > workers_map_;
+
+  // The directory.  Not owned.
+  syncable::Directory* directory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ModelTypeRegistry);
+};
+
+}  // namespace syncer
+
+#endif // SYNC_ENGINE_MODEL_TYPE_REGISTRY_H_
+