sync: Implement per-type commit interface

sync/engine/sync_directory_commit_contribution.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SYNC_ENGINE_SYNC_DIRECTORY_COMMIT_CONTRIBUTION_H_
+#define SYNC_ENGINE_SYNC_DIRECTORY_COMMIT_CONTRIBUTION_H_
+
+#include <map>

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

not needed

[rlarocque, 2013/10/09 20:00:19]

Done.

+
+#include "base/gtest_prod_util.h"
+#include "sync/base/sync_export.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/util/syncer_error.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/sessions/status_controller.h"
+
+namespace syncer {
+
+namespace sessions {
+class StatusController;
+}  // namespace sessions
+
+namespace syncable {
+class Directory;
+}  // namespace syncable
+
+// This class represents a set of items belonging to a particular data type that
+// have been selected from the syncable Directory and prepared for a commit.

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

for a commit -> for commit

[rlarocque, 2013/10/09 20:00:19]

Done.

+//
+// This class handles the bookkeeping related to the commit of these items,
+// including processing the commit response message and setting and unsetting
+// the SYNCING bits.
+class SYNC_EXPORT_PRIVATE SyncDirectoryCommitContribution {
+ public:
+  // This destructor may make model neutral changes to the directory.
+  // (It unsets its entities' SYNCING bits.)

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

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.

[rlarocque, 2013/10/09 20:00:19]

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.

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

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.

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

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.

+  ~SyncDirectoryCommitContribution();
+
+  // Build a CommitContribution from the IS_UNSYNCED items in |dir| with the
+  // given |type|.  The contribution will include at most |max_items| entries.
+  //
+  // This function may return NULL if this type has no items ready for and

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

doesn't "ready for" already imply "requiring"? I'd just say "ready for commit"

[rlarocque, 2013/10/09 20:00:19]

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.

+  // requiring commit.  This function may make model neutral changes to the
+  // directory.
+  static SyncDirectoryCommitContribution* Build(
+      syncable::Directory* dir,
+      ModelType type,
+      size_t max_items);
+
+  // Serialize this objects's entries to the given commit request |msg|.

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

this object's -> this contribution's (here and below)

[rlarocque, 2013/10/09 20:00:19]

Done.

+  //
+  // This function is not const.  It will update some state in this object that
+  // will be used when processing the associated commit response.  This function
+  // should not be called more than once.
+  void AddToCommitMessage(sync_pb::ClientToServerMessage* msg);
+
+  // Updates this object's contents in accordance with the provided |response|.
+  //
+  // This function may make model-neutral changes to the directory.  It is not
+  // valid to call this function unless AddToCommitMessage() was called earlier.
+  // This function should not be called more than once.
+  SyncerError ProcessCommitResponse(
+      const sync_pb::ClientToServerResponse& response,
+      sessions::StatusController* status);
+
+  // Returns the number of entries included in this contribution.
+  size_t GetNumEntries();
+
+ private:
+  class SyncDirectoryCommitContributionTest;
+  FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest, GatherByTypes);
+  FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest,
+                           GatherAndTruncate);
+
+  SyncDirectoryCommitContribution(
+      const std::vector<int64>& metahandles,
+      const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entities,
+      syncable::Directory* directory);
+
+  syncable::Directory* dir_;
+  const std::vector<int64> metahandles_;
+  const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities_;
+  size_t entries_start_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncDirectoryCommitContribution);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_ENGINE_SYNC_DIRECTORY_COMMIT_CONTRIBUTION_H_