sync: Implement per-type commit interface

sync/engine/sync_directory_commit_contribution.h

Index: sync/engine/sync_directory_commit_contribution.h
diff --git a/sync/engine/sync_directory_commit_contribution.h b/sync/engine/sync_directory_commit_contribution.h
new file mode 100644
index 0000000000000000000000000000000000000000..9a619b27908d60f7a48d6e339c3d6664e0959de8
--- /dev/null
+++ b/sync/engine/sync_directory_commit_contribution.h
@@ -0,0 +1,91 @@
+// 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>
+
+#include "base/gtest_prod_util.h"
+#include "base/stl_util.h"

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

what is this for?

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

Apparently nothing.  Removed.

+#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 syncable {

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

nit, order after sessions (ABC order)

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

Done.

+class Directory;
+}  // namespace syncable
+
+namespace sessions {
+class StatusController;
+}  // namespace sessions
+
+// 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.
+//
+// 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.)
+  ~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
+  // requiring commit.  This function may make model neutral changes to the
+  // directory.
+  static SyncDirectoryCommitContribution* Build(

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

Would it be better to explicitly pass model neutral write transactions around in
functions that make model neutral changes to the directory?

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

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.

+      syncable::Directory* dir,
+      ModelType type,
+      size_t max_items);
+
+  // Serialize this objects's entries to the given commit request |msg|.
+  //
+  // 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:
+  SyncDirectoryCommitContribution(
+      syncable::Directory* directory,

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

move to be last param

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

Done.

+      std::vector<int64> metahandles,

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

const ref

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

Done.

+      google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities);

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

const ref

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

Done.

+
+  syncable::Directory* dir_;
+  const std::vector<int64> metahandles_;
+  const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities_;
+  size_t entries_start_index_;
+
+  class SyncDirectoryCommitContributionTest;
+  FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest, GatherByTypes);

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

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.

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

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

+  FRIEND_TEST_ALL_PREFIXES(SyncDirectoryCommitContributionTest,
+                           GatherAndTruncate);
+
+  DISALLOW_COPY_AND_ASSIGN(SyncDirectoryCommitContribution);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_ENGINE_SYNC_DIRECTORY_COMMIT_CONTRIBUTION_H_