sync: Introduce ModelTypeRegistry and helpers

sync: Introduce ModelTypeRegistry and helpers

Introduce the ModelTypeRegistry class and use it to manage the creation
of UpdateHandlers and CommitContributors.  The ModelTypeRegistry also
gets some help from the newly introduced UpdaterList and CommitterList
classes.

This lets us move the verbose iteration logic out of the code that's 
focused on building and executing commits and updates, which should make 
those functions easier to read. It gives us more freedom to experiment 
with other ways to manage the lists of commit contributors and update 
handlers, should we choose to do so. It prevents us from leaking the 
set of enabled types through the per-type maps. 

This patch is one of the last in the stack related to building a 
per-type abstraction into the sync engine, and also one of the first 
steps towards implementing run-time enable and disable logic for the 
new-style sync types. 

BUG=278484
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245654

[rlarocque, 2013-12-11 00:46:39]

I admit this was not part of my original plan, but I think the introduction of
this class makes a lot of sense.  

Here are some justifications for it:
- The iteration logic was uglier than I expected.  This lets me hide it.  It
also avoids exposing the set of enabled types through the map keys.
- It lets me absorb some free-floating functions that were in a _util.cc file
and put them in a real class.
- It hides the maps, so if we decide to refactor the per-type sync classes (eg.
break out update application into a separate type, or merge commits and updates
into the same type) we could do it without disturbing code outside this class.
- I underestimated the difficulty of allowing Bismarck and non-Bismarck types to
coexist in the DataTypeManager and SyncBackendRegistrar.  By putting hiding
Bismarck-specific logic in this class, I might be able to sidestep that issue,
or at least defer dealing with it until later.  

This CL only moves around code.  There should be no changes in behavior.

Please review.

[tim (not reviewing), 2013-12-11 18:10:08]

https://codereview.chromium.org/93433006/diff/1/sync/engine/sync_engine_type_...
File sync/engine/sync_engine_type_manager.h (right):

https://codereview.chromium.org/93433006/diff/1/sync/engine/sync_engine_type_...
sync/engine/sync_engine_type_manager.h:45: class SYNC_EXPORT_PRIVATE
SyncEngineTypeManager {
non-publically exported class in the sync/ directory and syncer:: namespace
don't need 'sync' in the name.  I notice this class as well as
SyncDirectoryCommitContribution, which is additionally awkward because the class
is Directory (or syncable::Directory, if anything) not SyncDirectory.

https://codereview.chromium.org/93433006/diff/1/sync/engine/sync_engine_type_...
sync/engine/sync_engine_type_manager.h:83: void RegisterCommitOnlyTypeForTest(
The fact that this class has a hodgepodge of public methods that were pulled
piecewise here and there from every different operation in the syncer suggests
this will increasingly become a dumping ground (the generic name 'Manager'
doesn't help either). As it stands we already have code pretty spread out for an
operation like "get updates" or "process updates", but it really seems possible
to keep that code localized with an abstraction in place to deal with type
partitioning. Seeing code pulled from download.cc, commit.cc,
process_updates_util.cc and dumped here is a bad smell.

Did you consider trying to design things in such a way that type partitioning be
done in a consistent manner used by all different parts of a sync cycle, versus
each step having to re-implement?  

The constraint that we want to keep code specific to the different actions that
comprise a sync cycle close together makes it easier to translate a high level
understanding of what a "sync cycle" is to the actual code. What happens if you
try to design things under that constraint?

[rlarocque, 2013-12-17 19:54:30]

I met with Tim offline to discuss his concerns.  I think patch set 2, which
splits the type manager into separate components for updates and commits
addresses most of them.

PTAL.

[Nicolas Zea, 2013-12-18 22:36:09]

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
File sync/engine/committer_list.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.cc:72: }
for safety sake, perhaps also DCHECK_LT(num_entries, max_entries)

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_list.h
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:25: typedef std::map<ModelType,
SyncDirectoryCommitContribution*> ContributionMap;
Do no other places declare this typedef? Does it need to be within the syncable
namespace or can it be scoped to CommitterList to avoid conflicts?

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:39: void SetEnabledSyncDirectoryTypes(
Why not just SetEnabledTypes? In the bismarck world, will this class not be able
to figure out whether a type is a bismarck  type or directory type, and act
accordingly?

Also, given that you'll be calling this on every configuration, and
ModelSafeWorkers and the directory don't actually change, perhaps they can be
passed at construction time?

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:40: const
std::vector<scoped_refptr<ModelSafeWorker> > workers,
const ref

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:50: // Registration a commit contributor directly. 
Useful in tests.
Registration -> Register

https://codereview.chromium.org/93433006/diff/20001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/download.cc#n...
sync/engine/download.cc:139:
updater_list->PrepareGetUpdates(proto_request_types, get_updates);
remind me why this is named proto_request_types, and not just enabled_types or
something?

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list.cc
File sync/engine/updater_list.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list....
sync/engine/updater_list.cc:84: DLOG(WARNING) << "Skipping update for unexpected
type "
Should this be a NOTREACHED as well?

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list....
sync/engine/updater_list.cc:139: for ( ; (progress_marker_iter !=
progress_index_by_type.end()
nit: remove space before ;? (not sure what the style guide rule is about this,
but grepping the codebase shows the lack of space is more common).

[rlarocque, 2014-01-03 01:46:40]

Patch rebased and refactored.  PTAL.

The first attempt at this patch had both the CommitContributors and
UpdateHandlers managed by a single class.

The second attempt split up this class into separate UpdaterList and
CommitterList classes.

This third patch is in between both previous approaches.  It has a single large
class (ModelTypeRegistry) that delegates to the UpdaterList and CommitterList
classes.

The top-level class makes it easier to have the workers passed in at
construction time, and it removes some duplicate iteration logic.  It could also
make the management of Bismarck type routing easier, if we decide to do it at
this level.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
File sync/engine/committer_list.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.cc:72: }
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> for safety sake, perhaps also DCHECK_LT(num_entries, max_entries)

Done.  Implemented with a NOTREACHED().

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_list.h
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:25: typedef std::map<ModelType,
SyncDirectoryCommitContribution*> ContributionMap;
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> Do no other places declare this typedef? Does it need to be within the
syncable
> namespace or can it be scoped to CommitterList to avoid conflicts?

It could be moved into CommitterList, but I think it would have to be public,
since it is referenced in the definition of the public function
GatherCommitContributions.

It's also re-declared privately in commit.h.

We can either choose to keep this global and remove the redeclaration in
commit.h, or we can make it private in both instances.  I'm leaning towards
making them both private, since that will allow commit.h to not include
committer_list.h.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:39: void SetEnabledSyncDirectoryTypes(
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> Why not just SetEnabledTypes? In the bismarck world, will this class not be
able
> to figure out whether a type is a bismarck  type or directory type, and act
> accordingly?

I'm thinking it won't have access to that information.  I'd prefer to leave the
choice of what kind of sync to use for which type up to
ProfileSyncComponentsFactoryImpl::RegisterDataTypes() so we can toggle it with
command line flags.

I suppose we could pass along some of the list of Bismarck types down to this
level, but I think it would be easier to track it at the ProfileSyncService
level.

> Also, given that you'll be calling this on every configuration, and
> ModelSafeWorkers and the directory don't actually change, perhaps they can be
> passed at construction time?

Yep.  The latest upload will include something like that.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:40: const
std::vector<scoped_refptr<ModelSafeWorker> > workers,
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> const ref

Removed in the most recent patch.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/committer_lis...
sync/engine/committer_list.h:50: // Registration a commit contributor directly. 
Useful in tests.
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> Registration -> Register

Done.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/download.cc#n...
sync/engine/download.cc:139:
updater_list->PrepareGetUpdates(proto_request_types, get_updates);
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> remind me why this is named proto_request_types, and not just enabled_types or
> something?

"request": Because not all enabled types are asked for in every request. 
Throttled types will not be included.

"proto": Because (I think) there was a time when the proxy types might still be
in the set of enabled types that got exposed to the SyncScheduler.  If we can
guarantee that no non-proto types wind up in the routing info, then this part is
unnecessary.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list.cc
File sync/engine/updater_list.cc (right):

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list....
sync/engine/updater_list.cc:84: DLOG(WARNING) << "Skipping update for unexpected
type "
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> Should this be a NOTREACHED as well?

Yes, probably.  Updated.

https://codereview.chromium.org/93433006/diff/20001/sync/engine/updater_list....
sync/engine/updater_list.cc:139: for ( ; (progress_marker_iter !=
progress_index_by_type.end()
On 2013/12/18 22:36:10, Nicolas Zea wrote:
> nit: remove space before ;? (not sure what the style guide rule is about this,
> but grepping the codebase shows the lack of space is more common).

Done.

[Nicolas Zea, 2014-01-04 02:09:34]

I think this is moving in the right direction. So I'm clear, the hierarchy +
output is basically:
ModelTypeRegistry
- CommiterList
  - CommitContributor
    -> CommitContribution
- UpdaterList
  - UpdateHandler
    -> SyncEntityList

CommitContributions are used by the Commit class, while the SyncEntityList is
used by the various classes in download.cc (is there a plan to create something
like thge Download/ApplyUpdates equivalent of the Commit class?)

https://codereview.chromium.org/93433006/diff/90001/sync/engine/committer_list.h
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/committer_lis...
sync/engine/committer_list.h:57: STLValueDeleter<CommitContributorMap>
commit_contributor_deleter_;
disallow copy and assign

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.cc:30: updater_list_.reset(new UpdaterList());
is it necessary to rebuild them on each reconfig? Why not just support updating
them?

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:27: syncable::Directory* directory,
nit: have directory be second param (input/output ordering)

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
Given that ModelSafeWorkers are refcounted, I think it's incorrect to be passing
them via raw pointer.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:47: syncable::Directory* directory_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/93433006/diff/90001/sync/engine/updater_list.h
File sync/engine/updater_list.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/updater_list....
sync/engine/updater_list.h:74: STLValueDeleter<UpdateHandlerMap>
update_handler_deleter_;
disallow copy and assign

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/public/...
File sync/internal_api/public/internal_components_factory.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/public/...
sync/internal_api/public/internal_components_factory.h:27: class
ModelTypeRegistry;
nit: typically these should be in abc order

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.h:318: // Maintains mostly-static state that
affects the way we interact with
elaborate on what "mostly" means?

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:30: class ModelTypeRegistry;
nit: abc order

[rlarocque, 2014-01-06 20:03:32]

Uploaded a new patch.  PTAL.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/committer_list.h
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/committer_lis...
sync/engine/committer_list.h:57: STLValueDeleter<CommitContributorMap>
commit_contributor_deleter_;
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.cc:30: updater_list_.reset(new UpdaterList());
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> is it necessary to rebuild them on each reconfig? Why not just support
updating
> them?

It's not necessary.  However it is easy, and more obviously correct.  The
performance cost should be negligible.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:27: syncable::Directory* directory,
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> nit: have directory be second param (input/output ordering)

Done.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> Given that ModelSafeWorkers are refcounted, I think it's incorrect to be
passing
> them via raw pointer.

I agree.  This looks wrong to me, too.

However, for some reason this is how they're passed around inside
SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was hoping
there was a good reason for that, but I have no idea what that reason is.  Even
if there is no good reason for it, it seems wrong to change it in this CL.

For what it's worth, this class will end up taking a scoped_refptr to these
workers and keep it for the rest of the sync backend's lifetime.  I think that
matches what the old code did.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:47: syncable::Directory* directory_;
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/updater_list.h
File sync/engine/updater_list.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/updater_list....
sync/engine/updater_list.h:74: STLValueDeleter<UpdateHandlerMap>
update_handler_deleter_;
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/public/...
File sync/internal_api/public/internal_components_factory.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/public/...
sync/internal_api/public/internal_components_factory.h:27: class
ModelTypeRegistry;
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> nit: typically these should be in abc order

Done.  Sorted the entire list.

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.h:318: // Maintains mostly-static state that
affects the way we interact with
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> elaborate on what "mostly" means?

Done.  It changes during configures, but is otherwise static.

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:30: class ModelTypeRegistry;
On 2014/01/04 02:09:34, Nicolas Zea wrote:
> nit: abc order

Done.

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_unittest.cc (right):

https://codereview.chromium.org/93433006/diff/90001/sync/sessions/sync_sessio...
sync/sessions/sync_session_unittest.cc:28: class SyncSessionTest : public
testing::Test,
I just realized that all tests were moved out of here in r239905.  I guess I
should just delete this file.  We can always fetch it from SVN if we ever need
it again.

[rlarocque, 2014-01-06 20:30:07]

On 2014/01/04 02:09:34, Nicolas Zea wrote:
> I think this is moving in the right direction. So I'm clear, the hierarchy +
> output is basically:
> ModelTypeRegistry
> - CommiterList
>   - CommitContributor
>     -> CommitContribution
> - UpdaterList
>   - UpdateHandler
>     -> SyncEntityList
> 
> CommitContributions are used by the Commit class, while the SyncEntityList is
> used by the various classes in download.cc (is there a plan to create
something
> like thge Download/ApplyUpdates equivalent of the Commit class?)

Your summary is correct.  My only nit is that I don't consider SyncEntityList to
be an object in the same way that CommitContribution is.  It's just a typedef to
make some function signatures easier to declare.

I have no plans to create a Download/ApplyUpdates equivalent of the Commit
class.  The main reason why it exists is to ensure that the SYNCING bits of all
entities get properly unset after a commit has completed.  The download an apply
updates operations have no such requirements.

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

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
On 2014/01/06 20:03:33, rlarocque wrote:
> On 2014/01/04 02:09:34, Nicolas Zea wrote:
> > Given that ModelSafeWorkers are refcounted, I think it's incorrect to be
> passing
> > them via raw pointer.
> 
> I agree.  This looks wrong to me, too.
> 
> However, for some reason this is how they're passed around inside
> SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was
hoping
> there was a good reason for that, but I have no idea what that reason is. 
Even
> if there is no good reason for it, it seems wrong to change it in this CL.
> 
> For what it's worth, this class will end up taking a scoped_refptr to these
> workers and keep it for the rest of the sync backend's lifetime.  I think that
> matches what the old code did.

Fair enough, maybe file a bug to fix this in a separate CL?

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
File sync/engine/committer_list.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
sync/engine/committer_list.cc:52: if (num_entries == max_entries) {
I think it's a bit cleaner to write this as:
if (num >= max) {
  DCHECK_EQ(num, max) << "...";
  break;
}

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
sync/engine/committer_list.h:40: void RegisterType(
It's only valid to do this at initialization time right? Perhaps mention that in
a comment? I also wonder if it's worth modifying this to make that more
explicit, such as turning this into an Initialize method that takes a map/vector
or something.

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
File sync/engine/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
sync/engine/model_type_registry.cc:17: : updater_list_(new UpdaterList()),
why allocate updater list and commiter list here? Doesn't
SetEnabledDirectoryTypes have to be called anyways?

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:32: // Sets the set of enabled types.
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?

https://codereview.chromium.org/93433006/diff/370001/sync/engine/updater_list.cc
File sync/engine/updater_list.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/updater_list...
sync/engine/updater_list.cc:143: && updates_iter == updates_by_type.end());
nit: have && at end of previous line

[rlarocque, 2014-01-08 01:37:22]

Patch updated.  PTAL.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> On 2014/01/06 20:03:33, rlarocque wrote:
> > On 2014/01/04 02:09:34, Nicolas Zea wrote:
> > > Given that ModelSafeWorkers are refcounted, I think it's incorrect to be
> > passing
> > > them via raw pointer.
> > 
> > I agree.  This looks wrong to me, too.
> > 
> > However, for some reason this is how they're passed around inside
> > SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was
> hoping
> > there was a good reason for that, but I have no idea what that reason is. 
> Even
> > if there is no good reason for it, it seems wrong to change it in this CL.
> > 
> > For what it's worth, this class will end up taking a scoped_refptr to these
> > workers and keep it for the rest of the sync backend's lifetime.  I think
that
> > matches what the old code did.
> 
> Fair enough, maybe file a bug to fix this in a separate CL?

Done: crbug.com/332251

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
File sync/engine/committer_list.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
sync/engine/committer_list.cc:52: if (num_entries == max_entries) {
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> I think it's a bit cleaner to write this as:
> if (num >= max) {
>   DCHECK_EQ(num, max) << "...";
>   break;
> }

Done.

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/committer_li...
sync/engine/committer_list.h:40: void RegisterType(
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> It's only valid to do this at initialization time right? Perhaps mention that
in
> a comment? I also wonder if it's worth modifying this to make that more
> explicit, such as turning this into an Initialize method that takes a
map/vector
> or something.

In the current system, they are only registered at init time.  I could foresee
scenarios where dynamically adding to or removing from the list is not wrong. 
Depending on how we deal with late-arriving Bismarck types, dynamically adding
and removing these things might be normal.  I see no reason to prohibit it.

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
File sync/engine/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
sync/engine/model_type_registry.cc:17: : updater_list_(new UpdaterList()),
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> why allocate updater list and commiter list here? Doesn't
> SetEnabledDirectoryTypes have to be called anyways?

I think it's sensible to guarantee that these lists are always not NULL as long
as the class exists.

When we start configuring non-directory types, this could start to matter.  I'd
prefer to not bake in any assumptions about calling SetEnabledDirectoryTypes
unless I have to.

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:32: // Sets the set of enabled types.
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> 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?

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.

https://codereview.chromium.org/93433006/diff/370001/sync/engine/updater_list.cc
File sync/engine/updater_list.cc (right):

https://codereview.chromium.org/93433006/diff/370001/sync/engine/updater_list...
sync/engine/updater_list.cc:143: && updates_iter == updates_by_type.end());
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> nit: have && at end of previous line

Done.

[tim (not reviewing), 2014-01-08 19:33:41]

I like where this is going. Your changes are really helping me understand what's
happening, and I appreciate you bearing with me on this.  I still have some
observations and suggestions for improving things that I think are really
important in making the code and encapsulation boundaries clearer and easier to
add to down the road.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
On 2014/01/07 23:11:40, Nicolas Zea wrote:
> On 2014/01/06 20:03:33, rlarocque wrote:
> > On 2014/01/04 02:09:34, Nicolas Zea wrote:
> > > Given that ModelSafeWorkers are refcounted, I think it's incorrect to be
> > passing
> > > them via raw pointer.
> > 
> > I agree.  This looks wrong to me, too.
> > 
> > However, for some reason this is how they're passed around inside
> > SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was
> hoping
> > there was a good reason for that, but I have no idea what that reason is. 
> Even
> > if there is no good reason for it, it seems wrong to change it in this CL.
> > 
> > For what it's worth, this class will end up taking a scoped_refptr to these
> > workers and keep it for the rest of the sync backend's lifetime.  I think
that
> > matches what the old code did.
> 
> Fair enough, maybe file a bug to fix this in a separate CL?

I think I traced this back to
https://chromiumcodereview.appspot.com/10388187/diff2/4001:8002/chrome/browse...


It suggests (loosely) that there may not be a reason other than just wanting a
quick way to convert from the Info struct to a collection of MSWs to pass in.
Though maybe Richard will remember since he wrote the patch :)  

If you don't want to change it as part of this patch that's fine, but we should
take the opportunity to prevent a potential use-after-free bug! (because in
theory the address could be garbage before you even have a chance to addref).
The ideal thing would be fix first and rebase.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/committer_li...
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/committer_li...
sync/engine/committer_list.h:28: // It owns these types and hides the details of
iterating over all of them.
Stuff responsible for shielding engine/ code from the full Directory and adding
knowledge of and scoping required for sync sessions like this (a good cause!)
today lives in sessions/.  The intent was to make it clear that engine code
should almost* always use sessions:: layer constructs instead of interacting
with the Directory directly. 

So I think this class, and (tangential to this review) things like
SyncDirectoryUpdateHandler and SyncDirectoryCommitContributor should shuffle
over to sessions/ (and they also shouldn't have Sync* in their name.  A follow
up patch to move/rename is fine, sorry I didn't see this (or review?) the
initial patch!).  We can discuss alternatives if there is strong protest to
continue having a sessions/ concept like that, but for now that's the way the
code is structured.

* at least in every case where we're iterating over or stashing entries from the
Directory for consumption or modification by engine/ code. A deps rule
forbidding includes is probably a pipe dream at this point although would be
nice when learning the code.)

https://codereview.chromium.org/93433006/diff/450001/sync/engine/committer_li...
sync/engine/committer_list.h:32: class SYNC_EXPORT_PRIVATE CommitterList {
(if you've read my UpdaterList comment, CommitterList seems like a much more
fitting name in this case, given the operations it exposes.)

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
2014 (check other files)

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:25: class SYNC_EXPORT_PRIVATE
ModelTypeRegistry {
Class comment, and in same vein as the CommitterList comment, this Directory
wrapper belongs in sessions/, although you should read the rest of my comments
here and draw your own conclusion once the dust settles.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/syncer.cc#ne...
sync/engine/syncer.cc:117:
session->context()->updater_list()->ApplyUpdatesForAllTypes(
:) 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?

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list.h
File sync/engine/updater_list.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list...
sync/engine/updater_list.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
2014

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list...
sync/engine/updater_list.h:43: class SYNC_EXPORT_PRIVATE UpdaterList {
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.)

https://codereview.chromium.org/93433006/diff/450001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.h:319: // This state will be updated during
configuration cycles, but otherwise
Perhaps saying "updated when routing info changes, such as when entering or
exiting a configuration cycle" (or "changes when entering or exiting a
configuration cycle" if you want to avoid "routing info").

[rlarocque, 2014-01-09 00:31:17]

Fixed a few things.  

I've also started working on a separate patch for scopedptr-izing the
ModelSafeWorkers vectors.

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re...
sync/engine/model_type_registry.h:28: const std::vector<ModelSafeWorker*>&
workers);
On 2014/01/08 19:33:41, timsteele wrote:
> On 2014/01/07 23:11:40, Nicolas Zea wrote:
> > On 2014/01/06 20:03:33, rlarocque wrote:
> > > On 2014/01/04 02:09:34, Nicolas Zea wrote:
> > > > Given that ModelSafeWorkers are refcounted, I think it's incorrect to be
> > > passing
> > > > them via raw pointer.
> > > 
> > > I agree.  This looks wrong to me, too.
> > > 
> > > However, for some reason this is how they're passed around inside
> > > SyncBackendHostCore's DoInitializeOptions and SyncManager::Init().  I was
> > hoping
> > > there was a good reason for that, but I have no idea what that reason is. 
> > Even
> > > if there is no good reason for it, it seems wrong to change it in this CL.
> > > 
> > > For what it's worth, this class will end up taking a scoped_refptr to
these
> > > workers and keep it for the rest of the sync backend's lifetime.  I think
> that
> > > matches what the old code did.
> > 
> > Fair enough, maybe file a bug to fix this in a separate CL?
> 
> I think I traced this back to
>
https://chromiumcodereview.appspot.com/10388187/diff2/4001:8002/chrome/browse...
> 
> 
> It suggests (loosely) that there may not be a reason other than just wanting a
> quick way to convert from the Info struct to a collection of MSWs to pass in.
> Though maybe Richard will remember since he wrote the patch :)  
> 
> If you don't want to change it as part of this patch that's fine, but we
should
> take the opportunity to prevent a potential use-after-free bug! (because in
> theory the address could be garbage before you even have a chance to addref).
> The ideal thing would be fix first and rebase.  

Oh, I see what happened there.  I think I was trying to keep it consistent with
the old code, where the ModelSafeWorker array was retrieved from the registrar
as a vector<ModelSafeWorker*>.  The question then becomes: why did the registrar
expose these workers, which were at the time and still are refcounted, as raw
pointers?

You may be right about the use after free bug.  That's a good reason to think of
this quirk as not harmless, and therefore worth fixing.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/committer_li...
File sync/engine/committer_list.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/committer_li...
sync/engine/committer_list.h:28: // It owns these types and hides the details of
iterating over all of them.
On 2014/01/08 19:33:42, timsteele wrote:
> Stuff responsible for shielding engine/ code from the full Directory and
adding
> knowledge of and scoping required for sync sessions like this (a good cause!)
> today lives in sessions/.  The intent was to make it clear that engine code
> should almost* always use sessions:: layer constructs instead of interacting
> with the Directory directly. 
> 
> So I think this class, and (tangential to this review) things like
> SyncDirectoryUpdateHandler and SyncDirectoryCommitContributor should shuffle
> over to sessions/ (and they also shouldn't have Sync* in their name.  A follow
> up patch to move/rename is fine, sorry I didn't see this (or review?) the
> initial patch!).  We can discuss alternatives if there is strong protest to
> continue having a sessions/ concept like that, but for now that's the way the
> code is structured.
> 
> * at least in every case where we're iterating over or stashing entries from
the
> Directory for consumption or modification by engine/ code. A deps rule
> forbidding includes is probably a pipe dream at this point although would be
> nice when learning the code.)
> 

Can we defer the move to a future CL?  That way we can move them all at once. 
And probably rename do the s/SyncDirectory/Directory/ change while we're at it.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
File sync/engine/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2014/01/08 19:33:42, timsteele wrote:
> 2014 (check other files)

Done.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/model_type_r...
sync/engine/model_type_registry.h:25: class SYNC_EXPORT_PRIVATE
ModelTypeRegistry {
On 2014/01/08 19:33:42, timsteele wrote:
> Class comment, and in same vein as the CommitterList comment, this Directory
> wrapper belongs in sessions/, although you should read the rest of my comments
> here and draw your own conclusion once the dust settles.

Added class comment.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/syncer.cc#ne...
sync/engine/syncer.cc:117:
session->context()->updater_list()->ApplyUpdatesForAllTypes(
On 2014/01/08 19:33:42, timsteele wrote:
> :) 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.

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.  

> 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?

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.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list.h
File sync/engine/updater_list.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list...
sync/engine/updater_list.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2014/01/08 19:33:42, timsteele wrote:
> 2014

Done.

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list...
sync/engine/updater_list.h:43: class SYNC_EXPORT_PRIVATE UpdaterList {
On 2014/01/08 19:33:42, timsteele wrote:
> 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. 

In what ways does this class have a different function from CommitterList?
 
> 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.

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.

> 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.)

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?

https://codereview.chromium.org/93433006/diff/450001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.h:319: // This state will be updated during
configuration cycles, but otherwise
On 2014/01/08 19:33:42, timsteele wrote:
> Perhaps saying "updated when routing info changes, such as when entering or
> exiting a configuration cycle" (or "changes when entering or exiting a
> configuration cycle" if you want to avoid "routing info").

Done.  I went with the second suggestion.

[rlarocque, 2014-01-09 05:10:38]

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list.h
File sync/engine/updater_list.h (right):

https://codereview.chromium.org/93433006/diff/450001/sync/engine/updater_list...
sync/engine/updater_list.h:43: class SYNC_EXPORT_PRIVATE UpdaterList {
On 2014/01/09 00:31:18, rlarocque wrote:
> On 2014/01/08 19:33:42, timsteele wrote:
> > 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. 
> 
> In what ways does this class have a different function from CommitterList?
>  
> > 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.
> 
> 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.
> 
> > 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.)
> 
> 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?

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

[rlarocque, 2014-01-10 21:59:29]

Ping, I think?  I'm not sure what the status of this patch is.

I've got a separate review for refcounting the ModelSafeWorkers, but I think
this review can proceed in parallel with that one.

I've uploaded updates in response to some comments, and pushed back on others. 
What's next?  Should we continue with rounds of comments, or meet in person to
talk it out?

[tim (not reviewing), 2014-01-10 23:56:09]

The main remaining point of contention, I think, is that I still believe
the RegisterType function (and the task of filling the UpdateHandlerMap),
is separate from the GetUpdatesProcessing work that "UpdaterList" does. I'm
actually very much in favor of keeping the data and the GetUpdatesProcessor
work coupled as you say (in a class, in fact I would strongly prefer that
to the "free function" suggestion I made), but the bottom line is that I
think the Syncer needs a thing that has that map and knows how to process
it for GetUpdates, and the ModelTypeRegistry just needs to add to a list
(as you said in your 'plurality' comment).

I don't think it breaks the abstraction or anything if we have to pass the
set from one component to the other at some point. It could be I'm still
misunderstanding (should the set of handlers change, from the Syncer's
perspective, between the time the scheduler calls (e.g) NormalSyncShare and
that call returns, or is the set const from the syncer's perspective?).
 Perhaps we should talk in person on Monday to sort this out. In my head
anyway, what I'm proposing it *not* the same as what you had in patch set
1.  I still very much agree with this new design that only passes update
state to update functions, and commit state to commit functions.

(I also strongly insist that UpdaterList is a misleading name because it
has no list state, has no add/remove/iterate methods, and "list" is a
fundamentally well understood ADT.)


On Fri, Jan 10, 2014 at 1:59 PM, <rlarocque@chromium.org> wrote:

> Ping, I think?  I'm not sure what the status of this patch is.
>
> I've got a separate review for refcounting the ModelSafeWorkers, but I
> think
> this review can proceed in parallel with that one.
>
> I've uploaded updates in response to some comments, and pushed back on
> others.
> What's next?  Should we continue with rounds of comments, or meet in
> person to
> talk it out?
>
> https://codereview.chromium.org/93433006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rlarocque, 2014-01-14 01:24:50]

Here's my understanding of this morning's discussion.

There are three things to be done here.  There must be entities that:
1. Own the maps.
2. Add items to the maps.
3. Iterate over the maps and do stuff.

Personally, I'd like to hide the typedef as much as possible.

I believe Tim would like the iteration over the maps to be available only from
engine/, and for the registration of new types to be available only from
sessions/.

To move us towards this goal, I made the ModelTypeRegistry own the maps
directly.  The SyncSessionContext now owns the UpdaterList and CommitterList. 
Those classes are now wrapped around maps that they do not own.  The UpdaterList
and CommitterList are available only through the SyncSessionContext and are now
not available to the ModelTypeRegistry.  Registration of new types is possible
only through the ModelTypeRegistry.

This patch is not complete yet.  If we decide to go with this approach, then
I'll need to go and update some names (I mostly agree that "*List" are not great
names) and to update some comments.

Before I do that, though, I wanted to see if everyone agrees with the division
of responsibilities presented in this latest patch.  Please discuss.

[tim (not reviewing), 2014-01-14 20:26:46]

Just one comment about ownership that may have been less clear during our
discussion yesterday.

https://codereview.chromium.org/93433006/diff/980001/sync/sessions/sync_sessi...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/93433006/diff/980001/sync/sessions/sync_sessi...
sync/sessions/sync_session_context.h:171: scoped_ptr<UpdaterList> updater_list_;
In the sessions/engine approach this patch is shooting for, UpdaterList
(GetUpdatesProcessor) is an engine concept, and SyncSessionContext isn't the
right place for it to live.

Syncer is probably the right place. If possible, better than having the Syncer
own an UpdaterList (which still feels like a longer-than-necessary-lifetime?)
would be for the DownloadAndApplyUpdates function to take an UpdaterList
(GetUpdatesProcessor) that was newly constructed on the stack by the Syncer,
using either a const ModelTypeRegistry that was passed in to NormalSyncShare
(wlog) or the SSContext.

Having NormalSyncShare (wlog) build the UpdaterList and eventually *not*
perpetuate the Session or Context down the chain from there moves us towards
reducing SyncSessionContext dependencies deep in syncer code by passing in just
the parts we need at the start of a sync share operation.

(I still feel that CommitterList and UpdaterList are different here because
"UpdaterList" is concerned with building actual GU messages directly. That said
I'm not strongly opposed to moving CommitterList as well; it seems like it would
make more sense from a lifetime perspective because it's tied to a *SyncShare
operation.)

[rlarocque, 2014-01-15 01:16:46]

Patch updated.  PTAL.

https://codereview.chromium.org/93433006/diff/980001/sync/sessions/sync_sessi...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/93433006/diff/980001/sync/sessions/sync_sessi...
sync/sessions/sync_session_context.h:171: scoped_ptr<UpdaterList> updater_list_;
On 2014/01/14 20:26:46, timsteele wrote:
> In the sessions/engine approach this patch is shooting for, UpdaterList
> (GetUpdatesProcessor) is an engine concept, and SyncSessionContext isn't the
> right place for it to live.
> 
> Syncer is probably the right place. If possible, better than having the Syncer
> own an UpdaterList (which still feels like a longer-than-necessary-lifetime?)
> would be for the DownloadAndApplyUpdates function to take an UpdaterList
> (GetUpdatesProcessor) that was newly constructed on the stack by the Syncer,
> using either a const ModelTypeRegistry that was passed in to NormalSyncShare
> (wlog) or the SSContext.
> 
> Having NormalSyncShare (wlog) build the UpdaterList and eventually *not*
> perpetuate the Session or Context down the chain from there moves us towards
> reducing SyncSessionContext dependencies deep in syncer code by passing in
just
> the parts we need at the start of a sync share operation.
> 
> (I still feel that CommitterList and UpdaterList are different here because
> "UpdaterList" is concerned with building actual GU messages directly. That
said
> I'm not strongly opposed to moving CommitterList as well; it seems like it
would
> make more sense from a lifetime perspective because it's tied to a *SyncShare
> operation.)

Done, I think.

[tim (not reviewing), 2014-01-15 18:02:58]

Yup.  Aside from 

1) renaming UpdaterList to GetUpdatesProcessor
2) moving ModelTypeRegistry to sessions/
3) maybe finding a way to clean up HandleCycleBegin and share UpdaterList
construction

(and in a followup, perform the move + rename mentioned in an earlier patch set
re: SyncDirectoryUpdateHandler)

this is more, I think, in line with the current engine design that draws a line
between the "steady state", when sync cycles aren't occuring and we're just
tracking what needs to happen, and then the core "engine" logic that comes
freshly into play when a sync cycle is initiated by the scheduler / Syncer.

Does anyone else (e.g Nicolas) have a thought on this?

[rlarocque, 2014-01-16 00:34:34]

On 2014/01/15 18:02:58, timsteele wrote:
> Yup.  Aside from 
> 
> 1) renaming UpdaterList to GetUpdatesProcessor
> 2) moving ModelTypeRegistry to sessions/
> 3) maybe finding a way to clean up HandleCycleBegin and share UpdaterList
> construction
> 
> (and in a followup, perform the move + rename mentioned in an earlier patch
set
> re: SyncDirectoryUpdateHandler)
> 
> this is more, I think, in line with the current engine design that draws a
line
> between the "steady state", when sync cycles aren't occuring and we're just
> tracking what needs to happen, and then the core "engine" logic that comes
> freshly into play when a sync cycle is initiated by the scheduler / Syncer.
> 
> Does anyone else (e.g Nicolas) have a thought on this?

The renames are a bit of a pain, so I wanted to save them for last.

I've just uploaded a patch that addresses items 1 and 2.

I don't think item 3 is worth dealing with right now.  I'll be chipping away at
the ugliness that's currently in that area of the code with the next few
patches.  I expect significant improvements once we can eliminate the
SyncSessionSnapshot.  We should revisit the issue then.

[rlarocque, 2014-01-16 00:35:13]

On 2014/01/16 00:34:34, rlarocque wrote:
> On 2014/01/15 18:02:58, timsteele wrote:
> > Yup.  Aside from 
> > 
> > 1) renaming UpdaterList to GetUpdatesProcessor
> > 2) moving ModelTypeRegistry to sessions/
> > 3) maybe finding a way to clean up HandleCycleBegin and share UpdaterList
> > construction
> > 
> > (and in a followup, perform the move + rename mentioned in an earlier patch
> set
> > re: SyncDirectoryUpdateHandler)
> > 
> > this is more, I think, in line with the current engine design that draws a
> line
> > between the "steady state", when sync cycles aren't occuring and we're just
> > tracking what needs to happen, and then the core "engine" logic that comes
> > freshly into play when a sync cycle is initiated by the scheduler / Syncer.
> > 
> > Does anyone else (e.g Nicolas) have a thought on this?
> 
> The renames are a bit of a pain, so I wanted to save them for last.
> 
> I've just uploaded a patch that addresses items 1 and 2.
> 
> I don't think item 3 is worth dealing with right now.  I'll be chipping away
at
> the ugliness that's currently in that area of the code with the next few
> patches.  I expect significant improvements once we can eliminate the
> SyncSessionSnapshot.  We should revisit the issue then.

Oops, I sent that message prematurely.  The patch will be uploaded in ~5 mins.

[rlarocque, 2014-01-16 00:46:14]

On 2014/01/16 00:35:13, rlarocque wrote:
> On 2014/01/16 00:34:34, rlarocque wrote:
> > On 2014/01/15 18:02:58, timsteele wrote:
> > > Yup.  Aside from 
> > > 
> > > 1) renaming UpdaterList to GetUpdatesProcessor
> > > 2) moving ModelTypeRegistry to sessions/
> > > 3) maybe finding a way to clean up HandleCycleBegin and share UpdaterList
> > > construction
> > > 
> > > (and in a followup, perform the move + rename mentioned in an earlier
patch
> > set
> > > re: SyncDirectoryUpdateHandler)
> > > 
> > > this is more, I think, in line with the current engine design that draws a
> > line
> > > between the "steady state", when sync cycles aren't occuring and we're
just
> > > tracking what needs to happen, and then the core "engine" logic that comes
> > > freshly into play when a sync cycle is initiated by the scheduler /
Syncer.
> > > 
> > > Does anyone else (e.g Nicolas) have a thought on this?
> > 
> > The renames are a bit of a pain, so I wanted to save them for last.
> > 
> > I've just uploaded a patch that addresses items 1 and 2.
> > 
> > I don't think item 3 is worth dealing with right now.  I'll be chipping away
> at
> > the ugliness that's currently in that area of the code with the next few
> > patches.  I expect significant improvements once we can eliminate the
> > SyncSessionSnapshot.  We should revisit the issue then.
> 
> Oops, I sent that message prematurely.  The patch will be uploaded in ~5 mins.

Done.  The new patch is now uploaded.

[tim (not reviewing), 2014-01-16 18:28:22]

LGTM, contingent on:

1) addressing comments below
2) a future CL to sort out ProcessUpdatesUtil, GetUpdatesProcessor, and
SyncDirectoryUpdateHandler, which have partially overlapping responsibilities as
it is now. 
3) finding a way to clean up HandleCycleBegin to share GetUpdatesProcessor
construction in the cycle prep code path (future CL)

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/download.cc...
sync/engine/download.cc:10: #include "sync/engine/process_updates_util.h"
I don't think we need this here now.

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/process_upd...
File sync/engine/process_updates_util.h (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/process_upd...
sync/engine/process_updates_util.h:55: void ProcessUpdate(
As far as I can tell, nothing calls this, or VerifyUpdate, except
ProcessDownloadedUpdates.  Seems like that should be the only function exposed
here?  Which overlaps somewhat with GetUpdatesProcessor, but one thing at a
time.  We can sort that out when more dust settles.

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
sync/sessions/model_type_registry.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit, remove the (c)

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
sync/sessions/model_type_registry.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit, remove the (c)

[rlarocque, 2014-01-16 20:28:41]

I've uploaded a new CL to address Tim's latest comments.

I agree very much that there's lots of cleanup to be done around the separation
of responsibilities, attaching free functions to classes, etc.

For the record, my hopes for the future include:

- Make UpdateHandler and CommitContributor into interfaces that
SyncDirectoryUpdateHandler and SyncDirectoryCommitContributor inherit from. This
will be a good opportunity to rename them to remove the 'Sync' prefix.

- Continue to chip away at the SyncSessionSnapshot.  Once we remove it, we can
finally remove the last remnants of the 'sync cycle' concept.  This will allow
us to further separate GU and commit logic, enable better handling of
exceptional cases, and let us remove the "HandleCycleBegin/HandleCycleEnd"
boilerplate code.

- Make the Configuration download updates function more special.  This will
allow us to perform configure cycles while only needing to reconfigure the set
of UpdateProcessors and CommitContributors once.  Also, it makes it possible to
implement an optimization that will allow us to avoid allocating a big chunk of
memory during initial sync.

- Once some of the previous bullet point is addressed, add support for
configurations of 'distributed sync' types.  Expect a design doc on this topic
soon.

I'll try to address your other points as this work proceeds.  However, in some
cases I think it would make sense to wait until the dust settles from some of
these changes before putting significant effort into making the code prettier. 
We should address this on a case by case basis.

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/download.cc...
sync/engine/download.cc:10: #include "sync/engine/process_updates_util.h"
On 2014/01/16 18:28:22, timsteele wrote:
> I don't think we need this here now.

Removed.

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/process_upd...
File sync/engine/process_updates_util.h (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/engine/process_upd...
sync/engine/process_updates_util.h:55: void ProcessUpdate(
On 2014/01/16 18:28:22, timsteele wrote:
> As far as I can tell, nothing calls this, or VerifyUpdate, except
> ProcessDownloadedUpdates.  Seems like that should be the only function exposed
> here?  Which overlaps somewhat with GetUpdatesProcessor, but one thing at a
> time.  We can sort that out when more dust settles.

You're right, there's no reason to expose these here.

I've hidden them in the .cc file.

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
sync/sessions/model_type_registry.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/01/16 18:28:22, timsteele wrote:
> nit, remove the (c)

Done.

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/93433006/diff/1100001/sync/sessions/model_typ...
sync/sessions/model_type_registry.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/01/16 18:28:22, timsteele wrote:
> nit, remove the (c)

Done.

[Nicolas Zea, 2014-01-16 21:08:25]

LGTM with nits

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
File sync/engine/get_updates_processor.cc (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
sync/engine/get_updates_processor.cc:33: namespace {
optional nit: I find it useful to have the anon namespace methods at the top of
the file so they're clearly separated.

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
File sync/engine/get_updates_processor.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
sync/engine/get_updates_processor.h:45: GetUpdatesProcessor(UpdateHandlerMap*
update_handler_map);
explicit

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/process_upd...
File sync/engine/process_updates_util.cc (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/process_upd...
sync/engine/process_updates_util.cc:303: 
remove extra newline

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/syncer.h#ne...
sync/engine/syncer.h:89: CommitProcessor* committer_list);
update name

https://codereview.chromium.org/93433006/diff/1190001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl.h:37: class ModelTypeRegistry;
nit: abc order

[rlarocque, 2014-01-16 21:51:28]

Addressed nits and updated the patch.

I'll start running it against the trybots to make sure it still passes all
tests.

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
File sync/engine/get_updates_processor.cc (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
sync/engine/get_updates_processor.cc:33: namespace {
On 2014/01/16 21:08:26, Nicolas Zea wrote:
> optional nit: I find it useful to have the anon namespace methods at the top
of
> the file so they're clearly separated.

Done.

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
File sync/engine/get_updates_processor.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/get_updates...
sync/engine/get_updates_processor.h:45: GetUpdatesProcessor(UpdateHandlerMap*
update_handler_map);
On 2014/01/16 21:08:26, Nicolas Zea wrote:
> explicit

Done.

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/process_upd...
File sync/engine/process_updates_util.cc (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/process_upd...
sync/engine/process_updates_util.cc:303: 
On 2014/01/16 21:08:26, Nicolas Zea wrote:
> remove extra newline

Done.

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/engine/syncer.h#ne...
sync/engine/syncer.h:89: CommitProcessor* committer_list);
On 2014/01/16 21:08:26, Nicolas Zea wrote:
> update name

Done.

https://codereview.chromium.org/93433006/diff/1190001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/93433006/diff/1190001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl.h:37: class ModelTypeRegistry;
On 2014/01/16 21:08:26, Nicolas Zea wrote:
> nit: abc order

Done.

[commit-bot: I haz the power, 2014-01-17 21:35:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/93433006/1320001

[commit-bot: I haz the power, 2014-01-17 23:22:01]

Change committed as 245654