sync: Implement per-type commit interface

sync: Implement per-type commit interface

Move most commit logic into per-type instances of
SyncDirectoryCommitContributor and SyncDirectoryCommitContribution
classes.  Make GetCommitIdsCommand and ProcessCommitResponseCommand
into container classes with only static methods.  Remove
OrderedCommitSet.

The point of these changes is to make way for different kinds of entity
committers.  The SyncDirectoryCommitContributor and
SyncDirectoryCommitContribution will eventually refactored into
implementations of more generic "commit contributor" and "commit
contribution" interfaces.

This commit leaves us with some structures that might look a bit odd to
someone unfamiliar with the history of this code.  In particular, many
of the old SyncerCommands look out of place.  We plan to refactor them
in future CLs.  For now, it's more important to make sure this CL has an
easy to read diff.

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

[rlarocque, 2013-10-02 21:02:51]

Here's the first per-type sync refactor CL.

This change is not as scary as it looks.  Most of the big diffs are due to code
that's been moved but not significantly changed.

There are a few ugly parts in here.  The initialization of CommitContributors in
the SyncSessionContext is particularly bad.  The long-term plan is to have the
context contain a map of per-type sync objects, and to remove its routing info
and workers members.  Until that plan is complete, though, I think we'll need to
tolerate both systems existing side-by-side with each other.

Please review.

[Nicolas Zea, 2013-10-05 00:06:03]

At a high level, I find the notion of a SyncDirectoryCommitContrbution unclear.
On the one hand, it replaces the notion we had before of a CommitSet, which was
basically an extraction of all those entries we wanted to commit. But now,
that's been augmented with logic to actually interact with server messages and
modify the directory directly.

I wonder if that logic should remain compartmentalized. I.e., keep extracting
the data to commit separate from actually performing a commit. Perhaps a
DataTypeCommitHelper makes sense, which can take a contribution and handle the
directory interactions that result.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:11: #include "base/stl_util.h"
what is this for?

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:20: namespace syncable {
nit, order after sessions (ABC order)

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:46: static
SyncDirectoryCommitContribution* Build(
Would it be better to explicitly pass model neutral write transactions around in
functions that make model neutral changes to the directory?

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:72: syncable::Directory*
directory,
move to be last param

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:73: std::vector<int64>
metahandles,
const ref

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:74:
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities);
const ref

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:82:
FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest, GatherByTypes);
nit: friending should be at top of private list. Also, I personally find it
cleaner to friend the test class itself (SyncDirectoryCommitContributionTest in
this case), and have that test class expose getters/setters for the individual
tests, rather than having to friend each individual test.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contributor.h:20: class
SyncDirectoryCommitContributor {
what is the purpose of this class? It just seems like a wrapper around
BuildContribution.

[rlarocque, 2013-10-07 23:00:05]

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:11: #include "base/stl_util.h"
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> what is this for?

Apparently nothing.  Removed.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:20: namespace syncable {
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> nit, order after sessions (ABC order)

Done.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:46: static
SyncDirectoryCommitContribution* Build(
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> Would it be better to explicitly pass model neutral write transactions around
in
> functions that make model neutral changes to the directory?

I'm not sure.

I think it makes sense to think of the CommitContribution as being at the right
"level" that it is allowed to open its own transactions.  I can also see the
argument against, though.

I think what tips the scales for me is the fact that this class will need to
open a ModelNeutralTransaction in its destructor.  (Which is, admittedly, a bit
odd.)  If we decide to keep that behavior, then there is precedent for allowing
member functions of this class to open transactions.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:72: syncable::Directory*
directory,
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> move to be last param

Done.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:73: std::vector<int64>
metahandles,
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> const ref

Done.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:74:
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities);
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> const ref

Done.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contribution.h:82:
FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest, GatherByTypes);
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> nit: friending should be at top of private list. 

Done.

> Also, I personally find it
> cleaner to friend the test class itself (SyncDirectoryCommitContributionTest
in
> this case), and have that test class expose getters/setters for the individual
> tests, rather than having to friend each individual test.

That strategy only works when you have a limited number of instances of the
object and you can have it defined in the test framework's constructor or
SetUp() methods.  The CommitContributionTests need to initialize some
test-specific initial state in the directory before initializing the
CommitContribution, so that won't work very well in this case.

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/3001/sync/engine/sync_directory...
sync/engine/sync_directory_commit_contributor.h:20: class
SyncDirectoryCommitContributor {
On 2013/10/05 00:06:03, Nicolas Zea wrote:
> what is the purpose of this class? It just seems like a wrapper around
> BuildContribution.

Yep, that's exactly what it is.  

It will be more important when I get around to defining the class hierarchy that
supports different kinds of CommitContributors.  The
SyncDirectoryCommitContributor is the first concrete implementation of the
CommitContributor interface (which I haven't bothered to explicitly define yet).
 In the future there will be another implementation, BismarckCommitContributor*
that creates BismarckCommitContribution* objects.  Having different factories is
necessary because the different kinds of contributions will be build using
different kinds of inputs.  For example, the BismarckCommitContribution will not
need a syncable::Directory.  

* Hopefully I'll have though of better names by the time I start writing those
CLs.

[Nicolas Zea, 2013-10-09 00:17:39]

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
File sync/engine/build_commit_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
sync/engine/build_commit_command.h:31: class SYNC_EXPORT_PRIVATE
BuildCommitCommand : public SyncerCommand {
Why not just make these all standalone functions at this point? (possibly as
helper functions within Commit.cc)

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:56: for (ModelTypeSet::Iterator it =
requested_types.First();
worth DCHECKING that requested_types.size() == contributor_map.size()?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:70: contributions.insert(std::make_pair(it.Get(),
contribution));
break if num_entries == max_entries?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:99: enabled_types.Put(it->first);
just use requested types?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:106: contributions.begin(); it != contributions.end();
++it) {
indent four more spaces

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:134: request_types.Put(it->first);
possibly extract this logic into a helper function (ContributionMapToTypes) in
commit_contributor.h?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:179: model_neutral_state().num_successful_bookmark_commits
== 0) {
indent by four more?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: SyncerError BuildAndPostCommits(
Does it make sense to incorporate this into the Commit object?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:60: class SYNC_EXPORT_PRIVATE Commit {
Can this be declared in the cc file? It's not used outside of
BuildAndPostCommits right?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:64: CommitContributorMap* contributor_map,
move down below the input params

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:71: std::map<ModelType, SyncDirectoryCommitContribution*>
contributions,
const ref for first these params

also, doesn't need the explicit keyword

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:53: void GetCommitIdsForType(
why not just have this at the bottom of the file now, so you don't have to
forward declare functions, and so you don't have multiple anon namespaces within
the file?

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.h:22: // These functions return handles in "commit
order".  A valid commit ordering is
This function returns.

That said, at this point I'm not sure there's much need to have this in a
separate file.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/process_commi...
File sync/engine/process_commit_response_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:40: static
sync_pb::CommitResponse::ResponseType ProcessSingleCommitResponse(
Again, seems odd to have a class with nothing but static functions.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.cc:16: using syncable::SYNCER;
move below GET_BY_HANDLE

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:8: #include <map>
not needed

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:28: // have been selected from
the syncable Directory and prepared for a commit.
for a commit -> for commit

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:36: // (It unsets its entities'
SYNCING bits.)
As you mentioned, it's odd to be doing this work in the destructor. I think it
would be cleaner if there were a CleanupCommit or FinishCommit method that did
this work.

That would also allow for passing an explicit transaction through the directory
changing methods as well, which I tend to prefer to avoid bugs where someone
holding a transaction calls one of these methods.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:42: // This function may return
NULL if this type has no items ready for and
doesn't "ready for" already imply "requiring"? I'd just say "ready for commit"

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:50: // Serialize this objects's
entries to the given commit request |msg|.
this object's -> this contribution's (here and below)

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution_unittest.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution_unittest.cc:6: #include
"sync/engine/sync_directory_commit_contribution.h"
have as first #include with a newline after

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:33: typedef std::map<ModelType,
SyncDirectoryCommitContributor*>
Would using linked_ptrs make sense here? (So you don't need to have
STLValueDeleters)

[rlarocque, 2013-10-09 20:00:18]

Upated the patch and addressed many comments.  PTAL.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
File sync/engine/build_commit_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
sync/engine/build_commit_command.h:31: class SYNC_EXPORT_PRIVATE
BuildCommitCommand : public SyncerCommand {
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> Why not just make these all standalone functions at this point? (possibly as
> helper functions within Commit.cc)

For now, I was trying to keep the diffs readable by not moving code between
files if I can avoid it.  I'd like to write a follow-up CL to clean up this mess
after this Cl has landed.

If you'd like, I could easily make these functions standalone and leave them in
this file.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:56: for (ModelTypeSet::Iterator it =
requested_types.First();
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> worth DCHECKING that requested_types.size() == contributor_map.size()?

That's not necessarily true.  I believe throttling is controlled with
requested_types.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:70: contributions.insert(std::make_pair(it.Get(),
contribution));
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> break if num_entries == max_entries?

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:99: enabled_types.Put(it->first);
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> just use requested types?

Maybe.  Should we list throttled types among the list of enabled types when
committing?  

I think the current behavior is that we do include them, and that this code
matches that behavior.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:106: contributions.begin(); it != contributions.end();
++it) {
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> indent four more spaces

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:134: request_types.Put(it->first);
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> possibly extract this logic into a helper function (ContributionMapToTypes) in
> commit_contributor.h?

ContributionMap is internal to Commit, and this seems to be the only instance of
this pattern.  I don't think the helper function is necessary right now.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.cc#new...
sync/engine/commit.cc:179: model_neutral_state().num_successful_bookmark_commits
== 0) {
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> indent by four more?

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: SyncerError BuildAndPostCommits(
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> Does it make sense to incorporate this into the Commit object?

Maybe.  I kind of like it the way it is, since it doesn't get access to Commit's
private members this way.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:60: class SYNC_EXPORT_PRIVATE Commit {
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> Can this be declared in the cc file? It's not used outside of
> BuildAndPostCommits right?

Good idea.  Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:64: CommitContributorMap* contributor_map,
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> move down below the input params

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:71: std::map<ModelType, SyncDirectoryCommitContribution*>
contributions,
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> const ref for first these params
> 
> also, doesn't need the explicit keyword

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:53: void GetCommitIdsForType(
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> why not just have this at the bottom of the file now, so you don't have to
> forward declare functions, and so you don't have multiple anon namespaces
within
> the file?

This is also intended to avoid unnecessary diffs.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.h:22: // These functions return handles in "commit
order".  A valid commit ordering is
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> This function returns.
> 
> That said, at this point I'm not sure there's much need to have this in a
> separate file.

Fixed.

I agree this code is not currently well organized.  This is part of the effort
to keep the diff size manageable.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/process_commi...
File sync/engine/process_commit_response_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:40: static
sync_pb::CommitResponse::ResponseType ProcessSingleCommitResponse(
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> Again, seems odd to have a class with nothing but static functions.

I agree.

I could make these functions standalone, but I don't think that really solves
the problem.  Eventually, I'd like to make these functions standalone then find
them a new home.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.cc:16: using syncable::SYNCER;
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> move below GET_BY_HANDLE

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:8: #include <map>
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> not needed

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:28: // have been selected from
the syncable Directory and prepared for a commit.
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> for a commit -> for commit

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:36: // (It unsets its entities'
SYNCING bits.)
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> As you mentioned, it's odd to be doing this work in the destructor. I think it
> would be cleaner if there were a CleanupCommit or FinishCommit method that did
> this work.
> 
> That would also allow for passing an explicit transaction through the
directory
> changing methods as well, which I tend to prefer to avoid bugs where someone
> holding a transaction calls one of these methods.

I can see that side of the argument.  However, unsetting those SYNCING bits in
the destructor helps to prevent a different, more subtle kind of bug where we
might end up forgetting to unset those bits.  I'll have to give it some more
thought.

UPDATE:
I tried to modify the code to make the transaction acquisition more explicit,
but ran into a problem.  We don't want commit.cc to be aware of directories and
transactions.  That code is supposed to be shared between new-style and
old-style types.

There's an up side to this, too.  If commit.cc can't open transactions in order
to pass them to the SyncDirectoryCommitContribution's functions, then it can't
open them in a way that conflicts with it, either.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:42: // This function may return
NULL if this type has no items ready for and
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> doesn't "ready for" already imply "requiring"? I'd just say "ready for commit"

I think of an item as "requiring" commit if its IS_UNSYNCED bit is set.  That
item needs to be committed at some point in the future.  However, sometimes it's
not ready for a commit at this time.  It could be waiting for an encryption
passphrase or for a throttling interval to expire.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:50: // Serialize this objects's
entries to the given commit request |msg|.
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> this object's -> this contribution's (here and below)

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution_unittest.cc (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution_unittest.cc:6: #include
"sync/engine/sync_directory_commit_contribution.h"
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> have as first #include with a newline after

Done.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:33: typedef std::map<ModelType,
SyncDirectoryCommitContributor*>
On 2013/10/09 00:17:39, Nicolas Zea wrote:
> Would using linked_ptrs make sense here? (So you don't need to have
> STLValueDeleters)

I think I prefer the STLValueDeleters.  They're not perfect, but they're better
than reference counting.

[tim (not reviewing), 2013-10-10 17:22:36]

Couple comments.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: SyncerError BuildAndPostCommits(
On 2013/10/09 20:00:19, rlarocque wrote:
> On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > Does it make sense to incorporate this into the Commit object?
> 
> Maybe.  I kind of like it the way it is, since it doesn't get access to
Commit's
> private members this way.

The flip side is it's way more of a PITA to track down the definition of this
function for the same reasons that the style guide mandates .h/cc files match
class or namespace names.  When you see this function out in the wild you have
no clue where it's coming from, whether it's closely related to and trustable as
much as commit.cc or what.  

Making it a static member would help this immensely and not give you access to
private members.  Not that I love static methods, but they suck for many of the
same reasons as this free floating function does (e.g difficult to mock in
tests).

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:33: typedef std::map<ModelType,
SyncDirectoryCommitContributor*>
On 2013/10/09 20:00:19, rlarocque wrote:
> On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > Would using linked_ptrs make sense here? (So you don't need to have
> > STLValueDeleters)
> 
> I think I prefer the STLValueDeleters.  They're not perfect, but they're
better
> than reference counting.

There's been some debate about this on chromium-dev.  For maps in particular
there was no consensus reached because linked_ptr offers a few advantages (and
note, afaik, there's no 'ScopedVector' analog, although if you wanted to
investigate it I'm sure you could find support :P). 

I think if you're declaring a truly internal type (in a private: section, only
possibly instantiated by one class ever) and opt for using an STLValueDeleter,
that makes sense, but if you're declaring a generic container and expecting
people who use it to remember to add one of these too, and ensure destruction
ordering, it seems more error prone.  

Also, could any code ever (now-or-future) remove from this map?  The Deleter
won't help this case.

[rlarocque, 2013-10-10 19:05:00]

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: SyncerError BuildAndPostCommits(
On 2013/10/10 17:22:36, timsteele wrote:
> On 2013/10/09 20:00:19, rlarocque wrote:
> > On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > > Does it make sense to incorporate this into the Commit object?
> > 
> > Maybe.  I kind of like it the way it is, since it doesn't get access to
> Commit's
> > private members this way.
> 
> The flip side is it's way more of a PITA to track down the definition of this
> function for the same reasons that the style guide mandates .h/cc files match
> class or namespace names.  When you see this function out in the wild you have
> no clue where it's coming from, whether it's closely related to and trustable
as
> much as commit.cc or what.  
> 
> Making it a static member would help this immensely and not give you access to
> private members.  Not that I love static methods, but they suck for many of
the
> same reasons as this free floating function does (e.g difficult to mock in
> tests).

That's a good point.

I just realized that there's not a lot of old code in this function, so I could
move it without uglifying the diff too much.  That gives us a few more options.

How about we move it to the Syncer?  I'd feel better if this function were a
non-static member of some class, and it already takes a Syncer* as a parameter,
which suggests it might belong there.  

We'd probably have to make the Commit class public again.  Either that, or move
it into syncer.cc, which seems worse.  Making Commit public is probably a good
idea for other reasons.  For example, we'd need to make it public if we want to
unit test it or make testable mocks of it.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:33: typedef std::map<ModelType,
SyncDirectoryCommitContributor*>
On 2013/10/10 17:22:36, timsteele wrote:
> On 2013/10/09 20:00:19, rlarocque wrote:
> > On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > > Would using linked_ptrs make sense here? (So you don't need to have
> > > STLValueDeleters)
> > 
> > I think I prefer the STLValueDeleters.  They're not perfect, but they're
> better
> > than reference counting.
> 
> There's been some debate about this on chromium-dev.  For maps in particular
> there was no consensus reached because linked_ptr offers a few advantages (and
> note, afaik, there's no 'ScopedVector' analog, although if you wanted to
> investigate it I'm sure you could find support :P). 
> 
> I think if you're declaring a truly internal type (in a private: section, only
> possibly instantiated by one class ever) and opt for using an STLValueDeleter,
> that makes sense, but if you're declaring a generic container and expecting
> people who use it to remember to add one of these too, and ensure destruction
> ordering, it seems more error prone.  
> 
> Also, could any code ever (now-or-future) remove from this map?  The Deleter
> won't help this case.  

Yeah, I re-read that thread when I saw this comment.

I think this use case are simple enough that we can get away with using the
STLValueDeleter.  Eventually, the contributors will be owned by the
SyncSessionContext and modified only by a small handful of its member functions.
 It should be possible to implement it correctly, and we can write unit tests
that will break (under valgrind) if we mess it up.

[rlarocque, 2013-10-10 20:16:39]

Patch updated.  PTAL.

I tried moving BuildAndPostCommits to see what it would look like.  It looked
good to me, so I uploaded the change.

[Nicolas Zea, 2013-10-10 21:34:45]

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
File sync/engine/build_commit_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
sync/engine/build_commit_command.h:31: class SYNC_EXPORT_PRIVATE
BuildCommitCommand : public SyncerCommand {
On 2013/10/09 20:00:19, rlarocque wrote:
> On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > Why not just make these all standalone functions at this point? (possibly as
> > helper functions within Commit.cc)
> 
> For now, I was trying to keep the diffs readable by not moving code between
> files if I can avoid it.  I'd like to write a follow-up CL to clean up this
mess
> after this Cl has landed.
> 
> If you'd like, I could easily make these functions standalone and leave them
in
> this file.

I think this patch is already significant enough, and the diffs are already
confused enough due to moving logic around, that it's too late to keep the diff
clean. I'd rather have the final patch itself result in clean self-contained
code. For example, the comment above no longer applies.

Perhaps just rename this file commit_util or something, and put the methods in a
commit_util namespace? Having them within a "command" is confusing I find.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:36: // (It unsets its entities'
SYNCING bits.)
On 2013/10/09 20:00:19, rlarocque wrote:
> On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > As you mentioned, it's odd to be doing this work in the destructor. I think
it
> > would be cleaner if there were a CleanupCommit or FinishCommit method that
did
> > this work.
> > 
> > That would also allow for passing an explicit transaction through the
> directory
> > changing methods as well, which I tend to prefer to avoid bugs where someone
> > holding a transaction calls one of these methods.
> 
> I can see that side of the argument.  However, unsetting those SYNCING bits in
> the destructor helps to prevent a different, more subtle kind of bug where we
> might end up forgetting to unset those bits.  I'll have to give it some more
> thought.
> 
> UPDATE:
> I tried to modify the code to make the transaction acquisition more explicit,
> but ran into a problem.  We don't want commit.cc to be aware of directories
and
> transactions.  That code is supposed to be shared between new-style and
> old-style types.
> 
> There's an up side to this, too.  If commit.cc can't open transactions in
order
> to pass them to the SyncDirectoryCommitContribution's functions, then it can't
> open them in a way that conflicts with it, either.

Even if we don't have transactions passed through, I still think there's benefit
in not having complex logic during the destructor. You could always have a
CHECK(cleanup_called_) or something.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:39: static Commit* Init(
I think technically the constructor/destructor should be first in the
declaration order

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: explicit Commit(
remove explicit

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
File sync/engine/process_commit_response_command.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:40: static
sync_pb::CommitResponse::ResponseType ProcessSingleCommitResponse(
Same comment about possibly renaming the file to *_util and updating comments
(and eventually better organizing all this)

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:48: static void
ProcessSuccessfulCommitResponse(
move all of these to cc file?

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution_unittest.cc (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution_unittest.cc:166:
TEST_F(SyncDirectoryCommitContributionTest, ProcessCommitResponse) {
comment about what the test does?

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:20: class
SyncDirectoryCommitContributor {
Add comment about what this class is

[rlarocque, 2013-10-11 23:03:30]

Patch updated with changes in response to latest review.  PTAL.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
File sync/engine/build_commit_command.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/build_commit_...
sync/engine/build_commit_command.h:31: class SYNC_EXPORT_PRIVATE
BuildCommitCommand : public SyncerCommand {
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> On 2013/10/09 20:00:19, rlarocque wrote:
> > On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > > Why not just make these all standalone functions at this point? (possibly
as
> > > helper functions within Commit.cc)
> > 
> > For now, I was trying to keep the diffs readable by not moving code between
> > files if I can avoid it.  I'd like to write a follow-up CL to clean up this
> mess
> > after this Cl has landed.
> > 
> > If you'd like, I could easily make these functions standalone and leave them
> in
> > this file.
> 
> I think this patch is already significant enough, and the diffs are already
> confused enough due to moving logic around, that it's too late to keep the
diff
> clean. I'd rather have the final patch itself result in clean self-contained
> code. For example, the comment above no longer applies.
> 
> Perhaps just rename this file commit_util or something, and put the methods in
a
> commit_util namespace? Having them within a "command" is confusing I find.

Fair enough.  I moved everything into the files 'commit_util.cc/h' and namespace
syncer::commit_util.

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/13001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:36: // (It unsets its entities'
SYNCING bits.)
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> On 2013/10/09 20:00:19, rlarocque wrote:
> > On 2013/10/09 00:17:39, Nicolas Zea wrote:
> > > As you mentioned, it's odd to be doing this work in the destructor. I
think
> it
> > > would be cleaner if there were a CleanupCommit or FinishCommit method that
> did
> > > this work.
> > > 
> > > That would also allow for passing an explicit transaction through the
> > directory
> > > changing methods as well, which I tend to prefer to avoid bugs where
someone
> > > holding a transaction calls one of these methods.
> > 
> > I can see that side of the argument.  However, unsetting those SYNCING bits
in
> > the destructor helps to prevent a different, more subtle kind of bug where
we
> > might end up forgetting to unset those bits.  I'll have to give it some more
> > thought.
> > 
> > UPDATE:
> > I tried to modify the code to make the transaction acquisition more
explicit,
> > but ran into a problem.  We don't want commit.cc to be aware of directories
> and
> > transactions.  That code is supposed to be shared between new-style and
> > old-style types.
> > 
> > There's an up side to this, too.  If commit.cc can't open transactions in
> order
> > to pass them to the SyncDirectoryCommitContribution's functions, then it
can't
> > open them in a way that conflicts with it, either.
> 
> Even if we don't have transactions passed through, I still think there's
benefit
> in not having complex logic during the destructor. You could always have a
> CHECK(cleanup_called_) or something.

I was hoping to avoid that.  That DCHECK() means that some ASSERT statements in
my unit tests will now cause a DCHECK() if they fail because they would destroy
the CommitContribution without calling the CleanUp() method.

But I admit it's an ugly trade-off either way.  Done.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:39: static Commit* Init(
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> I think technically the constructor/destructor should be first in the
> declaration order

Wasn't there also some rule about static members coming first?  The style
guideline appears to be offline at the moment so I can't check.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:47: explicit Commit(
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> remove explicit

Oops, that regressed when I was moving this code around.  Thanks for catching it
again.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
File sync/engine/process_commit_response_command.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:40: static
sync_pb::CommitResponse::ResponseType ProcessSingleCommitResponse(
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> Same comment about possibly renaming the file to *_util and updating comments
> (and eventually better organizing all this)

Done.  (Merged into commit_util.cc/h)

https://codereview.chromium.org/25638003/diff/30001/sync/engine/process_commi...
sync/engine/process_commit_response_command.h:48: static void
ProcessSuccessfulCommitResponse(
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> move all of these to cc file?

These have been moved to an anonymous namespace in commit_util.cc.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution_unittest.cc (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution_unittest.cc:166:
TEST_F(SyncDirectoryCommitContributionTest, ProcessCommitResponse) {
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> comment about what the test does?

Done.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contributor.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contributor.h:20: class
SyncDirectoryCommitContributor {
On 2013/10/10 21:34:45, Nicolas Zea wrote:
> Add comment about what this class is

Done.

[Nicolas Zea, 2013-10-14 21:45:34]

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:39: static Commit* Init(
On 2013/10/11 23:03:30, rlarocque wrote:
> On 2013/10/10 21:34:45, Nicolas Zea wrote:
> > I think technically the constructor/destructor should be first in the
> > declaration order
> 
> Wasn't there also some rule about static members coming first?  The style
> guideline appears to be offline at the moment so I can't check.

That's for static const values, not methods. All methods (static or not) should
be below constructor/destructor declarations.

https://codereview.chromium.org/25638003/diff/41001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

https://codereview.chromium.org/25638003/diff/41001/sync/engine/commit.cc#new...
sync/engine/commit.cc:98: for (ContributionMap::iterator it =
contributions_.begin();
This still has the issue of making it an implicit cleanup on destruction. Have
an explicit Commit::CleanUp method?

https://codereview.chromium.org/25638003/diff/41001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/41001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:35: // This destructor may make
model neutral changes to the directory.
comment no longer applies

[rlarocque, 2013-10-14 23:23:27]

Patch updated.  PTAL.

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h
File sync/engine/commit.h (right):

https://codereview.chromium.org/25638003/diff/30001/sync/engine/commit.h#newc...
sync/engine/commit.h:39: static Commit* Init(
On 2013/10/14 21:45:35, Nicolas Zea wrote:
> On 2013/10/11 23:03:30, rlarocque wrote:
> > On 2013/10/10 21:34:45, Nicolas Zea wrote:
> > > I think technically the constructor/destructor should be first in the
> > > declaration order
> > 
> > Wasn't there also some rule about static members coming first?  The style
> > guideline appears to be offline at the moment so I can't check.
> 
> That's for static const values, not methods. All methods (static or not)
should
> be below constructor/destructor declarations.

OK.  Fixed.

https://codereview.chromium.org/25638003/diff/41001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

https://codereview.chromium.org/25638003/diff/41001/sync/engine/commit.cc#new...
sync/engine/commit.cc:98: for (ContributionMap::iterator it =
contributions_.begin();
On 2013/10/14 21:45:35, Nicolas Zea wrote:
> This still has the issue of making it an implicit cleanup on destruction. Have
> an explicit Commit::CleanUp method?

Done.

https://codereview.chromium.org/25638003/diff/41001/sync/engine/sync_director...
File sync/engine/sync_directory_commit_contribution.h (right):

https://codereview.chromium.org/25638003/diff/41001/sync/engine/sync_director...
sync/engine/sync_directory_commit_contribution.h:35: // This destructor may make
model neutral changes to the directory.
On 2013/10/14 21:45:35, Nicolas Zea wrote:
> comment no longer applies

Done.

[Nicolas Zea, 2013-10-14 23:29:24]

LGTM

[commit-bot: I haz the power, 2013-10-15 01:10:24]

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

[commit-bot: I haz the power, 2013-10-15 02:13:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-15 19:57:27]

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

[commit-bot: I haz the power, 2013-10-15 21:54:52]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-15 22:18:22]

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

[commit-bot: I haz the power, 2013-10-16 00:03:52]

Change committed as 228810