Fix handling of undeletion within the syncer.

Accomplished by:
(a) Plumbing the const CommitMessage into process_commit_response_command.cc, so that we can know the server state better (IS_DEL is no longer authoritative if !
which spilled over into a couple test expectations.
(b) Relaxing the enforced invariants that version==0 implied !ServerKnows, for items with client tags.
(c) Use ID renaming instead of delete/recreate to handle collisions on the unique tag.
(d) Upon deletion, version number goes to 0 so that the server knows to process the item as a recreate, if it gets undeleted.  Previously, the server number would get synthesized on the client by incrementing the last known version number.
(e) Add unit tests for pretty much every ordering of delete/undelete I could come up with.

BUG=46769
TEST=unit tests; repeatedly uninstalled/installed extensions

Review URL: http://codereview.chromium.org/2844037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@52209 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 83 insertions, 19 deletions

diff --git a/chrome/test/sync/engine/mock_connection_manager.cc b/chrome/test/sync/engine/mock_connection_manager.cc
index ac1e0f1..5aec1ba 100644
--- a/chrome/test/sync/engine/mock_connection_manager.cc
+++ b/chrome/test/sync/engine/mock_connection_manager.cc
@@ -44,7 +44,6 @@ MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr,
       fail_next_postbuffer_(false),
       directory_manager_(dirmgr),
       directory_name_(name),
-      mid_commit_callback_(NULL),
       mid_commit_observer_(NULL),
       throttling_(false),
       fail_with_auth_invalid_(false),
@@ -57,9 +56,6 @@ MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr,
 };
 
 MockConnectionManager::~MockConnectionManager() {
-  for (size_t i = 0; i < commit_messages_.size(); i++)
-    delete commit_messages_[i];
-  delete mid_commit_callback_;
 }
 
 void MockConnectionManager::SetCommitTimeRename(string prepend) {
@@ -67,7 +63,7 @@ void MockConnectionManager::SetCommitTimeRename(string prepend) {
 }
 
 void MockConnectionManager::SetMidCommitCallback(Closure* callback) {
-  mid_commit_callback_ = callback;
+  mid_commit_callback_.reset(callback);
 }
 
 void MockConnectionManager::SetMidCommitObserver(
@@ -148,7 +144,8 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params,
   }
 
   response.SerializeToString(params->buffer_out);
-  if (mid_commit_callback_) {
+  if (post.message_contents() == ClientToServerMessage::COMMIT &&
+      mid_commit_callback_.get()) {
     mid_commit_callback_->Run();
   }
   if (mid_commit_observer_) {
@@ -250,8 +247,47 @@ SyncEntity* MockConnectionManager::AddUpdateBookmark(string id,
   return AddUpdateFull(id, parent_id, name, version, sync_ts, false);
 }
 
+SyncEntity* MockConnectionManager::AddUpdateFromLastCommit() {
+  EXPECT_EQ(1, last_sent_commit().entries_size());
+  EXPECT_EQ(1, last_commit_response().entryresponse_size());
+  EXPECT_EQ(CommitResponse::SUCCESS,
+      last_commit_response().entryresponse(0).response_type());
+
+  if (last_sent_commit().entries(0).deleted()) {
+    AddUpdateTombstone(syncable::Id::CreateFromServerId(
+        last_sent_commit().entries(0).id_string()));
+  } else {
+    SyncEntity* ent = updates_.add_entries();
+    ent->CopyFrom(last_sent_commit().entries(0));
+    ent->clear_insert_after_item_id();
+    ent->clear_old_parent_id();
+    ent->set_position_in_parent(
+        last_commit_response().entryresponse(0).position_in_parent());
+    ent->set_version(
+        last_commit_response().entryresponse(0).version());
+    ent->set_id_string(
+        last_commit_response().entryresponse(0).id_string());
+    // Tests don't currently care about the following:
+    // originator_cache_guid, originator_client_item_id, parent_id_string,
+    // name, non_unique_name.
+  }
+  return GetMutableLastUpdate();
+}
+
+void MockConnectionManager::AddUpdateTombstone(const syncable::Id& id) {
+  // Tombstones have only the ID set and dummy values for the required fields.
+  SyncEntity* ent = updates_.add_entries();
+  ent->set_id_string(id.GetServerId());
+  ent->set_version(0);
+  ent->set_name("");
+  ent->set_deleted(true);
+}
+
 void MockConnectionManager::SetLastUpdateDeleted() {
-  GetMutableLastUpdate()->set_deleted(true);
+  // Tombstones have only the ID set.  Wipe anything else.
+  string id_string = GetMutableLastUpdate()->id_string();
+  updates_.mutable_entries()->RemoveLast();
+  AddUpdateTombstone(syncable::Id::CreateFromServerId(id_string));
 }
 
 void MockConnectionManager::SetLastUpdateOriginatorFields(
@@ -368,8 +404,8 @@ void MockConnectionManager::ProcessCommit(ClientToServerMessage* csm,
   map <string, string> changed_ids;
   const CommitMessage& commit_message = csm->commit();
   CommitResponse* commit_response = response_buffer->mutable_commit();
-  commit_messages_.push_back(new CommitMessage);
-  commit_messages_.back()->CopyFrom(commit_message);
+  commit_messages_->push_back(new CommitMessage);
+  commit_messages_->back()->CopyFrom(commit_message);
   map<string, CommitResponse_EntryResponse*> response_map;
   for (int i = 0; i < commit_message.entries_size() ; i++) {
     const sync_pb::SyncEntity& entry = commit_message.entries(i);
@@ -411,6 +447,7 @@ void MockConnectionManager::ProcessCommit(ClientToServerMessage* csm,
       er->set_id_string(new_id);
     }
   }
+  commit_responses_->push_back(new CommitResponse(*commit_response));
 }
 
 SyncEntity* MockConnectionManager::AddUpdateDirectory(
@@ -428,13 +465,18 @@ SyncEntity* MockConnectionManager::AddUpdateBookmark(
 }
 
 SyncEntity* MockConnectionManager::GetMutableLastUpdate() {
-  DCHECK(updates_.entries_size() > 0);
+  EXPECT_TRUE(updates_.entries_size() > 0);
   return updates_.mutable_entries()->Mutable(updates_.entries_size() - 1);
 }
 
 const CommitMessage& MockConnectionManager::last_sent_commit() const {
-  DCHECK(!commit_messages_.empty());
-  return *commit_messages_.back();
+  EXPECT_TRUE(!commit_messages_.empty());
+  return *commit_messages_->back();
+}
+
+const CommitResponse& MockConnectionManager::last_commit_response() const {
+  EXPECT_TRUE(!commit_responses_.empty());
+  return *commit_responses_->back();
 }
 
 void MockConnectionManager::ThrottleNextRequest(
diff --git a/chrome/test/sync/engine/mock_connection_manager.h b/chrome/test/sync/engine/mock_connection_manager.h
index 4eff4d4..be2144f 100644
--- a/chrome/test/sync/engine/mock_connection_manager.h
+++ b/chrome/test/sync/engine/mock_connection_manager.h
@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/scoped_vector.h"
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
@@ -57,6 +58,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
   // once. This mock object doesn't simulate a changelist, it simulates server
   // responses.
   void ResetUpdates();
+
   // Generic versions of AddUpdate functions. Tests using these function should
   // compile for both the int64 and string id based versions of the server.
   // The SyncEntity returned is only valid until the Sync is completed
@@ -94,6 +96,17 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
                                          string name,
                                          int64 version,
                                          int64 sync_ts);
+
+  // Find the last commit sent by the client, and replay it for the next get
+  // updates command.  This can be used to simulate the GetUpdates that happens
+  // immediately after a successful commit.
+  sync_pb::SyncEntity* AddUpdateFromLastCommit();
+
+  // Add a deleted item.  Deletion records typically contain no
+  // additional information beyond the deletion, and no specifics.
+  // The server may send the originator fields.
+  void AddUpdateTombstone(const syncable::Id& id);
+
   void SetLastUpdateDeleted();
   void SetLastUpdateServerTag(const string& tag);
   void SetLastUpdateClientTag(const string& tag);
@@ -134,11 +147,17 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
     return committed_ids_;
   }
   const std::vector<sync_pb::CommitMessage*>& commit_messages() const {
-    return commit_messages_;
+    return commit_messages_.get();
+  }
+  const std::vector<sync_pb::CommitResponse*>& commit_responses() const {
+    return commit_responses_.get();
   }
   // Retrieve the last sent commit message.
   const sync_pb::CommitMessage& last_sent_commit() const;
 
+  // Retrieve the last returned commit response.
+  const sync_pb::CommitResponse& last_commit_response() const;
+
   // Retrieve the last request submitted to the server (regardless of type).
   const sync_pb::ClientToServerMessage& last_request() const {
     return last_request_;
@@ -179,6 +198,11 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   void SetServerNotReachable();
 
+  std::string store_birthday() { return store_birthday_; }
+
+  // Locate the most recent update message for purpose of alteration.
+  sync_pb::SyncEntity* GetMutableLastUpdate();
+
  private:
   sync_pb::SyncEntity* AddUpdateFull(syncable::Id id, syncable::Id parentid,
                                      string name, int64 version,
@@ -198,9 +222,6 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   void AddDefaultBookmarkData(sync_pb::SyncEntity* entity, bool is_folder);
 
-  // Locate the most recent update message for purpose of alteration.
-  sync_pb::SyncEntity* GetMutableLastUpdate();
-
   // Determine if one entry in a commit should be rejected with a conflict.
   bool ShouldConflictThisCommit();
 
@@ -224,8 +245,9 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
   bool conflict_all_commits_;
   int conflict_n_commits_;
 
-  // Commit messages we've sent
-  std::vector<sync_pb::CommitMessage*> commit_messages_;
+  // Commit messages we've sent, and responses we've returned.
+  ScopedVector<sync_pb::CommitMessage> commit_messages_;
+  ScopedVector<sync_pb::CommitResponse> commit_responses_;
 
   // The next id the mock will return to a commit.
   int next_new_id_;
@@ -245,7 +267,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   // The updates we'll return to the next request.
   sync_pb::GetUpdatesResponse updates_;
-  Closure* mid_commit_callback_;
+  scoped_ptr<Closure> mid_commit_callback_;
   MidCommitObserver* mid_commit_observer_;
 
   // The AUTHENTICATE response we'll return for auth requests.