sync: Implement per-type commit interface

sync/engine/commit.h

 // Copyright (c) 2012 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_COMMIT_H_
 #define SYNC_ENGINE_COMMIT_H_
 
+#include <map>
+
+#include "base/stl_util.h"
+#include "sync/base/sync_export.h"
+#include "sync/engine/sync_directory_commit_contributor.h"
 #include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/util/syncer_error.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/util/extensions_activity.h"
+
+namespace sync_pb {
+class ClientToServerMessage;
+class ClientToServerResponse;
+}
 
 namespace syncer {
 
 namespace sessions {
+class StatusController;
 class SyncSession;
 }
 
+class SyncDirectoryCommitContribution;
+class SyncDirectoryCommitContributor;
 class Syncer;
 
 // This function will commit batches of unsynced items to the server until the
 // number of unsynced and ready to commit items reaches zero or an error is
 // encountered.  A request to exit early will be treated as an error and will
 // abort any blocking operations.
 //
 // The Syncer parameter is provided only for access to its ExitRequested()
 // method.  This is technically unnecessary since an early exit request should
 // be detected as we attempt to contact the sync server.
 //
 // The SyncSession parameter contains pointers to various bits of state,
 // including the syncable::Directory that contains all sync items and the
 // ServerConnectionManager used to contact the server.
 SyncerError BuildAndPostCommits(

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

Does it make sense to incorporate this into the Commit object?

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

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

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

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

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

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.  

     ModelTypeSet request_types,
     Syncer* syncer,
     sessions::SyncSession* session);
 
+// This class wraps the actions related to building and executing a single
+// commit operation.
+//
+// This class' most important responsibility is to manage the ContributionsMap.
+// This class serves as a container for those objects.  Although it would have
+// been acceptable to let this class be a dumb container object, it turns out
+// that there was no other convenient place to put the Init() and
+// PostAndProcessCommitResponse() functions.  So they ended up here.
+class SYNC_EXPORT_PRIVATE Commit {

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

Can this be declared in the cc file? It's not used outside of
BuildAndPostCommits right?

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

Good idea.  Done.

+ public:
+  static Commit* Init(
+      ModelTypeSet requested_types,
+      CommitContributorMap* contributor_map,

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

move down below the input params

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

Done.

+      size_t max_entries,
+      const std::string& account_name,
+      const std::string& cache_guid,
+      ExtensionsActivity* extensions_activity);
+
+  explicit Commit(
+      std::map<ModelType, SyncDirectoryCommitContribution*> contributions,

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

const ref for first these params

also, doesn't need the explicit keyword

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

Done.

+      sync_pb::ClientToServerMessage message,
+      ExtensionsActivity::Records extensions_activity_buffer);
+  ~Commit();
+
+  SyncerError PostAndProcessResponse(
+      sessions::SyncSession* session,
+      sessions::StatusController* status,
+      ExtensionsActivity* extensions_activity);
+
+ private:
+  typedef std::map<ModelType, SyncDirectoryCommitContribution*> ContributionMap;
+
+  ContributionMap contributions_;
+  STLValueDeleter<ContributionMap> deleter_;
+
+  sync_pb::ClientToServerMessage message_;
+  sync_pb::ClientToServerResponse response_;
+  ExtensionsActivity::Records extensions_activity_buffer_;
+};
+
 }  // namespace syncer
 
 #endif  // SYNC_ENGINE_COMMIT_H_